fix: correctly identify alluxio root mount path in multi-mount datasets#6103
fix: correctly identify alluxio root mount path in multi-mount datasets#6103Priya-Sharma25 wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @Priya-Sharma25. Thanks for your PR. I'm waiting for a fluid-cloudnative member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates GenAlluxioUFSRootPath to iterate through all mount items to find the root path in the cache UFS, rather than only checking when there is a single item, and adds a corresponding test case. A review comment correctly points out that taking the address of the loop variable m (&m) returns a pointer to a local copy of the slice element, which can lead to bugs or inefficiencies. The reviewer suggests iterating by index to safely reference the actual slice element.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| for _, m := range items { | ||
| // iff m matches all of the two following conditions (1) m is not a fluid-native mount point; (2) m.Path is "/", it should be the root path in cache UFS. | ||
| if !common.IsFluidNativeScheme(m.MountPoint) && u.GenUFSPathInUnifiedNamespace(m) == common.RootDirPath { | ||
| return m.MountPoint, &m | ||
| } |
There was a problem hiding this comment.
Taking the address of the loop variable m (&m) returns a pointer to a local copy of the slice element rather than the actual element in the slice. If the caller modifies the returned mount, those changes will be lost. Additionally, copying the entire datav1alpha1.Mount struct on every iteration is inefficient.
To fix this, iterate by index and use a pointer to the slice element.
for i := range items {
m := &items[i]
// iff m matches all of the two following conditions (1) m is not a fluid-native mount point; (2) m.Path is "/", it should be the root path in cache UFS.
if !common.IsFluidNativeScheme(m.MountPoint) && u.GenUFSPathInUnifiedNamespace(*m) == common.RootDirPath {
return m.MountPoint, m
}
|
Could you please take a look when you have a chance? @cheyang @TrafalgarZZZ @xliuqq Thanks! |
Priya-Sharma25
force-pushed
the
fix/issue-3379-multi-mount-root
branch
2 times, most recently
from
cabf68f to
5d568f5
Compare
|
/copilot review |
|
@gemini-code-assist /review |
There was a problem hiding this comment.
Code Review
This pull request updates GenAlluxioUFSRootPath to support identifying the root path in multi-mount scenarios instead of restricting it to single-mount configurations, and adds a corresponding unit test. The feedback suggests updating the function's documentation comment, which is now outdated as it still references the single-mount restriction.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // e.g. alluxio fs mount /underFSStorage / && alluxio fs mount s3://mybucket /mybucket | ||
| // | ||
| // [unified namespace]: https://docs.alluxio.io/os/user/stable/en/core-services/Unified-Namespace.html | ||
| func (u UFSPathBuilder) GenAlluxioUFSRootPath(items []datav1alpha1.Mount) (string, *datav1alpha1.Mount) { |
There was a problem hiding this comment.
The documentation comment for GenAlluxioUFSRootPath (lines 50-53) is now outdated. It still states that the root path is only used if there is only a single mount item defined in dataset.spec.mounts. Since this PR enables identifying the root path in multi-mount scenarios, please update the documentation comment to reflect this new behavior.
|
Clean bug fix. The logic change is correct: iterating all mounts to find the root-path candidate instead of gating on |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6103 +/- ##
=======================================
Coverage 64.76% 64.76%
=======================================
Files 485 485
Lines 33895 33895
=======================================
Hits 21952 21952
Misses 10220 10220
Partials 1723 1723 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Priya-Sharma25
force-pushed
the
fix/issue-3379-multi-mount-root
branch
from
5d568f5 to
120356d
Compare
|
@cheyang , Thanks for the review and catching that! I've updated the GoDoc comment for GenAlluxioUFSRootPath to properly describe the new multi-mount behavior instead of the old single-item restriction. |
|
/copilot review |
|
@gemini-code-assist /review |
There was a problem hiding this comment.
Code Review
This pull request updates GenAlluxioUFSRootPath to iterate through all mount items to find the root path, rather than restricting the check to when only a single mount item exists. A corresponding unit test was also added. The review feedback highlights a potential issue where returning a pointer directly to a slice element (&items[i]) could lead to accidental mutations of the original slice; it is recommended to copy the element by value first and return a pointer to the copy.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| for i := range items { | ||
| m := &items[i] | ||
| // only iff m matches all of the two following conditions (1) m is not a fluid-native mount point; (2) m.Path is "/", it should be the root path in cache UFS. | ||
| if !common.IsFluidNativeScheme(m.MountPoint) && u.GenUFSPathInUnifiedNamespace(m) == common.RootDirPath { | ||
| return m.MountPoint, &m | ||
| if !common.IsFluidNativeScheme(m.MountPoint) && u.GenUFSPathInUnifiedNamespace(*m) == common.RootDirPath { | ||
| return m.MountPoint, m | ||
| } | ||
| } |
There was a problem hiding this comment.
Returning a pointer directly to the slice element (&items[i]) can lead to accidental mutations of the original slice elements (which typically belong to dataset.Spec.Mounts in Kubernetes controllers). This breaks the read-only assumption of the spec and can cause subtle bugs or unexpected reconciliations.
To prevent this, copy the slice element by value first, and then return a pointer to the copy (which will be safely allocated on the heap by Go's escape analysis). This also simplifies the code by avoiding the need to dereference *m when calling GenUFSPathInUnifiedNamespace.
| for i := range items { | |
| m := &items[i] | |
| // only iff m matches all of the two following conditions (1) m is not a fluid-native mount point; (2) m.Path is "/", it should be the root path in cache UFS. | |
| if !common.IsFluidNativeScheme(m.MountPoint) && u.GenUFSPathInUnifiedNamespace(m) == common.RootDirPath { | |
| return m.MountPoint, &m | |
| if !common.IsFluidNativeScheme(m.MountPoint) && u.GenUFSPathInUnifiedNamespace(*m) == common.RootDirPath { | |
| return m.MountPoint, m | |
| } | |
| } | |
| for i := range items { | |
| m := items[i] | |
| // only iff m matches all of the two following conditions (1) m is not a fluid-native mount point; (2) m.Path is "/", it should be the root path in cache UFS. | |
| if !common.IsFluidNativeScheme(m.MountPoint) && u.GenUFSPathInUnifiedNamespace(m) == common.RootDirPath { | |
| return m.MountPoint, &m | |
| } | |
| } |
|
@cheyang , I've addressed your GoDoc comment. The gemini-code-assist bot is suggesting a copy-based pointer return, but since the original code already used &items[0] with the same pattern and you confirmed the pointer handling is sound, I believe this is ready. Please let me know if any further changes are needed! |
There was a problem hiding this comment.
Pull request overview
Fixes AlluxioRuntime root mount selection for datasets with multiple mount entries by scanning all mounts for a unified-namespace root path (/) instead of only handling the single-mount case. This aligns Alluxio root UFS mounting with dataset specs that explicitly set path: "/" on one of several mounts.
Changes:
- Update
GenAlluxioUFSRootPathto iterate over all mount items and return the first eligible mount whose unified-namespace path is/. - Return a pointer to the actual slice element (
&items[i]) when a matching mount is found. - Add a unit test covering multi-mount datasets where one mount explicitly targets root (
path: "/").
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/utils/ufs_path_builder.go | Adjusts Alluxio root UFS selection logic to support multi-mount datasets by scanning all mounts for /. |
| pkg/utils/ufs_path_builder_test.go | Adds a test to ensure root path mounts are correctly identified in multi-mount scenarios. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // 1. If any `mount` item defined in `dataset.spec.mounts[*]` has its ufs path equal to "/", its `mountpoint` should be on the root path. | ||
| // e.g. alluxio fs mount s3://mybucket / |
cheyang
left a comment
cheyang
left a comment
There was a problem hiding this comment.
Thanks for the contribution. I have a concern about the direction of this fix.
The original len(items) == 1 check was intentional: path: "/" (mounting a UFS endpoint directly to the Alluxio root) is only a valid configuration when the Dataset has a single mount. In a multi-mount Dataset, each mount is expected to live under its own sub-path (e.g. /{name}), and the Alluxio root is backed by the default local storage /underFSStorage.
Allowing path: "/" in a multi-mount Dataset introduces ambiguity — what happens to the other mounts that also need to be accessible under the unified namespace root? This is not a supported use case.
Instead of extending GenAlluxioUFSRootPath to iterate over all items, the fix should go in the opposite direction:
- Add validation (either in a webhook or at the controller/engine level) that rejects a Dataset if
len(mounts) > 1and any mount haspath: "/". This is an invalid configuration and the user should receive a clear error message. - Keep the existing single-mount root path logic as-is (or fix minor issues like the pointer-to-copy problem separately).
Could you share the use case that motivated this change? If there's a real scenario where multi-mount + root path is needed, I'd like to understand it better before we loosen this constraint.
|
@cheyang Thank you for the detailed explanation. I want to share the context that motivated this change: Issue #3379 was filed by @xliuqq with a concrete use case: a Dataset with two mounts where one explicitly sets path: "/" to mount a specific UFS endpoint at the Alluxio root, while the other mount lives under its own sub-path (/spark). The expected behavior described in the issue was that the path: "/" mount should be honored, and the issue author identified the len(items) == 1 check as the root cause. I understand your concern about ambiguity. However, in the use case from #3379, there is no conflict — the flink mount takes / and spark takes /spark. They don't overlap. That said, I agree that validation would be valuable as well. Would a combined approach work?
|
|
@Priya-Sharma25 Thanks for the context and for referencing #3379. I understand the use case, but I'd like to stick with my original position here. The example in #3379 works only because the paths happen not to collide —
My suggestion for the fix direction:
I'll also revisit #3379 to update the resolution direction there. cc @xliuqq |
|
@cheyang Thank you for the clear explanation — the implicit overlap risk and the inability to validate remote UFS structures at admission time are convincing points. I'll update this PR to take the validation approach instead: Revert the GenAlluxioUFSRootPath changes back to the original logic |
Priya-Sharma25
force-pushed
the
fix/issue-3379-multi-mount-root
branch
2 times, most recently
from
952a2cb to
4da0740
Compare
|
@cheyang Thank you for the guidance! I've updated the PR to follow your suggested approach. The GenAlluxioUFSRootPath logic has been reverted, and I've added a validation rule in the Dataset controller that sets the Dataset to a Failed phase with an error message when len(mounts) > 1 and path: "/" is used. Let me know if everything looks good! |
|
/copilot review |
|
@gemini-code-assist /review |
There was a problem hiding this comment.
Code Review
This pull request introduces validation to ensure that root-path mounting is only supported for single-mount Datasets, updating the dataset phase to FailedDatasetPhase and adding a corresponding unit test. The review feedback highlights two important issues: first, the validation check can be bypassed if mount.Path is empty (since it defaults to /{Name}), and second, returning a requeue error for a non-transient validation failure when the dataset is already in a failed phase can cause an infinite reconcile loop.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| for _, mount := range ctx.Dataset.Spec.Mounts { | ||
| if mount.Path == common.RootDirPath { |
There was a problem hiding this comment.
The validation check mount.Path == common.RootDirPath can be bypassed if mount.Path is omitted (empty string) and mount.Name is also empty. According to the Mount struct definition in api/v1alpha1/dataset_types.go, if Path is not set, it defaults to /{Name}. If both Path and Name are empty, the effective mount path is / (which is common.RootDirPath), but the current check mount.Path == common.RootDirPath will evaluate to "" == "/", which is false.
To make this validation robust and prevent bypassing, we should resolve the effective path of the mount before comparing it to common.RootDirPath.
| for _, mount := range ctx.Dataset.Spec.Mounts { | |
| if mount.Path == common.RootDirPath { | |
| for _, mount := range ctx.Dataset.Spec.Mounts { | |
| path := mount.Path | |
| if path == "" { | |
| path = "/" + mount.Name | |
| } | |
| if path == common.RootDirPath { |
| if ctx.Dataset.Status.Phase != datav1alpha1.FailedDatasetPhase { | ||
| dataset := ctx.Dataset.DeepCopy() | ||
| dataset.Status.Phase = datav1alpha1.FailedDatasetPhase | ||
| if err := r.Status().Update(ctx, dataset); err != nil { | ||
| ctx.Log.Error(err, "Failed to update the dataset phase to Failed", "StatusUpdateError", ctx) | ||
| } | ||
| } | ||
| return utils.RequeueIfError(err) |
There was a problem hiding this comment.
Returning utils.RequeueIfError(err) for a non-transient validation error when the dataset phase is already FailedDatasetPhase causes the controller to enter an infinite reconcile loop, continuously logging errors and consuming CPU resources.
To prevent this, we can check if the phase is already FailedDatasetPhase and return utils.NoRequeue() to stop reconciling. If the status update fails, we should return the update error to retry the status update.
if ctx.Dataset.Status.Phase == datav1alpha1.FailedDatasetPhase {
return utils.NoRequeue()
}
dataset := ctx.Dataset.DeepCopy()
dataset.Status.Phase = datav1alpha1.FailedDatasetPhase
if err := r.Status().Update(ctx, dataset); err != nil {
ctx.Log.Error(err, "Failed to update the dataset phase to Failed", "StatusUpdateError", ctx)
return utils.RequeueIfError(err)
}
return utils.RequeueIfError(err)
| } | ||
|
|
||
| // 0.5 Validate multiple mounts with root path | ||
| if len(ctx.Dataset.Spec.Mounts) > 1 { |
There was a problem hiding this comment.
This validation sits ahead of the deletion handling in step 1, so it also fires while a Dataset is being torn down. Consider a Dataset that was created valid (so it already picked up the finalizer at step 2), then edited into a multi-mount config where one mount uses /. From that point on, every reconcile returns here via RequeueIfError before ever reaching reconcileDatasetDeletion, which is the only place the finalizer is removed. Once someone deletes that object, the finalizer never gets cleared and it stays stuck in Terminating.
spec.mounts is mutable, so this is reachable in practice: create a valid Dataset, add a second mount at /, then delete it. The name check at step 0 doesn't have this problem because metadata.name is immutable, but the mount list isn't.
Easiest fix is to move this block below the deletion check (after step 1), or to skip it when utils.HasDeletionTimestamp(ctx.Dataset.ObjectMeta) is true, so deletion can always proceed. A minor secondary point: returning RequeueIfError on a spec that can only be fixed by a user edit means the controller keeps re-queuing with backoff and re-emitting the error event; a watch on the spec change will re-trigger reconcile anyway, so returning without an error after setting the Failed phase would be quieter.
| Expect(result).To(Equal(ctrl.Result{})) | ||
| }) | ||
|
|
||
| It("returns error and sets FailedDatasetPhase when dataset has multiple mounts and one has root path", func() { |
There was a problem hiding this comment.
The new case covers create-time rejection and the Failed phase, which is good. It doesn't exercise deletion of an already-finalized multi-mount / Dataset. Once the ordering above is fixed, a case that sets a deletion timestamp on such a Dataset and asserts the finalizer is removed / deletion completes would lock the fix in and guard against the Terminating-stuck regression.
Priya-Sharma25
force-pushed
the
fix/issue-3379-multi-mount-root
branch
from
4da0740 to
fd0c082
Compare
|
@cheyang Thank you both for the excellent feedback and catching those edge cases! I've pushed an update that addresses all the points raised: 1. Deletion Lock Fixed: I moved the validation block down to step 2.5 (after the deletion check and finalizer addition). This ensures that if a Dataset is edited into an invalid state, the validation won't block the controller from removing the finalizer and deleting the object. Let me know if there's anything else you'd like me to adjust! |
Priya-Sharma25
force-pushed
the
fix/issue-3379-multi-mount-root
branch
2 times, most recently
from
cc620af to
7c1d439
Compare
|
/copilot review |
|
@gemini-code-assist /review |
There was a problem hiding this comment.
Code Review
This pull request introduces validation to prevent root-path mounting when multiple mounts are defined in a Dataset, and refactors reference dataset runtime creation into a helper method. It also adds unit tests to verify these validation checks and ensure that invalid datasets can still be deleted. The review feedback highlights a critical issue where a dataset can get stuck in FailedDatasetPhase even after the user corrects the invalid multi-mount configuration. The reviewer suggests implementing a recovery mechanism using a specific dataset condition to reset the phase back to NotBoundDatasetPhase once validation passes, along with corresponding unit tests.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| func (r *DatasetReconciler) validateMultiMountRoot(ctx reconcileRequestContext) (bool, ctrl.Result, error) { | ||
| if len(ctx.Dataset.Spec.Mounts) <= 1 { | ||
| return false, ctrl.Result{}, nil | ||
| } | ||
|
|
||
| for _, mount := range ctx.Dataset.Spec.Mounts { | ||
| effectivePath := mount.Path | ||
| if effectivePath == "" { | ||
| effectivePath = fmt.Sprintf(common.UFSMountPathFormat, strings.TrimLeft(mount.Name, "/")) | ||
| } | ||
| if effectivePath == common.RootDirPath { | ||
| err := errors.New("root-path mounting is only supported for single-mount Datasets") | ||
| ctx.Log.Error(err, "Failed to validate dataset", "DatasetValidationError", ctx) | ||
| r.Recorder.Eventf(&ctx.Dataset, v1.EventTypeWarning, common.ErrorProcessDatasetReason, "Failed to validate dataset because err: %v", err) | ||
|
|
||
| if ctx.Dataset.Status.Phase == datav1alpha1.FailedDatasetPhase { | ||
| res, _ := utils.NoRequeue() | ||
| return true, res, nil | ||
| } | ||
| dataset := ctx.Dataset.DeepCopy() | ||
| dataset.Status.Phase = datav1alpha1.FailedDatasetPhase | ||
| if updateErr := r.Status().Update(ctx, dataset); updateErr != nil { | ||
| ctx.Log.Error(updateErr, "Failed to update the dataset phase to Failed", "StatusUpdateError", ctx) | ||
| res, err := utils.RequeueIfError(updateErr) | ||
| return true, res, err | ||
| } | ||
| res, _ := utils.NoRequeue() | ||
| return true, res, nil | ||
| } | ||
| } | ||
| return false, ctrl.Result{}, nil | ||
| } |
There was a problem hiding this comment.
The current implementation of validateMultiMountRoot sets the dataset phase to FailedDatasetPhase when an invalid root path is detected in a multi-mount dataset. However, if the user subsequently updates the dataset spec to fix the invalid configuration (either by correcting the path or reducing the number of mounts to 1), the dataset will remain stuck in FailedDatasetPhase because there is no mechanism to reset the phase back to NotBoundDatasetPhase once the validation passes.
To resolve this, we can set a specific condition (e.g., with reason InvalidDatasetSpec) when the validation fails. If the validation subsequently passes, we can check for this condition, clear it, and reset the phase to NotBoundDatasetPhase so that reconciliation can proceed normally.
func (r *DatasetReconciler) validateMultiMountRoot(ctx reconcileRequestContext) (bool, ctrl.Result, error) {
hasInvalidRoot := false
var validationErr error
if len(ctx.Dataset.Spec.Mounts) > 1 {
for _, mount := range ctx.Dataset.Spec.Mounts {
effectivePath := mount.Path
if effectivePath == "" {
effectivePath = fmt.Sprintf(common.UFSMountPathFormat, strings.TrimLeft(mount.Name, "/"))
}
if effectivePath == common.RootDirPath {
hasInvalidRoot = true
validationErr = errors.New("root-path mounting is only supported for single-mount Datasets")
break
}
}
}
if hasInvalidRoot {
ctx.Log.Error(validationErr, "Failed to validate dataset", "DatasetValidationError", ctx)
r.Recorder.Eventf(&ctx.Dataset, v1.EventTypeWarning, common.ErrorProcessDatasetReason, "Failed to validate dataset because err: %v", validationErr)
if ctx.Dataset.Status.Phase == datav1alpha1.FailedDatasetPhase {
res, _ := utils.NoRequeue()
return true, res, nil
}
dataset := ctx.Dataset.DeepCopy()
dataset.Status.Phase = datav1alpha1.FailedDatasetPhase
cond := utils.NewDatasetCondition(
datav1alpha1.DatasetReady,
"InvalidDatasetSpec",
validationErr.Error(),
v1.ConditionFalse,
)
dataset.Status.Conditions = utils.UpdateDatasetCondition(dataset.Status.Conditions, cond)
if updateErr := r.Status().Update(ctx, dataset); updateErr != nil {
ctx.Log.Error(updateErr, "Failed to update the dataset phase to Failed", "StatusUpdateError", ctx)
res, err := utils.RequeueIfError(updateErr)
return true, res, err
}
res, _ := utils.NoRequeue()
return true, res, nil
}
// If validation passes, check if we need to recover from a previous InvalidDatasetSpec failure
idx, cond := utils.GetDatasetCondition(ctx.Dataset.Status.Conditions, datav1alpha1.DatasetReady)
if idx != -1 && cond != nil && cond.Reason == "InvalidDatasetSpec" {
dataset := ctx.Dataset.DeepCopy()
dataset.Status.Phase = datav1alpha1.NotBoundDatasetPhase
// Remove the InvalidDatasetSpec condition
var newConditions []datav1alpha1.DatasetCondition
for _, c := range dataset.Status.Conditions {
if c.Type == datav1alpha1.DatasetReady && c.Reason == "InvalidDatasetSpec" {
continue
}
newConditions = append(newConditions, c)
}
dataset.Status.Conditions = newConditions
if updateErr := r.Status().Update(ctx, dataset); updateErr != nil {
ctx.Log.Error(updateErr, "Failed to reset dataset phase from Failed", "StatusUpdateError", ctx)
res, err := utils.RequeueIfError(updateErr)
return true, res, err
}
return true, ctrl.Result{Requeue: true}, nil
}
return false, ctrl.Result{}, nil
}| It("allows deletion of a dataset that was edited into an invalid multi-mount config", func() { | ||
| now := metav1.Now() | ||
| ds := datav1alpha1.Dataset{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "del-invalid", | ||
| Namespace: "default", | ||
| Finalizers: []string{finalizer}, | ||
| DeletionTimestamp: &now, | ||
| }, | ||
| Spec: datav1alpha1.DatasetSpec{ | ||
| Mounts: []datav1alpha1.Mount{ | ||
| {Name: "m1", MountPoint: "local:///path1", Path: "/"}, | ||
| {Name: "m2", MountPoint: "local:///path2", Path: "/path2"}, | ||
| }, | ||
| }, | ||
| } | ||
| r := newTestReconciler(&ds) | ||
| ctx := makeReconcileCtx(r, ds) | ||
|
|
||
| result, err := r.reconcileDataset(ctx, false) | ||
| // Should proceed to deletion, not block on validation | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(result).To(Equal(ctrl.Result{})) | ||
|
|
||
| // Assert the finalizer is removed, demonstrating that the Terminating-stuck regression is fixed. | ||
| // Once the finalizer is removed on an object with a deletion timestamp, the API server (or mock client) deletes it. | ||
| stored := &datav1alpha1.Dataset{} | ||
| getErr := r.Get(ctx, types.NamespacedName{Namespace: "default", Name: "del-invalid"}, stored) | ||
| Expect(apierrors.IsNotFound(getErr)).To(BeTrue()) | ||
| }) |
There was a problem hiding this comment.
Add a unit test to verify that the dataset controller successfully recovers from FailedDatasetPhase back to NotBoundDatasetPhase once the invalid multi-mount configuration is corrected.
It("allows deletion of a dataset that was edited into an invalid multi-mount config", func() {
now := metav1.Now()
ds := datav1alpha1.Dataset{
ObjectMeta: metav1.ObjectMeta{
Name: "del-invalid",
Namespace: "default",
Finalizers: []string{finalizer},
DeletionTimestamp: &now,
},
Spec: datav1alpha1.DatasetSpec{
Mounts: []datav1alpha1.Mount{
{Name: "m1", MountPoint: "local:///path1", Path: "/"},
{Name: "m2", MountPoint: "local:///path2", Path: "/path2"},
},
},
}
r := newTestReconciler(&ds)
ctx := makeReconcileCtx(r, ds)
result, err := r.reconcileDataset(ctx, false)
// Should proceed to deletion, not block on validation
Expect(err).NotTo(HaveOccurred())
Expect(result).To(Equal(ctrl.Result{}))
// Assert the finalizer is removed, demonstrating that the Terminating-stuck regression is fixed.
// Once the finalizer is removed on an object with a deletion timestamp, the API server (or mock client) deletes it.
stored := &datav1alpha1.Dataset{}
getErr := r.Get(ctx, types.NamespacedName{Namespace: "default", Name: "del-invalid"}, stored)
Expect(apierrors.IsNotFound(getErr)).To(BeTrue())
})
It("recovers from FailedDatasetPhase when the invalid multi-mount config is fixed", func() {
ds := datav1alpha1.Dataset{
ObjectMeta: metav1.ObjectMeta{
Name: "recover-invalid",
Namespace: "default",
Finalizers: []string{finalizer},
},
Spec: datav1alpha1.DatasetSpec{
Mounts: []datav1alpha1.Mount{
{Name: "m1", MountPoint: "local:///path1", Path: "/"},
{Name: "m2", MountPoint: "local:///path2", Path: "/path2"},
},
},
Status: datav1alpha1.DatasetStatus{
Phase: datav1alpha1.FailedDatasetPhase,
Conditions: []datav1alpha1.DatasetCondition{
{
Type: datav1alpha1.DatasetReady,
Status: corev1.ConditionFalse,
Reason: "InvalidDatasetSpec",
},
},
},
}
r := newTestReconciler(&ds)
// Now update the spec to be valid
ds.Spec.Mounts[0].Path = "/path1"
ctx := makeReconcileCtx(r, ds)
result, err := r.reconcileDataset(ctx, false)
Expect(err).NotTo(HaveOccurred())
Expect(result).To(Equal(ctrl.Result{Requeue: true}))
stored := &datav1alpha1.Dataset{}
Expect(r.Get(ctx, types.NamespacedName{Namespace: "default", Name: "recover-invalid"}, stored)).To(Succeed())
Expect(stored.Status.Phase).To(Equal(datav1alpha1.NotBoundDatasetPhase))
idx, _ := utils.GetDatasetCondition(stored.Status.Conditions, datav1alpha1.DatasetReady)
Expect(idx).To(Equal(-1))
})
|
Round 4 - both items from last round are addressed. Validation now lives at step 2.5, after the deletion check at step 1, so a finalized Dataset edited into a multi-mount |
| return true, res, nil | ||
| } | ||
| dataset := ctx.Dataset.DeepCopy() | ||
| dataset.Status.Phase = datav1alpha1.FailedDatasetPhase |
There was a problem hiding this comment.
Moving this to step 2.5 (after the deletion handling at step 1) does fix the Terminating-stuck case, and the new delete-path test proves the finalizer still gets removed - thanks for that.
One thing is still open though: this is a one-way door into FailedDatasetPhase. When validation fails you flip the phase to Failed here, but the only code that ever moves a Dataset out of Failed is step 4, and that transition is gated on phase == NoneDatasetPhase. So if a user corrects the spec (drops the extra mount, or gives the root mount a real path), validateMultiMountRoot now returns cleanly, but the phase stays Failed and the Dataset never reconciles on to NotBound - it's silently stuck.
Since the spec is the source of truth, I think we should let it recover: when validation passes and the phase is Failed, reset it back to NotBound. Tagging the failure with a specific reason (e.g. InvalidDatasetSpec) so the reset only clears failures this check owns, rather than stomping other Failed causes, would keep it safe. Could you also add a unit test for the invalid->fixed recovery path, in the same style as the delete test?
Priya-Sharma25
force-pushed
the
fix/issue-3379-multi-mount-root
branch
from
7c1d439 to
28a1c36
Compare
Signed-off-by: Priya Sharma <priyasharma1001a@gmail.com> Signed-off-by: Priya Sharma <priyasharma1001a@gmail.com>
Priya-Sharma25
force-pushed
the
fix/issue-3379-multi-mount-root
branch
from
28a1c36 to
ad1d7e9
Compare
|
|
@cheyang Thank you for the review! I've implemented the recovery mechanism. Now, when validation fails, it sets an InvalidDatasetSpec condition. If the user corrects the spec, the controller detects this, removes the condition, and resets the phase to NotBoundDatasetPhase so it's no longer stuck. I also added the corresponding unit test for this recovery path. Additionally, I refactored validateMultiMountRoot into smaller helper functions to successfully reduce its Cognitive Complexity and resolve the SonarCloud failure. Let me know if everything looks good! |
Ⅰ. Describe what this PR does
This PR fixes a bug where
AlluxioRuntimefails to correctly mount the root path (/) if multiple mount points are defined in the dataset. Previously,GenAlluxioUFSRootPathonly checked for the root path iflen(items) == 1, defaulting to/underFSStoragefor multi-mount scenarios. This changes the logic to iterate through all mount items to accurately identify the root path.Ⅱ. Does this pull request fix one issue?
fixes #3379
Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
Added
test multi mount item with root pathtoTestGenUFSRootPathForAlluxioinpkg/utils/ufs_path_builder_test.goto ensure root paths are successfully identified in multi-mount scenarios.Ⅳ. Describe how to verify it
Run unit tests for
ufs_path_builder_test.go:go test ./pkg/utils/ -run "TestGenUFSRootPathForAlluxio" -vⅤ. Special notes for reviews
NONE