fix: do not trigger reconcile on DSC if no instance in the cluster #1323
Conversation
|
Skipping CI for Draft Pull Request. |
|
[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.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## incubation #1323 +/- ##
=============================================
Coverage ? 19.10%
=============================================
Files ? 30
Lines ? 3365
Branches ? 0
=============================================
Hits ? 643
Misses ? 2653
Partials ? 69 ☔ View full report in Codecov by Sentry. |
| @@ -384,8 +384,29 @@ var configMapPredicates = predicate.Funcs{ | |||
| if e.ObjectNew.GetName() == "inferenceservice-config" && (namespace == "redhat-ods-applications" || namespace == "opendatahub") { //nolint:goconst | |||
| return false | |||
| } | |||
| // Trigger reconcile function when uninstall configmap has this label | |||
There was a problem hiding this comment.
this seems to be unrelated right ?
There was a problem hiding this comment.
i have a feeling.......... this is not the code on my local branch :D
let me push it and see what the changes :D
| return true | ||
| }, | ||
| CreateFunc: func(e event.CreateEvent) bool { |
There was a problem hiding this comment.
this seems to be unrelated right ?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
EnqueueRequestForObjectwould create a request containing theNameandNamespaceof the object that is the source of the event, so the config map, which does not map to aDSCUninstallconfigMapPredicateswould only validate that source of the event is aConfigMapin the same namespace as the operator, effectively discarding any other event on any other deployedConfigMap.
So I'm not sure if this is the intended behavior.
There was a problem hiding this comment.
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) {...}
There was a problem hiding this comment.
mh, yep but still the enqueue function is not right, it should always map to a DSC
There was a problem hiding this comment.
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?
zdtsw
force-pushed
the
chore_135
branch
2 times, most recently
from
42914a4 to
b4825c8
Compare
- remove check on zeor DSC instance to not continue recontile with dummy name - uninstall is handled by UninstallconfigMapPredicates:when no DSC but CM for uninstall is created or updated Signed-off-by: Wen Zhou <[email protected]>
Description
configMapPredicates: CM for is created or updated in operator namespacewatchDataScienceClusterResourcesfor CM in operator namespace and with label : defer toconfigMapPredicatesandHasDeleteConfigMapgetRquestNameHow Has This Been Tested?
local build: quay.io/wenzhou/opendatahub-operator:2.16.1029-2
Screenshot or short clip
Merge criteria