-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Set workspace class based on user preference #11313
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
Conversation
40606c7
to
bfeb6ba
Compare
/werft run 👍 started the job as gitpod-build-fo-use-classes.11 |
/werft run with-preview with-clean-slate-deployment 👍 started the job as gitpod-build-fo-use-classes.12 |
@geropl PTAL |
@@ -237,6 +237,22 @@ func configmap(ctx *common.RenderContext) ([]runtime.Object, error) { | |||
// default limit for all cloneURLs | |||
"*": 50, | |||
}, | |||
WorkspaceClasses: []WorkspaceClass{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the config we want to deliver for Self-Hosted? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think we would want to have the two workspace classes as default for Self-Hosted. It's an advanced feature and the initial installation should be as simple as possible imho.
Would it be hard to have the default for self-hosted only be the standard?
cc @lucasvaltl as he is responsible for defining the self-hosted offering
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the idea was to deliver what we currently do for SH: one "default" class. This config should then be exposed under experimental.webapp.workspaceClasses
, and the concrete values set here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@geropl agreed, thanks for linking to the concrete config in ops
, too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah default for self-hosted for now should be a single workspace class - to keep installation simple. Right now even that class is not made obvious - but is rather just a default that sets a request but no limits.
We should however think about how to properly allow for configuration of this (and #10805 is likely a better place to discuss this). Fwiw, I don't think it makes sense to make this part of the installation config - this might be something a user wants to change over time, specifically a user that is not the "installer persona" but rather an "project configurer" / an admin. It is likely better to expose this in the admin UI or team settings page.
} | ||
|
||
if (!workspaceClass) { | ||
workspaceClass = this.config.workspaceClasses.find((cl) => cl.isDefault)?.id ?? ""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel this should be in a function inside a namesapce WorkspaceClasses
so that we can test it etc.
@Furisto Let me find a couple of minutes and I can provide a commit to show what I mean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Furisto Here is a commit showing what I mean: 7412eb0
I think it make sense to incorporate that commit + exposing of that config under "experimental" into this PR, and then we should be fine to merge. ✔️
☁️ Ideally we should also have tests for those functions.
@geropl PTAL |
@@ -210,6 +211,14 @@ type UsageConfig struct { | |||
CreditsPerMinuteByWorkspaceClass map[string]float64 `json:"creditsPerMinuteByWorkspaceClass"` | |||
} | |||
|
|||
type WebAppWorkspaceClass struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM (as discussed), tested and work as expected. 🎉
Note that:
- I could not test "g1-large" because preview envs now contain a single class only ✔️ (the UI still allows to select two classes, which we should clean up, by making it dynamic based on the config, in a follow up PR, before the SH release as discussed here /cc @lucasvaltl )
- I did test default -> g1-standard after enabling the feature flag and the other scenarios ✔️
- on workspace stop, I always got this error:
container workspace completed; containers of a workspace pod are not supposed to do that. Reason:
But I assume it's independent of this PR
Really looking forward to have this in prod! 🧡
- Use latest workspace instance to set workspace class - Add more detailed configuration for workspace classes - Make workspace classes configurable in installer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, I tried this on a self-hosted
instance with default settings (using local-preview
. This part of the build so not that hard to test. Just using the relevant build image should be enough)
-
But setting them/changing them does not seem to do anything and always give me a
default
workspace class
/ # kubectl describe pod -l workspaceType=regular | grep workspaceClass
gitpod.io/workspaceClass=default
gitpod.io/workspaceClass=default
So, We should not even be having the preferences option then right? seems like they seem to be set in the UI https://github.com/gitpod-io/gitpod/blob/main/components/dashboard/src/settings/selectClass.tsx#L50. Shouldn't it instead be based on a config?
This could be a separate PR. Approving this hence.
/hold just in case.
@Pothulapati That's right. There will be a separate PR for dynamically displaying the workspace classes. |
/unhold |
Description
This PR allows user to select their preferred workspace class and start a workspace with it. The feature is hidden behind a feature flag which is segmented to only activate the feature for members of gitpod. This means that WebApp can deploy this feature without having to wait for team workspace to configure the new workspace classes in the workspace clusters.
Currently there are 4 workspace classes that are supported:
Default and gitpod-internal-xl are the old internal workspace classes that will be eventually removed from the code. G1 are the new general purpose workspace classes that we are introducing.
Related Issue(s)
Fixes ##10843
How to test
Release Notes
Werft options: