OCPNODE-4554: Automate OCP-70203: ICSP and IDMS/ITMS can coexist in cluster#31229
OCPNODE-4554: Automate OCP-70203: ICSP and IDMS/ITMS can coexist in cluster#31229asahay19 wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@asahay19: This pull request references OCPNODE-4554 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 story 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis pull request adds a comprehensive end-to-end Ginkgo test that validates how ICSP, IDMS, and ITMS resources coexist in a cluster and affect MCP rollouts and /etc/containers/registries.conf configuration. The test sequences resource creation and deletion to verify precedence behavior and stability across different mirror policy types. ChangesICSP/IDMS/ITMS Coexistence and Precedence E2E Test
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: asahay19 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@test/extended/node/node_e2e/node.go`:
- Around line 455-457: The test currently logs the full registries.conf content
(registriesConf) using e2e.Logf after reading it via
nodeutils.ExecOnNodeWithChroot, which may leak internal hostnames or customer
data; change the logging to either redact hostnames/URLs or extract and log only
the specific entries asserted on (e.g., mirror hostnames or the presence/absence
of keys) instead of printing registriesConf wholesale, and apply the same change
for the other occurrences noted (around the code that reads/prints
registries.conf at the other locations).
- Around line 354-359: The MCP baseline config names must be captured before
issuing stale-resource deletions: move the calls to
imagepolicy.GetMCPCurrentSpecConfigName(oc, "worker") and
imagepolicy.GetMCPCurrentSpecConfigName(oc, "master") to before the deletion
logic so that the variables (e.g., staleWorkerSpec, staleMasterSpec) hold the
pre-change spec; then call imagepolicy.WaitForMCPConfigSpecChangeAndUpdated(oc,
"worker", staleWorkerSpec) and
imagepolicy.WaitForMCPConfigSpecChangeAndUpdated(oc, "master", staleMasterSpec)
after deletions to wait for the post-change update.
- Around line 472-520: The current no-rollout checks only take a single worker
MCP snapshot (using imagepolicy.GetMCPCurrentSpecConfigName) after sleeping,
which can miss delayed renders or master-only changes; replace these checks with
polling over the observation window (instead of time.Sleep) and verify both
worker and master MCP spec/status names remain equal to the pre-change values
(specBeforeIDMS/specAfterIDMS and specBeforeICSPDelete/specAfterICSPDelete) for
the entire window; use a polling helper (e.g., wait.PollImmediate or test
framework wait) to repeatedly call imagepolicy.GetMCPCurrentSpecConfigName for
"worker" and "master" and assert they never deviate from the corresponding
specBefore values before concluding the test.
- Around line 325-353: The pre-flight cleanup currently deletes ICSP/IDMS by
matching on RepositoryDigestMirrors/ImageDigestMirrors Source values, which can
remove unrelated cluster objects and misses ImageTagMirrorSets (ITMS); change
the logic in the node.go pre-flight block to instead target only test-owned
resources (e.g., check for a test-specific name prefix or a label you set on
created objects) when iterating operatorClient.ImageContentSourcePolicies().List
and configClient.ImageDigestMirrorSets().List, and add an analogous loop for
configClient.ImageTagMirrorSets().List to remove stale ITMS—use the same
test-owned selector (name prefix or label) to decide deletion, set staleCleaned
accordingly, and avoid deleting objects based solely on their Source fields.
- Around line 334-348: The pre-flight cleanup currently discards errors from
operatorClient.ImageContentSourcePolicies().Delete and
configClient.ImageDigestMirrorSets().Delete and sets staleCleaned = true
unconditionally; change each delete call to capture the returned error (err :=
...Delete(ctx, name, metav1.DeleteOptions{})), check it, and only set
staleCleaned = true when err == nil; on error, log the failure with context
(e.g., using e2e.Logf) and/or assert via o.Expect(err).NotTo(o.HaveOccurred(),
...) so API/auth failures are surfaced instead of swallowed for both the ICSP
deletion path (referencing icsp.Name, rdm.Source, staleCleaned) and the IDMS
deletion loop (referencing idms.Name, dim.Source, existingIDMSs).
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 188eb55b-d42e-43a5-8f01-7bd434fd2a9f
📒 Files selected for processing (1)
test/extended/node/node_e2e/node.go
|
Scheduling required tests: |
|
Actionable comments posted: 0 |
openshift-ci
Bot
added
the
ready-for-human-review
|
Scheduling required tests: |
|
@asahay19: 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. |
| }) | ||
|
|
||
| //author: asahay@redhat.com | ||
| g.It("[OTP] ICSP and IDMS/ITMS can coexist in cluster [OCP-70203]", ote.Informing(), func() { |
There was a problem hiding this comment.
Can we move this to a different file in the same folder?
|
@asahay19 Can you point me to the log containing the test result in the CI? I'm interested to see how long this test takes to run. |
3 participants
Migrates test case OCP-70203 from openshift-tests-private to origin.
Here is the Polarion link: https://polarion.engineering.redhat.com/polarion/#/project/OSE/workitem?id=OCP-70203
The test verifies that ImageContentSourcePolicy (ICSP), ImageDigestMirrorSet (IDMS), and ImageTagMirrorSet (ITMS) can coexist on a cluster without conflicts, including validating that:
PTAL @QiWang19 @cpmeadors @BhargaviGudi
Summary by CodeRabbit