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

Deploy registry only if needed #45

Merged
merged 8 commits into from
Apr 15, 2020

Conversation

JPinkney
Copy link
Contributor

@JPinkney JPinkney commented Apr 7, 2020

What does this PR do?

This PR creates an internal registry that holds the cloud shell meta. This makes it so that you won't have to deploy the plugin registry when just using cloud shell.

What issues does this PR fix or reference?

eclipse-che/che#16313

Is it tested? How?

Tested on crc with first setting env variable REGISTRY_ENABLED = false. As expected, the plugin registry wasn't deployed and when launching a cloud shell workspace it pulled from the internal registry and successfully started. When you try and start a workspace that uses theia, you will get an error saying something like could not retrieve meta.yaml from registry

Then I turned on REGISTRY_ENABLED = true and ran make deploy and the plugin registry was successfully deployed. Tried to start a cloud shell workspace and it worked. Then I tried to start python-sample.yaml and it worked

JPinkney added 2 commits April 7, 2020 13:36
Signed-off-by: Josh Pinkney <joshpinkney@gmail.com>
Signed-off-by: Josh Pinkney <joshpinkney@gmail.com>
@JPinkney JPinkney requested review from sleshchenko and amisevsk April 7, 2020 17:37
@JPinkney JPinkney self-assigned this Apr 7, 2020
Makefile Show resolved Hide resolved
pkg/internal_registry/registry.go Outdated Show resolved Hide resolved
Comment on lines 42 to 47
Command: []string{
"/go/bin/che-machine-exec",
"--static",
"/cloud-shell",
"--url",
"127.0.0.1:4444",
Copy link
Collaborator

@amisevsk amisevsk Apr 7, 2020

Choose a reason for hiding this comment

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

I put a json.MarshalIndent on the plugin meta we get from utils.GetPluginMeta and I see (formatted as yaml)

apiVersion: v2
spec:
  endpoints:
  - name: cloud-shell
    public: true
    targetPort: 4444
    attributes:
      cookiesAuthEnabled: "true"
      discoverable: "false"
      protocol: http
      secure: "true"
      type: ide
  containers:
  - name: che-machine-exec
    image: quay.io/eclipse/che-machine-exec:nightly
    env: []
    commands: []
    volumes: []
    ports:
    - exposedPort: 4444
    mountSources: false
    command: []
    args: []
  initContainers: []
  workspaceEnv: []
  extensions: []
id: eclipse/cloud-shell/nightly
name: cloud-shell
displayName: Cloud Shell Editor
publisher: eclipse
type: Che Editor
description: Cloud Shell provides an ability to use terminal widget like an editor.
version: nightly
title: Cloud Shell Editor
icon: https://www.eclipse.org/che/images/logo-eclipseche.svg

Copy link
Collaborator

Choose a reason for hiding this comment

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

The command in your meta.yaml was added in eclipse-che/che-plugin-registry@74fcb49, which is not included in the 7.8.0 release of the registry. I don't know if it matters significantly, but this does bring up the issue of how we maintain the offline registry (we can discuss in a separate issue)

Copy link
Collaborator

Choose a reason for hiding this comment

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

The command actually prevents machine-exec from listening outside the local network, which means cloud shell won't work with basic routing. It's not an issue for openshift-oauth as the proxy pod is included in the deployment and forwards requests.

Created issue eclipse-che/che#16568 to discuss this.

Copy link
Member

Choose a reason for hiding this comment

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

Probably we also may need some endpoint diagnostics tools in workspace controller, like now with nightly plugin registry we'll get Running but not working workspace
Screenshot_20200408_124846

@JPinkney JPinkney force-pushed the deploy-registry-only-if-needed branch from 609af25 to 2f11753 Compare April 7, 2020 20:04
JPinkney added 3 commits April 7, 2020 16:05
Signed-off-by: Josh Pinkney <joshpinkney@gmail.com>
Signed-off-by: Josh Pinkney <joshpinkney@gmail.com>
Signed-off-by: Josh Pinkney <joshpinkney@gmail.com>
Copy link
Collaborator

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

Tested cloud shell with basic routing and openshift-oauth + webhooks on crc, and it works great.

LGTM

@sleshchenko sleshchenko requested review from davidfestal and l0rd April 8, 2020 07:55
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.

Good job!

I think it's better to have internal plugin registries configuration as external files but I'm OK with addressing it in a separate PR since merging this PR would make my life easier with customizing machine-exec image to my built version)

pkg/internal_registry/registry.go Show resolved Hide resolved
@JPinkney JPinkney force-pushed the deploy-registry-only-if-needed branch from 11b4bc8 to 6d5a512 Compare April 8, 2020 16:55
Signed-off-by: Josh Pinkney <joshpinkney@gmail.com>
Signed-off-by: Josh Pinkney <joshpinkney@gmail.com>
Signed-off-by: Josh Pinkney <joshpinkney@gmail.com>
Copy link
Collaborator

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

Continues to LGTM

Edit: mean this review to be an "approve"

@sleshchenko sleshchenko merged commit 657f598 into devfile:master Apr 15, 2020
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.

3 participants