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

Use che.infrastructure.kubernetes.namespace.default during workspace namespace resolution #14828

Merged
merged 27 commits into from
Oct 23, 2019
Merged

Use che.infrastructure.kubernetes.namespace.default during workspace namespace resolution #14828

merged 27 commits into from
Oct 23, 2019

Conversation

metlos
Copy link
Contributor

@metlos metlos commented Oct 9, 2019 •
edited
Loading

What does this PR do?

This PR implements a quick and dirty way of implementing the namespace resolution according to the following rules specified in #14376 (comment).

This is a very minimal implementation that is (hopefully) going to change drastically in the near future once we add the ability for the user to actually select the namespace for a workspace.

Because the implementation tries to be very localized and minimal if mightily hacky, it deviates from the proposed solution in some minor details. Namely:

  • the name of the namespace is only stored in the workspace attributes on workspace start. The selection is NOT done during workspace creation. This is because to be able to do that we would have to build a whole new abstraction above the namespaces to make them infrastructure agnostic so that the workspacemanager can make the assignment.
  • The whole thing falls down if allow_user_defined is true, because we actually don't support that functionality yet. There's a warning emitted at start up time about that.
  • MOST IMPORTANTLY - as long as the namespace specified using the legacy che.infra.kubernetes.namespace or che.infra.openshift.project properties exists, the new property che.infra.kubernetes.namespace.default is completely ignored and even the new workspaces are deployed into the namespace/project specified by the legacy properties. This is the consequence of the minimal amount of change we wanted in this PR and this restriction will only be lifted once we implement user-defined target namespace of workspaces. At the same time, we think that introducing this kind of change at this point in time will enable smoother upgrade as we progress with the 7.x releases and people start to adopt Che 7.

What issues does this PR fix or reference?

#14376

@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/task Internal things, technical debt, and to-do tasks to be performed. labels Oct 9, 2019
@che-bot
Copy link
Contributor

che-bot commented Oct 9, 2019

E2E tests of Eclipse Che Multiuser on OCP has failed:

@che-bot
Copy link
Contributor

che-bot commented Oct 9, 2019

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

sleshchenko
sleshchenko reviewed Oct 9, 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, is OK for me.
Let me know when it is not WIP anymore, will do final review and test/play with changes locally.

...rg/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespaceFactory.java Outdated Show resolved Hide resolved
@metlos metlos changed the title 🚧 Use che.infrastructure.kubernetes.namespace.default during workspace namespace resolution Use che.infrastructure.kubernetes.namespace.default during workspace namespace resolution Oct 10, 2019
@che-bot
Copy link
Contributor

che-bot commented Oct 10, 2019

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

@che-bot
Copy link
Contributor

che-bot commented Oct 10, 2019

E2E tests of Eclipse Che Multiuser on OCP has failed:

@che-bot
Copy link
Contributor

che-bot commented Oct 10, 2019

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

@sleshchenko
Copy link
Member

ci-build

@che-bot
Copy link
Contributor

che-bot commented Oct 10, 2019

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

@che-bot
Copy link
Contributor

che-bot commented Oct 10, 2019

E2E tests of Eclipse Che Multiuser on OCP has failed:

@che-bot
Copy link
Contributor

che-bot commented Oct 10, 2019

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

@che-bot
Copy link
Contributor

che-bot commented Oct 10, 2019

E2E tests of Eclipse Che Multiuser on OCP has failed:

@che-bot
Copy link
Contributor

che-bot commented Oct 11, 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 Oct 11, 2019

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

@metlos metlos mentioned this pull request Oct 11, 2019
Merged
@metlos
Copy link
Contributor Author

metlos commented Oct 11, 2019

crw-ci-test

@che-bot
Copy link
Contributor

che-bot commented Oct 11, 2019

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

sleshchenko
sleshchenko reviewed Oct 11, 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.

The code LGTM

Approve will come after local testing of the changes

...rg/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespaceFactory.java Show resolved Hide resolved
@metlos
Rename isManagingNamespaces -> isManagingNamespace
dda0993 
& isCreatingNamespaces -> isCreatingNamespace.

Signed-off-by: Lukas Krejci <lkrejci@redhat.com>
@che-bot
Copy link
Contributor

che-bot commented Oct 21, 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 Oct 21, 2019

E2E tests of Eclipse Che Multiuser on OCP has failed:

@metlos
Merge remote-tracking branch 'upstream/master' into bug/14795-use-new…
b936f18 
…-props-for-wrkspc-namespace

Signed-off-by: Lukas Krejci <lkrejci@redhat.com>
@metlos
Clarified the logic in isManagingNamespace, updated the javadoc.
26bacf8 
Signed-off-by: Lukas Krejci <lkrejci@redhat.com>
@metlos
Include the original error message when evaluating the namespace name…
1663ab5 
… fails

Signed-off-by: Lukas Krejci <lkrejci@redhat.com>
@che-bot
Copy link
Contributor

che-bot commented Oct 22, 2019

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

@che-bot
Copy link
Contributor

che-bot commented Oct 22, 2019

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

@che-bot
Copy link
Contributor

che-bot commented Oct 22, 2019

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

@sleshchenko sleshchenko self-requested a review October 22, 2019 12:25
@sleshchenko sleshchenko dismissed their stale review October 22, 2019 12:26

A lot of changes happened after my latest approval. I wish redo review and test

@metlos
javadoc clarifications, simplification of isCreatingNamespace()
5d530b1 
Signed-off-by: Lukas Krejci <lkrejci@redhat.com>
@metlos
Improve error messages and logging.
639e989 
Signed-off-by: Lukas Krejci <lkrejci@redhat.com>
@metlos
Formatting.
1bcfc51 
Signed-off-by: Lukas Krejci <lkrejci@redhat.com>
@che-bot
Copy link
Contributor

che-bot commented Oct 22, 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 Oct 22, 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 Oct 22, 2019

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

@che-bot
Copy link
Contributor

che-bot commented Oct 22, 2019

E2E tests of Eclipse Che Multiuser on OCP has failed:

@metlos
Copy link
Contributor Author

metlos commented Oct 23, 2019

ci-test

sleshchenko
sleshchenko reviewed Oct 23, 2019
View reviewed changes
...rg/eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespaceFactory.java
// if it doesn't, we're using the new logic.
return checkNamespaceExists(
resolveLegacyNamespaceName(EnvironmentContext.getCurrent().getSubject(), workspaceId))
|| newLogic;
Copy link
Member

Choose a reason for hiding this comment

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

we use || and it means that is newLogic is true - then it does not matter if namespace exists or not.
It makes me thinking that something is wrong. Please double check that it's correct

Copy link
Member

@sleshchenko sleshchenko Oct 23, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Seems we can override it in the following way:

  public boolean isManagingNamespace(String workspaceId) throws InfrastructureException {
    // the new logic is quite simple.
    boolean isNewNamespaceManaged =
        defaultNamespaceName != null && defaultNamespaceName.contains(WORKSPACEID_PLACEHOLDER);
    
    if (isNewNamespaceManaged) {
      return true;
    }

    boolean legacyHasWorkspacePlaceholder = isNullOrEmpty(legacyNamespaceName) || legacyNamespaceName.contains(WORKSPACEID_PLACEHOLDER);
    return legacyHasWorkspacePlaceholder && checkNamespaceExists(resolveLegacyNamespaceName(EnvironmentContext.getCurrent().getSubject(), workspaceId));
  }

Not sure if it's simpler for understanding but at least conditions seem clearer.

@che-bot
Copy link
Contributor

che-bot commented Oct 23, 2019

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

sleshchenko
sleshchenko approved these changes Oct 23, 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.

👍

@metlos metlos merged commit 7d56354 into eclipse-che:master Oct 23, 2019
@che-bot che-bot added this to the 7.4.0 milestone Oct 23, 2019
@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 Oct 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amisevsk amisevsk amisevsk left review comments

@sleshchenko sleshchenko sleshchenko approved these changes

@skabashnyuk skabashnyuk skabashnyuk approved these changes

@ibuziuk ibuziuk Awaiting requested review from ibuziuk

@l0rd l0rd Awaiting requested review from l0rd

@nickboldt nickboldt Awaiting requested review from nickboldt

@rhopp rhopp Awaiting requested review from rhopp

@mshaposhnik mshaposhnik Awaiting requested review from mshaposhnik

Assignees
No one assigned
Labels
kind/task Internal things, technical debt, and to-do tasks to be performed.
Projects
None yet
Milestone
7.4.0
Development

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