From 562702493b677ea9c3dbcf831c77cdcfe444b80f Mon Sep 17 00:00:00 2001 From: Jonas Pettersson Date: Sat, 6 Jun 2020 02:13:55 +0200 Subject: [PATCH] StatefulSet is sensitive to long names - use hashed name 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 https://github.com/kubernetes/kubernetes/issues/64023 but I was not aware of it. /kind bug Fixes #2766 --- ...inerun-with-parallel-tasks-using-pvc.yaml} | 0 .../workspace-from-volumeclaimtemplate.yaml | 2 -- .../pipelinerun/affinity_assistant.go | 17 +++++++++-------- .../pipelinerun/affinity_assistant_test.go | 19 +++++++++++++++++-- pkg/reconciler/pipelinerun/pipelinerun.go | 2 +- .../pipelinerun/pipelinerun_test.go | 10 ++++------ 6 files changed, 31 insertions(+), 19 deletions(-) rename examples/v1beta1/pipelineruns/{pipeline-run-with-parallel-tasks-using-pvc.yaml => pipelinerun-with-parallel-tasks-using-pvc.yaml} (100%) diff --git a/examples/v1beta1/pipelineruns/pipeline-run-with-parallel-tasks-using-pvc.yaml b/examples/v1beta1/pipelineruns/pipelinerun-with-parallel-tasks-using-pvc.yaml similarity index 100% rename from examples/v1beta1/pipelineruns/pipeline-run-with-parallel-tasks-using-pvc.yaml rename to examples/v1beta1/pipelineruns/pipelinerun-with-parallel-tasks-using-pvc.yaml diff --git a/examples/v1beta1/pipelineruns/workspace-from-volumeclaimtemplate.yaml b/examples/v1beta1/pipelineruns/workspace-from-volumeclaimtemplate.yaml index 35c325f89b6..62e476d70c5 100644 --- a/examples/v1beta1/pipelineruns/workspace-from-volumeclaimtemplate.yaml +++ b/examples/v1beta1/pipelineruns/workspace-from-volumeclaimtemplate.yaml @@ -41,8 +41,6 @@ spec: workspaces: - name: ws volumeClaimTemplate: - metadata: - name: mypvc spec: accessModes: - ReadWriteOnce diff --git a/pkg/reconciler/pipelinerun/affinity_assistant.go b/pkg/reconciler/pipelinerun/affinity_assistant.go index 4f73cbfc3fb..92b420b9c01 100644 --- a/pkg/reconciler/pipelinerun/affinity_assistant.go +++ b/pkg/reconciler/pipelinerun/affinity_assistant.go @@ -17,6 +17,7 @@ limitations under the License. package pipelinerun import ( + "crypto/sha256" "fmt" "github.com/tektoncd/pipeline/pkg/apis/pipeline" @@ -40,7 +41,6 @@ const ( ReasonCouldntCreateAffinityAssistantStatefulSet = "CouldntCreateAffinityAssistantStatefulSet" featureFlagDisableAffinityAssistantKey = "disable-affinity-assistant" - affinityAssistantStatefulSetNamePrefix = "affinity-assistant-" ) // createAffinityAssistants creates an Affinity Assistant StatefulSet for every workspace in the PipelineRun that @@ -50,9 +50,8 @@ func (c *Reconciler) createAffinityAssistants(wb []v1alpha1.WorkspaceBinding, pr var errs []error for _, w := range wb { if w.PersistentVolumeClaim != nil || w.VolumeClaimTemplate != nil { - affinityAssistantName := getAffinityAssistantName(w.Name, pr.GetOwnerReference()) - affinityAssistantStatefulSetName := affinityAssistantStatefulSetNamePrefix + affinityAssistantName - _, err := c.KubeClientSet.AppsV1().StatefulSets(namespace).Get(affinityAssistantStatefulSetName, metav1.GetOptions{}) + affinityAssistantName := getAffinityAssistantName(w.Name, pr.Name) + _, err := c.KubeClientSet.AppsV1().StatefulSets(namespace).Get(affinityAssistantName, metav1.GetOptions{}) claimName := getClaimName(w, pr.GetOwnerReference()) switch { case apierrors.IsNotFound(err): @@ -86,7 +85,7 @@ func (c *Reconciler) cleanupAffinityAssistants(pr *v1beta1.PipelineRun) error { var errs []error for _, w := range pr.Spec.Workspaces { if w.PersistentVolumeClaim != nil || w.VolumeClaimTemplate != nil { - affinityAssistantStsName := affinityAssistantStatefulSetNamePrefix + getAffinityAssistantName(w.Name, pr.GetOwnerReference()) + affinityAssistantStsName := getAffinityAssistantName(w.Name, pr.Name) if err := c.KubeClientSet.AppsV1().StatefulSets(pr.Namespace).Delete(affinityAssistantStsName, &metav1.DeleteOptions{}); err != nil { errs = append(errs, fmt.Errorf("failed to delete StatefulSet %s: %s", affinityAssistantStsName, err)) } @@ -95,8 +94,10 @@ func (c *Reconciler) cleanupAffinityAssistants(pr *v1beta1.PipelineRun) error { return errorutils.NewAggregate(errs) } -func getAffinityAssistantName(pipelineWorkspaceName string, owner metav1.OwnerReference) string { - return fmt.Sprintf("%s-%s", pipelineWorkspaceName, owner.Name) +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]) } func getStatefulSetLabels(pr *v1beta1.PipelineRun, affinityAssistantName string) map[string]string { @@ -168,7 +169,7 @@ func affinityAssistantStatefulSet(name string, pr *v1beta1.PipelineRun, claimNam APIVersion: "apps/v1", }, ObjectMeta: metav1.ObjectMeta{ - Name: affinityAssistantStatefulSetNamePrefix + name, + Name: name, Labels: getStatefulSetLabels(pr, name), OwnerReferences: []metav1.OwnerReference{pr.GetOwnerReference()}, }, diff --git a/pkg/reconciler/pipelinerun/affinity_assistant_test.go b/pkg/reconciler/pipelinerun/affinity_assistant_test.go index 1b333b1e8dd..ab657051a23 100644 --- a/pkg/reconciler/pipelinerun/affinity_assistant_test.go +++ b/pkg/reconciler/pipelinerun/affinity_assistant_test.go @@ -17,7 +17,6 @@ limitations under the License. package pipelinerun import ( - "fmt" "testing" "github.com/tektoncd/pipeline/pkg/apis/pipeline" @@ -65,7 +64,7 @@ func TestCreateAndDeleteOfAffinityAssistant(t *testing.T) { t.Errorf("unexpected error from createAffinityAssistants: %v", err) } - expectedAffinityAssistantName := affinityAssistantStatefulSetNamePrefix + fmt.Sprintf("%s-%s", workspaceName, pipelineRunName) + expectedAffinityAssistantName := getAffinityAssistantName(workspaceName, testPipelineRun.Name) _, err = c.KubeClientSet.AppsV1().StatefulSets(testPipelineRun.Namespace).Get(expectedAffinityAssistantName, metav1.GetOptions{}) if err != nil { t.Errorf("unexpected error when retrieving StatefulSet: %v", err) @@ -134,6 +133,22 @@ func TestThatTheAffinityAssistantIsWithoutNodeSelectorAndTolerations(t *testing. } } +// TestThatAffinityAssistantNameIsNoLongerThan53 tests that the Affinity Assistant Name +// is no longer than 53 chars. This is a limitation with StatefulSet. +// See https://github.com/kubernetes/kubernetes/issues/64023 +// This is because the StatefulSet-controller adds a label with the name of the StatefulSet +// plus 10 chars for a hash. Labels in Kubernetes can not be longer than 63 chars. +// Typical output from the example below is affinity-assistant-0384086f62 +func TestThatAffinityAssistantNameIsNoLongerThan53(t *testing.T) { + affinityAssistantName := getAffinityAssistantName( + "pipeline-workspace-name-that-is-quite-long", + "pipelinerun-with-a-long-custom-name") + + if len(affinityAssistantName) > 53 { + t.Errorf("affinity assistant name can not be longer than 53 chars") + } +} + func TestDisableAffinityAssistant(t *testing.T) { for _, tc := range []struct { description string diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index 0ff25ec6a53..13e322edb75 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -790,7 +790,7 @@ func (c *Reconciler) createTaskRun(rprt *resources.ResolvedPipelineRunTask, pr * } if !c.isAffinityAssistantDisabled() && pipelinePVCWorkspaceName != "" { - tr.Annotations[workspace.AnnotationAffinityAssistantName] = getAffinityAssistantName(pipelinePVCWorkspaceName, pr.GetOwnerReference()) + tr.Annotations[workspace.AnnotationAffinityAssistantName] = getAffinityAssistantName(pipelinePVCWorkspaceName, pr.Name) } resources.WrapSteps(&tr.Spec, rprt.PipelineTask, rprt.ResolvedTaskResources.Inputs, rprt.ResolvedTaskResources.Outputs, storageBasePath) diff --git a/pkg/reconciler/pipelinerun/pipelinerun_test.go b/pkg/reconciler/pipelinerun/pipelinerun_test.go index 457a890c2ae..4ca093c0f81 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun_test.go +++ b/pkg/reconciler/pipelinerun/pipelinerun_test.go @@ -1925,13 +1925,11 @@ func TestReconcileWithAffinityAssistantStatefulSet(t *testing.T) { t.Fatalf("expected one StatefulSet created. %d was created", len(stsNames)) } - expectedAffinityAssistantName1 := fmt.Sprintf("%s-%s", workspaceName, pipelineRunName) - expectedAffinityAssistantName2 := fmt.Sprintf("%s-%s", workspaceName2, pipelineRunName) - expectedStsName1 := affinityAssistantStatefulSetNamePrefix + expectedAffinityAssistantName1 - expectedStsName2 := affinityAssistantStatefulSetNamePrefix + expectedAffinityAssistantName2 + expectedAffinityAssistantName1 := getAffinityAssistantName(workspaceName, pipelineRunName) + expectedAffinityAssistantName2 := getAffinityAssistantName(workspaceName2, pipelineRunName) expectedAffinityAssistantStsNames := make(map[string]bool) - expectedAffinityAssistantStsNames[expectedStsName1] = true - expectedAffinityAssistantStsNames[expectedStsName2] = true + expectedAffinityAssistantStsNames[expectedAffinityAssistantName1] = true + expectedAffinityAssistantStsNames[expectedAffinityAssistantName2] = true for _, stsName := range stsNames { _, found := expectedAffinityAssistantStsNames[stsName] if !found {