[wsman-bridge] Introduce admission preferences #6164

csweichel · 2021-10-11T22:45:34Z

Description

This PR adds admission preferences to the workspace cluster selection mechanism. While admission constraints limit the set of clusters a workspace can be started on, admission preferences express a preference for a set of cluster but still allow fall-back to clusters that do not match that preference.

With this change, we first try and choose a cluster from the preferred clusters (green circle below) following the usual score mechanism. If there are no preferred cluster, we'll fall back to all clusters matching the admission constraints.

Use-cases this addition is helpful for:

give paying customers their own cluster (original motivation for this change): this PR introduces a single admission preference named user-level. This in-descriptive level can be used for a variety of purposes, e.g. express "paying customer" status. In a follow-up PR (once I've figured out if a user is actually paying), we would set this level to paying-customer. All workspace cluster with an admission preference user-level=paying-customer and the (new) admission constraint has-user-level=paying-customer would then become exclusive for paying users.
explicitly select a cluster to start a workspace one: suppose we want to test a new workspace cluster, right now we add the has-permission=workspace-cluster constraint and a high score, hoping that after so many attempts we land on the new cluster. We could introduce a prefix context parser which takes a cluster name, and add an admission preference for that name.
hyper-local workspace clusters: if we added an admission preference with network locality or some other information we could use to predict quality of service, we could give regional/likely performant clusters preference. The admission preference need not be static, but can be negotiated on a per-user basis.

How to test

Release Notes

Add admission constraints to support fine-grained cluster selection

csweichel · 2021-10-11T22:51:26Z

/approve no-issue

aledbf · 2021-10-12T00:49:41Z

/werft run

👍 started the job as gitpod-build-cw-admission-prefs.2

aledbf · 2021-10-12T01:09:18Z

/werft run

👍 started the job as gitpod-build-cw-admission-prefs.3

csweichel · 2021-10-12T06:31:31Z

/werft run

👍 started the job as gitpod-build-cw-admission-prefs.4

meysholdt · 2021-10-12T07:39:08Z

great stuff!

explicitly select a cluster to start a workspace on

sounds like #4809

hyper-local workspace clusters

sounds like #5596

components/gitpod-db/src/typeorm/entity/db-workspace-cluster.ts

geropl · 2021-10-12T11:36:07Z

components/gitpod-db/src/typeorm/migration/1633989579520-AdmissionPreference.ts

+    public async up(queryRunner: QueryRunner): Promise<any> {
+        if (await tableExists(queryRunner, "d_b_workspace_cluster")) {
+            if (!(await columnExists(queryRunner, "d_b_workspace_cluster", "admissionPreferences"))) {
+                await queryRunner.query("ALTER TABLE d_b_workspace_cluster ADD COLUMN admissionPreferences TEXT");


Why not make this a JSON column?

to follow the same pattern as for admissionConstraints

geropl · 2021-10-12T11:40:51Z

components/ws-manager-api/typescript/src/client-provider.ts

+
+function admissionPreferencesFilter(user: ExtendedUser): (c: WorkspaceClusterWoTLS) => boolean {
+    return (c: WorkspaceClusterWoTLS) => {
+        if (!c.admissionPreferences) {


nit: superfluous

princerachit · 2021-10-13T05:37:37Z

components/gitpod-db/src/typeorm/migration/1633989579520-AdmissionPreference.ts

+ * See License-AGPL.txt in the project root for license information.
+ */
+
+import {MigrationInterface, QueryRunner} from "typeorm";


nit: formatting, no space

princerachit · 2021-10-13T05:51:25Z

components/ws-manager-bridge-api/cluster-service.proto

@@ -46,6 +47,13 @@ message AdmissionConstraint {
  oneof constraint {
    FeaturePreview has_feature_preview = 1;
    HasPermission has_permission = 2;
+    string has_user_level = 3;


Any specific reason not to have this as a message type instad of a string?

Yes. The message was a bad choice to beging with :) but for now we're stuck with it for backwards compatibility reasons.

princerachit · 2021-10-13T06:04:45Z

Overall lgtm other than a few clarifying questions. Also, there are no unit tests, given that we don't have them for any other file this is not a blocker, but we, should definitely discuss this.

geropl · 2021-10-13T07:54:45Z

components/ws-manager-api/typescript/src/client-provider.ts

        let availableCluster = await this.getAvailableStartCluster(user, workspace, instance);
        if (!!exceptInstallations) {
            availableCluster = availableCluster.filter(c => !exceptInstallations?.includes(c.name));
        }
-        const chosenCluster = chooseCluster(availableCluster);
+
+        let chosenCluster = chooseCluster(availableCluster.filter(admissionPreferencesFilter(user)))


components/ws-manager-api/typescript/src/client-provider.ts

geropl · 2021-10-13T07:58:13Z

/werft run

👍 started the job as gitpod-build-cw-admission-prefs.14

geropl · 2021-10-14T07:47:20Z

/werft run

👍 started the job as gitpod-build-cw-admission-prefs.15

geropl · 2021-10-14T08:43:14Z

components/gitpod-db/src/typeorm/entity/db-workspace-cluster.ts

+                    return JSON.stringify(value);
+                },
+                from(value: any): any {
+                    if (!value) {
                        return undefined;


Should return defaultValue.

geropl · 2021-10-14T08:46:27Z

dev/gpctl/cmd/clusters-list.go

 {{- range .Status }}
-{{ .Name }}	{{ .Url }}	{{ .Static }}	{{ .State }}	{{ .Score }}	{{ .Governed }}	{{ .AdmissionConstraints -}}
+{{ .Name }}	{{ .Url }}	{{ .Static }}	{{ .State }}	{{ .Score }}	{{ .Governed }}	{{ .AdmissionConstraint -}}	{{ .AdmissionPreference -}}


nit: the {{ .AdmissionConstraint -}} should be {{ .AdmissionConstraint }} (without the dash) to avoid the output to be skewed:

geropl · 2021-10-14T08:52:47Z

components/ws-manager-bridge/src/cluster-service-server.ts

@@ -164,8 +167,8 @@ export class ClusterService implements IClusterServiceServer {
                if (call.request.hasCordoned()) {
                    cluster.state = mapCordoned(req.cordoned);
                }
-                if (call.request.hasAdmissionConstraints()) {


Bumped into this while testing: A few lines up (L158/159) the error ALREADY_EXISTS is wrong. Not introduce with this PR, but worth fixing.

geropl

Finally found the time for testing: works as advertised!

There are a couple of things which are worth fixing, but am fine with doing those in a followup PR:

👍

roboquat · 2021-10-14T08:59:01Z

LGTM label has been added.

Git tree hash: 929e1e57a57fb0be37107c119e84af17325dd1ed

roboquat · 2021-10-14T08:59:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: csweichel, geropl

Associated issue requirement bypassed by: csweichel

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~components/gitpod-db/OWNERS~~ [geropl]
~~components/gitpod-protocol/OWNERS~~ [geropl]
~~components/ws-manager-api/OWNERS~~ [csweichel]
~~components/ws-manager-bridge/OWNERS~~ [geropl]
~~components/ws-manager-bridge-api/OWNERS~~ [csweichel,geropl]
~~dev/gpctl/OWNERS~~ [csweichel]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

roboquat added release-note do-not-merge/work-in-progress labels Oct 11, 2021

roboquat requested review from princerachit and rl-gitpod October 11, 2021 22:45

roboquat added the size/XXL label Oct 11, 2021

csweichel requested review from geropl and removed request for rl-gitpod October 11, 2021 22:51

csweichel force-pushed the cw/admission-prefs branch from bc1ea6a to f956e3a Compare October 12, 2021 07:30

csweichel force-pushed the cw/admission-prefs branch 4 times, most recently from 7fabcce to 3d3f2a1 Compare October 12, 2021 11:20

csweichel marked this pull request as ready for review October 12, 2021 11:20

roboquat removed the do-not-merge/work-in-progress label Oct 12, 2021

geropl reviewed Oct 12, 2021

View reviewed changes

components/gitpod-db/src/typeorm/entity/db-workspace-cluster.ts Show resolved Hide resolved

geropl reviewed Oct 12, 2021

View reviewed changes

csweichel force-pushed the cw/admission-prefs branch from 3d3f2a1 to 2e22630 Compare October 12, 2021 11:50

[wsman-bridge] Introduce admission preferences

d55abfd

csweichel force-pushed the cw/admission-prefs branch from 2e22630 to d55abfd Compare October 12, 2021 11:59

csweichel requested a review from geropl October 12, 2021 19:33

princerachit reviewed Oct 13, 2021

View reviewed changes

geropl reviewed Oct 13, 2021

View reviewed changes

components/ws-manager-api/typescript/src/client-provider.ts Show resolved Hide resolved

geropl reviewed Oct 14, 2021

View reviewed changes

geropl approved these changes Oct 14, 2021

View reviewed changes

roboquat assigned geropl Oct 14, 2021

roboquat added the lgtm label Oct 14, 2021

roboquat added the approved label Oct 14, 2021

roboquat merged commit 9605e0b into main Oct 14, 2021

roboquat deleted the cw/admission-prefs branch October 14, 2021 09:00

csweichel mentioned this pull request Oct 14, 2021

Tie "paying customer" to user level admission preference #6212

Closed

csweichel mentioned this pull request Jan 6, 2022

Support more regional workspace cluster #7490

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[wsman-bridge] Introduce admission preferences #6164

[wsman-bridge] Introduce admission preferences #6164

csweichel commented Oct 11, 2021 •

edited

Loading

csweichel commented Oct 11, 2021

aledbf commented Oct 12, 2021 •

edited by werft-gitpod-dev-com bot

Loading

aledbf commented Oct 12, 2021 •

edited by werft-gitpod-dev-com bot

Loading

csweichel commented Oct 12, 2021 •

edited by werft-gitpod-dev-com bot

Loading

meysholdt commented Oct 12, 2021

geropl Oct 12, 2021

csweichel Oct 12, 2021

geropl Oct 12, 2021

princerachit Oct 13, 2021

princerachit Oct 13, 2021

csweichel Oct 13, 2021

princerachit commented Oct 13, 2021

geropl Oct 13, 2021

geropl commented Oct 13, 2021 •

edited by werft-gitpod-dev-com bot

Loading

geropl commented Oct 14, 2021 •

edited by werft-gitpod-dev-com bot

Loading

geropl Oct 14, 2021

geropl Oct 14, 2021

geropl Oct 14, 2021

geropl left a comment

roboquat commented Oct 14, 2021

roboquat commented Oct 14, 2021

[wsman-bridge] Introduce admission preferences #6164

[wsman-bridge] Introduce admission preferences #6164

Conversation

csweichel commented Oct 11, 2021 • edited Loading

Description

How to test

Release Notes

csweichel commented Oct 11, 2021

aledbf commented Oct 12, 2021 • edited by werft-gitpod-dev-com bot Loading

aledbf commented Oct 12, 2021 • edited by werft-gitpod-dev-com bot Loading

csweichel commented Oct 12, 2021 • edited by werft-gitpod-dev-com bot Loading

meysholdt commented Oct 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

princerachit commented Oct 13, 2021

Choose a reason for hiding this comment

geropl commented Oct 13, 2021 • edited by werft-gitpod-dev-com bot Loading

geropl commented Oct 14, 2021 • edited by werft-gitpod-dev-com bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geropl left a comment

Choose a reason for hiding this comment

roboquat commented Oct 14, 2021

roboquat commented Oct 14, 2021

csweichel commented Oct 11, 2021 •

edited

Loading

aledbf commented Oct 12, 2021 •

edited by werft-gitpod-dev-com bot

Loading

aledbf commented Oct 12, 2021 •

edited by werft-gitpod-dev-com bot

Loading

csweichel commented Oct 12, 2021 •

edited by werft-gitpod-dev-com bot

Loading

geropl commented Oct 13, 2021 •

edited by werft-gitpod-dev-com bot

Loading

geropl commented Oct 14, 2021 •

edited by werft-gitpod-dev-com bot

Loading