Skip to content

Commit fd0c082

Browse files
fix: reject multiple mounts with root path for datasets
Signed-off-by: Priya Sharma <priyasharma1001a@gmail.com> Signed-off-by: Priya Sharma <priyasharma1001a@gmail.com>
1 parent afca615 commit fd0c082

2 files changed

Lines changed: 120 additions & 1 deletion

File tree

pkg/controllers/v1alpha1/dataset/dataset_controller.go

Lines changed: 33 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ package dataset
1717
import (
1818
"context"
1919
"errors"
20+
"fmt"
2021
"reflect"
2122
"strings"
2223
"time"
@@ -137,11 +138,42 @@ func (r *DatasetReconciler) reconcileDataset(ctx reconcileRequestContext, needRe
137138
return r.reconcileDatasetDeletion(ctx)
138139
}
139140

140-
// 2.Add finalizer
141+
// 2. Add finalizer
141142
if !utils.ContainsString(ctx.Dataset.ObjectMeta.GetFinalizers(), finalizer) {
142143
return r.addFinalizerAndRequeue(ctx)
143144
}
144145

146+
// 2.5 Validate multiple mounts with root path.
147+
// This check must come after deletion handling (step 1) so that a Dataset
148+
// edited into an invalid state can still be deleted and have its finalizer removed.
149+
if len(ctx.Dataset.Spec.Mounts) > 1 {
150+
for _, mount := range ctx.Dataset.Spec.Mounts {
151+
// Resolve the effective mount path: if mount.Path is explicitly "/", or
152+
// if both mount.Path and mount.Name are empty (which defaults to "/"),
153+
// the mount targets the root of the unified namespace.
154+
effectivePath := mount.Path
155+
if effectivePath == "" {
156+
effectivePath = fmt.Sprintf(common.UFSMountPathFormat, strings.TrimLeft(mount.Name, "/"))
157+
}
158+
if effectivePath == common.RootDirPath {
159+
err := errors.New("root-path mounting is only supported for single-mount Datasets")
160+
ctx.Log.Error(err, "Failed to validate dataset", "DatasetValidationError", ctx)
161+
r.Recorder.Eventf(&ctx.Dataset, v1.EventTypeWarning, common.ErrorProcessDatasetReason, "Failed to validate dataset because err: %v", err)
162+
163+
if ctx.Dataset.Status.Phase == datav1alpha1.FailedDatasetPhase {
164+
return utils.NoRequeue()
165+
}
166+
dataset := ctx.Dataset.DeepCopy()
167+
dataset.Status.Phase = datav1alpha1.FailedDatasetPhase
168+
if updateErr := r.Status().Update(ctx, dataset); updateErr != nil {
169+
ctx.Log.Error(updateErr, "Failed to update the dataset phase to Failed", "StatusUpdateError", ctx)
170+
return utils.RequeueIfError(updateErr)
171+
}
172+
return utils.NoRequeue()
173+
}
174+
}
175+
}
176+
145177
// 3. Create Runtime if it's reference dataset
146178
checkReferenceDataset, err := base.CheckReferenceDataset(&ctx.Dataset)
147179
if err != nil {

pkg/controllers/v1alpha1/dataset/dataset_reconciler_test.go

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -277,6 +277,93 @@ var _ = Describe("DatasetReconciler (fake client)", func() {
277277
Expect(err).To(HaveOccurred())
278278
Expect(result).To(Equal(ctrl.Result{}))
279279
})
280+
281+
It("sets FailedDatasetPhase and stops requeue when dataset has multiple mounts and one has root path", func() {
282+
ds := datav1alpha1.Dataset{
283+
ObjectMeta: metav1.ObjectMeta{
284+
Name: "multi-root",
285+
Namespace: "default",
286+
Finalizers: []string{finalizer},
287+
},
288+
Spec: datav1alpha1.DatasetSpec{
289+
Mounts: []datav1alpha1.Mount{
290+
{Name: "m1", MountPoint: "local:///path1", Path: "/"},
291+
{Name: "m2", MountPoint: "local:///path2", Path: "/path2"},
292+
},
293+
},
294+
Status: datav1alpha1.DatasetStatus{Phase: datav1alpha1.NotBoundDatasetPhase},
295+
}
296+
r := newTestReconciler(&ds)
297+
ctx := makeReconcileCtx(r, ds)
298+
299+
result, err := r.reconcileDataset(ctx, false)
300+
// NoRequeue: no error, empty result
301+
Expect(err).NotTo(HaveOccurred())
302+
Expect(result).To(Equal(ctrl.Result{}))
303+
304+
// Verify status phase is set to FailedDatasetPhase
305+
stored := &datav1alpha1.Dataset{}
306+
Expect(r.Get(ctx, types.NamespacedName{Namespace: "default", Name: "multi-root"}, stored)).To(Succeed())
307+
Expect(stored.Status.Phase).To(Equal(datav1alpha1.FailedDatasetPhase))
308+
})
309+
310+
It("catches implicit root path when mount.Path and mount.Name are both empty", func() {
311+
ds := datav1alpha1.Dataset{
312+
ObjectMeta: metav1.ObjectMeta{
313+
Name: "implicit-root",
314+
Namespace: "default",
315+
Finalizers: []string{finalizer},
316+
},
317+
Spec: datav1alpha1.DatasetSpec{
318+
Mounts: []datav1alpha1.Mount{
319+
{MountPoint: "local:///path1"},
320+
{Name: "m2", MountPoint: "local:///path2", Path: "/path2"},
321+
},
322+
},
323+
Status: datav1alpha1.DatasetStatus{Phase: datav1alpha1.NotBoundDatasetPhase},
324+
}
325+
r := newTestReconciler(&ds)
326+
ctx := makeReconcileCtx(r, ds)
327+
328+
result, err := r.reconcileDataset(ctx, false)
329+
Expect(err).NotTo(HaveOccurred())
330+
Expect(result).To(Equal(ctrl.Result{}))
331+
332+
stored := &datav1alpha1.Dataset{}
333+
Expect(r.Get(ctx, types.NamespacedName{Namespace: "default", Name: "implicit-root"}, stored)).To(Succeed())
334+
Expect(stored.Status.Phase).To(Equal(datav1alpha1.FailedDatasetPhase))
335+
})
336+
337+
It("allows deletion of a dataset that was edited into an invalid multi-mount config", func() {
338+
now := metav1.Now()
339+
ds := datav1alpha1.Dataset{
340+
ObjectMeta: metav1.ObjectMeta{
341+
Name: "del-invalid",
342+
Namespace: "default",
343+
Finalizers: []string{finalizer},
344+
DeletionTimestamp: &now,
345+
},
346+
Spec: datav1alpha1.DatasetSpec{
347+
Mounts: []datav1alpha1.Mount{
348+
{Name: "m1", MountPoint: "local:///path1", Path: "/"},
349+
{Name: "m2", MountPoint: "local:///path2", Path: "/path2"},
350+
},
351+
},
352+
}
353+
r := newTestReconciler(&ds)
354+
ctx := makeReconcileCtx(r, ds)
355+
356+
result, err := r.reconcileDataset(ctx, false)
357+
// Should proceed to deletion, not block on validation
358+
Expect(err).NotTo(HaveOccurred())
359+
Expect(result).To(Equal(ctrl.Result{}))
360+
361+
// Assert the finalizer is removed, demonstrating that the Terminating-stuck regression is fixed.
362+
// Once the finalizer is removed on an object with a deletion timestamp, the API server (or mock client) deletes it.
363+
stored := &datav1alpha1.Dataset{}
364+
getErr := r.Get(ctx, types.NamespacedName{Namespace: "default", Name: "del-invalid"}, stored)
365+
Expect(apierrors.IsNotFound(getErr)).To(BeTrue())
366+
})
280367
})
281368

282369
Describe("reconcileDatasetDeletion", func() {

0 commit comments

Comments
 (0)