CNTRLPLANE-2654: added the library functions for workloads - #2336
CNTRLPLANE-2654: added the library functions for workloads#2336sandeepknd wants to merge 1 commit into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@sandeepknd: This pull request references CNTRLPLANE-2654 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 epic 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. |
|
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 node cordon/uncordon helpers and OLM helpers for resource lifecycle, CSV lookup, and package-manifest mapping. ChangesNode workload helpers
OLM resource helpers
🎯 3 (Moderate) | ⏱️ ~30 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/assign @ingvagabund |
![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
🤖 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/library/workloads/node.go`:
- Around line 15-29: The Node cordon helper currently does a direct
get/mutate/update flow and can fail on normal resource-version conflicts. Update
the logic in the Node helper(s) around the get/set Unschedulable/Update sequence
to retry on conflicts using retry.RetryOnConflict, or replace the Update with a
targeted patch to spec.unschedulable. Keep the existing error handling in the
cordon function so transient conflicts are retried instead of surfacing as flaky
failures.
In `@test/library/workloads/olm.go`:
- Around line 343-353: The code in the OLM workload helpers is ignoring error
returns from unstructured.NestedString, which can hide a wrongly typed
currentCSV and leave startingCSV empty without any signal. Update the logic in
the channel-selection flow and the similar NestedString usage elsewhere to
capture and handle the returned error instead of discarding it; if parsing
currentCSV fails, propagate or report the error so the caller can detect the bad
channel data. Use the existing startingCSV, channels, and defaultChannel path to
locate the affected calls and apply the same fix consistently.
- Around line 100-115: The retry helpers for OperatorGroup and Subscription
creation/deletion are losing the real API error and are not idempotent. Update
the polling logic in the create/delete helpers (including CreateSubscription and
the delete counterparts) to preserve and return the last error from the closure
instead of returning false,nil on failures. Use apierrors.IsAlreadyExists to
treat pre-existing creates as success and apierrors.IsNotFound to treat missing
deletes as success, so PollUntilContextTimeout does not mask the underlying
failure and the helpers become idempotent.
🪄 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: 21bb9ee6-e14a-4d0b-a2f9-756db3e75852
📒 Files selected for processing (2)
test/library/workloads/node.gotest/library/workloads/olm.go
| err := wait.PollUntilContextTimeout(ctx, 5*time.Second, 20*time.Second, true, func(ctx context.Context) (bool, error) { | ||
| _, err := dynamicClient.Resource(GetOperatorGroupGVR()).Namespace(og.Namespace).Create(ctx, operatorGroup, metav1.CreateOptions{}) | ||
| if err != nil { | ||
| klog.Warningf("Failed to create OperatorGroup, retrying: %v", err) | ||
| return false, nil | ||
| } | ||
| return true, nil | ||
| }) | ||
|
|
||
| if err != nil { | ||
| return fmt.Errorf("failed to create OperatorGroup %s: %w", og.Name, err) | ||
| } | ||
|
|
||
| klog.Infof("Successfully created OperatorGroup %s", og.Name) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Create retry loses the real API error and is not idempotent.
The poll closure returns false, nil on every Create error, so when the 20s window expires PollUntilContextTimeout returns a generic timeout error — the actual API failure cause is discarded and only logged at warning level, making failures hard to debug. Additionally, if the OperatorGroup already exists, Create returns AlreadyExists and this helper retries until timeout and then fails, instead of treating a pre-existing resource as success. The same pattern applies to CreateSubscription (Lines 167-178), and the delete counterparts (Lines 121-128 and 188-195) similarly retry on NotFound instead of treating it as already-deleted.
Consider capturing the last error and short-circuiting on AlreadyExists/NotFound via apierrors.IsAlreadyExists/apierrors.IsNotFound.
🛠️ Sketch for create idempotency + error preservation
- err := wait.PollUntilContextTimeout(ctx, 5*time.Second, 20*time.Second, true, func(ctx context.Context) (bool, error) {
- _, err := dynamicClient.Resource(GetOperatorGroupGVR()).Namespace(og.Namespace).Create(ctx, operatorGroup, metav1.CreateOptions{})
- if err != nil {
- klog.Warningf("Failed to create OperatorGroup, retrying: %v", err)
- return false, nil
- }
- return true, nil
- })
+ var lastErr error
+ err := wait.PollUntilContextTimeout(ctx, 5*time.Second, 20*time.Second, true, func(ctx context.Context) (bool, error) {
+ _, createErr := dynamicClient.Resource(GetOperatorGroupGVR()).Namespace(og.Namespace).Create(ctx, operatorGroup, metav1.CreateOptions{})
+ if createErr != nil && !apierrors.IsAlreadyExists(createErr) {
+ lastErr = createErr
+ klog.Warningf("Failed to create OperatorGroup, retrying: %v", createErr)
+ return false, nil
+ }
+ return true, nil
+ })
+ if err != nil && lastErr != nil {
+ err = lastErr
+ }(requires apierrors "k8s.io/apimachinery/pkg/api/errors")
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| err := wait.PollUntilContextTimeout(ctx, 5*time.Second, 20*time.Second, true, func(ctx context.Context) (bool, error) { | |
| _, err := dynamicClient.Resource(GetOperatorGroupGVR()).Namespace(og.Namespace).Create(ctx, operatorGroup, metav1.CreateOptions{}) | |
| if err != nil { | |
| klog.Warningf("Failed to create OperatorGroup, retrying: %v", err) | |
| return false, nil | |
| } | |
| return true, nil | |
| }) | |
| if err != nil { | |
| return fmt.Errorf("failed to create OperatorGroup %s: %w", og.Name, err) | |
| } | |
| klog.Infof("Successfully created OperatorGroup %s", og.Name) | |
| return nil | |
| } | |
| var lastErr error | |
| err := wait.PollUntilContextTimeout(ctx, 5*time.Second, 20*time.Second, true, func(ctx context.Context) (bool, error) { | |
| _, createErr := dynamicClient.Resource(GetOperatorGroupGVR()).Namespace(og.Namespace).Create(ctx, operatorGroup, metav1.CreateOptions{}) | |
| if createErr != nil && !apierrors.IsAlreadyExists(createErr) { | |
| lastErr = createErr | |
| klog.Warningf("Failed to create OperatorGroup, retrying: %v", createErr) | |
| return false, nil | |
| } | |
| return true, nil | |
| }) | |
| if err != nil && lastErr != nil { | |
| err = lastErr | |
| } | |
| if err != nil { | |
| return fmt.Errorf("failed to create OperatorGroup %s: %w", og.Name, err) | |
| } | |
| klog.Infof("Successfully created OperatorGroup %s", og.Name) | |
| return nil |
🤖 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/library/workloads/olm.go` around lines 100 - 115, The retry helpers for
OperatorGroup and Subscription creation/deletion are losing the real API error
and are not idempotent. Update the polling logic in the create/delete helpers
(including CreateSubscription and the delete counterparts) to preserve and
return the last error from the closure instead of returning false,nil on
failures. Use apierrors.IsAlreadyExists to treat pre-existing creates as success
and apierrors.IsNotFound to treat missing deletes as success, so
PollUntilContextTimeout does not mask the underlying failure and the helpers
become idempotent.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/library/workloads/olm.go`:
- Around line 285-288: The CSV phase polling logic currently treats all
NestedString errors from status.phase the same as “not ready,” which causes
malformed CSV objects to keep retrying until timeout. Update the polling
callback around unstructured.NestedString in the CSV phase check to distinguish
a type/schema error from a missing field: return the error immediately when err
is non-nil, but keep returning false,nil only when phase is simply not found.
Use the existing csvName logging and the surrounding polling function to keep
the behavior consistent while making malformed CSV phase failures fail fast.
🪄 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: b22242e3-a2f1-4d5f-b57b-d84fcc2df547
📒 Files selected for processing (2)
test/library/workloads/node.gotest/library/workloads/olm.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/library/workloads/node.go
| phase, found, err := unstructured.NestedString(csv.Object, "status", "phase") | ||
| if err != nil || !found { | ||
| klog.Warningf("CSV %s has no phase yet", csvName) | ||
| return false, nil |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Return malformed CSV phase errors instead of polling until timeout.
Line 285 can return an error when status.phase exists with the wrong type, but Lines 286-288 treat it like a missing phase and retry until the 3-minute timeout. Split the cases so schema/type errors fail immediately while absent phase keeps polling.
Proposed fix
// Get the phase from status
phase, found, err := unstructured.NestedString(csv.Object, "status", "phase")
- if err != nil || !found {
+ if err != nil {
+ return false, fmt.Errorf("failed to read CSV %s phase: %w", csvName, err)
+ }
+ if !found {
klog.Warningf("CSV %s has no phase yet", csvName)
return false, nil
}As per path instructions, “Never ignore error returns”.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| phase, found, err := unstructured.NestedString(csv.Object, "status", "phase") | |
| if err != nil || !found { | |
| klog.Warningf("CSV %s has no phase yet", csvName) | |
| return false, nil | |
| phase, found, err := unstructured.NestedString(csv.Object, "status", "phase") | |
| if err != nil { | |
| return false, fmt.Errorf("failed to read CSV %s phase: %w", csvName, err) | |
| } | |
| if !found { | |
| klog.Warningf("CSV %s has no phase yet", csvName) | |
| return false, nil | |
| } |
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 288-288: error is not nil (line 285) but it returns nil
(nilerr)
🤖 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/library/workloads/olm.go` around lines 285 - 288, The CSV phase polling
logic currently treats all NestedString errors from status.phase the same as
“not ready,” which causes malformed CSV objects to keep retrying until timeout.
Update the polling callback around unstructured.NestedString in the CSV phase
check to distinguish a type/schema error from a missing field: return the error
immediately when err is non-nil, but keep returning false,nil only when phase is
simply not found. Use the existing csvName logging and the surrounding polling
function to keep the behavior consistent while making malformed CSV phase
failures fail fast.
Sources: Path instructions, Linters/SAST tools
| @@ -0,0 +1,63 @@ | |||
| package workloads | |||
There was a problem hiding this comment.
workloads is too generic. Better to have each set of helpers under its own name. E.g. test/library/olm or similar.
There was a problem hiding this comment.
Is it like
test/library/olm/olm.go
test/library/node/node.go ?
| } | ||
|
|
||
| // GetOperatorGroupGVR returns the GroupVersionResource for OperatorGroup | ||
| func GetOperatorGroupGVR() schema.GroupVersionResource { |
There was a problem hiding this comment.
GetOperatorGroupGVR -> OperatorGroupGVR as there's really no "Get" operation performed. The same for other GetXXX functions.
| } | ||
|
|
||
| // SkipMissingCatalogSources checks if required catalog sources are available | ||
| func (sub *Subscription) SkipMissingCatalogSources(ctx context.Context, dynamicClient dynamic.Interface) error { |
There was a problem hiding this comment.
Where the "Skip" keyword comes from? The method checks for an existence, it does not skip.
| continue | ||
| } | ||
| // Use k8s built-in helpers instead of manual map access | ||
| name, _, _ := unstructured.NestedString(imgMap, "name") |
There was a problem hiding this comment.
what if the field does not exists or an error is returned? The same for the other field.
| } | ||
|
|
||
| node.Spec.Unschedulable = true | ||
| _, err = client.CoreV1().Nodes().Update(ctx, node, metav1.UpdateOptions{}) |
There was a problem hiding this comment.
What about Patch instead of Update?
| } | ||
|
|
||
| node.Spec.Unschedulable = false | ||
| _, err = client.CoreV1().Nodes().Update(ctx, node, metav1.UpdateOptions{}) |
| } | ||
|
|
||
| if node.Spec.Unschedulable { | ||
| klog.Infof("Node %s is already cordoned", nodeName) |
There was a problem hiding this comment.
Is logging this information useful?
| return err | ||
| } | ||
|
|
||
| klog.Infof("Successfully cordoned node %s", nodeName) |
There was a problem hiding this comment.
Is logging this information useful? Instead, the invoker might check for err == nil and log this if needed.
| ) | ||
|
|
||
| // OperatorGroup represents an OLM OperatorGroup resource configuration | ||
| type OperatorGroup struct { |
There was a problem hiding this comment.
Are these new types duplicates of already existing definitions? E.g.:
- https://github.com/operator-framework/api/blob/492d6ba7263f792b5a3dcf24569fcd4f7d1c498d/pkg/operators/v1/operatorgroup_types.go#L118
- https://github.com/operator-framework/api/blob/492d6ba7263f792b5a3dcf24569fcd4f7d1c498d/pkg/operators/v1alpha1/subscription_types.go#L31
- etc.
Any way of reusing these? To avoid drifting from them.
There was a problem hiding this comment.
Re: Using github.com/operator-framework/api/pkg/operators/v1 OperatorGroup
This dependency is currently unavailable in the project. Tests confirm it's not present and would require adding a new dependency:
# Check if dependency exists
$ grep "operator-framework/api" go.mod go.sum
# (no output)
$ go mod why github.com/operator-framework/api
(main module does not need package github.com/operator-framework/api)
$ go list -m github.com/operator-framework/api
go: module github.com/operator-framework/api: not a known dependency
$ find vendor -type d -name "operator-framework"
# (no output)
Using the official v1.OperatorGroup type would require:
- go get github.com/operator-framework/api@
- go mod tidy && go mod vendor
Are we okay to import dependencies in test directory ?
There was a problem hiding this comment.
That is ok. The dependency will be vendored only when test/library/olm is imported.
| func (og *OperatorGroup) CreateOperatorGroup(ctx context.Context, dynamicClient dynamic.Interface) error { | ||
| klog.Infof("Creating OperatorGroup %s in namespace %s", og.Name, og.Namespace) | ||
|
|
||
| operatorGroup := &unstructured.Unstructured{ |
There was a problem hiding this comment.
Why not import https://github.com/operator-framework/api/blob/492d6ba7263f792b5a3dcf24569fcd4f7d1c498d/pkg/operators/v1/operatorgroup_types.go#L118 type directly and populate it? There might even be the corresponding clients available? The same for other places.
There's a chance most of the code here can be replaced by the github.com/operator-framework/*** code.
| // CordonNode marks a node as unschedulable, preventing new pods from being scheduled on it. | ||
| // This is commonly used during maintenance operations or when testing pod eviction scenarios. | ||
| // Uses retry.RetryOnConflict to handle concurrent node updates by kubelet/controllers. | ||
| func CordonNode(ctx context.Context, client kubernetes.Interface, nodeName string) error { |
There was a problem hiding this comment.
There was a problem hiding this comment.
This would need adding new dependencies.
k8s.io/kubectl/pkg/drain
There was a problem hiding this comment.
Actually, you should be able to use CordonHelper directly in the KDO repository. Without creating any helpers here.
There was a problem hiding this comment.
Available k8s.io packages in kdo vendor:
- k8s.io/api
- k8s.io/client-go
- k8s.io/kubernetes
Unavailable: - k8s.io/kubectl
So, CordonHelper usage will cause new dependency to be downloaded in kdo repo.
$ go list -m k8s.io/kubectl
go: module k8s.io/kubectl: can't resolve module using the vendor directory
(Use -mod=mod or -mod=readonly to bypass.)
There was a problem hiding this comment.
Hi @ingvagabund ,
Currently in kdo repo, k8s.io/kubectl is not used anywhere.
There was a problem hiding this comment.
This is my current concern.
#2336 (comment)
If I need to use CordonHelper directly in kdo, we need import the dependency k8s.io/kubectl. Are we good to go with the import of this dep ?
There was a problem hiding this comment.
Yes, it's ok to vendor the extra dependency.
1ff25c4 to
db795b4
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/library/workloads/olm.go`:
- Around line 303-307: WaitForCSVSucceeded currently retries on every CSV Get
error and hides non-transient failures behind a timeout. In the callback inside
WaitForCSVSucceeded, keep retrying only for expected NotFound cases from
dynamicClient.Resource(CSVGVR()).Namespace(namespace).Get, but return the actual
error immediately for RBAC, GVR, transport, or other non-NotFound failures so
PollUntilContextTimeout can fail fast and preserve the real cause.
- Around line 343-390: The PackageManifest fallback logic in the
subscription-building path is too permissive and can synthesize mismatched
source/channel/CSV values. In the code that reads catalogSource, defaultChannel,
and channels/currentCSV, remove the hardcoded defaults and first-channel
fallback; instead, require a valid default channel and its currentCSV to be
present and return an error from this workflow when they cannot be resolved.
Keep the validation and extraction localized around the
unstructured.NestedString/NestedSlice handling so the resulting Subscription is
only created from a coherent manifest tuple.
- Around line 23-29: The Subscription helper contract is missing the catalog
source namespace, causing CreateSubscription and VerifyCatalogSourceExists to
assume openshift-marketplace for every package. Add a SourceNamespace field to
Subscription, populate it in GetPackageManifest from
status.catalogSourceNamespace, and update CreateSubscription and
VerifyCatalogSourceExists to use Subscription.SourceNamespace instead of the
hard-coded namespace so packages from other catalog namespaces are handled
correctly.
🪄 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: cb883a7c-2016-43d8-b274-afd65adf6047
📒 Files selected for processing (2)
test/library/workloads/node.gotest/library/workloads/olm.go
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
test/library/workloads/olm.go (4)
387-389: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDon’t swallow malformed channel entries.
If
status.channels[*].namehas the wrong type, thiscontinuedegrades into a misleading “default channel not found” later. Return the parsing error instead of treating it as a non-match.🤖 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/library/workloads/olm.go` around lines 387 - 389, In the channel parsing logic around unstructured.NestedString in the workload test helper, do not treat a type error on status.channels[*].name as a simple non-match. Update the code path in the channel iteration to distinguish an error from a missing field and return the parsing error immediately instead of continuing, so malformed channel entries are surfaced instead of later appearing as “default channel not found.”Source: Path instructions
316-319: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winFail fast on malformed
status.phase.A
NestedStringtype/schema error is still treated like “phase not ready”, so bad CSV objects poll for three minutes and lose the real cause.🤖 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/library/workloads/olm.go` around lines 316 - 319, The CSV phase check in the workload polling logic is swallowing schema/type errors from NestedString and treating them like a missing phase, which delays the real failure. Update the CSV status handling in the phase-check path to distinguish a malformed status.phase from an unset phase: when unstructured.NestedString returns an error, fail fast and return that error instead of logging “no phase yet” and continuing. Keep the existing “not found yet” retry behavior only for the found=false case in the CSV polling code.Sources: Path instructions, Linters/SAST tools
234-242: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon’t pick an arbitrary CSV when the selector matches multiple objects.
Using
csvList.Items[0]makes downstream image/phase helpers depend on list ordering rather than a stable rule. Fail here or apply an explicit selection criterion.🤖 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/library/workloads/olm.go` around lines 234 - 242, The CSV selection in the OLM workload helper is currently arbitrary because `csvList.Items[0]` is used after `csvList.Items` may contain multiple matches. Update the `csvName` selection logic in `test/library/workloads/olm.go` to avoid depending on list order: either fail fast when more than one CSV matches, or add a deterministic selection rule before `klog.Infof("Using CSV: %s", csvName)`. Keep the behavior localized around the `csvList.Items` handling and the `klog.Warningf`/`klog.Infof` logging block.
102-109: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPreserve API failures and make the helpers idempotent.
These poll callbacks still convert every create/delete error into
false, nil, soAlreadyExists/NotFoundcases retry until timeout and real RBAC/GVR/transport failures get masked as a generic poll timeout.Also applies to: 123-130, 169-176, 190-197
🤖 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/library/workloads/olm.go` around lines 102 - 109, The polling helpers in OLM workload setup are swallowing create/delete errors by returning false, nil from the callback, which hides real failures and makes AlreadyExists/NotFound cases retry until timeout. Update the callbacks in the OperatorGroup and related helper logic to treat idempotent API responses as success while propagating unexpected errors from dynamicClient.Resource(...).Create/Delete instead of converting them to generic poll retries. Use the existing helper functions around the create/delete loops in olm.go to keep the behavior consistent across the affected blocks.Source: Path instructions
🤖 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/library/workloads/olm.go`:
- Around line 123-130: The deletion polling in the OperatorGroup cleanup logic
returns success as soon as Delete is accepted, which can race later
read/recreate steps. Update the wait.PollUntilContextTimeout loop in the
OperatorGroup removal flow to first issue the Delete and then poll using Get on
the same OperatorGroupGVR()/Namespace object until apierrors.IsNotFound is
returned. Apply the same change to the other matching cleanup block referenced
by the comment, keeping the retry logging around transient delete/Get errors.
---
Duplicate comments:
In `@test/library/workloads/olm.go`:
- Around line 387-389: In the channel parsing logic around
unstructured.NestedString in the workload test helper, do not treat a type error
on status.channels[*].name as a simple non-match. Update the code path in the
channel iteration to distinguish an error from a missing field and return the
parsing error immediately instead of continuing, so malformed channel entries
are surfaced instead of later appearing as “default channel not found.”
- Around line 316-319: The CSV phase check in the workload polling logic is
swallowing schema/type errors from NestedString and treating them like a missing
phase, which delays the real failure. Update the CSV status handling in the
phase-check path to distinguish a malformed status.phase from an unset phase:
when unstructured.NestedString returns an error, fail fast and return that error
instead of logging “no phase yet” and continuing. Keep the existing “not found
yet” retry behavior only for the found=false case in the CSV polling code.
- Around line 234-242: The CSV selection in the OLM workload helper is currently
arbitrary because `csvList.Items[0]` is used after `csvList.Items` may contain
multiple matches. Update the `csvName` selection logic in
`test/library/workloads/olm.go` to avoid depending on list order: either fail
fast when more than one CSV matches, or add a deterministic selection rule
before `klog.Infof("Using CSV: %s", csvName)`. Keep the behavior localized
around the `csvList.Items` handling and the `klog.Warningf`/`klog.Infof` logging
block.
- Around line 102-109: The polling helpers in OLM workload setup are swallowing
create/delete errors by returning false, nil from the callback, which hides real
failures and makes AlreadyExists/NotFound cases retry until timeout. Update the
callbacks in the OperatorGroup and related helper logic to treat idempotent API
responses as success while propagating unexpected errors from
dynamicClient.Resource(...).Create/Delete instead of converting them to generic
poll retries. Use the existing helper functions around the create/delete loops
in olm.go to keep the behavior consistent across the affected blocks.
🪄 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: eca8c9f5-1964-4c34-b117-cfc6783a0758
📒 Files selected for processing (2)
test/library/workloads/node.gotest/library/workloads/olm.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/library/workloads/node.go
sandeepknd
changed the title
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
52ec140 to
89c24b3
Compare
sandeepknd
changed the title
sandeepknd
changed the title
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
sandeepknd
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
| // PackageManifestGVR returns the GroupVersionResource for PackageManifest | ||
| func PackageManifestGVR() schema.GroupVersionResource { | ||
| return schema.GroupVersionResource{ | ||
| Group: "packages.operators.coreos.com", |
There was a problem hiding this comment.
The reason behind the hard-coded Group is that PackageManifest belongs to a different API group (packages.operators.coreos.com) that doesn't have constants defined in the operator-framework/api package we're using.
There was a problem hiding this comment.
There was a problem hiding this comment.
It introduces dependency conflicts and once again updates the go version to Go 1.26.x that creates the same issue which we encountered in "make verify:.
|
|
||
| // GetCSVName gets the CSV name for the operator using label selector. | ||
| // When multiple CSVs are found, it returns the one with the highest semver version. | ||
| func GetCSVName(ctx context.Context, dynamicClient dynamic.Interface, namespace, labelSelector string) (string, error) { |
There was a problem hiding this comment.
Given the function could be invoked to get the latest CSV it's better to return the whole CSV object.
There was a problem hiding this comment.
s/GetCSVName/GetTheLatestCSVName
|
|
||
| // BuildSubscriptionFromPackageManifest fetches a packagemanifest for a given package | ||
| // and builds a Subscription object populated with the default channel, catalog source, and starting CSV information. | ||
| func BuildSubscriptionFromPackageManifest(ctx context.Context, dynamicClient dynamic.Interface, packageName, namespace string) (*operatorsv1alpha1.Subscription, error) { |
There was a problem hiding this comment.
The function will get more usable if you pass the whole package manifest object. The type is defined at https://github.com/operator-framework/operator-lifecycle-manager/blob/master/pkg/package-server/apis/operators/v1/packagemanifest_types.go#L22.
There was a problem hiding this comment.
The same issue is encountered here on importing the types from
packagesv1 "github.com/operator-framework/operator-lifecycle-manager/pkg/package-server/apis/operators/v1"
go mod tidy 2>&1 | head -50)
⎿ go: finding module for package github.com/operator-framework/operator-lifecycle-manager/pkg/package-server/apis/operators/v1
go: toolchain upgrade needed to resolve github.com/operator-framework/operator-lifecycle-manager/pkg/package-server/apis/operators/v1
go: github.com/operator-framework/operator-lifecycle-manager@v0.45.0 requires go >= 1.26.3; switching to go1.26.5
which will cause
make verify
to fail
| } | ||
|
|
||
| // GetCSVRelatedImages gets the relatedImages from a CSV | ||
| func GetCSVRelatedImages(ctx context.Context, dynamicClient dynamic.Interface, namespace, csvName string) ([]RelatedImage, error) { |
There was a problem hiding this comment.
ditto. More useful to pass the whole csv object
| } | ||
|
|
||
| // DeleteSubscription deletes the Subscription | ||
| func DeleteSubscription(ctx context.Context, dynamicClient dynamic.Interface, sub *operatorsv1alpha1.Subscription) error { |
There was a problem hiding this comment.
Is deleting a subscription waiting for something? Or, how often does it happen the deletion errors?
| } | ||
|
|
||
| // DeleteOperatorGroup deletes the OperatorGroup | ||
| func DeleteOperatorGroup(ctx context.Context, dynamicClient dynamic.Interface, og *operatorsv1.OperatorGroup) error { |
There was a problem hiding this comment.
Is deleting a group waiting for something? Or, how often does it happen the deletion errors?
ingvagabund
left a comment
ingvagabund
left a comment
There was a problem hiding this comment.
Given the helpers receives either a name or an object and then convert it into unstructured object via runtime.DefaultUnstructuredConverter.ToUnstructured it's worth considering to pass only unstructured objects and perform the conversion in individual operator repositories. With a detailed comment about the intention. This way the OLM specific types vendoring can be moved to the operator repositories.
The current suggestion:
- updating the helper signatures to accept and return unstructured objects (with proper comment)
- vendoring the code changes into the KDO repository
- (de)converting objects
| // PackageManifestGVR returns the GroupVersionResource for PackageManifest | ||
| func PackageManifestGVR() schema.GroupVersionResource { | ||
| return schema.GroupVersionResource{ | ||
| Group: "packages.operators.coreos.com", |
There was a problem hiding this comment.
Does it mean reverting to the original changes without any of these imports ? |
3bd0318 to
8cbfad7
Compare
addressed. |
|
@sandeepknd: all tests passed! 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. |
|
Hi @ingvagabund , |
1 similar comment
|
Hi @ingvagabund , |
|
Hi @ingvagabund, can you take a look at this PR? |
|
/lgtm |
|
Relatable PR: https://github.com/openshift/cluster-kube-descheduler-operator/blob/main/go.mod#L71 got merged. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ropatil010, sandeepknd 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 |
8 participants
Added the library helper functions for workloads.
Kindly refer this openshift/cluster-kube-descheduler-operator#2030 (comment) for the actual context.
Summary by CodeRabbit