Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
Jun 10, 2020
Merged

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

merged 1 commit into from
Jun 10, 2020

Conversation

jlpettersson
Copy link
Member

@jlpettersson jlpettersson commented Jun 9, 2020 •
edited
Loading

Changes

This PR is identical to #2768 - before that one is rebased.

Both has commit id 2aecd8cbd9c050d033198b4f19d3172355b33213 - until the other one is rebased.

@tekton-robot tekton-robot added kind/bug Categorizes issue or PR as related to a bug. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 9, 2020
@tekton-robot
Copy link
Collaborator

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.

@tekton-robot
Copy link
Collaborator

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% 92.0% 0.3
pkg/reconciler/pipelinerun/pipelinerun.go 81.7% 80.5% -1.2

@jlpettersson jlpettersson changed the title StatefulSet is sensitive to long names - use hashed name PR into 0.13.x: StatefulSet is sensitive to long names - use hashed name Jun 9, 2020
@vdemeester vdemeester added this to the Pipelines 0.13.1 🐱 milestone Jun 10, 2020
@jlpettersson @bobcatfish
StatefulSet is sensitive to long names - use hashed name
fc79022 
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
@bobcatfish bobcatfish force-pushed the use_shorter_sts_name-new branch from 2aecd8c to fc79022 Compare June 10, 2020 13:45
@bobcatfish
Copy link
Collaborator

I just rebased + force pushed to your branch @jlpettersson , should resolve the error the tests were running into :D 🤞

@tekton-robot
Copy link
Collaborator

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% 92.2% 0.5
pkg/reconciler/pipelinerun/pipelinerun.go 81.7% 80.5% -1.2

@bobcatfish
Copy link
Collaborator

/approve
/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bobcatfish

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:

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

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 10, 2020
@tekton-robot tekton-robot merged commit 5627024 into tektoncd:release-v0.13.0x Jun 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bobcatfish bobcatfish Awaiting requested review from bobcatfish

@dlorenc dlorenc Awaiting requested review from dlorenc

Assignees

@bobcatfish bobcatfish

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
Pipelines 0.13.1 🐱
Development

Successfully merging this pull request may close these issues.
4 participants
@jlpettersson @tekton-robot @bobcatfish @vdemeester