Skip to content

Commit 3ec8092

Browse files
hxrshxzhxrshxz
committed
fix thin transform review issues
Signed-off-by: Harsh <harshmastic@gmail.com>
1 parent 9132f37 commit 3ec8092

3 files changed

Lines changed: 43 additions & 93 deletions

File tree

‎pkg/ddc/thin/master_internal.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -132,7 +132,7 @@ func (t *ThinEngine) ifRuntimeHelmValueEnable() bool {
132132
if err != nil {
133133
return true
134134
}
135-
return enableRuntimeHelmValueConfig
135+
return !enableRuntimeHelmValueConfig
136136
}
137137

138138
func (t *ThinEngine) getHelmValuesConfigMapName() string {

‎pkg/ddc/thin/master_test.go‎

Lines changed: 13 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -135,39 +135,10 @@ var _ = Describe("Master Tests", func() {
135135
})
136136

137137
Describe("ThinEngine.generateThinValueFile", func() {
138-
It("generates values when the runtime profile lookup falls back to nil", func() {
139-
dataset, runtimeObj, _ := mockFluidObjectsForTests(types.NamespacedName{Name: "test-dataset", Namespace: "default"})
140-
runtimeObj.Spec.Fuse = datav1alpha1.ThinFuseSpec{
141-
Image: "runtime-fuse",
142-
}
143-
144-
client := fake.NewFakeClientWithScheme(testScheme, dataset, runtimeObj)
145-
engine := mockThinEngineForTests(dataset, runtimeObj, nil)
146-
engine.Client = client
147-
engine.runtime = runtimeObj
148-
149-
generatedProfile, err := engine.getThinRuntimeProfile()
150-
Expect(err).To(HaveOccurred())
151-
Expect(generatedProfile).To(BeNil())
152-
153-
valueFile, err := engine.generateThinValueFile(runtimeObj, nil)
154-
Expect(err).NotTo(HaveOccurred())
155-
Expect(valueFile).To(BeAnExistingFile())
156-
157-
configMap := &corev1.ConfigMap{}
158-
err = engine.Client.Get(context.TODO(), types.NamespacedName{
159-
Name: engine.getHelmValuesConfigMapName(),
160-
Namespace: engine.namespace,
161-
}, configMap)
162-
Expect(err).NotTo(HaveOccurred())
163-
Expect(configMap.Labels).To(HaveKeyWithValue(common.LabelAnnotationDatasetId, dataset.Labels[common.LabelAnnotationDatasetId]))
164-
Expect(configMap.Data).To(HaveKey("data"))
165-
})
166-
167138
It("skips storing runtime helm values when runtime config map generation is disabled", func() {
168139
dataset, runtimeObj, profile := mockFluidObjectsForTests(types.NamespacedName{Name: "test-dataset", Namespace: "default"})
169140
runtimeObj.Spec.Fuse = datav1alpha1.ThinFuseSpec{Image: "runtime-fuse"}
170-
runtimeObj.Annotations = map[string]string{common.AnnotationDisableRuntimeHelmValueConfig: "false"}
141+
runtimeObj.Annotations = map[string]string{common.AnnotationDisableRuntimeHelmValueConfig: "true"}
171142

172143
engine := mockThinEngineForTests(dataset, runtimeObj, profile)
173144
engine.Client = fake.NewFakeClientWithScheme(testScheme, dataset, runtimeObj, profile)
@@ -188,29 +159,6 @@ var _ = Describe("Master Tests", func() {
188159
})
189160

190161
Describe("ThinEngine.setupMasterInternal", func() {
191-
It("continues with a missing runtime profile when the release already exists", func() {
192-
dataset, runtimeObj, _ := mockFluidObjectsForTests(types.NamespacedName{Name: "test-dataset", Namespace: "default"})
193-
runtimeObj.Spec.Fuse = datav1alpha1.ThinFuseSpec{Image: "runtime-fuse"}
194-
195-
engine := mockThinEngineForTests(dataset, runtimeObj, nil)
196-
engine.Client = fake.NewFakeClientWithScheme(testScheme, dataset, runtimeObj)
197-
engine.runtime = runtimeObj
198-
199-
checkReleasePatch := ApplyFunc(helm.CheckRelease, func(name string, namespace string) (bool, error) {
200-
Expect(name).To(Equal(engine.name))
201-
Expect(namespace).To(Equal(engine.namespace))
202-
return true, nil
203-
})
204-
installReleasePatch := ApplyFunc(helm.InstallRelease, func(string, string, string, string) error {
205-
Fail("InstallRelease should not be called when the release already exists")
206-
return nil
207-
})
208-
defer checkReleasePatch.Reset()
209-
defer installReleasePatch.Reset()
210-
211-
Expect(engine.setupMasterInternal()).To(Succeed())
212-
})
213-
214162
It("installs the release when it is not already present", func() {
215163
dataset, runtimeObj, profile := mockFluidObjectsForTests(types.NamespacedName{Name: "test-dataset", Namespace: "default"})
216164
runtimeObj.Spec.Fuse = datav1alpha1.ThinFuseSpec{Image: "runtime-fuse"}
@@ -292,14 +240,24 @@ var _ = Describe("Master Tests", func() {
292240
Expect(engine.ifRuntimeHelmValueEnable()).To(BeTrue())
293241
})
294242

295-
It("follows the parsed runtime annotation value", func() {
243+
It("disables runtime helm values when the disable annotation is true", func() {
296244
engine := &ThinEngine{runtime: &datav1alpha1.ThinRuntime{
297245
ObjectMeta: metav1.ObjectMeta{
298-
Annotations: map[string]string{common.AnnotationDisableRuntimeHelmValueConfig: "false"},
246+
Annotations: map[string]string{common.AnnotationDisableRuntimeHelmValueConfig: "true"},
299247
},
300248
}}
301249

302250
Expect(engine.ifRuntimeHelmValueEnable()).To(BeFalse())
303251
})
252+
253+
It("keeps runtime helm values enabled when the disable annotation is false", func() {
254+
engine := &ThinEngine{runtime: &datav1alpha1.ThinRuntime{
255+
ObjectMeta: metav1.ObjectMeta{
256+
Annotations: map[string]string{common.AnnotationDisableRuntimeHelmValueConfig: "false"},
257+
},
258+
}}
259+
260+
Expect(engine.ifRuntimeHelmValueEnable()).To(BeTrue())
261+
})
304262
})
305263
})

‎pkg/ddc/thin/transform_test.go‎

Lines changed: 29 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@
1717
package thin
1818

1919
import (
20-
data1alpha1 "github.com/fluid-cloudnative/fluid/api/v1alpha1"
20+
datav1alpha1 "github.com/fluid-cloudnative/fluid/api/v1alpha1"
2121
"github.com/fluid-cloudnative/fluid/pkg/common"
2222
"github.com/fluid-cloudnative/fluid/pkg/utils/fake"
2323
corev1 "k8s.io/api/core/v1"
@@ -32,9 +32,9 @@ import (
3232

3333
var _ = Describe("ThinEngine transform tests", Label("pkg.ddc.thin.transform_test.go"), func() {
3434
var (
35-
dataset *data1alpha1.Dataset
36-
thinruntime *data1alpha1.ThinRuntime
37-
profile *data1alpha1.ThinRuntimeProfile
35+
dataset *datav1alpha1.Dataset
36+
thinruntime *datav1alpha1.ThinRuntime
37+
profile *datav1alpha1.ThinRuntimeProfile
3838
engine *ThinEngine
3939
resources []runtime.Object
4040
)
@@ -46,7 +46,7 @@ var _ = Describe("ThinEngine transform tests", Label("pkg.ddc.thin.transform_tes
4646
})
4747

4848
JustBeforeEach(func() {
49-
engine.Client = fake.NewFakeClientWithScheme(data1alpha1.UnitTestScheme, resources...)
49+
engine.Client = fake.NewFakeClientWithScheme(datav1alpha1.UnitTestScheme, resources...)
5050
})
5151

5252
Describe("transform", func() {
@@ -58,7 +58,7 @@ var _ = Describe("ThinEngine transform tests", Label("pkg.ddc.thin.transform_tes
5858
})
5959

6060
It("returns a dataset lookup error when dataset is missing", func() {
61-
engine.Client = fake.NewFakeClientWithScheme(data1alpha1.UnitTestScheme, thinruntime, profile)
61+
engine.Client = fake.NewFakeClientWithScheme(datav1alpha1.UnitTestScheme, thinruntime, profile)
6262

6363
value, err := engine.transform(thinruntime, profile)
6464

@@ -85,7 +85,7 @@ var _ = Describe("ThinEngine transform tests", Label("pkg.ddc.thin.transform_tes
8585
Expect(value.ImagePullSecrets).To(Equal(profile.Spec.ImagePullSecrets))
8686
Expect(value.Worker.Enabled).To(BeFalse())
8787
Expect(value.Tolerations).To(Equal(dataset.Spec.Tolerations))
88-
Expect(value.PlacementMode).To(Equal(string(data1alpha1.ExclusiveMode)))
88+
Expect(value.PlacementMode).To(Equal(string(datav1alpha1.ExclusiveMode)))
8989
Expect(value.OwnerDatasetId).To(Equal(dataset.Labels[common.LabelAnnotationDatasetId]))
9090
Expect(value.RuntimeIdentity).To(Equal(common.RuntimeIdentity{
9191
Namespace: thinruntime.Namespace,
@@ -99,8 +99,8 @@ var _ = Describe("ThinEngine transform tests", Label("pkg.ddc.thin.transform_tes
9999
When("worker is enabled", func() {
100100
BeforeEach(func() {
101101
thinruntime.Spec.ImagePullSecrets = []corev1.LocalObjectReference{{Name: "runtime-secret"}}
102-
thinruntime.Spec.TieredStore.Levels = []data1alpha1.Level{{Path: "/runtime/cache"}}
103-
thinruntime.Spec.Worker = data1alpha1.ThinCompTemplateSpec{
102+
thinruntime.Spec.TieredStore.Levels = []datav1alpha1.Level{{Path: "/runtime/cache"}}
103+
thinruntime.Spec.Worker = datav1alpha1.ThinCompTemplateSpec{
104104
Enabled: true,
105105
Image: "runtime-worker",
106106
ImageTag: "runtime-tag",
@@ -109,7 +109,7 @@ var _ = Describe("ThinEngine transform tests", Label("pkg.ddc.thin.transform_tes
109109
Env: []corev1.EnvVar{{Name: "RUNTIME_ENV", Value: "runtime"}},
110110
Ports: []corev1.ContainerPort{{Name: "http", ContainerPort: 8080}},
111111
NodeSelector: map[string]string{"node": "runtime"},
112-
NetworkMode: data1alpha1.HostNetworkMode,
112+
NetworkMode: datav1alpha1.HostNetworkMode,
113113
VolumeMounts: []corev1.VolumeMount{{
114114
Name: "runtime-volume",
115115
MountPath: "/runtime/mount",
@@ -125,7 +125,7 @@ var _ = Describe("ThinEngine transform tests", Label("pkg.ddc.thin.transform_tes
125125
EmptyDir: &corev1.EmptyDirVolumeSource{},
126126
},
127127
}}
128-
profile.Spec.Worker = data1alpha1.ThinCompTemplateSpec{
128+
profile.Spec.Worker = datav1alpha1.ThinCompTemplateSpec{
129129
Image: "profile-worker",
130130
ImageTag: "profile-tag",
131131
ImagePullPolicy: string(corev1.PullAlways),
@@ -174,32 +174,22 @@ var _ = Describe("ThinEngine transform tests", Label("pkg.ddc.thin.transform_tes
174174
})
175175
})
176176

177-
It("handles a missing profile when callers continue after profile lookup not found", func() {
178-
thinruntime.Spec.Fuse.Image = "runtime-fuse"
179-
thinruntime.Spec.Fuse.ImageTag = "runtime-tag"
180-
181-
value, err := engine.transform(thinruntime, nil)
182-
183-
Expect(err).NotTo(HaveOccurred())
184-
Expect(value).NotTo(BeNil())
185-
Expect(value.ImagePullSecrets).To(BeNil())
186-
})
187177
})
188178

189179
Describe("transformWorkers", func() {
190180
var value *ThinValue
191181

192182
BeforeEach(func() {
193183
value = &ThinValue{}
194-
profile.Spec.Worker = data1alpha1.ThinCompTemplateSpec{
184+
profile.Spec.Worker = datav1alpha1.ThinCompTemplateSpec{
195185
Image: "profile-worker",
196186
ImageTag: "profile-tag",
197187
ImagePullPolicy: string(corev1.PullAlways),
198188
ImagePullSecrets: []corev1.LocalObjectReference{{Name: "profile-worker-secret"}},
199189
Env: []corev1.EnvVar{{Name: "PROFILE_ENV", Value: "profile"}},
200190
Ports: []corev1.ContainerPort{{Name: "profile-port", ContainerPort: 9090}},
201191
NodeSelector: map[string]string{"profile": "true"},
202-
NetworkMode: data1alpha1.ContainerNetworkMode,
192+
NetworkMode: datav1alpha1.ContainerNetworkMode,
203193
VolumeMounts: []corev1.VolumeMount{{
204194
Name: "profile-volume",
205195
MountPath: "/profile/mount",
@@ -211,13 +201,13 @@ var _ = Describe("ThinEngine transform tests", Label("pkg.ddc.thin.transform_tes
211201
EmptyDir: &corev1.EmptyDirVolumeSource{},
212202
},
213203
}}
214-
thinruntime.Spec.Worker = data1alpha1.ThinCompTemplateSpec{
204+
thinruntime.Spec.Worker = datav1alpha1.ThinCompTemplateSpec{
215205
ImageTag: "runtime-tag",
216206
ImagePullSecrets: []corev1.LocalObjectReference{{Name: "runtime-worker-secret"}},
217207
Env: []corev1.EnvVar{{Name: "RUNTIME_ENV", Value: "runtime"}},
218208
Ports: []corev1.ContainerPort{{Name: "runtime-port", ContainerPort: 8080}},
219209
NodeSelector: map[string]string{"runtime": "true"},
220-
NetworkMode: data1alpha1.HostNetworkMode,
210+
NetworkMode: datav1alpha1.HostNetworkMode,
221211
VolumeMounts: []corev1.VolumeMount{{
222212
Name: "runtime-volume",
223213
MountPath: "/runtime/mount",
@@ -229,7 +219,7 @@ var _ = Describe("ThinEngine transform tests", Label("pkg.ddc.thin.transform_tes
229219
EmptyDir: &corev1.EmptyDirVolumeSource{},
230220
},
231221
}}
232-
thinruntime.Spec.TieredStore.Levels = []data1alpha1.Level{{Path: "/runtime/cache"}}
222+
thinruntime.Spec.TieredStore.Levels = []datav1alpha1.Level{{Path: "/runtime/cache"}}
233223
})
234224

235225
It("applies profile defaults and runtime overrides", func() {
@@ -249,15 +239,17 @@ var _ = Describe("ThinEngine transform tests", Label("pkg.ddc.thin.transform_tes
249239
Expect(value.Worker.HostNetwork).To(BeTrue())
250240
})
251241

252-
It("swallows worker volume transformation errors", func() {
242+
It("logs but does not return worker volume transformation errors", func() {
243+
thinruntime.Spec.Worker.VolumeMounts = nil
244+
thinruntime.Spec.Volumes = nil
253245
profile.Spec.Worker.VolumeMounts = []corev1.VolumeMount{{Name: "missing-volume", MountPath: "/profile/mount"}}
254246
profile.Spec.Volumes = nil
255247

256248
err := engine.transformWorkers(thinruntime, profile, value)
257249

258250
Expect(err).NotTo(HaveOccurred())
259-
Expect(value.Worker.VolumeMounts).To(ContainElement(thinruntime.Spec.Worker.VolumeMounts[0]))
260-
Expect(value.Worker.Volumes).To(ContainElement(thinruntime.Spec.Volumes[0]))
251+
Expect(value.Worker.VolumeMounts).To(BeEmpty())
252+
Expect(value.Worker.Volumes).To(BeEmpty())
261253
})
262254
})
263255

@@ -275,7 +267,7 @@ var _ = Describe("ThinEngine transform tests", Label("pkg.ddc.thin.transform_tes
275267
})
276268

277269
It("copies worker settings from the profile", func() {
278-
profile.Spec.Worker = data1alpha1.ThinCompTemplateSpec{
270+
profile.Spec.Worker = datav1alpha1.ThinCompTemplateSpec{
279271
Image: "test",
280272
ImageTag: "v1",
281273
ImagePullPolicy: string(corev1.PullAlways),
@@ -312,7 +304,7 @@ var _ = Describe("ThinEngine transform tests", Label("pkg.ddc.thin.transform_tes
312304
SuccessThreshold: 1,
313305
FailureThreshold: 1,
314306
},
315-
NetworkMode: data1alpha1.HostNetworkMode,
307+
NetworkMode: datav1alpha1.HostNetworkMode,
316308
}
317309

318310
engine.parseFromProfile(profile, value)
@@ -342,7 +334,7 @@ var _ = Describe("ThinEngine transform tests", Label("pkg.ddc.thin.transform_tes
342334
ImagePullPolicy: string(corev1.PullAlways),
343335
ImagePullSecrets: []corev1.LocalObjectReference{{Name: "profile-secret"}},
344336
}}
345-
thinruntime.Spec.Worker = data1alpha1.ThinCompTemplateSpec{
337+
thinruntime.Spec.Worker = datav1alpha1.ThinCompTemplateSpec{
346338
ImageTag: "runtime-tag",
347339
ImagePullSecrets: []corev1.LocalObjectReference{{Name: "runtime-secret"}},
348340
}
@@ -360,17 +352,17 @@ var _ = Describe("ThinEngine transform tests", Label("pkg.ddc.thin.transform_tes
360352
It("defaults to exclusive mode when dataset placement mode is empty", func() {
361353
value := &ThinValue{}
362354

363-
engine.transformPlacementMode(&data1alpha1.Dataset{}, value)
355+
engine.transformPlacementMode(&datav1alpha1.Dataset{}, value)
364356

365-
Expect(value.PlacementMode).To(Equal(string(data1alpha1.ExclusiveMode)))
357+
Expect(value.PlacementMode).To(Equal(string(datav1alpha1.ExclusiveMode)))
366358
})
367359

368360
It("keeps an existing dataset placement mode", func() {
369361
value := &ThinValue{}
370362

371-
engine.transformPlacementMode(&data1alpha1.Dataset{Spec: data1alpha1.DatasetSpec{PlacementMode: data1alpha1.ShareMode}}, value)
363+
engine.transformPlacementMode(&datav1alpha1.Dataset{Spec: datav1alpha1.DatasetSpec{PlacementMode: datav1alpha1.ShareMode}}, value)
372364

373-
Expect(value.PlacementMode).To(Equal(string(data1alpha1.ShareMode)))
365+
Expect(value.PlacementMode).To(Equal(string(datav1alpha1.ShareMode)))
374366
})
375367
})
376368

@@ -383,7 +375,7 @@ var _ = Describe("ThinEngine transform tests", Label("pkg.ddc.thin.transform_tes
383375
Value: "b",
384376
}}
385377

386-
engine.transformTolerations(&data1alpha1.Dataset{Spec: data1alpha1.DatasetSpec{Tolerations: tolerations}}, value)
378+
engine.transformTolerations(&datav1alpha1.Dataset{Spec: datav1alpha1.DatasetSpec{Tolerations: tolerations}}, value)
387379

388380
Expect(value.Tolerations).To(Equal(tolerations))
389381
})

0 commit comments

Comments
 (0)