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.

Sign up for GitHub

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

Jump to bottom

Refactor plugin brokering process #15386

Merged
merged 1 commit into from
Jan 9, 2020
Merged

Refactor plugin brokering process #15386

merged 1 commit into from
Jan 9, 2020

Conversation

amisevsk
Copy link
Contributor

@amisevsk amisevsk commented Dec 3, 2019 •
edited
Loading

What does this PR do?

Rework plugin brokering process to accomodate changes in eclipse-che/che-plugin-broker#80.

Run metadata broker to get plugin tooling (similarly to how we're running the unified broker currently). Add the artifacts broker as an init container on the workspace pod to ensure extensions are downloaded.

One benefit with this change is that we do not need to do anything differently when running in ephemeral mode.

Since PR eclipse-che/che-plugin-broker#80 is not yet merged, to test this PR you can reuse my built broker images:

CHE_WORKSPACE_PLUGIN__BROKER_METADATA_IMAGE=amisevsk/che-metadata-plugin-broker:dev
CHE_WORKSPACE_PLUGIN__BROKER_ARTIFACTS_IMAGE=amisevsk/che-artifacts-plugin-broker:dev

What issues does this PR fix or reference?

#14494

Release Notes

Improved plugin brokering process for faster workspace startup.

Docs PR

N/A

Additional Info

Depends on eclipse-che/che-plugin-broker#80

@che-bot che-bot added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. kind/epic A long-lived, PM-driven feature request. Must include a checklist of items that must be completed. kind/task Internal things, technical debt, and to-do tasks to be performed. labels Dec 3, 2019
@che-bot
Copy link
Contributor

che-bot commented Dec 3, 2019

❌ E2E Happy path tests failed ❗

See Details

Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)

⚠️ https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

ℹ️ Use comment "crw-ci-test" to rerun happy path E2E test.

skabashnyuk
skabashnyuk reviewed Dec 3, 2019
View reviewed changes
assembly/assembly-wsmaster-war/src/main/webapp/WEB-INF/classes/che/che.properties
@@ -476,8 +476,8 @@ che.singleport.wildcard_domain.ipless=false

# Docker image of Che plugin broker app that resolves workspace tooling configuration and copies
# plugins dependencies to a workspace
che.workspace.plugin_broker.init.image=eclipse/che-init-plugin-broker:v0.24
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it backward compatible? What if someone uses it? should we add aliases for this kind of change?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is in a section called "Experimental properties". So I would not sweat over breaking back compat here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A migration step is necessary if a user is specifying their own plugin brokers, which is a good point and should be documented. What will currently happen is

  • Their custom brokers are ignored.
  • The upstream built brokers would be used.

For users deploying with the default properties, no there aren't issues with moving versions.

@che-bot
Copy link
Contributor

che-bot commented Dec 3, 2019

E2E tests of Eclipse Che Multiuser on OCP has failed:

@amisevsk amisevsk mentioned this pull request Dec 4, 2019
Closed
5 tasks
ibuziuk
ibuziuk approved these changes Dec 6, 2019
View reviewed changes
Copy link
Member

@ibuziuk ibuziuk left a comment

Choose a reason for hiding this comment

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

tested and verified on crc

@sleshchenko
Copy link
Member

sleshchenko commented Dec 12, 2019 •
edited
Loading

AFAIU metadata broker does not need any volumes anymore (at least at this stage), then maybe it makes sense to get rid of this https://github.com/amisevsk/che/blob/3e5640a8b6f2d25643c136beab151d460efe34cc/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/wsplugins/PluginBrokerManager.java#L111-L113 ?

@amisevsk
Copy link
Contributor Author

@sleshchenko That makes sense -- the PR already removes the plugins mount from the metadata broker. My only concern is that any other PVCs provisioned could still be affected -- e.g. I still see the broker pod getting a workspace-logs volume. It's likely not a problem, but could still result in the creation of workspace subdirs when using the common strategy.

@che-bot
Copy link
Contributor

che-bot commented Dec 13, 2019

❌ E2E Happy path tests failed ❗

See Details

Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)

⚠️ https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

ℹ️ Use comment "crw-ci-test" to rerun happy path E2E test.

@amisevsk
Copy link
Contributor Author

I've pushed a commit to rename the plugin brokers in the default properties according to eclipse-che/che-plugin-broker#80 (comment)

@sleshchenko I've opted to keep the isEphemeral bit for metadata broker, as I'd like to ensure we're not leaking data into a PVC that won't be cleaned when the workspace is deleted before removing it.

@che-bot
Copy link
Contributor

che-bot commented Dec 13, 2019

E2E tests of Eclipse Che Multiuser on OCP has failed:

@che-bot
Copy link
Contributor

che-bot commented Dec 20, 2019

❌ E2E Happy path tests failed ❗

See Details

Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)

⚠️ https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

ℹ️ Use comment "crw-ci-test" to rerun happy path E2E test.

@che-bot
Copy link
Contributor

che-bot commented Dec 20, 2019

E2E tests of Eclipse Che Multiuser on OCP has failed:

@che-bot
Copy link
Contributor

che-bot commented Dec 27, 2019

E2E tests of Eclipse Che Multiuser on OCP has been successful:

@che-bot
Copy link
Contributor

che-bot commented Dec 27, 2019

❌ E2E Happy path tests failed ❗

See Details

Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)

⚠️ https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

ℹ️ Use comment "crw-ci-test" to rerun happy path E2E test.

@dmytro-ndp
Copy link
Contributor

crw-ci-test

@che-bot
Copy link
Contributor

che-bot commented Dec 27, 2019

✅ E2E Happy path tests succeed 🎉

See Details

Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)

@amisevsk
Refactor plugin brokering process
8c27ba9 
Refactor the plugin brokering process to accomodate changes in plugin
brokers (splitting brokers into a metadata and artifacts broker)

Co-authored-by: Sergii Leshchenko <sleshche@redhat.com>

Signed-off-by: Angel Misevski <amisevsk@redhat.com>
@amisevsk amisevsk merged commit 21c7288 into eclipse-che:master Jan 9, 2020
@monaka monaka mentioned this pull request Jan 14, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@metlos metlos metlos left review comments

@skabashnyuk skabashnyuk skabashnyuk left review comments

@ibuziuk ibuziuk ibuziuk approved these changes

@sleshchenko sleshchenko sleshchenko approved these changes

@l0rd l0rd Awaiting requested review from l0rd

@nickboldt nickboldt Awaiting requested review from nickboldt

@rhopp rhopp Awaiting requested review from rhopp

Assignees
No one assigned
Labels
kind/epic A long-lived, PM-driven feature request. Must include a checklist of items that must be completed. kind/task Internal things, technical debt, and to-do tasks to be performed. status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@amisevsk @che-bot @sleshchenko @dmytro-ndp @metlos @ibuziuk @skabashnyuk