Skip to content

[registry-facade] Remove feature flag #3182

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

Merged
merged 3 commits into from
Feb 18, 2021
Merged

[registry-facade] Remove feature flag #3182

merged 3 commits into from
Feb 18, 2021

Conversation

csweichel
Copy link
Contributor

@csweichel csweichel commented Feb 12, 2021

This PR removes all variance surrounding registry-facade. Once this is merged, registry-facade is the sole way to start workspaces. To this end, we remove

  • the registry_facade feature flag and all variance it created in ws-manager
  • node-daemon entirely. The remaining few sysctls are now an init container on ws-daemon
  • Theia's static-server, because now that registry-facade is standard, so is blobserve.

How to test

  1. Start a workspace - if it starts, all is well

fixes #2655

@csweichel csweichel force-pushed the cw/remove-regfac-ff branch 3 times, most recently from 7d1a74f to 2266068 Compare February 12, 2021 11:55
@csweichel csweichel marked this pull request as ready for review February 12, 2021 12:02
and enable registry-facade by default.
Comment on lines -191 to -194
if ir.Config.Config.BlobServer == nil {
ir.handleRootWithoutBlobserve(route)
return
}
Copy link
Contributor

Choose a reason for hiding this comment

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

With the change in the ws-proxy-configmap.yaml above it's still possible that the blobServer config is missing (when blobserve component is disabled). Doesn't that conflict with removing this case here?

@corneliusludmann
Copy link
Contributor

I have also found a few lines with references to node-daemon. 😆

"theiaHostPath": "{{ .Values.components.nodeDaemon.theiaHostBasePath }}/theia/theia-{{ .Values.version }}",

This one breaks helm run when not using values.dev.yaml. This is because values.dev.yaml has:

# Use Hosts SSD for perf and to not spam the boot disk
nodeDaemon:
theiaHostBasePath: /mnt/disks/ssd0

And here are two self-hosted files with references to node-daemon. Don't know if you want to address it in this PR as well or create an issue for this:

nodeDaemon:
# Gitpod copies Theia to each node. This setting configures where Theia is copied to.
# We'll copy Theia to $theiaHostBasePath/theia/theia-$version
# The faster this location is (in terms of IO) the faster nodes will become available and the faster workspaces will start.
theiaHostBasePath: /mnt/disks/ssd0

nodeDaemon:
# Gitpod copies Theia to each node. This setting configures where Theia is copied to.
# We'll copy Theia to $theiaHostBasePath/theia/theia-$version
# The faster this location is (in terms of IO) the faster nodes will become available and the faster workspaces will start.
theiaHostBasePath: /mnt/disks/ssd0

Christian Weichel added 2 commits February 16, 2021 07:50
to the node anymore. Instead, registry-facade serves the IDE.
The remaining sysctls were moved as init container to ws-daemon.
With registry-facade becoming the default, we can also
make blobserve standard.
@csweichel
Copy link
Contributor Author

Thank you for pointing those bits out. I've gone over the PR one more time and removed these bits.

Copy link
Contributor

@corneliusludmann corneliusludmann left a comment

Choose a reason for hiding this comment

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

Tested with the preview environment and self-hosted (k3s image). Both work fine.

@csweichel csweichel merged commit c47c7e1 into master Feb 18, 2021
@csweichel csweichel deleted the cw/remove-regfac-ff branch February 18, 2021 08:25
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.

Make registry-facade standard
2 participants