Skip to content

Commit f60b5c3

Browse files
l-qingl-qing
committed
fix: resolve pod creation failure on retry when using workspace.<name>.volume
fix #7886 Change the naming method of the workspace volume from completely random to hashed, ensuring that the name generated during a single taskRun lifecycle is consistent each time, and is unique within all current workspaces. This way, we can reuse the logic of retrieving the taskSpec from the status, and also store the content after variable expansion in the taskSpec of the status for easy debugging; it will also not affect the reconstruction of the pod when retrying.
1 parent fba68b7 commit f60b5c3

File tree

7 files changed

+300
-71
lines changed

7 files changed

+300
-71
lines changed

‎pkg/names/generate.go

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,10 @@ package names
1818

1919
import (
2020
"fmt"
21+
"hash/fnv"
2122
"regexp"
23+
"strconv"
24+
"strings"
2225

2326
utilrand "k8s.io/apimachinery/pkg/util/rand"
2427
)
@@ -73,3 +76,19 @@ func (simpleNameGenerator) RestrictLength(base string) string {
7376
}
7477
return base
7578
}
79+
80+
// GenerateHashedName creates a unique name with a hashed suffix.
81+
func GenerateHashedName(prefix, name string, hashedLength int) string {
82+
if hashedLength <= 0 {
83+
hashedLength = randomLength
84+
}
85+
h := fnv.New32a()
86+
h.Write([]byte(name))
87+
suffix := strconv.FormatUint(uint64(h.Sum32()), 16)
88+
if ln := len(suffix); ln > hashedLength {
89+
suffix = suffix[:hashedLength]
90+
} else if ln < hashedLength {
91+
suffix += strings.Repeat("0", hashedLength-ln)
92+
}
93+
return fmt.Sprintf("%s-%s", prefix, suffix)
94+
}

‎pkg/names/generate_test.go

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,3 +67,64 @@ func TestRestrictLength(t *testing.T) {
6767
})
6868
}
6969
}
70+
71+
func TestGenerateHashedName(t *testing.T) {
72+
tests := []struct {
73+
title string
74+
prefix string
75+
name string
76+
randomLength int
77+
expectedHashedName string
78+
}{{
79+
title: "generate hashed name with custom random length",
80+
prefix: "ws",
81+
name: "workspace-name",
82+
randomLength: 10,
83+
expectedHashedName: "ws-d70baf7a00",
84+
}, {
85+
title: "generate hashed name with default random length",
86+
prefix: "ws",
87+
name: "workspace-name",
88+
randomLength: -1,
89+
expectedHashedName: "ws-d70ba",
90+
}, {
91+
title: "generate hashed name with empty prefix",
92+
prefix: "",
93+
name: "workspace-name",
94+
randomLength: 0,
95+
expectedHashedName: "-d70ba",
96+
}, {
97+
title: "consistent hashed name for different inputs - 1",
98+
prefix: "ws",
99+
name: "test-01097628",
100+
randomLength: 5,
101+
expectedHashedName: "ws-f32ff",
102+
}, {
103+
title: "consistent hashed name for different inputs - 2",
104+
prefix: "ws",
105+
name: "test-01617609",
106+
randomLength: 5,
107+
expectedHashedName: "ws-f32ff",
108+
}, {
109+
title: "consistent hashed name for different inputs - 3",
110+
prefix: "ws",
111+
name: "test-01675975",
112+
randomLength: 5,
113+
expectedHashedName: "ws-f32ff",
114+
}, {
115+
title: "consistent hashed name for different inputs - 4",
116+
prefix: "ws",
117+
name: "test-01809743",
118+
randomLength: 5,
119+
expectedHashedName: "ws-f32ff",
120+
}}
121+
122+
for _, tc := range tests {
123+
t.Run(tc.title, func(t *testing.T) {
124+
hashedName := pkgnames.GenerateHashedName(tc.prefix, tc.name, tc.randomLength)
125+
if hashedName != tc.expectedHashedName {
126+
t.Errorf("expected %q, got %q", tc.expectedHashedName, hashedName)
127+
}
128+
})
129+
}
130+
}

‎pkg/reconciler/taskrun/resources/apply_test.go

Lines changed: 15 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1141,29 +1141,29 @@ func TestApplyWorkspaces(t *testing.T) {
11411141
EmptyDir: &corev1.EmptyDirVolumeSource{},
11421142
}},
11431143
want: applyMutation(ts, func(spec *v1.TaskSpec) {
1144-
spec.StepTemplate.Env[0].Value = "ws-9l9zj"
1144+
spec.StepTemplate.Env[0].Value = "ws-b31db"
11451145
spec.StepTemplate.Env[1].Value = "foo"
11461146
spec.StepTemplate.Env[2].Value = ""
11471147

1148-
spec.Steps[0].Name = "ws-9l9zj"
1149-
spec.Steps[0].Image = "ws-mz4c7"
1150-
spec.Steps[0].WorkingDir = "ws-mz4c7"
1148+
spec.Steps[0].Name = "ws-b31db"
1149+
spec.Steps[0].Image = "ws-a6f34"
1150+
spec.Steps[0].WorkingDir = "ws-a6f34"
11511151
spec.Steps[0].Args = []string{"/workspace/myws"}
11521152

1153-
spec.Steps[1].VolumeMounts[0].Name = "ws-9l9zj"
1153+
spec.Steps[1].VolumeMounts[0].Name = "ws-b31db"
11541154
spec.Steps[1].VolumeMounts[0].MountPath = "path/to//foo"
1155-
spec.Steps[1].VolumeMounts[0].SubPath = "ws-9l9zj"
1155+
spec.Steps[1].VolumeMounts[0].SubPath = "ws-b31db"
11561156

1157-
spec.Steps[2].Env[0].Value = "ws-9l9zj"
1158-
spec.Steps[2].Env[1].ValueFrom.SecretKeyRef.LocalObjectReference.Name = "ws-9l9zj"
1159-
spec.Steps[2].Env[1].ValueFrom.SecretKeyRef.Key = "ws-9l9zj"
1160-
spec.Steps[2].EnvFrom[0].Prefix = "ws-9l9zj"
1161-
spec.Steps[2].EnvFrom[0].ConfigMapRef.LocalObjectReference.Name = "ws-9l9zj"
1157+
spec.Steps[2].Env[0].Value = "ws-b31db"
1158+
spec.Steps[2].Env[1].ValueFrom.SecretKeyRef.LocalObjectReference.Name = "ws-b31db"
1159+
spec.Steps[2].Env[1].ValueFrom.SecretKeyRef.Key = "ws-b31db"
1160+
spec.Steps[2].EnvFrom[0].Prefix = "ws-b31db"
1161+
spec.Steps[2].EnvFrom[0].ConfigMapRef.LocalObjectReference.Name = "ws-b31db"
11621162

1163-
spec.Volumes[0].Name = "ws-9l9zj"
1164-
spec.Volumes[0].VolumeSource.ConfigMap.LocalObjectReference.Name = "ws-9l9zj"
1165-
spec.Volumes[1].VolumeSource.Secret.SecretName = "ws-9l9zj"
1166-
spec.Volumes[2].VolumeSource.PersistentVolumeClaim.ClaimName = "ws-9l9zj"
1163+
spec.Volumes[0].Name = "ws-b31db"
1164+
spec.Volumes[0].VolumeSource.ConfigMap.LocalObjectReference.Name = "ws-b31db"
1165+
spec.Volumes[1].VolumeSource.Secret.SecretName = "ws-b31db"
1166+
spec.Volumes[2].VolumeSource.PersistentVolumeClaim.ClaimName = "ws-b31db"
11671167
}),
11681168
}, {
11691169
name: "optional-workspace-provided-variable-replacement",

‎pkg/reconciler/taskrun/taskrun_test.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1046,7 +1046,7 @@ spec:
10461046
},
10471047
wantPod: addVolumeMounts(expectedPod("test-taskrun-with-output-config-ws-pod", "", "test-taskrun-with-output-config-ws", "foo", config.DefaultServiceAccountValue, false,
10481048
[]corev1.Volume{{
1049-
Name: "ws-9l9zj",
1049+
Name: "ws-d872e",
10501050
VolumeSource: corev1.VolumeSource{
10511051
EmptyDir: &corev1.EmptyDirVolumeSource{},
10521052
},
@@ -1058,7 +1058,7 @@ spec:
10581058
cmd: "/mycmd",
10591059
}}),
10601060
[]corev1.VolumeMount{{
1061-
Name: "ws-9l9zj",
1061+
Name: "ws-d872e",
10621062
MountPath: "/workspace/data",
10631063
}}),
10641064
}} {
@@ -4034,8 +4034,8 @@ spec:
40344034
t.Fatalf("create pod threw error %v", err)
40354035
}
40364036

4037-
if vm := pod.Spec.Containers[0].VolumeMounts[0]; !strings.HasPrefix(vm.Name, "ws-9l9zj") || vm.MountPath != expectedMountPath {
4038-
t.Fatalf("failed to find expanded Workspace mountpath %v", expectedMountPath)
4037+
if vm := pod.Spec.Containers[0].VolumeMounts[0]; !strings.HasPrefix(vm.Name, "ws-f888c") || vm.MountPath != expectedMountPath {
4038+
t.Fatalf("failed to find expanded Workspace mountpath %v for %v", expectedMountPath, vm.Name)
40394039
}
40404040

40414041
if a := pod.Spec.Containers[0].Args; a[len(a)-1] != expectedReplacedArgs {

‎pkg/workspace/apply.go

Lines changed: 22 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,14 +21,15 @@ import (
2121
"fmt"
2222

2323
v1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1"
24-
"github.com/tektoncd/pipeline/pkg/names"
24+
pkgnames "github.com/tektoncd/pipeline/pkg/names"
2525
"github.com/tektoncd/pipeline/pkg/substitution"
2626
corev1 "k8s.io/api/core/v1"
2727
"k8s.io/apimachinery/pkg/util/sets"
2828
)
2929

3030
const (
31-
volumeNameBase = "ws"
31+
volumeNameBase = "ws"
32+
defaultRandomLength = 5
3233
)
3334

3435
// nameVolumeMap is a map from a workspace's name to its Volume.
@@ -42,15 +43,31 @@ func (nvm nameVolumeMap) setVolumeSource(workspaceName string, volumeName string
4243
}
4344
}
4445

46+
// generateVolumeName generates a unique name for a volume based on the workspace name.
47+
func generateVolumeName(name string) string {
48+
return pkgnames.GenerateHashedName(volumeNameBase, name, defaultRandomLength)
49+
}
50+
4551
// CreateVolumes will return a dictionary where the keys are the names of the workspaces bound in
4652
// wb and the value is a newly-created Volume to use. If the same Volume is bound twice, the
4753
// resulting volumes will both have the same name to prevent the same Volume from being attached
48-
// to a pod twice. The names of the returned volumes will be a short random string starting "ws-".
54+
// to a pod twice. The names of the returned volumes will be a short hash string starting "ws-".
4955
func CreateVolumes(wb []v1.WorkspaceBinding) map[string]corev1.Volume {
5056
pvcs := map[string]corev1.Volume{}
51-
v := make(nameVolumeMap)
57+
v := make(nameVolumeMap, len(wb))
58+
// Track the names we've used so far to avoid collisions
59+
usedNames := make(map[string]struct{}, len(wb))
60+
5261
for _, w := range wb {
53-
name := names.SimpleNameGenerator.RestrictLengthWithRandomSuffix(volumeNameBase)
62+
name := generateVolumeName(w.Name)
63+
64+
// If we've already generated this name, try appending extra characters until we find a unique name
65+
for _, exists := usedNames[name]; exists; _, exists = usedNames[name] {
66+
name = generateVolumeName(name + "$")
67+
}
68+
// Track the name we've used
69+
usedNames[name] = struct{}{}
70+
5471
switch {
5572
case w.PersistentVolumeClaim != nil:
5673
// If it's a PVC, we need to check if we've encountered it before so we avoid mounting it twice

0 commit comments

Comments
 (0)