Allow for branch name lengths of 45 #8692

mads-hartmann · 2022-03-09T15:06:28Z

Description

This relaxes the branch name restriction from 20 characters to 45. This is achieved by constructing the preview name based on the first 10 characters of the branch name and then the first 10 characters of the hashed value of the branch name if the branch name is longer than 20 characters. Once we move to Harvester we can likely increase the preview name to be longer than 20 characters but still not arbitrarily long - see this issue and the comments therein for more details on where these restrictions are coming from. The purpose of making it longer than 20 characters would be to have more readable preview names - e.g. https://mads-a-lond78d730379.staging.gitpod-dev.com isn't the most readable thing in the world.

The 45 limit of the branch name is now based on an implementation detail in Werft. The Werft Job name is used as the name of the pod. Kubernetes has a pod name limit of 63 characters. For the build job we use 13 chars for "gitpod-build-" prefix and 5 for the "." ending (as there's a max supported of 9999 jobs for the same branch) - see Werft source - this leaves 45 characters for the branch name. We could decide to just not care, as there would only be a collision if the first 45 characters are the same. For this PR I decided to care.

Some implementation details

This PR decouples the Werft job name from the preview name. The preview name is now computed on the full branch name and not the sanitized and shortened version that Werft provides (otherwise the hashed value would only be based on the first 45 characters of the branch name which would reduce the effect it has on avoiding collisions). It also gives us the benefit that we control the preview name regardless of underlying changes to Werft.
Ensures that the preview name is generated a single place pr. language (ts, bash) - previously we computed the preview name from the branch name in quite a lot of places.
The delete-preview-environment-cron Werft has been updated to be able to deal with both the new and old preview name patterns (and I have added some minor improvements to it).

Some consequences of merging this PR

If you have a branch that was started before this PR was merged, and you rebase on main, you'll effectively get a "clean-slate-deployment" as the name of the preview environment will be different.

Related Issue(s)

Fixes https://github.com/gitpod-io/ops/issues/1252

How to test

I have verified that preview environments with both with and without VM
I have verified that scripts/branch-namespace.sh works correctly
I have verified that the VM-related scripts still work (install k3s kubeconfig, ssh, grafana, etc.)

Release Notes

NONE

Documentation

N/A

ArthurSens · 2022-03-09T15:53:14Z

I believe the build failure is unrelated to your changes(Ref), but let's retry after the quota problem is solved 🙂

codecov · 2022-03-10T09:16:34Z

Codecov Report

Merging #8692 (496b5e1) into main (53668f1) will decrease coverage by 1.13%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #8692      +/-   ##
==========================================
- Coverage   12.31%   11.17%   -1.14%     
==========================================
  Files          20       18       -2     
  Lines        1161      993     -168     
==========================================
- Hits          143      111      -32     
+ Misses       1014      880     -134     
+ Partials        4        2       -2

Flag	Coverage Δ
components-gitpod-cli-app	`11.17% <ø> (ø)`
components-local-app-app-darwin-amd64	`?`
components-local-app-app-darwin-arm64	`?`
components-local-app-app-linux-amd64	`?`
components-local-app-app-linux-arm64	`?`
components-local-app-app-windows-386	`?`
components-local-app-app-windows-amd64	`?`
components-local-app-app-windows-arm64	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
components/local-app/pkg/auth/auth.go
components/local-app/pkg/auth/pkce.go

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

This relaxes the branch name restriction from 20 characters to 45. This is achieved by constructing the preview name based on the first 10 characters of the branch name and then the first 10 characters of the hashed value of the branch name. All places that (to my knowledge) rely on the preview name has been updated. Fixes gitpod-io/ops#1252

mads-hartmann · 2022-03-10T12:57:37Z

Adding the hold label as I want to ask the teams if I have overlooked any places where we rely on the old on the branch name <-> preview environment name pattern

geropl · 2022-03-10T13:10:43Z

@mads-hartmann Thx for looking into this!

I just wonder if it's worth putting in this additional complexity when (I just learned that) VM-based previews BETA is just around the corner...? E.g., the property of deriving the name of the preview in your head is very nice to have. And If I have to mentally cut-off my branch names for just another 2-3 weeks until we're migrated completly, I personally gladly do so. 🙂

mads-hartmann · 2022-03-10T13:36:41Z

@geropl That was our initial hope as well but I decided to take this on for the following reasons:

Even though the VM beta is just around the corner we don't know when we'll flip the switch and make them the default. The goal is before the quarter is over, but we don't know ☺️
Even when VMs are the default we still have to support core-dev until everyone has migrated to the Installer as VMs don't support Helm-based installs.

So the 20 character limit restriction isn't going away soon (in all cases, at least)

I got a sense that this is extremely frustrating to a lot of people and the request keeps coming up (slack, issue) and is confusing people (slack). In the last 14 days it has broken 58 builds and affected 32 different branches (query).

While the PR touches quite a few files/places, I don't think it's adding complexity as such. It decouples the preview name from the Werft job name (which is a good thing IMO) and consolidates the "name generation" to a single place per language.

I agree that the preview names can end up being not very readable, but that's only if you use more than 20 chars. And once we have fully migrated to VMs and deleted core-dev we can relax that so we get 45 (or 43, I forgot) characters for the branch name in the preview environment ☺️

geropl

and then the first 10 characters of the hashed value of the branch name if the branch name is longer than 20 characters

@mads-hartmann I overlooked this, sorry. 🙈 With this there is no sense in arguing - I'd even say keep the behavior for later (just with 20->45) then! 🙏

corneliusludmann · 2022-03-10T13:59:22Z

We could decide to just not care, as there would only be a collision if the first 45 characters are the same.

I personally would vote for cutting the branch names to 45 characters and accept the risk of a collision. I would guess the risk is quite low. However, nothing that should hold this pull request back but only my 2 cents for future decisions.

Thanks a lot for this pull request! 😍

ArthurSens

Nice job! Awesome to see the attention you've put onto details to ensure backwards compatibility 🙂

I just have a couple of questions

ArthurSens · 2022-03-10T15:38:01Z

.werft/delete-preview-environments/delete-preview-environments-cron.ts

-    .then(() => werft.endAllSpans())
    .catch((err) => {
        werft.rootSpan.setStatus({
            code: SpanStatusCode.ERROR,
            message: err
        })
+    })
+    .finally(() => {
+        werft.phase("Flushing telemetry", "Flushing telemetry before stopping job")


Just to make sure, this isn't related to the proposed change, right?

Yeah, this was a tiny improvement we should make it the other jobs as well ☺️ Right now the log lines about flushing traces etc. are show up in whatever the phase was the last one in the job. With this change we introduce a new phase for the flushing so the logs become easier to read :)

And it was moved to "finally" as we want to flush in both the error and success cases :)

ArthurSens · 2022-03-10T15:38:48Z

.werft/delete-preview-environments/delete-preview-environments-cron.ts

+    // During the transition from the old preview names to the new ones we have to check for the existence of both the old or new
+    // preview name patterns before it is safe to delete a namespace.
+    const expectedPreviewEnvironmentNamespaces = new Set(branches.flatMap(branch => [parseBranch(branch), expectedNamespaceFromBranch(branch)]));


Nice safety check! That didn't cross my mind when I tried to implement this

ArthurSens · 2022-03-10T15:47:37Z

.werft/util/preview.ts

+ * Based on the current branch name this will compute the name of the associated
+ * preview environment.
+ *
+ * NOTE: This needs to produce the same result as the function in dev/preview/util/preview-name-from-branch.sh


To avoid drift, could we just call scripts/branch-namespace.sh wherever this function is called instead?

I'd prefer not to for the following reasons:

Currently the function doens't have any side effects. That's generally nice for utility functions as it means you can usually read the "main" script which uses the utility functions to understand what changes are made to your environment when you invoke it.

The branch-namespace.sh function is core-dev specific and this utility function is used both for VMs and core-dev preview environments

ArthurSens

Alright, LGTM!

roboquat added do-not-merge/work-in-progress release-note-none size/S labels Mar 9, 2022

roboquat added size/M and removed size/S labels Mar 10, 2022

roboquat added size/L and removed size/M labels Mar 10, 2022

mads-hartmann force-pushed the mads/a-long-branch-name-of-precisely-45-chars branch from b76ad64 to 630c3e3 Compare March 10, 2022 12:29

mads-hartmann force-pushed the mads/a-long-branch-name-of-precisely-45-chars branch from 630c3e3 to 2a525f6 Compare March 10, 2022 12:43

Dont hash if the branch is 20 chars or less

496b5e1

mads-hartmann marked this pull request as ready for review March 10, 2022 12:56

mads-hartmann requested a review from a team March 10, 2022 12:56

roboquat removed the do-not-merge/work-in-progress label Mar 10, 2022

github-actions bot added the team: devx label Mar 10, 2022

mads-hartmann added the do-not-merge/hold label Mar 10, 2022

mads-hartmann mentioned this pull request Mar 10, 2022

werft/job-config: Make previews from 20+ character branches possible #8403

Closed

geropl approved these changes Mar 10, 2022

View reviewed changes

ArthurSens reviewed Mar 10, 2022

View reviewed changes

mads-hartmann requested a review from ArthurSens March 11, 2022 08:15

ArthurSens approved these changes Mar 11, 2022

View reviewed changes

mads-hartmann removed the do-not-merge/hold label Mar 11, 2022

roboquat merged commit 4ab9e3a into main Mar 11, 2022

roboquat deleted the mads/a-long-branch-name-of-precisely-45-chars branch March 11, 2022 08:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow for branch name lengths of 45 #8692

Allow for branch name lengths of 45 #8692

mads-hartmann commented Mar 9, 2022 •

edited

Loading

ArthurSens commented Mar 9, 2022 •

edited by werft-gitpod-dev-com bot

Loading

codecov bot commented Mar 10, 2022 •

edited

Loading

mads-hartmann commented Mar 10, 2022

geropl commented Mar 10, 2022 •

edited

Loading

mads-hartmann commented Mar 10, 2022 •

edited

Loading

geropl left a comment

corneliusludmann commented Mar 10, 2022

ArthurSens left a comment

ArthurSens Mar 10, 2022

mads-hartmann Mar 11, 2022

mads-hartmann Mar 11, 2022

ArthurSens Mar 10, 2022

ArthurSens Mar 10, 2022

mads-hartmann Mar 11, 2022

ArthurSens left a comment

Allow for branch name lengths of 45 #8692

Allow for branch name lengths of 45 #8692

Conversation

mads-hartmann commented Mar 9, 2022 • edited Loading

Description

Related Issue(s)

How to test

Release Notes

Documentation

ArthurSens commented Mar 9, 2022 • edited by werft-gitpod-dev-com bot Loading

codecov bot commented Mar 10, 2022 • edited Loading

Codecov Report

mads-hartmann commented Mar 10, 2022

geropl commented Mar 10, 2022 • edited Loading

mads-hartmann commented Mar 10, 2022 • edited Loading

geropl left a comment

Choose a reason for hiding this comment

corneliusludmann commented Mar 10, 2022

ArthurSens left a comment

Choose a reason for hiding this comment

ArthurSens Mar 10, 2022

Choose a reason for hiding this comment

mads-hartmann Mar 11, 2022

Choose a reason for hiding this comment

mads-hartmann Mar 11, 2022

Choose a reason for hiding this comment

ArthurSens Mar 10, 2022

Choose a reason for hiding this comment

ArthurSens Mar 10, 2022

Choose a reason for hiding this comment

mads-hartmann Mar 11, 2022

Choose a reason for hiding this comment

ArthurSens left a comment

Choose a reason for hiding this comment

mads-hartmann commented Mar 9, 2022 •

edited

Loading

ArthurSens commented Mar 9, 2022 •

edited by werft-gitpod-dev-com bot

Loading

codecov bot commented Mar 10, 2022 •

edited

Loading

geropl commented Mar 10, 2022 •

edited

Loading

mads-hartmann commented Mar 10, 2022 •

edited

Loading