Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move WS.NEXT flow to k8s infra implementation, rework it to use plugin broker #10740

Merged
merged 19 commits into from
Sep 3, 2018

Conversation

garagatyi
Copy link

@garagatyi garagatyi commented Aug 10, 2018

What does this PR do?

Note: This PR is designed to be reviewed in a commit-by-commit manner. Commit message of each commit explains what was changed in the corresponding commit.

Here is the explanation of everything that was done in this PR, but you can read the same explanation in commit messages:
Now we need to start a broker and pass meta.yaml files to it.
Starting a broker is infra-specific stuff, so it has to be done
on the infra implementation side because we don't have a part
in the infra SPI that would allow us to start a broker.
Passing Meta files using InternalEnvironment object is more
invasive than passing only attributes. So, this PR applies
less invasive scheme.
Add fetching of Che editor ID from workspace attributes.
Add fetching of Che plugins IDs from workspace attributes.
Use colon sign for separating editor/plugin ID and version
instead of the slash.
Remove old Workspace.Next model objects.
Use WS.NEXT in both k8s and OS infrastructure implementations.
Fixes the fact that some fields in workspace sidecar tooling model
POJOs were incorrectly named or required custom serialization of
fields.
Add code that allows listening for events from Che plugin broker.
An event might contain workspace tooling config as a result if the broker
finished successfully or error otherwise.
Adds PluginBrokerManager that configures/starts/waits Che plugin
broker.

This PR works on openshift infra and might work on the k8s infra but I haven't checked it yet.
There are technical difficulties in its work because not all the components are ready to be used for such a case, so here are prerequisites for successfully running walking skeleton:

  • recipes: openshift or kubernetes.
  • broker image: this PR uses my own docker image with plugin broker because I haven't merged all required changes to broker app yet
  • PVC strategy: PVC strategy common has to be used since it is hardcoded in walking skeleton for now
  • minishift: do not use latest minishift - there are some issues with the memory limits and workspace with 3GB of RAM do not start on a VM with 9 GB of RAM. And now we don't have memory limits in walking skeleton tooling containers, so all the containers get 1 GB of RAM by default
  • client disconnections, WS start failures. Because prepare method used now to start a broker is in synchronous part of the workspace start process pulling of broker image fails WS start, so to try walking skeleton it is needed to pre-pull broker image garagatyi/broker:websocket.

Me and che platform team will address technical limitations a bit later.

Here is the workspace config with Theia, hello plugin and service plugin https://gist.githubusercontent.com/garagatyi/1e6a58ae4bd386a71c818a8fd428f130/raw/4d7b955948262fc85e8a52f352f36d14d1e36265/ws.json

What issues does this PR fix or reference?

Fixes #10561, #10202

Release Notes

Docs PR

@benoitf benoitf added the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Aug 10, 2018
@l0rd
Copy link
Contributor

l0rd commented Aug 12, 2018

@garagatyi why do we need to move ws.next flow in k8s infra?

@garagatyi
Copy link
Author

@l0rd Now we need to start a broker and pass meta.yaml files to it. Starting a broker is infra-specific stuff, so it has to be done on the infra implementation side because we don't have a part in SPI that would allow us to start a broker. Passing Meta files using InternalEnvironment object might not be easy to merge stuff. So I decided to pass only attributes. This is less invasive change and I hope that Gennady and Che platform team would be OK with this. So we would be able to move faster with the review and get overall integration results faster.
Do you think that we should do that in some another way?

@gazarenkov
Copy link
Contributor

Would not it better to make another infrastructure(s) for your ws.next studies and take a freedom to develop it in the dedicated branch and then make a PR with something more tangible (yes, things marked as TODO)?

@garagatyi
Copy link
Author

Development in another branch leads to huge branches that can't be thoughtfully reviewed and contain solutions that can't be merged without significant redesign. Apart from that feature branch flow adds more pain because of rebasing/merging and changes in master that don't reflect changes in a feature branch.

@gazarenkov
Copy link
Contributor

I believe we discussed (or still discussing) theoretical fundamentals of "feature branch vs something else (like feature toggle)" in other thread.

I understood this approach as not requiring "significant redesign" and therefore "huge branch", so I would consider another implementation of infrastructure as "less invasive change" fitting feature branch pattern.

I can not get a reason of merging a placeholder and then experiment with implementation just in master. Is not it less risky to perform it in branch until it is good enough (at least have something tangible to review) to be merged?

@benoitf
Copy link
Contributor

benoitf commented Aug 13, 2018

As long as the existing code is not broken and that new stuff is only possible through attributes it can be called experimental and toggle is done with special attributes. So, no need to use another branch.

@gazarenkov
Copy link
Contributor

Sure, experimental feature toggle is fine as soon as we support it.
What I am saying is (by what I could understand from Alex's comments): if it will be implemented as another infrastructure (pluggable unit) it can be developed and tested in other branch and merged after.

@benoitf
Copy link
Contributor

benoitf commented Aug 14, 2018

We're already supporting experimental feature. WS next is using it. And goal is to have it on master and rolling it automatically on che.openshift.io as well. BTW no dedicated branch is required so there is no need to use one

@gazarenkov
Copy link
Contributor

@benoitf good to know since #10192 and related does not seem completed.
Would you mind to update (close?) it with corresponding docs?
https://github.com/eclipse/che/wiki would probably be a good place for it.

@l0rd
Copy link
Contributor

l0rd commented Aug 14, 2018

@garagatyi since it's just about starting a container I thought the simpler was to consider the broker as "special" plugin. But anyway that's not a blocker, it's just about doing the minimum amount of work to have ws.next usable with theia plugins and start having fun with it ;-)

Oleksandr Garagatyi added 8 commits August 29, 2018 09:08
Now we need to start a broker and pass meta.yaml files to it.
Starting a broker is infra-specific stuff, so it has to be done
on the infra implementation side because we don't have a part
in the infra SPI that would allow us to start a broker.
Passing Meta files using InternalEnvironment object is more
invasive than passing only attributes. So, this commit apply
less invasive scheme.
Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>
Add fetching of Che editor ID from workspace attributes.
Add fetching of Che plugins IDs from workspace attributes.
Use colon sign for separating editor/plugin ID and version
instead of slash.
Remove old Workspace.Next model objects.
Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>
Use WS.NEXT in both k8s and OS infrastructure implementations.
Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>
Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>
Fixes the fact that some fields in workspace sidecar tooling model
POJOs were incorrectly named or required custom serialization of
feilds.
Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>
Add code that allows to listen for events from Che plugin broker.
Event might contain workspace tooling config as a result if broker
finished successfully or error otherwise.
Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>
Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>
Adds PluginBrokerManager that configures/starts/waits Che plugin
broker.
Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>
@garagatyi
Copy link
Author

@sleshchenko you're right, my bad, sorry

@benoitf
Copy link
Contributor

benoitf commented Aug 31, 2018

@garagatyi I'm not sure about if fix for plugins should be that URL should contain or not /plugins but it's a different story and goal is to make it working so thx for the fix :-)

@garagatyi
Copy link
Author

@benoitf We need to expose URL of che plugin registry to UD and index.json path doesn't contain plugin part. So, I believe we don't need it in the deploy script

@riuvshin
Copy link
Contributor

ci-test build report:
Build details
Test report
selenium tests report data
docker image: eclipseche/che-server:10740
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@garagatyi
Copy link
Author

ci-test

Copy link
Contributor

@benoitf benoitf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was able to get it working on my side

@riuvshin
Copy link
Contributor

ci-test build report:
Build details
Test report
selenium tests report data
docker image: eclipseche/che-server:10740
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@garagatyi
Copy link
Author

@riuvshin CI results have no test report. I checked out other fresh PR and the situation in those was the same. Should I merge my PR or wait for some fix on the CI side?

@riuvshin
Copy link
Contributor

riuvshin commented Sep 1, 2018

ci-test

@riuvshin
Copy link
Contributor

riuvshin commented Sep 1, 2018

ci-test build report:
Build details
Test report
selenium tests report data
docker image: eclipseche/che-server:10740
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@riuvshin
Copy link
Contributor

riuvshin commented Sep 1, 2018

ci-test

@riuvshin
Copy link
Contributor

riuvshin commented Sep 1, 2018

@garagatyi I've fixed CI issues, but now it fails on build phase

@riuvshin
Copy link
Contributor

riuvshin commented Sep 1, 2018

ci-test build report:
Build details
Test report
selenium tests report data
docker image: eclipseche/che-server:10740
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@garagatyi
Copy link
Author

@riuvshin @skabashnyuk go agents tests fails on master branch too on my side

@garagatyi
Copy link
Author

ci-test

@riuvshin
Copy link
Contributor

riuvshin commented Sep 2, 2018

ci-test build report:
Build details
Test report
selenium tests report data
docker image: eclipseche/che-server:10740
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@garagatyi
Copy link
Author

@riuvshin can you triger the build of the master branch to check it? Looks like I lost permissions on CI that allowed me to do that.

@garagatyi
Copy link
Author

@riuvshin never mind, the build has just succeeded. But there are some other issues with the CI process again. Something related to filesystem I suppose. Can you take a look?

@ghost
Copy link

ghost commented Sep 2, 2018

same with other PRs.. pvcs are never attached, hence workspaces fail to start

@riuvshin
Copy link
Contributor

riuvshin commented Sep 2, 2018

ci-test

@riuvshin
Copy link
Contributor

riuvshin commented Sep 2, 2018

ci-test build report:
Build details
Test report
selenium tests report data
docker image: eclipseche/che-server:10740
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@garagatyi garagatyi merged commit 27bed82 into eclipse-che:master Sep 3, 2018
@garagatyi garagatyi deleted the wsnextInInfra branch September 3, 2018 05:57
@garagatyi
Copy link
Author

@riuvshin thank you for recovering the CI!

@benoitf benoitf added this to the 6.11.0 milestone Sep 3, 2018
@benoitf benoitf removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Sep 3, 2018
dmytro-ndp pushed a commit that referenced this pull request Sep 4, 2018
…0740)

* Move wsnext flow in Kubernetes infra implementation

Now we need to start a broker and pass meta.yaml files to it.
Starting a broker is infra-specific stuff, so it has to be done
on the infra implementation side because we don't have a part
in the infra SPI that would allow us to start a broker.
Passing Meta files using InternalEnvironment object is more
invasive than passing only attributes. So, this commit applies
less invasive scheme.

* CHE-10202,10561: Add fetching of Che editor, plugins meta from the registry

Add fetching of Che editor ID from workspace attributes.
Add fetching of Che plugins IDs from workspace attributes.
Use colon sign for separating editor/plugin ID and version
instead of the slash.
Remove old Workspace.Next model objects.

* CHE-10561: Share WS.NEXT between k8s and OS infras

Use WS.NEXT in both k8s and OS infrastructure implementations.

* CHE-10561: fix fetching meta.yaml files from che-plugin-registry

* CHE-10561: Fix sidecar model serialization

Fixes the fact that some fields in workspace sidecar tooling model
POJOs were incorrectly named or required custom serialization of
fields.

* CHE-10561: Add listening of che-plugin-broker

Add code that allows listening for events from Che plugin broker.
An event might contain workspace tooling config as a result if the broker
finished successfully or error otherwise.

* CHE-10561: Add PluginBrokerManager to control broker lifecycle

Adds PluginBrokerManager that configures/starts/waits Che plugin
broker.

Remove unused code.
Remove notion of Workspace next.

* Align plugin registry property between different components

* Fix extra  path in che plugin registry URL

Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/task Internal things, technical debt, and to-do tasks to be performed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Delivering of ChePlugins binaries to the editor container
8 participants