StatefulSet is sensitive to long names - use a hashed name #2768

jlpettersson · 2020-06-06T00:20:09Z

Changes

Names in Kubernetes can be up to 253 chars, but labels can only be up to 63 chars.

We are relatively conservative with the two labels we introduce for the Affinity Assistant

app.kubernetes.io/component: affinity-assistant
app.kubernetes.io/instance: ws-parallel-pipelinerun-bbx6w

But apparently, StatefulSets adds a label with the full StatefulSet Name + 10 chars (for a hash) as a label

controller-revision-hash: affinity-assistant-ws-parallel-pipelinerun-bbx6w-dd64c6c8d

This only leave users to use StatefulSet Names up to 53 chars. We use a prefix of 19 chars (affinity-assistant-) on the Affinity Assistant StatefulSet. This leaves Tekton users with only 34 chars for a combination of Workspace Name and the PipelineRun Name.

This commit use a hash of the Workspace Name and the PipelineRun Name to make sure that the name is not too long.
Typical labels after this commit will be:

labels:
  app.kubernetes.io/component: affinity-assistant
  app.kubernetes.io/instance: affinity-assistant-e067465fc0
  controller-revision-hash: affinity-assistant-e067465fc0-b78cb9478
  statefulset.kubernetes.io/pod-name: affinity-assistant-e067465fc0-0
  tekton.dev/pipeline: parallel-pipeline
  tekton.dev/pipelineRun: parallel-pipelinerun-wr9wd

Also the unnecessary name of the PVC in the volumeClaimTemplate-example is removed.

This limitation of StatefulSet is apparently a known problem kubernetes/kubernetes#64023 but I was not aware of it.

/kind bug
Fixes #2766
Fixes #2769
Fixes #2796

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

If you are adding a new binary/image to the cmd dir, please update
the release Task to build and release this image.

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

Release Notes

- There was a problem with long names for WorkspaceName+PipelineRunName, it could only be up to 33 chars. This commit uses a hashed name for the Affinity Assistant with a consistent length.

tekton-robot · 2020-06-06T00:21:46Z

This PR cannot be merged: expecting exactly one kind/ label

Available kind/ labels are:

kind/bug: Categorizes issue or PR as related to a bug.
kind/flake: Categorizes issue or PR as related to a flakey test
kind/cleanup: Categorizes issue or PR as related to cleaning up code, process, or technical debt.
kind/design: Categorizes issue or PR as related to design.
kind/documentation: Categorizes issue or PR as related to documentation.
kind/feature: Categorizes issue or PR as related to a new feature.
kind/misc: Categorizes issue or PR as a miscellaneuous one.

afrittoli

Thanks!
/approve

afrittoli

Thanks!
/approve

cameronbraid · 2020-06-06T08:29:05Z

Why not just hash the Workspace Name and the PipelineRun Name so that the length doesn't matter

jlpettersson · 2020-06-06T08:33:17Z

Why not just hash the Workspace Name and the PipelineRun Name so that the length doesn't matter

Yes, that is also a way to handle it. Hash both names and use a substring of 53 chars. But it is harder to se from where the pod come from - but it is visible in the other labels.

cameronbraid · 2020-06-06T08:42:43Z

Do both prefixName = 'aa-' + workspaceName + "-"+ pipelineName hashName = hash(prefixName) podName = leff(prefixName, MAX_LEN-len(hashName+1)) + '-' + hashName

…

On Sat, 6 Jun 2020 at 18:33, Jonas Pettersson ***@***.***> wrote: Why not just hash the Workspace Name and the PipelineRun Name so that the length doesn't matter Yes, that is also a way to handle it. Hash both names and use a substring of 53 chars. But it is harder to se from where the pod come from - but it is visible in the other labels. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2768 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFEFFMSWNVDFHHYQU2RERTRVH5NRANCNFSM4NVLTEIQ> .

jlpettersson · 2020-06-06T08:57:23Z

Yes, we could also keep the prefix and use a 34 chars substring of the hash of workspace+pipelinerun.

E.g. affinity-assistant-bbx6wdd64c6c8dbbx6wdd64c6c8dbbx6wd

That would make it easier to understand when doing kubectl get pods

Would that be a good alternative?
/hold

tekton-robot · 2020-06-06T15:14:08Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/affinity_assistant.go	91.8%	92.0%	0.2

jlpettersson · 2020-06-06T15:19:01Z

The implementation is now changed to use a hashed name, so that the length of the StatefulSet Name is consistent.

A typical name of the Affinity Assistant will now be:

affinity-assistant-e067465fc0

/hold cancel

cameronbraid · 2020-06-06T16:02:32Z

Cool

One question about the use of a stateful set, why isn't this 'affinity-assistant' just implemented via a single Pod ?

jlpettersson · 2020-06-06T16:07:10Z

One question about the use of a stateful set, why isn't this 'affinity-assistant' just implemented via a single Pod ?

@cameronbraid I wrote a motivation about why I use StatefulSet in the PR description (a bit down) #2630

tekton-robot · 2020-06-06T18:50:06Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/affinity_assistant.go	91.8%	92.0%	0.2

ghost · 2020-06-08T18:30:16Z

/lgtm

Thanks for the quick fix!

pritidesai · 2020-06-09T07:09:44Z

Appeared in the integration test logs 😿:

default            1s          Warning   FailedCreate              statefulset/affinity-assistant-shared-workspace-write-and-cleanup-workspace   create Pod affinity-assistant-shared-workspace-write-and-cleanup-workspace-0 in StatefulSet affinity-assistant-shared-workspace-write-and-cleanup-workspace failed error: Pod "affinity-assistant-shared-workspace-write-and-cleanup-workspace-0" is invalid: [metadata.labels: Invalid value: "affinity-assistant-shared-workspace-write-and-cleanup-workspace-55458fdf66": must be no more than 63 characters, metadata.labels: Invalid value: "affinity-assistant-shared-workspace-write-and-cleanup-workspace-0": must be no more than 63 characters, spec.hostname: Invalid value: "affinity-assistant-shared-workspace-write-and-cleanup-workspace-0": must be no more than 63 characters]

jlpettersson · 2020-06-09T07:54:57Z

@pritidesai from what logs?

The latest is pull-tekton-pipeline-integration-tests

pritidesai · 2020-06-09T20:04:48Z

@pritidesai from what logs?

The latest is pull-tekton-pipeline-integration-tests

One of the PRs 😉 #2661, tests

bobcatfish · 2020-06-09T20:26:12Z

Thanks for the excellent commit message @jlpettersson !!!!!

bobcatfish · 2020-06-09T20:28:05Z

pkg/reconciler/pipelinerun/affinity_assistant.go

+func getAffinityAssistantName(pipelineWorkspaceName string, pipelineRunName string) string {
+ hashBytes := sha256.Sum256([]byte(pipelineWorkspaceName + pipelineRunName))
+ hashString := fmt.Sprintf("%x", hashBytes)
+ return fmt.Sprintf("%s-%s", "affinity-assistant", hashString[:10])


we actually have some existing libs around this that you might be able to use: https://github.com/tektoncd/pipeline/blob/master/pkg/names/generate.go

I think I had a look at it, but it is "random"

random suffix

I my use case, I want a consistent hash - from the Workspace Name + PipelineRun Name - so that the same combination always get the same hash.

bobcatfish · 2020-06-09T20:52:53Z

@jlpettersson it looks like once the conflicts are resolved this is good to go - don't worry too much about the conflicts, I'm happy to work around them when cherry picking this commit in tomorrow (tho I really appreciate you making it easier to cherry pick!!)

Names in Kubernetes can be up to 253 chars, but labels can only be up to 63 chars. We are relatively conservative with the two labels we introduce for the Affinity Assistant app.kubernetes.io/component: affinity-assistant app.kubernetes.io/instance: ws-parallel-pipelinerun-bbx6w But apparently, StatefulSets adds a label with the full StatefulSet Name + 10 chars (for a hash) as a label controller-revision-hash: affinity-assistant-ws-parallel-pipelinerun-bbx6w-dd64c6c8d This only leave users to use StatefulSet Names up to 53 chars. We use a prefix of 19 chars (affinity-assistant-) on the Affinity Assistant StatefulSet. This leaves Tekton users with only 34 chars for a combination of Workspace Name and the PipelineRun Name. This commit use a hash of the Workspace Name and the PipelineRun Name to make sure that the name is not too long. Typical labels after this commit will be: labels: app.kubernetes.io/component: affinity-assistant app.kubernetes.io/instance: affinity-assistant-e067465fc0 controller-revision-hash: affinity-assistant-e067465fc0-b78cb9478 statefulset.kubernetes.io/pod-name: affinity-assistant-e067465fc0-0 tekton.dev/pipeline: parallel-pipeline tekton.dev/pipelineRun: parallel-pipelinerun-wr9wd Also the unnecessary name of the PVC in the volumeClaimTemplate-example is removed. This limitation of StatefulSet is apparently a known problem kubernetes/kubernetes#64023 but I was not aware of it. /kind bug Fixes tektoncd#2766

tekton-robot · 2020-06-10T14:57:16Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/affinity_assistant.go	91.7%	91.8%	0.2

vdemeester

/lgtm

tekton-robot · 2020-06-10T14:57:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afrittoli, vdemeester

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:

~~OWNERS~~ [afrittoli,vdemeester]

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

tekton-robot added the kind/bug Categorizes issue or PR as related to a bug. label Jun 6, 2020

tekton-robot requested review from afrittoli and dlorenc June 6, 2020 00:20

tekton-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jun 6, 2020

jlpettersson mentioned this pull request Jun 6, 2020

v0.13.0 pod scheduling fails - didn't match pod affinity rules #2769

Closed

afrittoli reviewed Jun 6, 2020

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 6, 2020

afrittoli added the needs-cherry-pick Indicates a PR needs to be cherry-pick to a release branch label Jun 6, 2020

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 6, 2020

jlpettersson force-pushed the use_shorter_sts_name branch from b923e18 to 6bc75f5 Compare June 6, 2020 15:11

tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 6, 2020

jlpettersson changed the title ~~StatefulSet is sensitive to long names - remove prefix~~ StatefulSet is sensitive to long names - use a hashed name Jun 6, 2020

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 6, 2020

jlpettersson force-pushed the use_shorter_sts_name branch from 6bc75f5 to 2aecd8c Compare June 6, 2020 18:47

vdemeester modified the milestones: Pipelines 0.13 🐱, Pipelines 0.13.1 🐱 Jun 8, 2020

tekton-robot assigned ghost Jun 8, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 8, 2020

bobcatfish reviewed Jun 9, 2020

View reviewed changes

This was referenced Jun 9, 2020

PR into 0.13.x: StatefulSet is sensitive to long names - use hashed name #2794

Merged

Regression with PVCs and node affinity #2796

Closed

bobcatfish modified the milestones: Pipelines 0.13.1 🐱, Pipelines v0.14 Jun 10, 2020

jlpettersson force-pushed the use_shorter_sts_name branch from 2aecd8c to b01d860 Compare June 10, 2020 14:54

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 10, 2020

vdemeester approved these changes Jun 10, 2020

View reviewed changes

tekton-robot assigned vdemeester Jun 10, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 10, 2020

tekton-robot merged commit 7e74e96 into tektoncd:master Jun 10, 2020

vdemeester removed the needs-cherry-pick Indicates a PR needs to be cherry-pick to a release branch label Jun 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

StatefulSet is sensitive to long names - use a hashed name #2768

StatefulSet is sensitive to long names - use a hashed name #2768

jlpettersson commented Jun 6, 2020 •

edited by vdemeester

Loading

tekton-robot commented Jun 6, 2020

afrittoli left a comment

afrittoli left a comment

cameronbraid commented Jun 6, 2020

jlpettersson commented Jun 6, 2020

cameronbraid commented Jun 6, 2020 via email

jlpettersson commented Jun 6, 2020

tekton-robot commented Jun 6, 2020

jlpettersson commented Jun 6, 2020

cameronbraid commented Jun 6, 2020

jlpettersson commented Jun 6, 2020

tekton-robot commented Jun 6, 2020

ghost commented Jun 8, 2020

pritidesai commented Jun 9, 2020

jlpettersson commented Jun 9, 2020 •

edited

Loading

pritidesai commented Jun 9, 2020

bobcatfish commented Jun 9, 2020

bobcatfish Jun 9, 2020

jlpettersson Jun 9, 2020

bobcatfish commented Jun 9, 2020

tekton-robot commented Jun 10, 2020

vdemeester left a comment

tekton-robot commented Jun 10, 2020

StatefulSet is sensitive to long names - use a hashed name #2768

StatefulSet is sensitive to long names - use a hashed name #2768

Conversation

jlpettersson commented Jun 6, 2020 • edited by vdemeester Loading

Changes

Submitter Checklist

Reviewer Notes

Release Notes

tekton-robot commented Jun 6, 2020

afrittoli left a comment

Choose a reason for hiding this comment

afrittoli left a comment

Choose a reason for hiding this comment

cameronbraid commented Jun 6, 2020

jlpettersson commented Jun 6, 2020

cameronbraid commented Jun 6, 2020 via email

jlpettersson commented Jun 6, 2020

tekton-robot commented Jun 6, 2020

jlpettersson commented Jun 6, 2020

cameronbraid commented Jun 6, 2020

jlpettersson commented Jun 6, 2020

tekton-robot commented Jun 6, 2020

ghost commented Jun 8, 2020

pritidesai commented Jun 9, 2020

jlpettersson commented Jun 9, 2020 • edited Loading

pritidesai commented Jun 9, 2020

bobcatfish commented Jun 9, 2020

bobcatfish Jun 9, 2020

Choose a reason for hiding this comment

jlpettersson Jun 9, 2020

Choose a reason for hiding this comment

bobcatfish commented Jun 9, 2020

tekton-robot commented Jun 10, 2020

vdemeester left a comment

Choose a reason for hiding this comment

tekton-robot commented Jun 10, 2020

jlpettersson commented Jun 6, 2020 •

edited by vdemeester

Loading

jlpettersson commented Jun 9, 2020 •

edited

Loading