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

Plugin ephemeral volume feature for CheContainers #14539

Merged
merged 14 commits into from Sep 26, 2019
Merged

Plugin ephemeral volume feature for CheContainers #14539

merged 14 commits into from Sep 26, 2019

Conversation

AndrienkoAleksandr
Copy link
Contributor

@AndrienkoAleksandr AndrienkoAleksandr commented Sep 13, 2019 •
edited

What does this PR do?

Plugin ephemeral volume feature for CheContainers

What issues does this PR fix or reference?

#13387
Depends on eclipse-che/che-plugin-broker#74

@AndrienkoAleksandr
Add plugin ephemeral volume feature
1440aab 
Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>
@AndrienkoAleksandr
Move plugin containers volume logic to external class.
d1f9c85 
Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>
@che-bot
Copy link
Contributor

che-bot commented Sep 13, 2019

E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has failed:

@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/enhancement A feature request - must adhere to the feature request template. labels Sep 13, 2019
@AndrienkoAleksandr AndrienkoAleksandr mentioned this pull request Sep 13, 2019
Closed
@che-bot
Copy link
Contributor

che-bot commented Sep 13, 2019

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

@nickboldt nickboldt added the area/install Issues related to installation, including offline/air gap and initial setup label Sep 18, 2019
@nickboldt
Copy link
Contributor

Not technically an install issue, but critical for productization. Need this implemented ASAP so release process can be adapted to use artifacts from this new approach.

nickboldt
nickboldt reviewed Sep 18, 2019
View reviewed changes
Copy link
Contributor

@nickboldt nickboldt left a comment

Choose a reason for hiding this comment

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

I'm not clear on the technical details here so I can't approve. Must rely on people with more deep knowledge of the codebase.

@AndrienkoAleksandr
Fix volume solution to work with unique pvc strategy
2a69eeb 
Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>
@che-bot
Copy link
Contributor

che-bot commented Sep 18, 2019

E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has failed:

@che-bot
Copy link
Contributor

che-bot commented Sep 18, 2019

E2E tests of Eclipse Che Multiuser on OCP has failed:

@AndrienkoAleksandr
Clean up.
99c7b7c 
Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>
@AndrienkoAleksandr
Clean up
b8ca5bc 
Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>
@AndrienkoAleksandr
Clean up.
32ca776 
Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>
@AndrienkoAleksandr
Copy link
Contributor Author

AndrienkoAleksandr commented Sep 19, 2019 •
edited

Hello, guys, updated pr, latest changes seems without regression. Tested with common and unique pvc strategies.

@AndrienkoAleksandr
Copy link
Contributor Author

AndrienkoAleksandr commented Sep 19, 2019 •
edited

I am worry about one thing. Pr depends on eclipse-che/che-plugin-broker#74 If use understood correctly, even if I merge both pr's, feature won't work until release, because we have hardcoded version https://github.com/eclipse/che/blob/master/assembly/assembly-wsmaster-war/src/main/webapp/WEB-INF/classes/che/che.properties#L596 , and we don't have next or nigltly tag... @ibuziuk Should I set up version image tag with git hash, until release?

@che-bot
Copy link
Contributor

che-bot commented Sep 19, 2019

E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has failed:

@che-bot
Copy link
Contributor

che-bot commented Sep 19, 2019

E2E tests of Eclipse Che Multiuser on OCP has failed:

@AndrienkoAleksandr AndrienkoAleksandr mentioned this pull request Sep 20, 2019
Merged
@che-bot
Copy link
Contributor

che-bot commented Sep 20, 2019

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

@AndrienkoAleksandr
Rename field persistVolume to ephemeral
ef5dc84 
Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>
@AndrienkoAleksandr
Fix up.
7fdd2e2 
Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>
sleshchenko
sleshchenko reviewed Sep 24, 2019
View reviewed changes
Copy link
Member

@sleshchenko sleshchenko left a comment

Choose a reason for hiding this comment

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

in general LGTM
Just a few minor comments.

And before approving it, I would like to play with your changes and test different cases by myself.
Will do soon

...src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/KubernetesInfraModule.java Outdated Show resolved Hide resolved
...a/org/eclipse/che/workspace/infrastructure/kubernetes/wsplugins/ChePluginsVolumeApplier.java Show resolved Hide resolved
...t/src/main/java/org/eclipse/che/workspace/infrastructure/openshift/OpenShiftInfraModule.java Outdated Show resolved Hide resolved
...a/org/eclipse/che/workspace/infrastructure/kubernetes/wsplugins/ChePluginsVolumeApplier.java Show resolved Hide resolved
...a/org/eclipse/che/workspace/infrastructure/kubernetes/wsplugins/ChePluginsVolumeApplier.java Outdated Show resolved Hide resolved
...a/org/eclipse/che/workspace/infrastructure/kubernetes/wsplugins/ChePluginsVolumeApplier.java Outdated Show resolved Hide resolved
@che-bot
Copy link
Contributor

che-bot commented Sep 24, 2019

E2E tests of Eclipse Che Multiuser on OCP has failed:

@che-bot
Copy link
Contributor

che-bot commented Sep 24, 2019

E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has failed:

@che-bot
Copy link
Contributor

che-bot commented Sep 24, 2019

E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has failed:

@amisevsk amisevsk mentioned this pull request Sep 24, 2019
Closed
@AndrienkoAleksandr
Apply proposed changes changes.
e556db2 
Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>
@AndrienkoAleksandr
Apply proposed changes + enhasments.
e320089 
Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>
amisevsk
amisevsk approved these changes Sep 25, 2019
View reviewed changes
Copy link
Contributor

@amisevsk amisevsk left a comment

Choose a reason for hiding this comment

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

LGTM apart from @sleshchenko's comments. Approving so that this PR can be merged once those are addressed.

Consider also updating default brokers to v0.21.0 to include the ephemeral support from eclipse-che/che-plugin-broker#74

@AndrienkoAleksandr
Reuse pvc claim from k8sEnv
92bbe31 
Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>
@AndrienkoAleksandr
Use released version che-plugin-broker.
2a4bc30 
Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>
@AndrienkoAleksandr
Merge branch 'master' of github.com:eclipse/che into pluginEphemeralV…
e0244c3 
…olumeFeature
@che-bot
Copy link
Contributor

che-bot commented Sep 25, 2019

E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has been successful:

  • build details
  • "che-server" docker image: maxura/che-server:14539

@che-bot
Copy link
Contributor

che-bot commented Sep 25, 2019

E2E tests of Eclipse Che Multiuser on OCP has failed:

@AndrienkoAleksandr
Use newer version che-init-plugin-broker
f035194 
Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>
@che-bot
Copy link
Contributor

che-bot commented Sep 26, 2019

E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has been successful:

  • build details
  • "che-server" docker image: maxura/che-server:14539

sleshchenko
sleshchenko approved these changes Sep 26, 2019
View reviewed changes
Copy link
Member

@sleshchenko sleshchenko left a comment

Choose a reason for hiding this comment

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

Great job! 👍

@che-bot
Copy link
Contributor

che-bot commented Sep 26, 2019

E2E tests of Eclipse Che Multiuser on OCP has failed:

@SkorikSergey
Copy link
Contributor

Selenium tests execution on Eclipse Che Multiuser on OCP (https://ci.codenvycorp.com/job/che-pullrequests-test-ocp/2431//Selenium_20tests_20report/) doesn't show any regression against this Pull Request.

@AndrienkoAleksandr AndrienkoAleksandr merged commit dd47af6 into master Sep 26, 2019
@AndrienkoAleksandr AndrienkoAleksandr deleted the pluginEphemeralVolumeFeature branch September 26, 2019 11:11
@che-bot che-bot 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 26, 2019
@che-bot che-bot added this to the 7.2.0 milestone Sep 26, 2019
This was referenced Oct 2, 2019
Closed
Merged
@sleshchenko sleshchenko mentioned this pull request Oct 13, 2020
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickboldt nickboldt nickboldt left review comments

@davidfestal davidfestal davidfestal approved these changes

@sleshchenko sleshchenko sleshchenko approved these changes

@SkorikSergey SkorikSergey SkorikSergey approved these changes

@amisevsk amisevsk amisevsk approved these changes

@l0rd l0rd Awaiting requested review from l0rd

@metlos metlos Awaiting requested review from metlos

@mshaposhnik mshaposhnik Awaiting requested review from mshaposhnik

@rhopp rhopp Awaiting requested review from rhopp

@ibuziuk ibuziuk Awaiting requested review from ibuziuk

Assignees
No one assigned
Labels
area/install Issues related to installation, including offline/air gap and initial setup kind/enhancement A feature request - must adhere to the feature request template.
Projects
None yet
Milestone
7.2.0
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@AndrienkoAleksandr @che-bot @nickboldt @sleshchenko @SkorikSergey @davidfestal @amisevsk