OCPNODE-4535: Automate OCP-44820 change container registry config#31182
OCPNODE-4535: Automate OCP-44820 change container registry config#31182Chandan9112 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
|
@Chandan9112: This pull request references OCPNODE-4535 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. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
@Chandan9112: GitHub didn't allow me to request PR reviews from the following users: openshift/sig-node. Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs. 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 kubernetes-sigs/prow repository. |
|
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 imports for conflict-aware retries and an operator test helper, and a disruptive serial e2e that patches cluster ImageConfig (allowed/search registries) with RetryOnConflict, waits for MCP rollout, verifies node registry config, and restores the original ImageConfig. ChangesNode e2e registry config
sequenceDiagram
participant Test as ImageRegistryConfigTest
participant API as ImageConfig API
participant MCP as MachineConfigPool
participant Node as Worker node
Test->>API: GET current image.config.openshift.io/cluster
Test->>API: RetryOnConflict PATCH update registry sources
API->>MCP: apply updated spec
MCP->>Node: rollout MachineConfig
Test->>Node: check /etc/containers/registries.conf.d/01-image-searchRegistries.conf
Test->>API: RetryOnConflict PATCH restore original image.config
MCP->>Node: converge to restored spec
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 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 |
Chandan9112
force-pushed
the
automate-ocp-44820
branch
from
ef40051 to
6e92bbc
Compare
Chandan9112
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
openshift-merge-bot
Bot
added
the
ready-for-human-review
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/extended/node/node_e2e/node.go (1)
357-360: 💤 Low valueEarly return on restore failure prevents MCP convergence wait.
If restoring the original
image.configfails, the cleanup returns immediately without waiting for MCP rollout or operator stabilization. This may leave the cluster in an intermediate state. However, since the restore itself failed, there's little value in waiting for MCP convergence of a potentially incomplete update.Consider adding a log message indicating that MCP waits are being skipped due to restore failure, to make the behavior more explicit.
📝 Proposed improvement for clarity
if restoreErr != nil { - e2e.Logf("WARNING: failed to restore original image.config: %v", restoreErr) + e2e.Logf("WARNING: failed to restore original image.config: %v; skipping MCP convergence wait", restoreErr) return }🤖 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 `@test/extended/node/node_e2e/node.go` around lines 357 - 360, The cleanup path currently returns immediately when restoreErr != nil in the block that attempts to restore the original image.config; update that block (the if restoreErr != nil branch) to log an explicit message that MCP rollout/operator stabilization waits are being skipped because the restore failed (mention restoreErr in the log for context) before returning so readers know why MCP waits are omitted; modify the code around the restore of image.config (the restoreErr check) to add this log entry referencing restoreErr and then return as before.
🤖 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 373-389: The code assigns to imageConfig.Spec.RegistrySources
fields without nil-checking; inside the retry.RetryOnConflict block (where you
call oc.AdminConfigClient().ConfigV1().Images().Get and later Update), check if
imageConfig.Spec.RegistrySources is nil and if so initialize it (e.g., set
imageConfig.Spec.RegistrySources = &configv1.RegistrySources{} ) before setting
AllowedRegistries and ContainerRuntimeSearchRegistries to avoid a runtime panic
when RegistrySources is nil.
---
Nitpick comments:
In `@test/extended/node/node_e2e/node.go`:
- Around line 357-360: The cleanup path currently returns immediately when
restoreErr != nil in the block that attempts to restore the original
image.config; update that block (the if restoreErr != nil branch) to log an
explicit message that MCP rollout/operator stabilization waits are being skipped
because the restore failed (mention restoreErr in the log for context) before
returning so readers know why MCP waits are omitted; modify the code around the
restore of image.config (the restoreErr check) to add this log entry referencing
restoreErr and then return as before.
🪄 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: f2d72291-5f8c-48e4-9b6b-2790534d5327
📒 Files selected for processing (1)
test/extended/node/node_e2e/node.go
| g.By("Update image.config to add search registry and allowed registries") | ||
| err = retry.RetryOnConflict(retry.DefaultBackoff, func() error { | ||
| imageConfig, getErr := oc.AdminConfigClient().ConfigV1().Images().Get(ctx, "cluster", metav1.GetOptions{}) | ||
| if getErr != nil { | ||
| return getErr | ||
| } | ||
| imageConfig.Spec.RegistrySources.AllowedRegistries = []string{ | ||
| "registry.access.redhat.com", "docker.io", "quay.io", searchRegistry, | ||
| "image-registry.openshift-image-registry.svc:5000", | ||
| } | ||
| imageConfig.Spec.RegistrySources.ContainerRuntimeSearchRegistries = []string{ | ||
| "registry.access.redhat.com", "docker.io", "quay.io", searchRegistry, | ||
| } | ||
| _, updateErr := oc.AdminConfigClient().ConfigV1().Images().Update(ctx, imageConfig, metav1.UpdateOptions{}) | ||
| return updateErr | ||
| }) | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "failed to update image.config.openshift.io/cluster") |
There was a problem hiding this comment.
Guard against nil RegistrySources to prevent panic.
Lines 379 and 383 set fields on imageConfig.Spec.RegistrySources without checking if it's nil. In the OpenShift configv1 API, ImageSpec.RegistrySources is a pointer and may be nil on a fresh cluster or after certain operations. Accessing fields on a nil pointer will cause a runtime panic.
🛡️ Proposed fix to initialize RegistrySources if nil
g.By("Update image.config to add search registry and allowed registries")
err = retry.RetryOnConflict(retry.DefaultBackoff, func() error {
imageConfig, getErr := oc.AdminConfigClient().ConfigV1().Images().Get(ctx, "cluster", metav1.GetOptions{})
if getErr != nil {
return getErr
}
+ if imageConfig.Spec.RegistrySources == nil {
+ imageConfig.Spec.RegistrySources = &configv1.RegistrySources{}
+ }
imageConfig.Spec.RegistrySources.AllowedRegistries = []string{
"registry.access.redhat.com", "docker.io", "quay.io", searchRegistry,
"image-registry.openshift-image-registry.svc:5000",
}🤖 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 `@test/extended/node/node_e2e/node.go` around lines 373 - 389, The code assigns
to imageConfig.Spec.RegistrySources fields without nil-checking; inside the
retry.RetryOnConflict block (where you call
oc.AdminConfigClient().ConfigV1().Images().Get and later Update), check if
imageConfig.Spec.RegistrySources is nil and if so initialize it (e.g., set
imageConfig.Spec.RegistrySources = &configv1.RegistrySources{} ) before setting
AllowedRegistries and ContainerRuntimeSearchRegistries to avoid a runtime panic
when RegistrySources is nil.
There was a problem hiding this comment.
addressed in latest commit
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "RegistrySources" test/extended/node/node_e2e/node.go
Length of output: 235
@Chandan9112, it looks like the nil guard hasn't landed in the PR branch yet. The current code on automate-ocp-44820 still directly accesses RegistrySources fields without checking for nil (Lines 379 and 383):
imageConfig.Spec.RegistrySources.AllowedRegistries = []string{...}
imageConfig.Spec.RegistrySources.ContainerRuntimeSearchRegistries = []string{...}Could you double-check that the commit was pushed to the remote branch? Once the nil guard is present, the fix would look like:
if imageConfig.Spec.RegistrySources == nil {
imageConfig.Spec.RegistrySources = &configv1.RegistrySources{}
}
imageConfig.Spec.RegistrySources.AllowedRegistries = []string{
"registry.access.redhat.com", "docker.io", "quay.io", searchRegistry,
"image-registry.openshift-image-registry.svc:5000",
}
imageConfig.Spec.RegistrySources.ContainerRuntimeSearchRegistries = []string{
"registry.access.redhat.com", "docker.io", "quay.io", searchRegistry,
}
There was a problem hiding this comment.
@Chandan9112 Could you please address this comment. Thanks
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@coderabbitai., RegistrySources is a value type (struct), not a pointer, so it's always safe to assign fields on it directly — there's no nil dereference risk here. So I think no guard needed.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the ImageSpec and RegistrySources type definitions in the configv1 package
rg -n "RegistrySources" vendor/github.com/openshift/api/config/v1/ -A2 -B2 | head -60
Length of output: 83
🏁 Script executed:
#!/bin/bash
# Find the openshift API config v1 types file
fd "types_image.go" --type f
fd "types.go" --type f | xargs grep -l "RegistrySources" 2>/dev/null | head -5
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Try to find the types_image.go file in the repo
fd "types_image" --type f 2>/dev/null
# Also search in any Go file for ImageSpec definition
rg -n "RegistrySources\s" --type go -A1 -B1 2>/dev/null | head -30
Length of output: 358
🏁 Script executed:
#!/bin/bash
# Check the image_registry_config.go file
cat test/extended/node/node_e2e/image_registry_config.go
echo "---"
# Find where openshift/api is defined (go.mod)
grep "openshift/api" go.mod | head -5
echo "---"
# Search for ImageSpec struct definition in any Go file in the repo
rg -n "type ImageSpec struct" --type go -A 10 2>/dev/null | head -40
echo "---"
# Check the Go module cache or vendor paths
find . -path "*/openshift/api/config/v1/types_image.go" 2>/dev/null | head -3
Length of output: 5272
🌐 Web query:
openshift api config v1 ImageSpec RegistrySources struct type definition golang
💡 Result:
Top web search findings:
- config/v1/types_image.go at 406574a7aa1eb0e916f4029433e5c28c6a53099b · openshift/api: // Image holds cluster-wide information about how to handle images. The canonical name is
clustertype Image struct { metav1.TypeMetajson:",inline"// Standard object's metadata. metav1.ObjectMetajson:"metadata,omitempty"// spec holds user settable values for configuration // +kubebuilder:validation:Required // +required Spec ImageSpecjson:"spec"// status holds observed values from the cluster. They may not be overridden. // +optional Status ImageStatusjson:"status"} [...] type ImageSpec struct { // AllowedRegistriesForImport limits the container image registries that normal users may import // images from. Set this list to the registries that you trust to contain valid Docker // images and that you want applications to be able to import from. Users with // permission to create Images or ImageStreamMappings via the API are not affected by // this policy - typically only administrators or system integrations will have those // permissions. // +optional AllowedRegistriesForImport []RegistryLocationjson:"allowedRegistriesForImport,omitempty"// externalRegistryHostnames provides the hostnames for the default external image // registry. The external hostname should be set only when the image registry // is exposed externally. The first value is used in 'publicDockerImageRepository' // field in ImageStreams. The value must be in "hostname[:port]" format. // +optional ExternalRegistryHostnames []stringjson:"externalRegistryHostnames,omitempty"[...] // AdditionalTrustedCA is a reference to a ConfigMap containing additional CAs that // should be trusted during imagestream import, pod image pull, and imageregistry // pullthrough. // The namespace for this config map is openshift-config. // +optional AdditionalTrustedCA ConfigMapNameReferencejson:"additionalTrustedCA"// RegistrySources contains configuration that determines how the container runtime // should treat individual registries when accessing images for builds+pods. (e.g. // whether or not to allow insecure access). It does not contain configuration for the // internal cluster registry. // +optional RegistrySources RegistrySourcesjson:"registrySources"} [...] // RegistrySources holds cluster-wide information about how to handle the registries config. type RegistrySources struct { // InsecureRegistries are registries which do not have a valid TLS certificates or only support HTTP connections. // +optional InsecureRegistries []stringjson:"insecureRegistries,omitempty"// BlockedRegistries are blacklisted from image pull/push. All other registries are allowed. // // Only one of BlockedRegistries or AllowedRegistries may be set. // +optional BlockedRegistries []stringjson:"blockedRegistries,omitempty"// AllowedRegistries are whitelisted for image pull/push. All other registries are blocked. // // Only one of BlockedRegistries or AllowedRegistries may be set. // +optional AllowedRegistries []stringjson:"allowedRegistries,omitempty"} - v1 package versions - github.com/openshift/api/config/v1 - Go Packages: +type Image struct [...] +type ImageSpec struct [...] +AdditionalTrustedCA ConfigMapNameReference [...] +AllowedRegistriesForImport []RegistryLocation [...] +ExternalRegistryHostnames []string [...] +RegistrySources RegistrySources [...] +func (ImageSpec) SwaggerDoc() map[string]string [...] +func (in *ImageSpec) DeepCopy() *ImageSpec [...] +func (in *ImageSpec) DeepCopyInto(out *ImageSpec)
- Chapter 6. Image configuration resources | Images | OpenShift Container Platform | 4.1 | Red Hat Documentation:
ImageSpec[...] that you trust to [...]registrySources: Contains configuration that determines how the container runtime should treat individual registries when accessing images for builds and pods. For instance, whether or not to allow insecure access. It does not contain configuration for the internal cluster registry. [...]RegistrySources[...] Holds cluster-wide information about how to handle the registries config. [...] ```yaml name: myconfigmap registrySources: [...]registrySources: Contains configuration that determines how the container runtime should treat individual registries when accessing images for builds and pods. For instance, whether or not to allow insecure access. It does not contain configuration for the internal cluster registry. [...] spec: allowedRegistriesForImport: - domainName: quay.io insecure: false additionalTrustedCA: name: myconfigmap registrySources: insecureRegistries: - v1 package - github.com/openshift/api/config/v1 - Go Packages: The canonical location of the OpenShift API definition. This repo holds the API type definitions and serialization code used by openshift/client-go APIs in this repo ship inside OCP payloads. [...] ## defining new APIs [...] When defining a new API, please follow the OpenShift API conventions, and then follow the instructions below to regenerate CRDs (if necessary) and submit a pull request with your new API definitions and generated files. [...] New APIs (new CRDs) must be added first as an unstable API (v1alpha1). Once the feature is more developed, and ready to be promoted to stable, the API can be promoted to v1. [...] ### Adding a new stable API (v1) [...] // // Compatibility level 1 [...] Stable within a major release for a minimum of 12 months or 3 minor releases (whichever is longer). [...] // +openshift:compatibility-gen [...] level=1 [...] // +openshift [...] _50 [...] operatorOrdering=01 [...] // +kubebuilder:object:root=true // +kubebuilder:subresource:status // +kubebuilder:resource:path=myapis,scope=Cluster // +openshift:api-approved.openshift.io=https://github.com/openshift/api/pull/ // +openshift:capability=IfYouHaveOne // +kubebuilder:printcolumn:name=Column Name,JSONPath=.status.something,type=string,description=how users should interpret this. // +kubebuilder:metadata:annotations=key=value // +kubebuilder:metadata:labels=key=value // +kubebuilder:validation:XValidation:rule= type MyAPI struct { metav1.TypeMeta
json:",inline"// metadata is the standard object's metadata. // More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#metadata metav1.ObjectMetajson:"metadata,omitempty"// spec is the desired state of the cluster version - the operator will work // to ensure that the desired version is applied to the cluster. // +kubebuilder:validation:Required Spec MyAPISpecjson:"spec"// status contains information about the available updates and any in-progress // updates. // +optional Status MyAPIStatusjson:"status"}[...] ### Adding a new unstable API (v1alpha) [...]go // +kubebuilder:validation:XValidation:rule= // +openshift:enable:FeatureGate=MyFeatureGate type MyAPI struct { ... }[...]go // +openshift:validation:FeatureGateAwareXValidation:featureGate=MyFeatureGate,rule="has(oldSelf.coolNewField) ? has(self.coolNewField) : true",message="coolNewField may not be removed once set" type MyAPI struct { // +openshift:enable:FeatureGate=MyFeatureGate // +optional CoolNewField stringjson:"coolNewField"} - Image [config.openshift.io/v1] - Config APIs | API reference | OKD 4.11:
object[...]object[...]object[...]object[...]object[...]registrySources[...]object[...] registrySources contains configuration that determines how the container runtime should treat individual registries when accessing images for builds+pods. (e.g. whether or not to allow insecure access). It does not contain configuration for the internal cluster registry. [...]object[...]string[...]object[...] | Property | Type | Description | | --- | --- | --- | [...] ### .spec.registrySources [...] registrySources contains configuration that determines how the container runtime should treat individual registries when accessing images for builds+pods. (e.g. whether or not to allow insecure access). It does not contain configuration for the internal cluster registry. [...]object[...]allowedRegistries[...]blockedRegistries[...]array (string)[...] AllowedRegistries [...]containerRuntimeSearchRegistries[...]array (string)[...]insecureRegistries[...]array (string)[...] insecureRegistries [...] | Description | [...] | --- | --- | --- | [...]object[...] (string)[...]string[...] --- | --- [...]string[...]string[...]string[...]string[...]string[...]string[...]string`
Citations:
- 1: https://github.com/openshift/api/blob/406574a7aa1eb0e916f4029433e5c28c6a53099b/config/v1/types_image.go
- 2: https://pkg.go.dev/github.com/openshift/api/config/v1?tab=versions
- 3: https://docs.redhat.com/en/documentation/openshift_container_platform/4.1/html/images/image-configuration
- 4: https://pkg.go.dev/github.com/openshift/api/config/v1
- 5: https://docs.okd.io/4.11/rest_api/config_apis/image-config-openshift-io-v1.html
@Chandan9112, you're absolutely right — I apologize for the incorrect review. In the OpenShift API, ImageSpec.RegistrySources is declared as RegistrySources RegistrySources (a plain struct value, not a pointer), so it is always zero-initialized by Go and can never be nil. Accessing fields on it directly is completely safe, and no nil guard is needed. I'll retract the comment.
🏁 Script executed:
#!/bin/bash
# Resolve our own comment on this PR
gh api graphql -f query='
{
repository(owner: "openshift", name: "origin") {
pullRequest(number: 31182) {
reviewThreads(first: 50) {
nodes {
id
isResolved
comments(first: 5) {
nodes {
id
author { login }
body
}
}
}
}
}
}
}'
Length of output: 4431
🏁 Script executed:
#!/bin/bash
# Resolve the review thread since the original comment was incorrect
gh api graphql -f query='
mutation {
resolveReviewThread(input: {threadId: "PRRT_kwDOAVZyrM6CTwEE"}) {
thread {
id
isResolved
}
}
}'
Length of output: 266
@Chandan9112, you are completely correct, and I apologize for the incorrect review. I've now verified against the upstream openshift/api source that ImageSpec.RegistrySources is declared as a plain struct value:
type ImageSpec struct {
// ...
RegistrySources RegistrySources `json:"registrySources"`
}Since it's a value type (not a pointer), Go zero-initializes it automatically and it can never be nil. No nil guard is needed. The code is correct as-is.
I was unable to resolve the thread programmatically due to permissions, but please feel free to resolve it manually.
✏️ Learnings added
Learnt from: Chandan9112
Repo: openshift/origin PR: 31182
File: test/extended/node/node_e2e/node.go:0-0
Timestamp: 2026-05-19T12:28:55.867Z
Learning: In the OpenShift `configv1` API (`github.com/openshift/api/config/v1`), `ImageSpec.RegistrySources` is declared as a plain struct value (`RegistrySources RegistrySources`), NOT a pointer. It is always zero-initialized by Go and can never be nil. No nil-guard is needed before accessing its fields `AllowedRegistries`, `ContainerRuntimeSearchRegistries`, etc.
Learnt from: Chandan9112
Repo: openshift/origin PR: 31182
File: test/extended/node/node_e2e/node.go:0-0
Timestamp: 2026-05-19T12:28:55.867Z
Learning: In the OpenShift `configv1` API (`github.com/openshift/api/config/v1`), `ImageSpec.RegistrySources` is declared as a plain struct value (`RegistrySources RegistrySources`), NOT a pointer. It is always zero-initialized by Go and can never be nil. No nil-guard is needed before accessing its fields such as `AllowedRegistries` or `ContainerRuntimeSearchRegistries`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Chandan9112
force-pushed
the
automate-ocp-44820
branch
3 times, most recently
from
e7848ff to
e37496d
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
Scheduling required tests: |
|
/retest |
|
Job Failure Risk Analysis for sha: e37496d
|
|
/retest |
|
@Chandan9112 we should have someone review this that is familiar with code, preferably someone who worked on the feature. |
| }) | ||
| }) | ||
|
|
||
| var _ = g.Describe("[Suite:openshift/disruptive-longrunning][sig-node][Disruptive] Image registry config", func() { |
There was a problem hiding this comment.
Can you put in a new file under the same directory?
Hi @cpmeadors ., I got to know for MCO related PR we can get it reviewed from Neeraj. |
|
@Chandan9112: 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. |
|
Tests need to be documented in test/extended/node/README.md |
Move the OCP-44820 test into its own file (image_registry_config.go) and document it in README.md. Co-authored-by: Cursor <cursoragent@cursor.com>
Chandan9112
force-pushed
the
automate-ocp-44820
branch
from
e37496d to
072dec4
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Chandan9112 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 |
|
Scheduling required tests: |
|
/payload-job periodic-ci-openshift-release-main-nightly-4.22-e2e-aws-disruptive-longrunning |
|
@Chandan9112: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/f90f4f20-5382-11f1-9dcd-bb8b500d5873-0 |
|
|
||
| g.BeforeEach(func() { | ||
| isMicroShift, err := exutil.IsMicroShiftCluster(oc.AdminKubeClient()) | ||
| if err != nil { |
There was a problem hiding this comment.
Can we follow a common pattern:
o.Expect(err).NotTo(o.HaveOccurred())
if isMicroShift {
g.Skip("Skipping test on MicroShift cluster")
}
Skipping this test is not good if there's an error in detecting the platform. The reports won't show the test itself and we'll think everything is passing
| return updateErr | ||
| }) | ||
| if restoreErr != nil { | ||
| e2e.Logf("WARNING: failed to restore original image.config: %v; skipping MCP convergence wait", restoreErr) |
There was a problem hiding this comment.
Why only a warning? Restoring to the correct state is also important for the test right? This should fail the test in my view.
|
|
||
| e2e.Logf("Cleanup: waiting for all cluster operators to settle") | ||
| if waitErr := operator.WaitForOperatorsToSettle(ctx, oc.AdminConfigClient(), 10); waitErr != nil { | ||
| e2e.Logf("WARNING: cluster operators did not settle after restore: %v", waitErr) |
There was a problem hiding this comment.
Same as above. Why only warn?
| operator "github.com/openshift/origin/test/extended/util/operator" | ||
| ) | ||
|
|
||
| var _ = g.Describe("[Suite:openshift/disruptive-longrunning][sig-node][Disruptive] Image registry config", func() { |
There was a problem hiding this comment.
From the description I see that this test case takes 32 minutes to complete. Can we apply the config to just one master and one worker to reduce the time for this test case
| } | ||
| return nil | ||
| }, 30*time.Second, 5*time.Second).Should(o.Succeed(), | ||
| "search registry %s not found in registries config on node %s", searchRegistry, workers[0].Name) |
There was a problem hiding this comment.
Why worker[0] is chosen here?
| var registriesConf string | ||
| o.Eventually(func() error { | ||
| var execErr error | ||
| registriesConf, execErr = nodeutils.ExecOnNodeWithChroot(oc, workers[0].Name, |
There was a problem hiding this comment.
Is there another test case to check /etc/containers/policy.json is updated? If not, we need to check if that's updated
| o.Eventually(func() error { | ||
| var execErr error | ||
| registriesConf, execErr = nodeutils.ExecOnNodeWithChroot(oc, workers[0].Name, | ||
| "cat", "/etc/containers/registries.conf.d/01-image-searchRegistries.conf") |
There was a problem hiding this comment.
Why use cat? Can we directly use grep?
5 participants
Summary
image.config.openshift.io/clusterwithcontainerRuntimeSearchRegistriesandallowedRegistriestriggers an MCO rollout and the change is reflected in/etc/containers/registries.conf.d/01-image-searchRegistries.confon worker nodes.Here is the test case link: Polarian-44820
Test Results
It passed successfully while executing on a live OCP 4.22 cluster:
Summary by CodeRabbit