Skip to content
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.

Sign up for GitHub

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

fix: do not trigger reconcile on DSC if no instance in the cluster #1323

Closed
wants to merge 1 commit into from
+16 −18
Closed

fix: do not trigger reconcile on DSC if no instance in the cluster #1323

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 16 additions & 18 deletions controllers/datasciencecluster/datasciencecluster_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,6 +388,19 @@ var configMapPredicates = predicate.Funcs{
},
}

var UninstallconfigMapPredicates = predicate.Funcs{
UpdateFunc: func(e event.UpdateEvent) bool {
// Trigger reconcile function for updated CM in operator namespace
operatorNs, _ := cluster.GetOperatorNamespace()
return e.ObjectNew.GetNamespace() == operatorNs
},
CreateFunc: func(e event.CreateEvent) bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems to be unrelated right ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think the main reason to add this new one UninstallconfigMapPredicates is to keep uninstallation with configmap still working if we remove the 0 dsc instance switch-case.

or i misundertsand this comment?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not very sure how this is expected to work to be honest as now the watch is configured like:

Watches(
    &corev1.ConfigMap{},
    &handler.EnqueueRequestForObject{},
    builder.WithPredicates(UninstallconfigMapPredicates),

Which in my understanding means:

  1. EnqueueRequestForObject would create a request containing the Name and Namespace of the object that is the source of the event, so the config map, which does not map to a DSC
  2. UninstallconfigMapPredicates would only validate that source of the event is a ConfigMap in the same namespace as the operator, effectively discarding any other event on any other deployed ConfigMap.

So I'm not sure if this is the intended behavior.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what i was thinking:

1.Owns(&corev1.ConfigMap{})is for all the configmap that created by Operator
2. Watches(&corev1.ConfigMap{},....) is for the ones that user created by themself.

when user create a new CM in operator namespace with expected label, or they add expected label to CM. => to trigger DSC reconcile=> into if upgrade.HasDeleteConfigMap(ctx, r.Client) {...}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mh, yep but still the enqueue function is not right, it should always map to a DSC

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what i was thinking:

1.Owns(&corev1.ConfigMap{})is for all the configmap that created by Operator 2. Watches(&corev1.ConfigMap{},....) is for the ones that user created by themself.

when user create a new CM in operator namespace with expected label, or they add expected label to CM. => to trigger DSC reconcile=> into if upgrade.HasDeleteConfigMap(ctx, r.Client) {...}

Since Owns is a special kind of Watches and the result anyway is just triggering of the same Reconcile should it be one Watch?

// Trigger reconcile function for created CM in operator namespace
operatorNs, _ := cluster.GetOperatorNamespace()
return e.Object.GetNamespace() == operatorNs
},
}

// reduce unnecessary reconcile triggered by odh component's deployment change due to ManagedByODHOperator annotation.
var componentDeploymentPredicates = predicate.Funcs{
UpdateFunc: func(e event.UpdateEvent) bool {
Expand Down Expand Up @@ -519,10 +532,8 @@ func (r *DataScienceClusterReconciler) SetupWithManager(ctx context.Context, mgr
)).
Watches(
&corev1.ConfigMap{},
handler.EnqueueRequestsFromMapFunc(func(ctx context.Context, a client.Object) []reconcile.Request {
return r.watchDataScienceClusterResources(ctx, a)
}),
builder.WithPredicates(configMapPredicates),
&handler.EnqueueRequestForObject{},
builder.WithPredicates(UninstallconfigMapPredicates),
).
Watches(
&apiextensionsv1.CustomResourceDefinition{},
Expand Down Expand Up @@ -568,19 +579,6 @@ func (r *DataScienceClusterReconciler) watchDataScienceClusterResources(ctx cont
}}
}

// Trigger reconcile function when uninstall configmap is created
operatorNs, err := cluster.GetOperatorNamespace()
if err != nil {
return nil
}
if a.GetNamespace() == operatorNs {
cmLabels := a.GetLabels()
if val, ok := cmLabels[upgrade.DeleteConfigMapLabel]; ok && val == "true" {
return []reconcile.Request{{
NamespacedName: types.NamespacedName{Name: requestName},
}}
}
}
return nil
}

Expand All @@ -595,7 +593,7 @@ func (r *DataScienceClusterReconciler) getRequestName(ctx context.Context) (stri
case len(instanceList.Items) == 1:
return instanceList.Items[0].Name, nil
case len(instanceList.Items) == 0:
return "default-dsc", nil
return "", errors.New("none DataScienceCluster instance found")
default:
return "", errors.New("multiple DataScienceCluster instances found")
}
Expand Down