-
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 7 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,13 +114,23 @@ 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) | ||
if err != nil { | ||
func CheckReferenceDatasetRuntime(ctx cruntime.ReconcileRequestContext, runtime *datav1alpha1.ThinRuntime) (bool, error) { | ||
dataset, err := utils.GetDataset(ctx.Client, runtime.Name, runtime.Namespace) | ||
if err != nil && utils.IgnoreNotFound(err) != nil { | ||
// return if it is not a not-found error | ||
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, |
||
} | ||
|
||
mounted := base.GetMountedDatasetNamespacedName(dataset) | ||
var mounted []types.NamespacedName | ||
if dataset != nil { | ||
// getMountedDataset from dataset first | ||
ctx.Log.V(1).Info("Get physical dataset from virtual dataset mounts") | ||
mounted = base.GetPhysicalDatasetFromMounts(dataset.Spec.Mounts) | ||
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. I suggest adding more logging info for debugging. 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. Done. |
||
} else if len(runtime.Status.Mounts) != 0 { | ||
// Virtual dataset not found, try to getMountedDataset from runtime | ||
ctx.Log.V(1).Info("Virtual dataset not found, try to get physical dataset from runtime mounts") | ||
mounted = base.GetPhysicalDatasetFromMounts(runtime.Status.Mounts) | ||
} | ||
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. |
||
// not mount other datasets | ||
if len(mounted) == 0 { | ||
return false, nil | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,6 +33,11 @@ func (e *ReferenceDatasetEngine) getMountedDatasetRuntimeStatus() (status *datav | |
return status, err | ||
} | ||
|
||
// if mountedRuntimeInfo is nil and no err, the runtime is deleting. | ||
if mountedRuntimeInfo == nil { | ||
return nil, nil | ||
} | ||
|
||
return base.GetRuntimeStatus(e.Client, mountedRuntimeInfo.GetRuntimeType(), | ||
mountedRuntimeInfo.GetName(), mountedRuntimeInfo.GetNamespace()) | ||
} | ||
|
@@ -91,22 +96,40 @@ func (e *ReferenceDatasetEngine) getRuntimeInfo() (base.RuntimeInfoInterface, er | |
e.Log.Info("Setup with dataset done", "exclusive", e.runtimeInfo.IsExclusive()) | ||
|
||
return e.runtimeInfo, nil | ||
|
||
} | ||
|
||
// getMountedRuntimeInfo get mountedRuntimeInfo from dataset. | ||
// If could not get dataset, getMountedRuntimeInfo try to get mountedRuntimeInfo from runtime status. | ||
func (e *ReferenceDatasetEngine) getMountedRuntimeInfo() (base.RuntimeInfoInterface, error) { | ||
if e.mountedRuntimeInfo != nil { | ||
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. Please add comment: // If already have mountedRuntimeInfo, return it directly |
||
return e.mountedRuntimeInfo, nil | ||
} | ||
|
||
dataset, err := utils.GetDataset(e.Client, e.name, e.namespace) | ||
runtime, err := e.getRuntime() | ||
if err != nil { | ||
return e.mountedRuntimeInfo, err | ||
} | ||
|
||
mountedNameSpacedNames := base.GetMountedDatasetNamespacedName(dataset) | ||
dataset, err := utils.GetDataset(e.Client, e.name, e.namespace) | ||
if err != nil && utils.IgnoreNotFound(err) != nil { | ||
// return if it is not a not-found error | ||
return e.mountedRuntimeInfo, err | ||
} | ||
|
||
var mountedNameSpacedNames []types.NamespacedName | ||
if dataset != nil { | ||
// get mountedRuntimeInfo from dataset first | ||
mountedNameSpacedNames = base.GetPhysicalDatasetFromMounts(dataset.Spec.Mounts) | ||
} else if len(runtime.Status.Mounts) != 0 { | ||
// then try to get mountedRuntimeInfo from runtime status | ||
mountedNameSpacedNames = base.GetPhysicalDatasetFromMounts(runtime.Status.Mounts) | ||
} else { | ||
// err can only be not-found error | ||
return e.mountedRuntimeInfo, err | ||
} | ||
|
||
if len(mountedNameSpacedNames) != 1 { | ||
return e.mountedRuntimeInfo, fmt.Errorf("ThinEngine with no profile name can only handle dataset only mounting one dataset") | ||
return e.mountedRuntimeInfo, fmt.Errorf("ThinEngine with no profile name can only handle dataset only mounting one dataset but get %v", len(mountedNameSpacedNames)) | ||
} | ||
namespacedName := mountedNameSpacedNames[0] | ||
|
||
|
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.