Skip to content

Commit

Permalink
StatefulSet is sensitive to long names - use hashed name
Browse files Browse the repository at this point in the history
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
  • Loading branch information
jlpettersson authored and tekton-robot committed Jun 10, 2020
1 parent 65c1a88 commit 5627024
Show file tree
Hide file tree
Showing 6 changed files with 31 additions and 19 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,6 @@ spec:
workspaces:
- name: ws
volumeClaimTemplate:
metadata:
name: mypvc
spec:
accessModes:
- ReadWriteOnce
Expand Down
17 changes: 9 additions & 8 deletions pkg/reconciler/pipelinerun/affinity_assistant.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package pipelinerun

import (
"crypto/sha256"
"fmt"

"github.com/tektoncd/pipeline/pkg/apis/pipeline"
Expand All @@ -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
Expand All @@ -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):
Expand Down Expand Up @@ -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))
}
Expand All @@ -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 {
Expand Down Expand Up @@ -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()},
},
Expand Down
19 changes: 17 additions & 2 deletions pkg/reconciler/pipelinerun/affinity_assistant_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ limitations under the License.
package pipelinerun

import (
"fmt"
"testing"

"github.com/tektoncd/pipeline/pkg/apis/pipeline"
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion pkg/reconciler/pipelinerun/pipelinerun.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
10 changes: 4 additions & 6 deletions pkg/reconciler/pipelinerun/pipelinerun_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down

0 comments on commit 5627024

Please sign in to comment.