fix(RHOAIENG-34953): detect ConfigMap name changes in gorch#690
Conversation
…strator When a user changes the ConfigMap name referenced by a GuardrailsOrchestrator CR (orchestratorConfig or sidecarGatewayConfig), the operator did not detect the change if the new ConfigMap had the same content as the old one. This left the Deployment's volume sources pointing at the old ConfigMap name, out of sync with the CR spec. Root cause: setConfigMapHashAnnotations() only compared content hashes (SHA-256), not ConfigMap names. Same content = same hash = no change detected = no redeployment. Fix: Track ConfigMap names in deployment annotations alongside content hashes. When either the name or the hash changes, flag the config as changed so redeployOnConfigMapChange() triggers a redeployment. New annotations: - trustyai.opendatahub.io/orchestrator-config-name - trustyai.opendatahub.io/orchestrator-gateway-config-name Includes 6 regression tests covering: - Orchestrator CM name change (same content) - Gateway CM name change (same content) - Name + content change together - Content-only change (same name) - Idempotency (no spurious redeployments) - Gateway idempotency Fixes RHOAIENG-34953 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
[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 |
📝 WalkthroughWalkthroughThe changes extend ConfigMap change detection in the orchestrator reconciler to track both content hash and name changes. A function now compares new ConfigMap names against stored annotations in addition to content hashes, and updates corresponding annotations accordingly. Comprehensive regression and idempotency tests verify correct behavior across different change scenarios. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
controllers/gorch/configmap_name_change_repro_test.go (1)
1-15: Test framework does not match coding guidelines.The coding guidelines specify that controller tests should use Ginkgo v2 with Gomega assertions, but this file uses standard Go testing with
testify/assert. Consider converting to Ginkgo/Gomega for consistency with the rest of the test suite.That said, the current tests are functional and provide good regression coverage. This could be deferred if there's a preference for simpler unit tests for isolated function testing.
As per coding guidelines: "
controllers/**/*_test.go: Use Ginkgo v2 with Gomega assertions and controller-runtime envtest for all controller tests"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controllers/gorch/configmap_name_change_repro_test.go` around lines 1 - 15, This test file currently uses Go's testing + testify/assert (see import of "testing" and "github.com/stretchr/testify/assert") which violates the project's guideline to use Ginkgo v2/Gomega and controller-runtime envtest for controller tests; convert the test to a Ginkgo v2 spec by replacing testing-based functions with Describe/Context/It blocks, swap all testify/assert calls to Gomega's Expect assertions, add a BeforeSuite/AfterSuite to start/stop controller-runtime envtest and a kube client for exercising gorchv1alpha1 objects, and update any fake client usage to use the envtest client (or a controller-runtime client) so the test uses the same Ginkgo/Gomega + envtest patterns as other controller tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@controllers/gorch/configmap_name_change_repro_test.go`:
- Around line 1-15: This test file currently uses Go's testing + testify/assert
(see import of "testing" and "github.com/stretchr/testify/assert") which
violates the project's guideline to use Ginkgo v2/Gomega and controller-runtime
envtest for controller tests; convert the test to a Ginkgo v2 spec by replacing
testing-based functions with Describe/Context/It blocks, swap all testify/assert
calls to Gomega's Expect assertions, add a BeforeSuite/AfterSuite to start/stop
controller-runtime envtest and a kube client for exercising gorchv1alpha1
objects, and update any fake client usage to use the envtest client (or a
controller-runtime client) so the test uses the same Ginkgo/Gomega + envtest
patterns as other controller tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 32c5f51f-e686-40ae-b750-5a74a906cb3e
📒 Files selected for processing (2)
controllers/gorch/config_generation.gocontrollers/gorch/configmap_name_change_repro_test.go
|
@m-misiura: 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. |
1 participant
Summary
Fixes RHOAIENG-34953: Updating a ConfigMap name in a
GuardrailsOrchestratorCR (orchestratorConfigorsidecarGatewayConfig) now correctly triggers a Deployment update.Root cause:
setConfigMapHashAnnotations()only compared ConfigMap content hashes (SHA-256). When a user changed the CM name but the new CM had the same content, the hash matched and no redeployment was triggered. The Deployment's volume sources continued referencing the old CM name.Fix: Track ConfigMap names in deployment annotations (
trustyai.opendatahub.io/orchestrator-config-name,trustyai.opendatahub.io/orchestrator-gateway-config-name) alongside content hashes. When either the name or hash changes, flag the config as changed soredeployOnConfigMapChange()triggers a redeployment.Changes
controllers/gorch/config_generation.go— Add name tracking annotations tosetConfigMapHashAnnotations()for both orchestrator and gateway ConfigMapscontrollers/gorch/configmap_name_change_repro_test.go(new) — 6 regression testsBackward compatibility
Fully backward compatible. Existing deployments without the name annotations get a one-time redeployment on first reconciliation (empty annotation ≠ actual name), but
patchDeployment()usesreflect.DeepEqualso no actual update occurs if the spec hasn't changed.Test plan
TestConfigMapNameChange_OrchestratorSameContent_RHOAIENG34953)TestConfigMapNameChange_GatewaySameContent_RHOAIENG34953)TestConfigMapNameChange_DifferentContent)TestConfigMapHashAnnotations_Idempotent,TestConfigMapHashAnnotations_GatewayIdempotent)TestConfigMapContentChange_SameName)To open upstream: Create PR from
m-misiura:bugfix/RHOAIENG-34953-configmap-name-change-detection→trustyai-explainability:mainFixes RHOAIENG-34953
🤖 Generated with Claude Code
Summary by CodeRabbit