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

Add an ability to disable waiting for PVCs to become bound #13409

Merged
merged 1 commit into from
May 29, 2019
Merged

Add an ability to disable waiting for PVCs to become bound #13409

merged 1 commit into from
May 29, 2019

Conversation

sleshchenko
Copy link
Member

@sleshchenko sleshchenko commented May 23, 2019 •
edited by l0rd
Loading

What does this PR do?

This PR adds an ability to disable waiting for PVCs to become bound.
It's needed when PVCs are configured with volumeBindingMode=WaitForFirstConsumer .
It would be better to make Che Server detect WaitForFirstConsumer automatically and do not wait for bindings of PVC in such case, but there were faced some issues with implementing such feature and was decided to introduce this property to unblock Che Server on OpenShift v4.

CHE_INFRA_KUBERNETES_PVC_WAIT__BOUND is an environment variable that should be used to disable/enable waiting for PVCs binding.

What issues does this PR fix or reference?

#12889

Release Notes

N/A

Docs PR

A few words should be added about the special configuration of Che Server if StorageClass=WaitForFirstConsumer is used. But Docs changes will be made in a separate PR to avoid merge of this issue.

@sleshchenko sleshchenko added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. kind/task Internal things, technical debt, and to-do tasks to be performed. labels May 23, 2019
@sleshchenko sleshchenko self-assigned this May 23, 2019
@sleshchenko
Copy link
Member Author

@rhopp @nickboldt Guys, do you think we need a new issue to implement auto-detecting of StorageClass=WaitForFirstConsumer on Che Server side, or current solution is good enough?

@sleshchenko
Copy link
Member Author

ci-test

@sleshchenko

This comment has been minimized.

Sign in to view
@dmytro-ndp
Copy link
Contributor

dmytro-ndp commented May 23, 2019 •
edited
Loading

@sleshchenko: autodetect of StorageClass=WaitForFirstConsumer on Che Server side is what was expected from the issue, because CodeReady Workspaces doesn't have additional logic to handle such a case.
Adding new property CHE_INFRA_KUBERNETES_PVC_WAIT__BOUND=false will not commonly fix a problem, because there could be different PVs used in the same OpenShift, and StorageClass could be changed after installation. It also will complicate CRW deployment.

@nickboldt
Copy link
Contributor

I can't speak to the technical merits of this solution as I'm unfamiliar with the codebase.

But I provided a couple small nitpicks about console messages. And overall the solution looks reasonable, but IANASME. :) (I am not a subject matter expert.)

@che-bot
Copy link
Contributor

che-bot commented May 23, 2019

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:13409
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@sleshchenko sleshchenko mentioned this pull request May 24, 2019
Closed
@SkorikSergey
Copy link
Contributor

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

@sleshchenko
Copy link
Member Author

sleshchenko commented May 24, 2019 •
edited
Loading

This is an easy fix that should help to workaround an issue with waitForFirstConsumer and I believe it works.
I'm already started another investigation/work on how Che Server can handle this case by itself. As far as I see, Che Server is not able to check default or configured StorageClass configuration and the only way to fix the issue is to create a tooling pod, that will do nothing, like echo hello, but will be the first consumer for workspaces' PVCs. Hopefully, we'll have another alternative PR soon.
cc @nickboldt @dmytro-ndp

P.S. Note that with such a solution we will create an additional pod (light one I believe) on each first start of workspace (only when PVCs for workspace are created, during second start they should already exist)

dmytro-ndp
dmytro-ndp approved these changes May 24, 2019
View reviewed changes
Copy link
Contributor

@dmytro-ndp dmytro-ndp left a comment

Choose a reason for hiding this comment

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

This PR is correct workaround for downstream issue https://issues.jboss.org/browse/CRW-206 taking into a account disadvantages #13409 (comment)

@dmytro-ndp
Copy link
Contributor

Can't check fixup, because had a problem on OCP 4.1.rc7:
Screenshot from 2019-05-28 07-32-26
The was https://codeready-workspaces-jenkins.rhev-ci-vms.eng.rdu2.redhat.com/job/pre-release-upstream-che-e2e-tests-against-OCP-rebuild-che/ used to deploy CRW.
@rhopp could you, please, take a look?

@sleshchenko
Copy link
Member Author

rebased against a master without any change. IMHO no needs to rerun selenium tests. Will do manual testing before the merge.

@sleshchenko
Add an ability to disable waiting for PVCs to become bound
0abd052 
Signed-off-by: Sergii Leshchenko <sleshche@redhat.com>
@rhopp
Copy link
Contributor

rhopp commented May 29, 2019

I've just tried this on OCP 4.1 and everything works as expected - though I've tried that with image before your rebase.

@sleshchenko
Copy link
Member Author

ci-test

@che-bot
Copy link
Contributor

che-bot commented May 29, 2019

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:13409
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@sleshchenko sleshchenko merged commit 3bc9fe7 into eclipse-che:master May 29, 2019
@sleshchenko sleshchenko deleted the optionalPVCWaiting branch May 29, 2019 12:24
nickboldt pushed a commit that referenced this pull request May 29, 2019
@sleshchenko @nickboldt
Add an ability to disable waiting for PVCs to become bound (#13409)
b59b541 
Signed-off-by: Sergii Leshchenko <sleshche@redhat.com>
@nickboldt
Copy link
Contributor

Cherrypicked to 6.19.x branch where we need this for 6.19.5.

@amisevsk amisevsk mentioned this pull request Aug 22, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rhopp rhopp rhopp left review comments

@dmytro-ndp dmytro-ndp dmytro-ndp approved these changes

@nickboldt nickboldt nickboldt left review comments

@amisevsk amisevsk amisevsk approved these changes

@mshaposhnik mshaposhnik mshaposhnik approved these changes

@skabashnyuk skabashnyuk skabashnyuk approved these changes

@ibuziuk ibuziuk ibuziuk approved these changes

@l0rd l0rd l0rd approved these changes

@metlos metlos Awaiting requested review from metlos

Assignees

@sleshchenko sleshchenko

Labels
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.