feat(chaos): integrate operator-chaos shift-left validation for L1-L3 maturity - #525
Merged
openshift-merge-bot[bot] merged 2 commits intoMay 26, 2026
Conversation
… maturity Integrate operator-chaos SDK into model-registry-operator to enable shift-left chaos validation across L1 through L3 maturity levels: - L1 Breaking Change Detection: CI workflow validates knowledge model and experiment definitions against operator schema changes - L2 Experiment/Knowledge Validation: knowledge model describing operator components, managed resources, webhooks, finalizers, and steady-state checks; 9 chaos experiment definitions covering pod-kill, network-partition, webhook-disrupt, config-drift, rbac-revoke, finalizer-block, catalog-pod-kill, and catalog-config-drift scenarios - L3 Chaos Test Execution: ChaosClient SDK tests for ModelRegistry and ModelCatalog reconcilers with 10 Ginkgo specs exercising fault injection, recovery validation, and steady-state assertions Adds CI workflow (chaos-validate.yml), Makefile targets (test-chaos, chaos-validate, operator-chaos install), and operator-chaos Go dependency. RHOAIENG-63106 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Chris Hambridge <chambrid@redhat.com>
📝 WalkthroughWalkthroughThis pull request adds a Chaos Validation GitHub Actions workflow, Makefile targets and an operator-chaos installer, a chaos knowledge model, nine ChaosExperiment manifests covering pod kills, config drift, network partition, webhook disruption, RBAC revoke, and finalizer blocking, a go.mod dependency on operator-chaos, and a 450-line Ginkgo test suite that wires chaos-enabled reconcilers for ModelRegistry and ModelCatalog and asserts ChaosError propagation and recovery. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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.
Actionable comments posted: 6
🤖 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 @.github/workflows/chaos-validate.yml:
- Around line 17-18: The checkout action usages (actions/checkout@v6) currently
persist credentials to the local git config; update each checkout step to set
persist-credentials: false to prevent storing the workflow token in the
workspace and reduce token exposure (apply to the checkout steps shown and the
other occurrence around lines 44-49). Locate the steps that call
actions/checkout@v6 (the checkout action blocks) and add the input
persist-credentials: false to each step so credentials are not written to local
git config.
- Around line 14-16: The validate job currently doesn't set job-level
GITHUB_TOKEN permissions; add an explicit permissions block under the validate
job to scope GITHUB_TOKEN to only the least-privilege scopes required by its
steps (e.g., permissions: contents: read, actions: read, or other minimal scopes
your validation steps need) so that the job named validate uses an explicit,
minimal permission set rather than inheriting broad workflow-level permissions.
- Around line 17-20: Replace the mutable action tags by pinning
actions/checkout@v6 and actions/setup-go@v6 to their respective full commit SHAs
in the workflow (update the occurrences of actions/checkout and
actions/setup-go), add job-level permissions for jobs.validate with the minimal
scope (e.g., permissions: contents: read) and set persist-credentials: false on
both checkout steps to avoid leaving credentials available to subsequent steps;
make these changes where the workflow references actions/checkout and
actions/setup-go and in the jobs.validate block.
In `@internal/controller/modelregistry_chaos_test.go`:
- Around line 155-167: AfterEach currently ignores errors from
k8sClient.Get/Delete and os.Unsetenv which can leak resources; update the
AfterEach cleanup to surface failures instead of swallowing them by asserting or
failing the test on errors (e.g. use Gomega's Expect/Fail or require-like
checks). Specifically, in the AfterEach block referencing namespace, found
(&v1beta1.ModelRegistry{}), typeNamespaceName, k8sClient.Get and
k8sClient.Delete, and the os.Unsetenv calls for config.RestImage,
config.PostgresImage, config.KubeRBACProxyImage, check each error return and
handle it (log and Fail the spec or Expect(err).NotTo(HaveOccurred())) so
cleanup errors are visible and cause the test to fail immediately.
- Around line 101-109: The createRegistry function uses static namespace names
causing cross-test collisions; update createRegistry to generate a unique
namespace string (e.g., append a timestamp/UUID/random suffix) and use that
value for namespace.ObjectMeta.Name, namespace.ObjectMeta.Namespace and
typeNamespaceName (types.NamespacedName) so each test run gets a distinct
namespace and avoids AlreadyExists flakes.
In `@Makefile`:
- Line 278: The recipe currently checks/installs to $(LOCALBIN)/operator-chaos
but the intended target variable is $(OPERATOR_CHAOS); update the install rule
to check the actual OPERATOR_CHAOS path (test -s $(OPERATOR_CHAOS)) and install
the binary into the directory of $(OPERATOR_CHAOS) by setting GOBIN to the
dirname of $(OPERATOR_CHAOS) before calling go install
github.com/opendatahub-io/operator-chaos/cmd/operator-chaos@$(OPERATOR_CHAOS_VERSION),
so overrides of OPERATOR_CHAOS are honored; adjust any references in the same
recipe to use $(OPERATOR_CHAOS) instead of $(LOCALBIN)/operator-chaos.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: c2aad5ed-5ebf-4709-a546-ae5287618927
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (14)
.github/workflows/chaos-validate.ymlMakefilechaos/experiments/catalog-config-drift.yamlchaos/experiments/catalog-pod-kill.yamlchaos/experiments/config-drift.yamlchaos/experiments/finalizer-block.yamlchaos/experiments/mutating-webhook-disrupt.yamlchaos/experiments/network-partition.yamlchaos/experiments/pod-kill.yamlchaos/experiments/rbac-revoke.yamlchaos/experiments/webhook-disrupt.yamlchaos/knowledge/model-registry.yamlgo.modinternal/controller/modelregistry_chaos_test.go
pboyd
reviewed
May 26, 2026
| target: | ||
| operator: model-registry | ||
| component: model-catalog | ||
| resource: ConfigMap/model-catalog-sources-default |
Contributor
There was a problem hiding this comment.
This was renamed to default-catalog-sources (there are a few instances of the old name in this PR).
pboyd
reviewed
May 26, 2026
|
|
||
| components: | ||
| - name: model-registry-operator | ||
| controller: DataScienceCluster |
Contributor
There was a problem hiding this comment.
Should this be ModelRegistryReconciller?
…and experiments Rename ConfigMap from model-catalog-sources-default to default-catalog-sources in catalog-config-drift experiment (target resource and injection parameter) and model-registry knowledge base. Fix controller name from DataScienceCluster to ModelRegistryReconciler in model-registry knowledge base. Addresses PR review feedback. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Chris Hambridge <chambrid@redhat.com>
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)
chaos/experiments/catalog-config-drift.yaml (1)
9-18:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate ConfigMap restoration explicitly.
This experiment can pass without proving the drift was reconciled. It only waits for
Deployment/model-catalogto beAvailable, while the hypothesis saysConfigMap/default-catalog-sourcesshould be restored. Since the injected key is also removed by TTL cleanup after 120s, a broken reconciler can still look "recovered" beforerecoveryTimeoutexpires. Add an assertion against the mutatedConfigMapstate itself and keep deployment availability as a secondary health signal.As per coding guidelines, prioritize "Bug-prone patterns and error handling gaps".
Also applies to: 20-33
🤖 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 `@chaos/experiments/catalog-config-drift.yaml` around lines 9 - 18, Add an explicit steadyState check that verifies the mutated ConfigMap itself was restored: for resource ConfigMap/default-catalog-sources add a check (e.g., conditionEquals/resourceField/jsonPath) that the injected key is absent or that the expected data key/value is present, then keep the existing conditionTrue check for Deployment model-catalog as a secondary signal; also ensure the experiment's recoveryTimeout is greater than the ConfigMap TTL (e.g., >120s) so the check cannot pass due to TTL cleanup.
🤖 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.
Outside diff comments:
In `@chaos/experiments/catalog-config-drift.yaml`:
- Around line 9-18: Add an explicit steadyState check that verifies the mutated
ConfigMap itself was restored: for resource ConfigMap/default-catalog-sources
add a check (e.g., conditionEquals/resourceField/jsonPath) that the injected key
is absent or that the expected data key/value is present, then keep the existing
conditionTrue check for Deployment model-catalog as a secondary signal; also
ensure the experiment's recoveryTimeout is greater than the ConfigMap TTL (e.g.,
>120s) so the check cannot pass due to TTL cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: ca43c740-5b84-4b2d-9cad-baca8ff4d0c6
📒 Files selected for processing (2)
chaos/experiments/catalog-config-drift.yamlchaos/knowledge/model-registry.yaml
✅ Files skipped from review due to trivial changes (1)
- chaos/knowledge/model-registry.yaml
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chambridge, pboyd 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 |
Merged
8 tasks done
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.
Description
Integrate operator-chaos SDK into model-registry-operator to enable shift-left chaos validation across L1 through L3 maturity levels:
Adds CI workflow (chaos-validate.yml), Makefile targets (test-chaos, chaos-validate, operator-chaos install), and operator-chaos Go dependency.
How Has This Been Tested?
This essentially adds new tests and CI to run the chaos testing functionality
Merge criteria:
Summary by CodeRabbit
New Features
Tests
Chores