[RHOAIENG-48289] Don't reconcile labels we don't own#779
Open
jstourac wants to merge 1 commit intoopendatahub-io:mainfrom
Open
[RHOAIENG-48289] Don't reconcile labels we don't own#779jstourac wants to merge 1 commit intoopendatahub-io:mainfrom
jstourac wants to merge 1 commit intoopendatahub-io:mainfrom
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 |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThis pull request changes reconciliation behavior across multiple controllers and helper utilities to merge controller-managed labels and annotations from desired resources into existing ones instead of replacing entire maps. Files changed include reconcilehelper/util.go (CopyStatefulSetFields, CopyDeploymentSetFields, CopyServiceFields) and several notebook-related controllers (notebook_controller, odh-notebook-controller: route, referencegrant, network, kube-rbac proxy, dspa secret, tests). Reconciliation now initializes nil maps and copies only managed keys from source into destination, preserving non-managed keys; specs and replica checks remain part of update detection. New unit tests verify preservation of extra (non-managed) labels. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Critical Issues
Moderate Issues
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Member
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
/group-test |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/odh-notebook-controller/controllers/notebook_dspa_secret.go (1)
395-407:⚠️ Potential issue | 🟠 MajorAdd conflict retry loop to Secret update for concurrent external mutations.
Line 405: Single
cli.Update()on a shared resource that external tooling may also mutate will fail transiently on conflict errors. In webhook-triggered reconciliation paths, this blocks notebook admission. Wrap the update inretry.RetryOnConflict(retry.DefaultRetry, func() error {...})with a freshGetper attempt to re-apply mutations on the latest object state.Proposed fix
import ( "context" "encoding/json" "fmt" "reflect" "github.com/go-logr/logr" nbv1 "github.com/kubeflow/kubeflow/components/notebook-controller/api/v1" dspav1 "github.com/opendatahub-io/data-science-pipelines-operator/api/v1" routev1 "github.com/openshift/api/route/v1" corev1 "k8s.io/api/core/v1" apierrs "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" + "k8s.io/client-go/util/retry" "k8s.io/client-go/dynamic" "k8s.io/client-go/rest" "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" ) if requiresUpdate { log.Info("Updating existing Elyra runtime config secret", "name", elyraRuntimeSecretName) - // Set controller-managed label and data. Preserve any other labels that - // may have been added by external tooling. - if existingSecret.Labels == nil { - existingSecret.Labels = map[string]string{} - } - existingSecret.Labels[managedByKey] = managedByValue - existingSecret.Data = desiredSecret.Data - - if err := cli.Update(ctx, existingSecret); err != nil { + if err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + latest := &corev1.Secret{} + if err := cli.Get(ctx, client.ObjectKey{ + Name: elyraRuntimeSecretName, Namespace: notebook.Namespace, + }, latest); err != nil { + return err + } + // Set controller-managed label and data. Preserve any other labels that + // may have been added by external tooling. + if latest.Labels == nil { + latest.Labels = map[string]string{} + } + latest.Labels[managedByKey] = managedByValue + latest.Data = desiredSecret.Data + return cli.Update(ctx, latest) + }); err != nil { log.Error(err, "Failed to update existing Elyra runtime config secret") return err } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/odh-notebook-controller/controllers/notebook_dspa_secret.go` around lines 395 - 407, The update of the shared Secret (existingSecret) should be retried on conflicts: wrap the cli.Update call inside retry.RetryOnConflict(retry.DefaultRetry, func() error { ... }), and in each attempt re-fetch the latest Secret via cli.Get, re-apply controller-managed label (managedByKey = managedByValue) and set Data from desiredSecret.Data before calling cli.Update; on persistent error log via log.Error and return the error from the retry wrapper.
🧹 Nitpick comments (2)
components/common/reconcilehelper/util.go (1)
109-134: Extract shared map-merge reconciliation helperThe same managed-key merge logic for labels/annotations is duplicated in three functions. Centralizing it reduces divergence risk.
Proposed refactor sketch
+func mergeManagedStringMap(dst map[string]string, src map[string]string) (map[string]string, bool) { + updated := false + if dst == nil { + dst = map[string]string{} + } + for k, v := range src { + if dst[k] != v { + updated = true + } + dst[k] = v + } + return dst, updated +}Then call this helper for
LabelsandAnnotationsin:
CopyStatefulSetFieldsCopyDeploymentSetFieldsCopyServiceFieldsAs per coding guidelines,
**: REVIEW PRIORITIES: 2. Architectural issues and anti-patterns.Also applies to: 151-173, 191-213
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/common/reconcilehelper/util.go` around lines 109 - 134, The label/annotation merge logic is duplicated; extract it into a single helper (e.g., reconcileManagedMap or mergeManagedMap) that takes (from map[string]string, to *map[string]string) and returns whether an update is required, then replace the duplicated blocks in CopyStatefulSetFields, CopyDeploymentSetFields, and CopyServiceFields to call that helper for both Labels and Annotations (preserve existing extra keys on `to`, create `to` if nil, copy controller-owned keys from `from`, and accumulate the requireUpdate result).components/odh-notebook-controller/controllers/notebook_referencegrant.go (1)
75-78: Centralize managed ReferenceGrant label keysManaged label keys are duplicated between compare and reconcile paths. A future one-sided edit will desynchronize detection vs remediation.
Proposed refactor
+var notebookReferenceGrantManagedLabelKeys = []string{ + "app.kubernetes.io/managed-by", + "opendatahub.io/component", +} func CompareNotebookReferenceGrants(rg1 gatewayv1beta1.ReferenceGrant, rg2 gatewayv1beta1.ReferenceGrant) bool { - managedLabelKeys := []string{ - "app.kubernetes.io/managed-by", - "opendatahub.io/component", - } - for _, k := range managedLabelKeys { + for _, k := range notebookReferenceGrantManagedLabelKeys { v1, ok1 := rg1.Labels[k] v2, ok2 := rg2.Labels[k] if ok1 != ok2 || v1 != v2 { return false } } return reflect.DeepEqual(rg1.Spec, rg2.Spec) } - for _, k := range []string{"app.kubernetes.io/managed-by", "opendatahub.io/component"} { + for _, k := range notebookReferenceGrantManagedLabelKeys { if desiredRefGrant.Labels != nil { if v, ok := desiredRefGrant.Labels[k]; ok { foundRefGrant.Labels[k] = v } } }As per coding guidelines,
**: REVIEW PRIORITIES: 2. Architectural issues and anti-patterns.Also applies to: 132-133
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/odh-notebook-controller/controllers/notebook_referencegrant.go` around lines 75 - 78, Extract the duplicated slice literal managedLabelKeys into a single shared package-level variable or constant (e.g., managedLabelKeys or ManagedLabelKeys) and replace both inline occurrences (the current managedLabelKeys declarations used in compare and reconcile paths) to reference that shared identifier; ensure the shared identifier is declared once at the top of the file (or in a common file in the same package) so both the compare and reconcile code paths use the same source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/odh-notebook-controller/controllers/notebook_controller_test.go`:
- Around line 156-160: The test creates shardedNotebook (shardedNotebookName,
created via createNotebook and cli.Create) but only deletes it much later, so
failures leave resources behind; immediately after successfully creating
shardedNotebook call a cleanup to always remove it (use t.Cleanup or defer a
deletion that calls cli.Delete with the same ctx and shardedNotebook) so the
resource is deleted on test exit/failure; update the block around
createNotebook/cli.Create to register that cleanup right after the
Expect(cli.Create(...)).Should(Succeed()) to guarantee removal even if later
assertions fail.
In `@components/odh-notebook-controller/controllers/notebook_controller.go`:
- Around line 115-119: The comparison uses the embedded ObjectMeta field
directly which triggers staticcheck QF1008; update the equality check in the
notebook reconciliation (the code comparing nb1 and nb2) to use the promoted
field names—replace nb1.ObjectMeta.Annotations with nb1.Annotations and
nb2.ObjectMeta.Annotations with nb2.Annotations in the reflect.DeepEqual
expression so the annotations comparison reads
reflect.DeepEqual(nb1.Annotations, nb2.Annotations) &&
reflect.DeepEqual(nb1.Spec, nb2.Spec).
In `@components/odh-notebook-controller/controllers/notebook_route.go`:
- Around line 144-145: The lint error is caused by accessing the embedded
ObjectMeta field explicitly; replace uses of r1.ObjectMeta.Labels and
r2.ObjectMeta.Labels with the promoted Labels field (r1.Labels and r2.Labels) in
the comparison logic where v1, ok1 := r1.ObjectMeta.Labels[k] and v2, ok2 :=
r2.ObjectMeta.Labels[k] are set; update those expressions to v1, ok1 :=
r1.Labels[k] and v2, ok2 := r2.Labels[k] so the code uses the promoted field and
resolves the QF1008 staticcheck warning.
---
Outside diff comments:
In `@components/odh-notebook-controller/controllers/notebook_dspa_secret.go`:
- Around line 395-407: The update of the shared Secret (existingSecret) should
be retried on conflicts: wrap the cli.Update call inside
retry.RetryOnConflict(retry.DefaultRetry, func() error { ... }), and in each
attempt re-fetch the latest Secret via cli.Get, re-apply controller-managed
label (managedByKey = managedByValue) and set Data from desiredSecret.Data
before calling cli.Update; on persistent error log via log.Error and return the
error from the retry wrapper.
---
Nitpick comments:
In `@components/common/reconcilehelper/util.go`:
- Around line 109-134: The label/annotation merge logic is duplicated; extract
it into a single helper (e.g., reconcileManagedMap or mergeManagedMap) that
takes (from map[string]string, to *map[string]string) and returns whether an
update is required, then replace the duplicated blocks in CopyStatefulSetFields,
CopyDeploymentSetFields, and CopyServiceFields to call that helper for both
Labels and Annotations (preserve existing extra keys on `to`, create `to` if
nil, copy controller-owned keys from `from`, and accumulate the requireUpdate
result).
In `@components/odh-notebook-controller/controllers/notebook_referencegrant.go`:
- Around line 75-78: Extract the duplicated slice literal managedLabelKeys into
a single shared package-level variable or constant (e.g., managedLabelKeys or
ManagedLabelKeys) and replace both inline occurrences (the current
managedLabelKeys declarations used in compare and reconcile paths) to reference
that shared identifier; ensure the shared identifier is declared once at the top
of the file (or in a common file in the same package) so both the compare and
reconcile code paths use the same source of truth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 2b6676fb-bbb7-456b-b960-a2d0c07c7d59
📒 Files selected for processing (10)
components/common/reconcilehelper/util.gocomponents/notebook-controller/controllers/notebook_controller.gocomponents/odh-notebook-controller/controllers/notebook_controller.gocomponents/odh-notebook-controller/controllers/notebook_controller_test.gocomponents/odh-notebook-controller/controllers/notebook_dspa_secret.gocomponents/odh-notebook-controller/controllers/notebook_kube_rbac_auth.gocomponents/odh-notebook-controller/controllers/notebook_network.gocomponents/odh-notebook-controller/controllers/notebook_referencegrant.gocomponents/odh-notebook-controller/controllers/notebook_route.gocomponents/odh-notebook-controller/controllers/notebook_route_compare_test.go
Comment on lines
+156
to
+160
| By("By creating a new Notebook dedicated to this test") | ||
| shardedNotebookName := "test-notebook-shard-label" | ||
| shardedNotebook := createNotebook(shardedNotebookName, Namespace) | ||
| Expect(cli.Create(ctx, shardedNotebook)).Should(Succeed()) | ||
|
|
There was a problem hiding this comment.
Make test cleanup failure-safe
Line 159 creates shardedNotebook, but cleanup happens only at Line 247. Any earlier failed assertion leaves resources behind and can cascade failures.
Proposed patch
shardedNotebook := createNotebook(shardedNotebookName, Namespace)
Expect(cli.Create(ctx, shardedNotebook)).Should(Succeed())
+ defer func() {
+ _ = cli.Delete(ctx, shardedNotebook)
+ }()
@@
- By("By deleting the Notebook created for this test")
- Expect(cli.Delete(ctx, shardedNotebook)).Should(Succeed())As per coding guidelines, **: REVIEW PRIORITIES: 3. Bug-prone patterns and error handling gaps.
Also applies to: 246-248
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/odh-notebook-controller/controllers/notebook_controller_test.go`
around lines 156 - 160, The test creates shardedNotebook (shardedNotebookName,
created via createNotebook and cli.Create) but only deletes it much later, so
failures leave resources behind; immediately after successfully creating
shardedNotebook call a cleanup to always remove it (use t.Cleanup or defer a
deletion that calls cli.Delete with the same ctx and shardedNotebook) so the
resource is deleted on test exit/failure; update the block around
createNotebook/cli.Create to register that cleanup right after the
Expect(cli.Create(...)).Should(Succeed()) to guarantee removal even if later
assertions fail.
components/odh-notebook-controller/controllers/notebook_route.go
Outdated
Show resolved
Hide resolved
For the CRs we managed we should reconcile only those labels that we're managing. Other labels may be added/removed by other parties and we may thus break their functionality.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/notebook-controller/controllers/notebook_controller.go (1)
190-211:⚠️ Potential issue | 🟠 MajorPod-template label drift only reconciled on replica change; label-only drift skipped entirely.
The label merge at lines 194–207 runs only when replicas differ.
CopyStatefulSetFieldsdoes not reconcileSpec.Template.Labels(onlySpec.Template.Spec). If a desired template label is removed while replicas remain stable, the drift is never detected and persisted; new pods inherit stale labels. Move the merge unconditionally and include its result in the update predicate.Patch sketch
- if ss.Spec.Replicas != nil && foundStateful.Spec.Replicas != nil && *ss.Spec.Replicas != *foundStateful.Spec.Replicas { - // Reconcile only controller-owned labels (present on `ss`), preserve any - // extra labels on the existing StatefulSet pod template. - needsLabelUpdate := false - for k, v := range ss.Spec.Template.Labels { - if foundStateful.Spec.Template.Labels == nil || foundStateful.Spec.Template.Labels[k] != v { - needsLabelUpdate = true - break - } - } - if needsLabelUpdate { - if foundStateful.Spec.Template.Labels == nil { - foundStateful.Spec.Template.Labels = map[string]string{} - } - for k, v := range ss.Spec.Template.Labels { - foundStateful.Spec.Template.Labels[k] = v - } - } + // Reconcile only controller-owned labels (present on `ss`), preserve any + // extra labels on the existing StatefulSet pod template. + needsTemplateLabelUpdate := false + for k, v := range ss.Spec.Template.Labels { + if foundStateful.Spec.Template.Labels == nil || foundStateful.Spec.Template.Labels[k] != v { + needsTemplateLabelUpdate = true + break + } + } + if needsTemplateLabelUpdate { + if foundStateful.Spec.Template.Labels == nil { + foundStateful.Spec.Template.Labels = map[string]string{} + } + for k, v := range ss.Spec.Template.Labels { + foundStateful.Spec.Template.Labels[k] = v + } } // Update the foundStateful object and write the result back if there are any changes - if !justCreated && reconcilehelper.CopyStatefulSetFields(ss, foundStateful) { + if !justCreated && (needsTemplateLabelUpdate || reconcilehelper.CopyStatefulSetFields(ss, foundStateful)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/notebook-controller/controllers/notebook_controller.go` around lines 190 - 211, The label-merge logic currently guarded by the replica-change check should be performed unconditionally so Pod-template label drift is always reconciled: move the block that compares and merges ss.Spec.Template.ObjectMeta.Labels into foundStateful.Spec.Template.ObjectMeta.Labels out of the if that checks replicas (so it runs regardless of replicas), and ensure its result (e.g., a boolean needsLabelUpdate or merged flag) is factored into the same update predicate used with reconcilehelper.CopyStatefulSetFields(foundStateful, ss) and the justCreated guard so the controller will update the StatefulSet whenever labels were merged even if replicas didn’t change; reference ss, foundStateful, Spec.Template.ObjectMeta.Labels, justCreated and reconcilehelper.CopyStatefulSetFields when applying this change.
♻️ Duplicate comments (3)
components/odh-notebook-controller/controllers/notebook_route.go (1)
143-145:⚠️ Potential issue | 🟠 MajorUse promoted
Labelshere to unblock staticcheck.Lines 144-145 still use
ObjectMeta.Labels, which is what QF1008 is complaining about.HTTPRoutealready promotesObjectMeta, sor1.Labels/r2.Labelsare equivalent.Patch
- v1, ok1 := r1.ObjectMeta.Labels[k] - v2, ok2 := r2.ObjectMeta.Labels[k] + v1, ok1 := r1.Labels[k] + v2, ok2 := r2.Labels[k]#!/bin/bash rg -nP --type=go '\bObjectMeta\.Labels\b' components/odh-notebook-controller/controllers/notebook_route.go # Expected: no matches after the fix🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/odh-notebook-controller/controllers/notebook_route.go` around lines 143 - 145, The loop over managedLabelKeys uses the promoted ObjectMeta incorrectly; replace r1.ObjectMeta.Labels and r2.ObjectMeta.Labels with the promoted fields r1.Labels and r2.Labels in the for loop that references managedLabelKeys (and any other occurrences in the same function), so the code reads v1, ok1 := r1.Labels[k] and v2, ok2 := r2.Labels[k] to satisfy staticcheck QF1008.components/odh-notebook-controller/controllers/notebook_controller.go (1)
115-119:⚠️ Potential issue | 🟠 MajorUse the promoted
Annotationsfield here to clear the lint blocker.
nb1.ObjectMeta.Annotationsandnb2.ObjectMeta.Annotationson Line 118 still trigger QF1008.NotebookpromotesObjectMeta, sonb1.Annotations/nb2.Annotationsare equivalent and keep CI green.Patch
- return reflect.DeepEqual(nb1.ObjectMeta.Annotations, nb2.ObjectMeta.Annotations) && + return reflect.DeepEqual(nb1.Annotations, nb2.Annotations) && reflect.DeepEqual(nb1.Spec, nb2.Spec)#!/bin/bash rg -nP --type=go '\bObjectMeta\.Annotations\b' components/odh-notebook-controller/controllers/notebook_controller.go # Expected: no matches after the fix🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/odh-notebook-controller/controllers/notebook_controller.go` around lines 115 - 119, The equality check uses nb1.ObjectMeta.Annotations and nb2.ObjectMeta.Annotations which triggers QF1008; update the reflect.DeepEqual call in notebook_controller.go (the comparison that returns reflect.DeepEqual(... ) && reflect.DeepEqual(nb1.Spec, nb2.Spec)) to use the promoted fields nb1.Annotations and nb2.Annotations instead of nb1.ObjectMeta.Annotations/nb2.ObjectMeta.Annotations so the linter warning is cleared.components/odh-notebook-controller/controllers/notebook_controller_test.go (1)
156-160:⚠️ Potential issue | 🟡 MinorRegister cleanup immediately after creating
shardedNotebook.If any assertion between Line 159 and Line 247 fails, this notebook stays behind in
defaultand can skew later namespace-scoped assertions. Defer the delete right aftercli.Create(...).Patch
shardedNotebook := createNotebook(shardedNotebookName, Namespace) Expect(cli.Create(ctx, shardedNotebook)).Should(Succeed()) + defer func() { + _ = cli.Delete(ctx, shardedNotebook) + }() @@ - By("By deleting the Notebook created for this test") - Expect(cli.Delete(ctx, shardedNotebook)).Should(Succeed())Also applies to: 246-247
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/odh-notebook-controller/controllers/notebook_controller_test.go` around lines 156 - 160, After creating the test resource shardedNotebook via createNotebook and cli.Create (shardedNotebookName, shardedNotebook, Namespace), register a deferred cleanup immediately—i.e., call cli.Delete within a defer using the same ctx and shardedNotebook so the notebook is removed even if subsequent assertions fail; reference the createNotebook, shardedNotebook, shardedNotebookName and cli.Create/cli.Delete symbols to locate where to add the defer.
🧹 Nitpick comments (2)
components/odh-notebook-controller/controllers/notebook_dspa_secret.go (1)
48-49: CentralizemanagedByKeyandmanagedByValueconstants to prevent drift.Constants defined at
notebook_dspa_secret.go:48-49are not reused; the label"opendatahub.io/managed-by": "workbenches"is hardcoded innotebook_controller.go:573,notebook_webhook.go:643, andnotebook_runtime.go:150. Extract both constants to a shared package to ensure consistency across all notebook controllers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/odh-notebook-controller/controllers/notebook_dspa_secret.go` around lines 48 - 49, Extract the string literals into shared exported constants (e.g., ManagedByKey and ManagedByValue) instead of the local managedByKey/managedByValue in notebook_dspa_secret.go, place them in a small shared package (e.g., labels or common) and update all call sites—replace hardcoded `"opendatahub.io/managed-by"`/`"workbenches"` usages in notebook_controller.go (where used), notebook_webhook.go, and notebook_runtime.go to import and reference the new exported constants (ManagedByKey, ManagedByValue); ensure the original managedByKey/managedByValue are removed or redirected to the shared constants to prevent duplication.components/common/reconcilehelper/util.go (1)
109-134: Extract the managed-metadata merge into one helper.The same three-phase pattern now exists here and in multiple controllers: diff desired keys, initialize a nil map, then copy desired keys. The pod-template drift bug in this PR is already a sign these paths are diverging. Centralizing the merge logic will keep nil handling, change detection, and any future key-removal policy consistent across StatefulSets, Deployments, Services, Routes, ReferenceGrants, and ConfigMaps.
Also applies to: 151-172, 191-212
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/common/reconcilehelper/util.go` around lines 109 - 134, Extract the repeated three-step pattern (diff desired keys against target, initialize a nil map on target, then copy desired keys) into a single helper (e.g., MergeManagedMap or MergeManagedMetadata) that accepts the source map (from.Labels / from.Annotations), a pointer/reference to the target map (to.Labels / to.Annotations) and the current requireUpdate flag and returns the updated target map and a bool indicating whether an update is required; replace the duplicated blocks in this file (the labels block and the annotations block) and the other occurrences you noted (the blocks at the equivalents of lines 151-172 and 191-212) to call this helper, ensuring nil handling, change detection (compare values), and assignment of keys are centralized and that requireUpdate is updated based on the helper's returned bool.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@components/notebook-controller/controllers/notebook_controller.go`:
- Around line 190-211: The label-merge logic currently guarded by the
replica-change check should be performed unconditionally so Pod-template label
drift is always reconciled: move the block that compares and merges
ss.Spec.Template.ObjectMeta.Labels into
foundStateful.Spec.Template.ObjectMeta.Labels out of the if that checks replicas
(so it runs regardless of replicas), and ensure its result (e.g., a boolean
needsLabelUpdate or merged flag) is factored into the same update predicate used
with reconcilehelper.CopyStatefulSetFields(foundStateful, ss) and the
justCreated guard so the controller will update the StatefulSet whenever labels
were merged even if replicas didn’t change; reference ss, foundStateful,
Spec.Template.ObjectMeta.Labels, justCreated and
reconcilehelper.CopyStatefulSetFields when applying this change.
---
Duplicate comments:
In `@components/odh-notebook-controller/controllers/notebook_controller_test.go`:
- Around line 156-160: After creating the test resource shardedNotebook via
createNotebook and cli.Create (shardedNotebookName, shardedNotebook, Namespace),
register a deferred cleanup immediately—i.e., call cli.Delete within a defer
using the same ctx and shardedNotebook so the notebook is removed even if
subsequent assertions fail; reference the createNotebook, shardedNotebook,
shardedNotebookName and cli.Create/cli.Delete symbols to locate where to add the
defer.
In `@components/odh-notebook-controller/controllers/notebook_controller.go`:
- Around line 115-119: The equality check uses nb1.ObjectMeta.Annotations and
nb2.ObjectMeta.Annotations which triggers QF1008; update the reflect.DeepEqual
call in notebook_controller.go (the comparison that returns
reflect.DeepEqual(... ) && reflect.DeepEqual(nb1.Spec, nb2.Spec)) to use the
promoted fields nb1.Annotations and nb2.Annotations instead of
nb1.ObjectMeta.Annotations/nb2.ObjectMeta.Annotations so the linter warning is
cleared.
In `@components/odh-notebook-controller/controllers/notebook_route.go`:
- Around line 143-145: The loop over managedLabelKeys uses the promoted
ObjectMeta incorrectly; replace r1.ObjectMeta.Labels and r2.ObjectMeta.Labels
with the promoted fields r1.Labels and r2.Labels in the for loop that references
managedLabelKeys (and any other occurrences in the same function), so the code
reads v1, ok1 := r1.Labels[k] and v2, ok2 := r2.Labels[k] to satisfy staticcheck
QF1008.
---
Nitpick comments:
In `@components/common/reconcilehelper/util.go`:
- Around line 109-134: Extract the repeated three-step pattern (diff desired
keys against target, initialize a nil map on target, then copy desired keys)
into a single helper (e.g., MergeManagedMap or MergeManagedMetadata) that
accepts the source map (from.Labels / from.Annotations), a pointer/reference to
the target map (to.Labels / to.Annotations) and the current requireUpdate flag
and returns the updated target map and a bool indicating whether an update is
required; replace the duplicated blocks in this file (the labels block and the
annotations block) and the other occurrences you noted (the blocks at the
equivalents of lines 151-172 and 191-212) to call this helper, ensuring nil
handling, change detection (compare values), and assignment of keys are
centralized and that requireUpdate is updated based on the helper's returned
bool.
In `@components/odh-notebook-controller/controllers/notebook_dspa_secret.go`:
- Around line 48-49: Extract the string literals into shared exported constants
(e.g., ManagedByKey and ManagedByValue) instead of the local
managedByKey/managedByValue in notebook_dspa_secret.go, place them in a small
shared package (e.g., labels or common) and update all call sites—replace
hardcoded `"opendatahub.io/managed-by"`/`"workbenches"` usages in
notebook_controller.go (where used), notebook_webhook.go, and
notebook_runtime.go to import and reference the new exported constants
(ManagedByKey, ManagedByValue); ensure the original managedByKey/managedByValue
are removed or redirected to the shared constants to prevent duplication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 9ea1028e-ed39-413f-8a5a-1a37a1b70f71
📒 Files selected for processing (10)
components/common/reconcilehelper/util.gocomponents/notebook-controller/controllers/notebook_controller.gocomponents/odh-notebook-controller/controllers/notebook_controller.gocomponents/odh-notebook-controller/controllers/notebook_controller_test.gocomponents/odh-notebook-controller/controllers/notebook_dspa_secret.gocomponents/odh-notebook-controller/controllers/notebook_kube_rbac_auth.gocomponents/odh-notebook-controller/controllers/notebook_network.gocomponents/odh-notebook-controller/controllers/notebook_referencegrant.gocomponents/odh-notebook-controller/controllers/notebook_route.gocomponents/odh-notebook-controller/controllers/notebook_route_compare_test.go
|
@jstourac: The following test failed, say
Full PR test history. Your PR dashboard. 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-sigs/prow repository. I understand the commands that are listed here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
For the CRs we managed we should reconcile only those labels that we're managing. Other labels may be added/removed by other parties and we may thus break their functionality.
https://redhat.atlassian.net/browse/RHOAIENG-48289
Description
How Has This Been Tested?
Merge criteria:
Summary by CodeRabbit
Bug Fixes
Tests