MGMT-24443: watch configmap for CNI config update#10384
Open
rccrdpccl wants to merge 6 commits into
Open
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@rccrdpccl: This pull request references MGMT-24443 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
the
size/L
openshift-ci
Bot
added
the
kind/dependency-change
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rccrdpccl The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a Manifests API method and wrapper, makes the ClusterDeployment controller sync per-cluster custom-manifest usage and watch ConfigMap changes to re-enqueue reconciles, updates unit and integration tests to exercise these flows, and includes small build/tooling and commit-guidance edits. ChangesCustom Manifest ConfigMap Usage
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 7 | ❌ 5❌ Failed checks (4 warnings, 1 inconclusive)
✅ Passed checks (7 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
rccrdpccl
force-pushed
the
worktree-mgmt-24443-configmap-watch
branch
from
054d811 to
8917fb5
Compare
![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
🧹 Nitpick comments (2)
internal/controller/controllers/clusterdeployments_controller_test.go (1)
3473-3512: ⚡ Quick winAdd an explicit empty-ConfigMap-data test for CustomManifest usage.
Current cases cover “exists with data” and “missing”, but not “exists with empty data”. Add a test asserting
SetCustomManifestUsage(..., false)when the referenced ConfigMap is present with emptyData.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/controllers/clusterdeployments_controller_test.go` around lines 3473 - 3512, Add a new test case in clusterdeployments_controller_test.go mirroring the other two "sets CustomManifest FeatureUsage when ConfigMap has data" and "unsets CustomManifest FeatureUsage when ConfigMap is missing" cases: set aci.Spec.ManifestsConfigMapRefs to the new configMapName, create a ConfigMap object with the same name and Namespace but with an empty Data map (e.g., Data: map[string]string{}), call c.Create(ctx, cm), set the expectation mockManifestsApi.EXPECT().SetCustomManifestUsage(gomock.Any(), sId, false).Return(nil).Times(1), invoke request := newClusterDeploymentRequest(cluster) and call cr.Reconcile(ctx, request), and assert err is nil and result equals ctrl.Result{} so the controller treats an empty ConfigMap as "no custom manifest".Dockerfile.assisted-service-build (1)
41-41: 💤 Low value
whichis still required for disconnected LSO installs (or refactor it away).
hack/utils.shalready usescommand -v, butdeploy/operator/setup_lso.shstill runswhich opmwhenDISCONNECTED=true(around line 53), so thewhichpackage shouldn’t be removed unless that usage is also switched tocommand -v.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Dockerfile.assisted-service-build` at line 41, The Dockerfile removed the which package but deploy/operator/setup_lso.sh still calls which opm when DISCONNECTED=true, so either restore the which package in the apt/yum install list (re-add "which") or change deploy/operator/setup_lso.sh to use command -v opm (consistent with hack/utils.sh) and remove the which dependency; locate the string "which opm" in deploy/operator/setup_lso.sh and replace with command -v invocation or revert the package removal in the Dockerfile where "which" was removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/controller/controllers/clusterdeployments_controller.go`:
- Around line 1471-1474: The call site in clusterdeployments_controller.go
treats a ConfigMap NotFound as expected but getManifestConfigMap currently logs
NotFound as an error, producing noisy alerts; update getManifestConfigMap so it
does not log a NotFound as an error (either remove the error-level log for
k8serrors.IsNotFound, lower it to debug/info, or only log the unexpected error
cases) and ensure it still returns the NotFound error so callers like
r.getManifestConfigMap(...) can detect k8serrors.IsNotFound and continue; make
the change inside the getManifestConfigMap implementation and verify other
callers still handle the returned error appropriately.
- Around line 1465-1467: The early return when len(configMapNames) == 0 leaves
any prior CustomManifest usage stale; instead, detect the empty configMapNames
case, clear/unset the ClusterDeployment's CustomManifest references (e.g. remove
or nil-out the CustomManifest field and any related status flags/conditions on
the ClusterDeployment) and persist that change via the controller client
(Update/Status().Update) before returning. Locate the if block that checks
configMapNames and update it to perform the clearing of CustomManifest and
status updates (using the same ClusterDeployment object you already have) when
configMapNames is empty, then return nil.
In `@subsystem/kubeapi/kubeapi_test.go`:
- Around line 5331-5333: The test assumes IPv4 by calling
hostutil.GenerateIPv4Addresses and utils_test.DefaultCIDRv4; change the setup to
be IP-family-agnostic by deriving the desired CIDR from the test/infrastructure
(e.g., inspect infraEnv or a utils_test helper that exposes the cluster/network
CIDR) and then call the appropriate address generator for that family (replace
GenerateIPv4Addresses with a generator that accepts the CIDR or family, or call
GenerateIPv6Addresses when the CIDR is IPv6), then pass the resulting ip
(ips[0]) into utils_test.TestContext.GenerateFullMeshConnectivity as before;
update references around RegisterNode, GenerateIPv4Addresses, DefaultCIDRv4 and
GenerateFullMeshConnectivity so the test works for both IPv4 and IPv6.
---
Nitpick comments:
In `@Dockerfile.assisted-service-build`:
- Line 41: The Dockerfile removed the which package but
deploy/operator/setup_lso.sh still calls which opm when DISCONNECTED=true, so
either restore the which package in the apt/yum install list (re-add "which") or
change deploy/operator/setup_lso.sh to use command -v opm (consistent with
hack/utils.sh) and remove the which dependency; locate the string "which opm" in
deploy/operator/setup_lso.sh and replace with command -v invocation or revert
the package removal in the Dockerfile where "which" was removed.
In `@internal/controller/controllers/clusterdeployments_controller_test.go`:
- Around line 3473-3512: Add a new test case in
clusterdeployments_controller_test.go mirroring the other two "sets
CustomManifest FeatureUsage when ConfigMap has data" and "unsets CustomManifest
FeatureUsage when ConfigMap is missing" cases: set
aci.Spec.ManifestsConfigMapRefs to the new configMapName, create a ConfigMap
object with the same name and Namespace but with an empty Data map (e.g., Data:
map[string]string{}), call c.Create(ctx, cm), set the expectation
mockManifestsApi.EXPECT().SetCustomManifestUsage(gomock.Any(), sId,
false).Return(nil).Times(1), invoke request :=
newClusterDeploymentRequest(cluster) and call cr.Reconcile(ctx, request), and
assert err is nil and result equals ctrl.Result{} so the controller treats an
empty ConfigMap as "no custom manifest".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ab8c9133-ffff-4763-8556-d717bdde20d2
📒 Files selected for processing (8)
Dockerfile.assisted-service-buildhack/utils.shinternal/controller/controllers/clusterdeployments_controller.gointernal/controller/controllers/clusterdeployments_controller_test.gointernal/manifests/api/interface.gointernal/manifests/api/mock_manifests_internal.gointernal/manifests/manifests.gosubsystem/kubeapi/kubeapi_test.go
rccrdpccl
force-pushed
the
worktree-mgmt-24443-configmap-watch
branch
2 times, most recently
from
84e25cb to
8047409
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
internal/controller/controllers/clusterdeployments_controller_test.go (1)
3473-3545: ⚡ Quick winAdd a test for referenced ConfigMap with empty
Data.This context verifies “present with data” and “missing”, but not “present and empty.” Since usage is tied to ConfigMap content, add an assertion that empty data unsets usage (
false).Suggested test addition
+ It("unsets CustomManifest FeatureUsage when ConfigMap exists but has no data", func() { + configMapName := "empty-cni-manifests" + aci.Spec.ManifestsConfigMapRefs = []hiveext.ManifestsConfigMapReference{ + {Name: configMapName}, + } + Expect(c.Update(ctx, aci)).Should(BeNil()) + + cm := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: testNamespace, + Name: configMapName, + }, + Data: map[string]string{}, + } + Expect(c.Create(ctx, cm)).To(BeNil()) + + mockInstallerInternal.EXPECT().GetClusterByKubeKey(gomock.Any()).Return(backEndCluster, nil) + mockManifestsApi.EXPECT().SetCustomManifestUsage(gomock.Any(), sId, false).Return(nil).Times(1) + + request := newClusterDeploymentRequest(cluster) + result, err := cr.Reconcile(ctx, request) + Expect(err).To(BeNil()) + Expect(result).To(Equal(ctrl.Result{})) + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/controllers/clusterdeployments_controller_test.go` around lines 3473 - 3545, The tests cover ConfigMap present-with-data and missing but not the case where the ConfigMap exists with empty Data; add a new It block that sets aci.Spec.ManifestsConfigMapRefs (or the deprecated aci.Spec.ManifestsConfigMapRef) to a configMapName, create a corev1.ConfigMap with that name and an empty Data map, call Expect(c.Create(ctx, cm)).To(BeNil()), mockInstallerInternal.EXPECT().GetClusterByKubeKey(...).Return(backEndCluster, nil) and expect mockManifestsApi.EXPECT().SetCustomManifestUsage(gomock.Any(), sId, false).Return(nil).Times(1), then call request := newClusterDeploymentRequest(cluster) and result, err := cr.Reconcile(ctx, request) and assert err is nil and result equals ctrl.Result{}.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLAUDE.md`:
- Around line 656-664: Update the two fenced code blocks containing the
contributor lines ("Assisted-by: Claude Code <[email protected]>" and
"Co-Authored-By: Claude Code <[email protected]>") to include a language
identifier (e.g., text) after the opening triple backticks so they satisfy
markdownlint MD040; specifically change the opening fences from ``` to ```text
for both occurrences.
In `@hack/utils.sh`:
- Line 40: Quote the command substitution used to find the podman-remote3 binary
to avoid word-splitting: change the ln invocation that currently uses unquoted
$(command -v podman-remote3) so it uses "$(command -v podman-remote3)" and
preserve the target /tools/podman-remote; i.e. update the line referencing the
command substitution for podman-remote3 to wrap it in double quotes so ln
receives a single pathname argument.
- Line 43: The ln invocation in hack/utils.sh uses unquoted command substitution
for command -v podman-remote4 which can cause word-splitting; update the ln call
that references command -v podman-remote4 so the command substitution is quoted
(i.e., pass the resolved path as a single quoted argument) when creating
/tools/podman-remote, ensuring ln receives one intact pathname.
---
Nitpick comments:
In `@internal/controller/controllers/clusterdeployments_controller_test.go`:
- Around line 3473-3545: The tests cover ConfigMap present-with-data and missing
but not the case where the ConfigMap exists with empty Data; add a new It block
that sets aci.Spec.ManifestsConfigMapRefs (or the deprecated
aci.Spec.ManifestsConfigMapRef) to a configMapName, create a corev1.ConfigMap
with that name and an empty Data map, call Expect(c.Create(ctx,
cm)).To(BeNil()),
mockInstallerInternal.EXPECT().GetClusterByKubeKey(...).Return(backEndCluster,
nil) and expect mockManifestsApi.EXPECT().SetCustomManifestUsage(gomock.Any(),
sId, false).Return(nil).Times(1), then call request :=
newClusterDeploymentRequest(cluster) and result, err := cr.Reconcile(ctx,
request) and assert err is nil and result equals ctrl.Result{}.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b18fb880-4381-4160-a003-77b568e51404
📒 Files selected for processing (9)
CLAUDE.mdDockerfile.assisted-service-buildhack/utils.shinternal/controller/controllers/clusterdeployments_controller.gointernal/controller/controllers/clusterdeployments_controller_test.gointernal/manifests/api/interface.gointernal/manifests/api/mock_manifests_internal.gointernal/manifests/manifests.gosubsystem/kubeapi/kubeapi_test.go
✅ Files skipped from review due to trivial changes (1)
- internal/manifests/api/mock_manifests_internal.go
rccrdpccl
force-pushed
the
worktree-mgmt-24443-configmap-watch
branch
2 times, most recently
from
3876ea3 to
82e1ceb
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/controller/controllers/clusterdeployments_controller_test.go (2)
3473-3545: ⚡ Quick winAdd a test for existing ConfigMap with empty
Data.Current coverage checks “exists with data” and “missing,” but not “exists with no entries.” That edge case is central to the “contains data” contract and can regress silently.
Proposed test addition
+ It("unsets CustomManifest FeatureUsage when ConfigMap exists but has no data", func() { + configMapName := "empty-cni-manifests" + aci.Spec.ManifestsConfigMapRefs = []hiveext.ManifestsConfigMapReference{ + {Name: configMapName}, + } + Expect(c.Update(ctx, aci)).Should(BeNil()) + + cm := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: testNamespace, + Name: configMapName, + }, + Data: map[string]string{}, + } + Expect(c.Create(ctx, cm)).To(BeNil()) + + mockInstallerInternal.EXPECT().GetClusterByKubeKey(gomock.Any()).Return(backEndCluster, nil) + mockManifestsApi.EXPECT().SetCustomManifestUsage(gomock.Any(), sId, false).Return(nil).Times(1) + + request := newClusterDeploymentRequest(cluster) + result, err := cr.Reconcile(ctx, request) + Expect(err).To(BeNil()) + Expect(result).To(Equal(ctrl.Result{})) + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/controllers/clusterdeployments_controller_test.go` around lines 3473 - 3545, Add a new It test that covers the edge case where a referenced ConfigMap exists but has empty Data: create aci with aci.Spec.ManifestsConfigMapRefs (or deprecated aci.Spec.ManifestsConfigMapRef) pointing to a configMapName, create a corev1.ConfigMap with Data = map[string]string{} (empty map) in the testNamespace, then call cr.Reconcile(request) and assert mockManifestsApi.EXPECT().SetCustomManifestUsage(gomock.Any(), sId, false).Return(nil).Times(1) and that Reconcile returns (ctrl.Result{}, nil); mirror the structure of the existing "sets CustomManifest FeatureUsage when ConfigMap has data" test but expect false when Data is empty.
5699-5790: ⚡ Quick winAdd a namespace-isolation test for ConfigMap mapping.
Mapper tests currently validate name-based matches but not same-name/different-namespace behavior. Adding this guards against accidental cross-namespace enqueues.
Proposed test addition
+ It("does not enqueue when ConfigMap name matches but namespace differs", func() { + clusterName := "test-cd-ns" + aciName := "test-aci-ns" + cmName := "shared-name" + + cd := newClusterDeployment(clusterName, testNamespace, getDefaultClusterDeploymentSpec(clusterName, aciName, "pull-secret")) + Expect(c.Create(ctx, cd)).ShouldNot(HaveOccurred()) + + aciSpec := getDefaultAgentClusterInstallSpec(clusterName) + aciSpec.ManifestsConfigMapRefs = []hiveext.ManifestsConfigMapReference{{Name: cmName}} + aci := newAgentClusterInstall(aciName, testNamespace, aciSpec, cd) + Expect(c.Create(ctx, aci)).ShouldNot(HaveOccurred()) + + cm := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: cmName, + Namespace: "another-namespace", + }, + } + + requests := cr.mapConfigMapToClusterDeployment(ctx, cm) + Expect(requests).To(BeEmpty()) + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/controllers/clusterdeployments_controller_test.go` around lines 5699 - 5790, Add a test that verifies mapConfigMapToClusterDeployment is namespace-scoped: create two ClusterDeployments/AgentClusterInstalls (using newClusterDeployment and newAgentClusterInstall) in different namespaces but referencing the same ConfigMap name, create a ConfigMap in one namespace and call cr.mapConfigMapToClusterDeployment(ctx, cm) and assert it only enqueues the ClusterDeployment from the ConfigMap's namespace (and not the identically-named resource in the other namespace); also add the inverse case (ConfigMap in the other namespace) if desired to fully guard against cross-namespace matches.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/controller/controllers/clusterdeployments_controller_test.go`:
- Around line 3473-3545: Add a new It test that covers the edge case where a
referenced ConfigMap exists but has empty Data: create aci with
aci.Spec.ManifestsConfigMapRefs (or deprecated aci.Spec.ManifestsConfigMapRef)
pointing to a configMapName, create a corev1.ConfigMap with Data =
map[string]string{} (empty map) in the testNamespace, then call
cr.Reconcile(request) and assert
mockManifestsApi.EXPECT().SetCustomManifestUsage(gomock.Any(), sId,
false).Return(nil).Times(1) and that Reconcile returns (ctrl.Result{}, nil);
mirror the structure of the existing "sets CustomManifest FeatureUsage when
ConfigMap has data" test but expect false when Data is empty.
- Around line 5699-5790: Add a test that verifies
mapConfigMapToClusterDeployment is namespace-scoped: create two
ClusterDeployments/AgentClusterInstalls (using newClusterDeployment and
newAgentClusterInstall) in different namespaces but referencing the same
ConfigMap name, create a ConfigMap in one namespace and call
cr.mapConfigMapToClusterDeployment(ctx, cm) and assert it only enqueues the
ClusterDeployment from the ConfigMap's namespace (and not the identically-named
resource in the other namespace); also add the inverse case (ConfigMap in the
other namespace) if desired to fully guard against cross-namespace matches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d6fb18bb-451f-4f95-bcf2-492e6dbfcaae
📒 Files selected for processing (9)
CLAUDE.mdDockerfile.assisted-service-buildhack/utils.shinternal/controller/controllers/clusterdeployments_controller.gointernal/controller/controllers/clusterdeployments_controller_test.gointernal/manifests/api/interface.gointernal/manifests/api/mock_manifests_internal.gointernal/manifests/manifests.gosubsystem/kubeapi/kubeapi_test.go
✅ Files skipped from review due to trivial changes (2)
- internal/manifests/api/mock_manifests_internal.go
- CLAUDE.md
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #10384 +/- ##
==========================================
- Coverage 44.36% 44.35% -0.02%
==========================================
Files 420 420
Lines 73107 73259 +152
==========================================
+ Hits 32433 32491 +58
- Misses 37740 37832 +92
- Partials 2934 2936 +2
🚀 New features to boost your workflow:
|
rccrdpccl
force-pushed
the
worktree-mgmt-24443-configmap-watch
branch
from
82e1ceb to
7b46aa0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/controller/controllers/clusterdeployments_controller_test.go (1)
5669-5788: ⚡ Quick winAdd a namespace-isolation test for mapper behavior.
Please add a case where two ConfigMaps share the same name in different namespaces and assert only same-namespace
ClusterDeploymentis enqueued.Suggested additional test case
+ It("does not enqueue ClusterDeployment from a different namespace for same ConfigMap name", func() { + cmName := "shared-name" + + cd := newClusterDeployment("cd-ns-a", "ns-a", getDefaultClusterDeploymentSpec("cd-ns-a", "aci-ns-a", "pull-secret")) + Expect(c.Create(ctx, cd)).ShouldNot(HaveOccurred()) + aciSpec := getDefaultAgentClusterInstallSpec("cd-ns-a") + aciSpec.ManifestsConfigMapRefs = []hiveext.ManifestsConfigMapReference{{Name: cmName}} + aci := newAgentClusterInstall("aci-ns-a", "ns-a", aciSpec, cd) + Expect(c.Create(ctx, aci)).ShouldNot(HaveOccurred()) + + cm := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: cmName, Namespace: "ns-b"}} + requests := cr.mapConfigMapToClusterDeployment(ctx, cm) + Expect(requests).To(BeEmpty()) + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/controllers/clusterdeployments_controller_test.go` around lines 5669 - 5788, Add a new test in the mapConfigMapToClusterDeployment Describe block that verifies namespace isolation: create two ClusterDeployments/AgentClusterInstalls in different namespaces (use newClusterDeployment and newAgentClusterInstall) both referencing the same ManifestsConfigMap name via ManifestsConfigMapRefs (or deprecated ManifestsConfigMapRef for a variant), then create two ConfigMaps with the same Name but in different namespaces and call cr.mapConfigMapToClusterDeployment for each ConfigMap; assert that only the request returned for the ConfigMap in the same namespace as its ClusterDeployment contains that ClusterDeployment's Name (and that the other returns empty or does not include it), ensuring the mapper enqueues only same-namespace matches.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/controller/controllers/clusterdeployments_controller_test.go`:
- Around line 516-517: The test currently uses a permissive mock expectation
mockManifestsApi.EXPECT().SetCustomManifestUsage(gomock.Any(), gomock.Any(),
gomock.Any()).Return(nil).AnyTimes(), which hides regressions; update the
expectations to assert the exact calls and args instead of AnyTimes: replace
AnyTimes() with explicit EXPECT() constraints using gomock.Eq or specific
matchers for the clusterDeployment/manifest and the boolean usage flag and set
Times(1) (or the exact call count required) and verify both occurrences where
SetCustomManifestUsage is mocked (the current permissive calls). Ensure one
EXPECT matches the call that should set usage=true and the other matches the
call that should set usage=false (or correct args), so the test fails if the
reconciler sends wrong arguments or wrong number of invocations.
---
Nitpick comments:
In `@internal/controller/controllers/clusterdeployments_controller_test.go`:
- Around line 5669-5788: Add a new test in the mapConfigMapToClusterDeployment
Describe block that verifies namespace isolation: create two
ClusterDeployments/AgentClusterInstalls in different namespaces (use
newClusterDeployment and newAgentClusterInstall) both referencing the same
ManifestsConfigMap name via ManifestsConfigMapRefs (or deprecated
ManifestsConfigMapRef for a variant), then create two ConfigMaps with the same
Name but in different namespaces and call cr.mapConfigMapToClusterDeployment for
each ConfigMap; assert that only the request returned for the ConfigMap in the
same namespace as its ClusterDeployment contains that ClusterDeployment's Name
(and that the other returns empty or does not include it), ensuring the mapper
enqueues only same-namespace matches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1afd590a-eeef-4100-b792-a825ad691e78
📒 Files selected for processing (8)
CLAUDE.mdinternal/controller/controllers/clusterdeployments_controller.gointernal/controller/controllers/clusterdeployments_controller_test.gointernal/manifests/api/interface.gointernal/manifests/api/mock_manifests_api.gointernal/manifests/api/mock_manifests_internal.gointernal/manifests/manifests.gosubsystem/kubeapi/kubeapi_test.go
✅ Files skipped from review due to trivial changes (1)
- internal/manifests/api/mock_manifests_api.go
rccrdpccl
force-pushed
the
worktree-mgmt-24443-configmap-watch
branch
from
7b46aa0 to
0635fac
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
CLAUDE.md (1)
656-658:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd language identifiers to the fenced code blocks.
Both fences should include a language token (for example
text) to satisfy markdownlint MD040.Suggested fix
-``` +```text Assisted-by: Claude Code <[email protected]>-
+text
Co-Authored-By: Claude Code [email protected]Also applies to: 662-664
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CLAUDE.md` around lines 656 - 658, Add a language identifier to the two fenced code blocks that contain the contributor lines (the blocks with "Assisted-by: Claude Code <[email protected]>" and "Co-Authored-By: Claude Code <[email protected]>") so they satisfy markdownlint MD040; change the opening triple-backtick for each block to include a language token (e.g., use ```text) while keeping the block contents unchanged.
🧹 Nitpick comments (1)
subsystem/kubeapi/kubeapi_test.go (1)
5358-5363: ⚡ Quick winAdd best-effort cleanup for the first ConfigMap creation path.
If the assertion after the first
deployOrUpdateConfigMapfails, cleanup is skipped and the ConfigMap can leak into subsequent retries in the same namespace. Add a deferred delete immediately after initial creation.Suggested patch
By("Create ConfigMap with manifest data — validation should pass") content := `apiVersion: v1 kind: Namespace metadata: name: cilium` cm := deployOrUpdateConfigMap(ctx, kubeClient, configMapName, map[string]string{"cilium-ns.yaml": content}) + defer func() { + _ = kubeClient.Delete(ctx, cm) + }() checkAgentClusterInstallCondition(ctx, installkey, hiveext.ClusterRequirementsMetCondition, hiveext.ClusterReadyReason)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@subsystem/kubeapi/kubeapi_test.go` around lines 5358 - 5363, The first ConfigMap created by cm := deployOrUpdateConfigMap(...) can leak if subsequent assertions fail; immediately after creating cm in the initial path add a best-effort deferred cleanup (e.g., defer kubeClient.Delete(ctx, cm) or equivalent) so the ConfigMap is removed even on test failures; place this defer right after the deployOrUpdateConfigMap call so it always runs, and ensure it handles errors from kubeClient.Delete gracefully (ignore or log) without changing later explicit deletes in the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/controller/controllers/clusterdeployments_controller.go`:
- Around line 1477-1492: Currently the code sets CustomManifest usage as soon as
any ConfigMap has Data (using getManifestConfigMap and
Manifests.SetCustomManifestUsage), which can be false-positive; instead defer
flipping usage.CustomManifest until the manifests have been validated/imported
successfully — call the same validation/import path used by addCustomManifests
(or invoke addCustomManifests) and only on its successful return update usage
via Manifests.SetCustomManifestUsage; keep getManifestConfigMap only to discover
candidates but do not mark usage from presence alone, and ensure
cluster.FeatureUsage is updated after successful validation/import.
- Around line 2040-2050: matchesManifestConfigMap currently falls back to the
deprecated ManifestsConfigMapRef even when ManifestsConfigMapRefs is set,
causing ignored ConfigMaps to trigger reconciles; change
matchesManifestConfigMap to treat ManifestsConfigMapRefs as authoritative: if
aci.Spec.ManifestsConfigMapRefs is non-nil (and preferably non-empty) only
iterate and compare ref.Name to configMapName and return false otherwise, and
only check aci.Spec.ManifestsConfigMapRef when ManifestsConfigMapRefs is
nil/empty so the deprecated field is honored only when no new refs are provided.
- Around line 267-269: The reconcile currently swallows errors from
r.syncManifestConfigMapUsage (logged with log.WithError(err).Warn(...)) which
prevents requeueing on transient failures; change the handler so that when
r.syncManifestConfigMapUsage returns an error you propagate it (e.g., return
ctrl.Result{}, err) instead of only logging, so the controller-runtime will
requeue and retry (this affects the reconcile function where
r.syncManifestConfigMapUsage is called and where cluster.FeatureUsage is
updated).
---
Duplicate comments:
In `@CLAUDE.md`:
- Around line 656-658: Add a language identifier to the two fenced code blocks
that contain the contributor lines (the blocks with "Assisted-by: Claude Code
<[email protected]>" and "Co-Authored-By: Claude Code
<[email protected]>") so they satisfy markdownlint MD040; change the opening
triple-backtick for each block to include a language token (e.g., use ```text)
while keeping the block contents unchanged.
---
Nitpick comments:
In `@subsystem/kubeapi/kubeapi_test.go`:
- Around line 5358-5363: The first ConfigMap created by cm :=
deployOrUpdateConfigMap(...) can leak if subsequent assertions fail; immediately
after creating cm in the initial path add a best-effort deferred cleanup (e.g.,
defer kubeClient.Delete(ctx, cm) or equivalent) so the ConfigMap is removed even
on test failures; place this defer right after the deployOrUpdateConfigMap call
so it always runs, and ensure it handles errors from kubeClient.Delete
gracefully (ignore or log) without changing later explicit deletes in the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0010b375-e417-4b49-96d3-5973370b1389
📒 Files selected for processing (8)
CLAUDE.mdinternal/controller/controllers/clusterdeployments_controller.gointernal/controller/controllers/clusterdeployments_controller_test.gointernal/manifests/api/interface.gointernal/manifests/api/mock_manifests_api.gointernal/manifests/api/mock_manifests_internal.gointernal/manifests/manifests.gosubsystem/kubeapi/kubeapi_test.go
✅ Files skipped from review due to trivial changes (2)
- internal/manifests/api/mock_manifests_internal.go
- internal/manifests/api/mock_manifests_api.go
rccrdpccl
force-pushed
the
worktree-mgmt-24443-configmap-watch
branch
2 times, most recently
from
2c6953d to
5927f4e
Compare
Contributor
Author
|
/retest |
rccrdpccl
force-pushed
the
worktree-mgmt-24443-configmap-watch
branch
from
5927f4e to
ded06ef
Compare
Contributor
Author
|
/retest |
rccrdpccl
force-pushed
the
worktree-mgmt-24443-configmap-watch
branch
from
c70b6a8 to
35c3e1c
Compare
rccrdpccl
commented
May 28, 2026
|
|
||
| hasData := false | ||
| for _, name := range configMapNames { | ||
| cm, err := r.getManifestConfigMap(ctx, log, clusterInstall, name) |
Contributor
Author
There was a problem hiding this comment.
This resolves with a GET request, but it should be cached by the informer as we have a watch on configmaps here
Contributor
Author
|
/retest |
CrystalChun
reviewed
May 28, 2026
CrystalChun
reviewed
May 28, 2026
CrystalChun
reviewed
May 28, 2026
Comment on lines
+2034
to
+2053
| aciList := &hiveext.AgentClusterInstallList{} | ||
| if err := r.List(ctx, aciList, client.InNamespace(obj.GetNamespace())); err != nil { | ||
| log.WithError(err).Error("failed to list AgentClusterInstalls for ConfigMap mapper") | ||
| return []reconcile.Request{} | ||
| } | ||
|
|
||
| var requests []reconcile.Request | ||
| for i := range aciList.Items { | ||
| aci := &aciList.Items[i] | ||
| if matchesManifestConfigMap(aci, obj.GetName()) { | ||
| log.Debugf("ConfigMap %s matches ACI %s, enqueueing ClusterDeployment %s", | ||
| obj.GetName(), aci.Name, aci.Spec.ClusterDeploymentRef.Name) | ||
| requests = append(requests, reconcile.Request{ | ||
| NamespacedName: types.NamespacedName{ | ||
| Namespace: aci.Namespace, | ||
| Name: aci.Spec.ClusterDeploymentRef.Name, | ||
| }, | ||
| }) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Getting every single ACI every single time a configmap is changed is a bit worrisome...
Can we label or annotate the configmaps used by the ACI with its name/namespace? Then if a configmap has our annotation, we'll do something about it? What do you think?
Or label with the cluster deployment's name and namespace directly
Contributor
Author
There was a problem hiding this comment.
I was thinking about a solution for this (and for another part where we retrieve all configmaps one by one), however there's a little gap: for already existing ACIs with configmap attached we cannot label the configmap correctly, thus that configmap will stay with the current bug.
This pattern is already established in the repo, as we will hit the informer's cache is not as bad as it seems https://github.com/rccrdpccl/assisted-service/blame/35c3e1c444542cfd3499b9617e5049d3aca5d6d3/internal/controller/controllers/clusterdeployments_controller.go#L1918
The solution I had in mind would work as follow:
- if aci.spec.ManifestsConfigMapRefs changes, we can relabel all linked manifests
- we retrieve manifests by label
This will avoid hitting with List, but as mention will leave that usecase uncovered.
What are your thoughts here?
Personally I think it wouldn't be a huge deal to leave out that usecase, but if possible I'd like to include it.
Contributor
There was a problem hiding this comment.
If it's existing and we reconcile the ACI, can't we just add a label to the configmap when we see it? Like add an ensureLabeled in getManifestConfigMap https://github.com/openshift/assisted-service/blame/3b239e5acd500ce36946c3555cf49e51aed0e299/internal/controller/controllers/clusterdeployments_controller.go#L1445
Contributor
Author
There was a problem hiding this comment.
Re-reading this I can see there's no edge case left out: the service will reboot if we want to run the patched version, and it will scan all referenced configmaps and label them accordingly. From there, we can monitor changes in the configmap by labels.
CrystalChun
reviewed
May 28, 2026
Comment on lines
+1482
to
+1509
| func (r *ClusterDeploymentsReconciler) syncManifestConfigMapUsage(ctx context.Context, log logrus.FieldLogger, | ||
| clusterInstall *hiveext.AgentClusterInstall, cluster *common.Cluster) error { | ||
|
|
||
| if r.Manifests == nil || cluster.ID == nil { | ||
| return nil | ||
| } | ||
|
|
||
| configMapNames := getManifestConfigMapNames(clusterInstall) | ||
| if len(configMapNames) == 0 { | ||
| return r.Manifests.SetCustomManifestUsage(ctx, *cluster.ID, false) | ||
| } | ||
|
|
||
| hasData := false | ||
| for _, name := range configMapNames { | ||
| cm, err := r.getManifestConfigMap(ctx, log, clusterInstall, name) | ||
| if err != nil { | ||
| if k8serrors.IsNotFound(err) { | ||
| continue | ||
| } | ||
| return err | ||
| } | ||
| if len(cm.Data) > 0 { | ||
| hasData = true | ||
| break | ||
| } | ||
| } | ||
|
|
||
| return r.Manifests.SetCustomManifestUsage(ctx, *cluster.ID, hasData) |
Contributor
There was a problem hiding this comment.
Setting usage feels like it belongs in the general manifests file
What do you think about calling setusage from the function CreateClusterManifestInternal? Currently it's only called from V2CreateClusterManifest
Which is called from the controller too
Contributor
Author
There was a problem hiding this comment.
My issue is this: https://github.com/openshift/assisted-service/blob/master/internal/cluster/validator.go#L350
The current validation is checking feature usage, so we need to set it pre-install. Am I missing something or an easier way to achieve this? 🤔
You have a good point to unify Feature usage between REST and KUBE API mode though.
Contributor
There was a problem hiding this comment.
Right the validator, well could we move the logic to add custom manifests out of the ready? https://github.com/openshift/assisted-service/blame/3b239e5acd500ce36946c3555cf49e51aed0e299/internal/controller/controllers/clusterdeployments_controller.go#L455
(I'm not sure why it's gated by ready)
rccrdpccl
force-pushed
the
worktree-mgmt-24443-configmap-watch
branch
2 times, most recently
from
3a4f768 to
68851c8
Compare
rccrdpccl
force-pushed
the
worktree-mgmt-24443-configmap-watch
branch
from
68851c8 to
e3b0f54
Compare
openshift-ci
Bot
added
the
needs-rebase
The centos:stream9 base image no longer includes the which command, causing select_podman_client in hack/utils.sh to silently fail when creating the podman-remote symlink. This broke the skipper make unit-test workflow for local development. - Install which explicitly via dnf in the build Dockerfile - Replace which with command -v in hack/utils.sh as a fallback Assisted-by: Claude Code <[email protected]>
Assisted-by: Claude Code <[email protected]>
…nfigMaps When a ConfigMap referenced by ManifestsConfigMapRefs is deleted and recreated, the custom-manifests-requirements-satisfied validation stays stale because: (1) the ClusterDeployments controller does not watch ConfigMaps, so no reconciliation fires, and (2) the CustomManifest FeatureUsage flag is never set from the ConfigMap path. Fix both root causes: - Add a ConfigMap watch to the ClusterDeployments controller with a mapper that finds AgentClusterInstalls referencing the changed ConfigMap and enqueues their owning ClusterDeployments - Add syncManifestConfigMapUsage to the reconcile loop: checks whether referenced ConfigMaps contain data and sets/unsets the CustomManifest FeatureUsage flag accordingly, so validation reflects actual ConfigMap state - Add SetCustomManifestUsage to ClusterManifestsInternals interface to expose the existing setUsage method to the controller Both ManifestsConfigMapRefs and the deprecated ManifestsConfigMapRef are supported. The REST API manifest path is unaffected. Assisted-by: Claude Code <[email protected]>
…imization Assisted-by: Claude Code <[email protected]>
…ternals Move setUsage calls from the REST API handlers (V2CreateClusterManifest, V2DeleteClusterManifest) into the internal methods (CreateClusterManifestInternal, DeleteClusterManifestInternal). This ensures the CustomManifest FeatureUsage flag is set consistently regardless of whether manifests are created via the REST API or the KubeAPI controller path. The REST handlers become thin wrappers with no usage-tracking logic. Assisted-by: Claude Code <[email protected]>
Move addCustomManifests out of the installDay1 ready gate so manifest ConfigMaps are synced on every reconcile loop. Guard with IsDay2Cluster and nil Manifests check to avoid panics in day2 and test contexts that don't wire up the manifests API. Remove syncManifestConfigMapUsage (no longer needed), add ConfigMap labeling via ensureManifestConfigMapsLabelled, and fix test expectations for the new reconcile flow. Assisted-by: Claude Opus 4.6 <[email protected]>
rccrdpccl
force-pushed
the
worktree-mgmt-24443-configmap-watch
branch
from
e3b0f54 to
9f57eb6
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
@rccrdpccl: The following tests 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
3 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.
This solves issue MGMT-24443.
When creating custom manifest for the CNI and the config fails, deleting the config and reapplying has no effect, as its value is never refreshed.
List all the issues related to this PR
What environments does this code impact?
How was this code tested?
Checklist
docs, README, etc)Reviewers Checklist
Summary by CodeRabbit
New Features
Chores
Documentation
Tests