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

Support for the new way of specifying the workspace namespace in Che. #92

Conversation

metlos
Copy link
Contributor

@metlos metlos commented Oct 11, 2019

The default is still the current namespace if there is no oauth in the infra

With Openshift OAuth, the namespace is now <username>-che or
<username>-codeready depending on the che flavor.

Note that this change amounts to no workspace breakage even in case of upgrade from previous versions because of the workspace namespace resolution logic implemented in the che server.

The new logic (implemented in eclipse-che/che#14828) does the following when determining a workspace namespace during workspace start (not creation):

  1. Take the legacy che.infra.kubernetes.namespace or che.infra.openshift.project property (defaulting to <workspaceid> placeholder)
  2. See if a namespace with the above determined name exists, use it for deploying the workspace if it does
  3. If not, take the value of the new property che.infra.kubernetes.namespace.default (which is common for both k8s and openshift) and use the namespace determined by that.
  4. If the new property has no value, error out.
  5. store the target namespace in the workspace config so that we don't have to do the dance the next time the workspace is started.

This change, together with eclipse-che/che#14828 enables us to keep existing workspaces working in their original namespaces and use the new configuration for the new workspaces.

The only consequence is that users will see che workspaces potentially in multiple namespaces (but the workspaces will be able to access all their data, etc.).

For the pre-existing workspaces created using an older version of Che, this logic ensures correct namespace resolution after the upgrade to the latest version of operator and che in both the below cases:

No OAuth

Because there's no oauth, the workspaces will be still colocated in the operator's namespace and will be found during step 2. above.

OAuth

The existing workspaces will be in the namespaces called after their <workspaceid>. The above logic will find them in step 2. (previously, oauth meant namespace per workspace).

Depends on

eclipse-che/che#14828

@davidfestal
Copy link
Contributor

Are we sure we want to merge that before the 7.3 and CRW 2.0 ? I thought we would not change the default (=> PR eclipse-che/che#14828) before 7.3 ?

cc @l0rd

@davidfestal
Copy link
Contributor

I'm not against it, but would I'd like to be tested during an upgrade with the operator and already running workspaces, and with both scenarios (OAuth enabled / disabled). Maybe those tests were already done, then good :-)

BTW these tests should be done before merging the related Che PR (eclipse-che/che#14828) IMO.

@sleshchenko sleshchenko force-pushed the bug/14795-use-new-props-for-wrkspc-namespace branch from 12f6c8d to b3a22a2 Compare October 31, 2019 09:31
@sleshchenko
Copy link
Member

@davidfestal 7.3 is already released. I believe Lukas and me well-tested Che Server.
Could you approve this PR or you still have some concerns about it?

@davidfestal
Copy link
Contributor

Because this PR is changing the Go code and defaults, it would be included in CRW operator builds that are started from the master branch of the upstream repo.

Additionally the operator releasing process are a linear process. There is no simple way currently to issue an operator bug-fix release from a branch, due to how the CSVs + OperatorHub package work:
Each time we do an operator release, we have to merge back the release branch to master.

So I think this time we have to wait for CRW 2.0 to be released or at least frozen on a given generated docker image. Then we would be able to merge to master PRs that would not fit in CRW 2.0.

I assume that on your side, you can still test this PR by manually pushing the built operator docker image to a given tag, and overriding the operator image when you test it (in the operator deployment).
Or with chectl you can override the operator docker image afaik

@nickboldt @l0rd wdyt ? Am I missing some point ?

@sleshchenko sleshchenko changed the title Support for the new way of specifying the workspace namespace in Che. 🚧 Support for the new way of specifying the workspace namespace in Che. Oct 31, 2019
@sleshchenko
Copy link
Member

Added WIP status to avoid occasionally merge

@nickboldt
Copy link
Contributor

So I think this time we have to wait for CRW 2.0 to be released or at least frozen on a given generated docker image. Then we would be able to merge to master PRs that would not fit in CRW 2.0.

Or we produce a 2.x branch in this repo and I update my CRW operator build job to use 2.x branch.

Copy link
Contributor

@davidfestal davidfestal left a comment

Choose a reason for hiding this comment

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

Now we have a crw-2.0 branch from which we sync to the pkgs.devel product repo in order to build the CRW 2.0 artifacts.
So it seems we can now merge this on master.
cc @l0rd

@l0rd
Copy link
Contributor

l0rd commented Nov 12, 2019

👍 for merging

@metlos metlos changed the title 🚧 Support for the new way of specifying the workspace namespace in Che. Support for the new way of specifying the workspace namespace in Che. Nov 12, 2019
metlos and others added 2 commits November 12, 2019 15:23
The default is still the current namespace if there is no oauth in the infra

With Openshift OAuth, the namespace is now "<username>-che" or
"<username>-codeready" depending on the che flavor.
@sleshchenko sleshchenko force-pushed the bug/14795-use-new-props-for-wrkspc-namespace branch from 651c651 to 4d15f5d Compare November 12, 2019 13:32
@sleshchenko
Copy link
Member

I'm doing final tests before merging it.

@sleshchenko sleshchenko merged commit d121ea8 into eclipse-che:master Nov 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants