-
Notifications
You must be signed in to change notification settings - Fork 950
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bugfix: reconcile thinruntime failed when dataset is deleted #3300
Changes from all commits
d8803fd
7c17839
b1502eb
bab8073
d8b9502
be22993
23cfbbb
bab35e0
bfe7774
0d84355
21548bb
c3a4e09
a6a2f41
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -61,7 +61,7 @@ func Build(id string, ctx cruntime.ReconcileRequestContext) (base.Engine, error) | |
return nil, fmt.Errorf("engine %s is failed due to type conversion", ctx.Name) | ||
} | ||
|
||
isRef, err := CheckReferenceDatasetRuntime(ctx.Client, runtime) | ||
isRef, err := CheckReferenceDatasetRuntime(ctx, runtime) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
@@ -114,17 +114,26 @@ func Precheck(client client.Client, key types.NamespacedName) (found bool, err e | |
} | ||
|
||
// CheckReferenceDatasetRuntime judge if this runtime is used for handling dataset mounting another dataset. | ||
func CheckReferenceDatasetRuntime(client client.Client, runtime *datav1alpha1.ThinRuntime) (bool, error) { | ||
dataset, err := utils.GetDataset(client, runtime.Name, runtime.Namespace) | ||
func CheckReferenceDatasetRuntime(ctx cruntime.ReconcileRequestContext, runtime *datav1alpha1.ThinRuntime) (bool, error) { | ||
if len(runtime.Status.Mounts) != 0 { | ||
// get physical dataset from runtime mounts | ||
ctx.Log.V(1).Info("Get physical dataset from runtime mounts") | ||
physicalDataset := base.GetPhysicalDatasetFromMounts(runtime.Status.Mounts) | ||
if len(physicalDataset) != 0 { | ||
return true, nil | ||
} | ||
} | ||
|
||
dataset, err := utils.GetDataset(ctx.Client, runtime.Name, runtime.Namespace) | ||
if err != nil { | ||
return false, err | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should not return every error here because in cases where There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this case, |
||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What will happen if dataset is not found and the length of runtime mounts is 0? How will the user handle this situation? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The case will be protected by checking existence of reference datasets before removing any physical dataset. This can be done in the next PR. |
||
|
||
mounted := base.GetMountedDatasetNamespacedName(dataset) | ||
// not mount other datasets | ||
if len(mounted) == 0 { | ||
return false, nil | ||
// get physicalDataset from dataset | ||
ctx.Log.V(1).Info("Get physical dataset from virtual dataset mounts") | ||
physicalDataset := base.GetPhysicalDatasetFromMounts(dataset.Spec.Mounts) | ||
if len(physicalDataset) != 0 { | ||
return true, nil | ||
} | ||
|
||
return true, nil | ||
return false, nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe in future we can simply check
len(runtime.profileName) == 0
to indicate whether it is a VirtualRuntime or ThinRuntime instead of checking all the dataset mounts.