MCO-2211: MCO-2210: MCO-2234 MCO-Migrate MCO tests from openshift-tests-private#5902
Open
ptalgulk01 wants to merge 1 commit intoopenshift:mainfrom
Open
MCO-2211: MCO-2210: MCO-2234 MCO-Migrate MCO tests from openshift-tests-private#5902ptalgulk01 wants to merge 1 commit intoopenshift:mainfrom
ptalgulk01 wants to merge 1 commit intoopenshift:mainfrom
Conversation
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
Contributor
|
@ptalgulk01: This pull request references MCO-2211 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. |
WalkthroughAdds exported test utilities and helpers (logs, MachineConfig/MCP queries, node filtering, exec helpers, scaling), multiple long-duration MCO Ginkgo test suites (drain, MCP lifecycle, prune), and new YAML test fixtures. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (5 warnings, 2 inconclusive)
✅ Passed checks (5 passed)
✨ 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ptalgulk01 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: 9
🧹 Nitpick comments (1)
test/extended-priv/util.go (1)
1301-1318: Useerrors.Asfor more robust and idiomatic exec error detection.The current implementation manually unwraps one level and checks the type, which works for the current codebase structure but is less robust and idiomatic than using
errors.As. This pattern will handle arbitrary error wrapping depths and aligns with Go best practices.Proposed fix
func IsExecShellError(err error) bool { - if unwrapped := errors.Unwrap(err); unwrapped != nil { - _, ok := unwrapped.(*exec.ExitError) - return ok - } - _, ok := err.(*exec.ExitError) - return ok + var exitErr *exec.ExitError + return errors.As(err, &exitErr) } // UnwrapExecCode unwraps the error and extracts the stderr string if possible func UnwrapExecCode(err error) (int, error) { - if unwrapped := errors.Unwrap(err); unwrapped != nil { - exitError, ok := unwrapped.(*exec.ExitError) - if ok { - return exitError.ExitCode(), nil - } - } - return -1, fmt.Errorf("No exit code available in the provided error %s", err) + var exitErr *exec.ExitError + if errors.As(err, &exitErr) { + return exitErr.ExitCode(), nil + } + return -1, fmt.Errorf("no exit code available in error: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended-priv/util.go` around lines 1301 - 1318, The type checks in IsExecShellError and UnwrapExecCode only inspect one unwrap level; replace the manual unwrap logic with errors.As to robustly detect *exec.ExitError across arbitrary wrapping. In IsExecShellError use errors.As(err, &exitError) and return whether it matched; in UnwrapExecCode use errors.As to obtain the *exec.ExitError and return exitError.ExitCode(), otherwise return -1 and a formatted error mentioning the original err. Update references to IsExecShellError and UnwrapExecCode accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/extended-priv/machineconfig.go`:
- Around line 184-189: GetMCPRenderedMachineConfigsOrFail currently calls
GetRenderedMachineConfigForMaster, so it returns only master-rendered
MachineConfigs; change it to return MCP-specific rendered MachineConfigs by
either renaming the method to reflect the master-only behavior or, preferably,
implement and call an MCP-aware lookup (e.g., create/use
GetRenderedMachineConfigForMCP or a filter by MCP label/selector inside
GetMCPRenderedMachineConfigsOrFail) and ensure you pass the MCP identifier to
that lookup and update any callers accordingly; search for
GetMCPRenderedMachineConfigsOrFail and GetRenderedMachineConfigForMaster to
locate the code to modify.
In `@test/extended-priv/mco_drain.go`:
- Around line 88-94: The test currently assumes filterTimestampFromLogs returns
at least 3 timestamps and indexes timestamps[0..2], which can cause a panic if
the log format changes; update the test around filterTimestampFromLogs and
timestamps to first assert the slice length (e.g., require or
o.Expect(len(timestamps)).To(BeNumerically(">=", 3))) before accessing
timestamps[0], timestamps[1], timestamps[2], and then perform the existing
getTimeDifferenceInMinute checks so failures surface as test assertions rather
than panics.
In `@test/extended-priv/mco_machineconfigpool.go`:
- Around line 330-332: The call to workerNode.PatchDesiredConfig(desiredConfig)
is unchecked; capture its returned error (e.g., err :=
workerNode.PatchDesiredConfig(desiredConfig)) and handle it explicitly: if err
!= nil, log the failure with context using logger.Errorf("failed to patch
desiredConfig for worker %s: %v", workerNode.GetName(), err) and fail the test
or return the error from the enclosing function (use the existing test assertion
mechanism if applicable) so a patch failure surfaces immediately.
- Around line 99-102: The test is checking for "NotFound" in stdout but
oc.AsAdmin().WithoutNamespace().Run("get").Args("mcp/"+mcpName).Output() only
returns stdout; change the call to use a method that captures stderr (e.g.,
CombinedOutput()) and assert against that result (update the mcpOut variable
usage) so the "NotFound" message emitted on stderr is detected; locate the call
to oc.AsAdmin().WithoutNamespace().Run("get").Args("mcp/"+mcpName).Output() and
replace it with the CombinedOutput-equivalent and assert Combined output
contains "NotFound".
In `@test/extended-priv/mco_prune.go`:
- Around line 50-53: The current rendered-MC collection misses worker configs
because GetMCPRenderedMachineConfigsOrFail() delegates to the master-only helper
GetRenderedMachineConfigForMaster; update GetMCPRenderedMachineConfigsOrFail()
to iterate all MachineConfigPools and for each pool call the appropriate helper
based on pool role (use GetRenderedMachineConfigForMasterOrFail() for master
pools and the worker equivalent helper for worker pools, or a unified helper
that returns rendered config for any pool), then return the combined
master+worker rendered configs so the call site
(mcList.GetMCPRenderedMachineConfigsOrFail() in mco_prune.go) receives both
master and worker rendered machineconfigs.
- Around line 39-40: The call to mMcp.WaitImmediateForUpdatedStatus() is
currently ignored; capture its return value and fail the test if it indicates a
timeout/error before logging "OK!"; specifically, replace the bare call to
mMcp.WaitImmediateForUpdatedStatus() with code that assigns the result (e.g.,
ok, err, or bool) and assert/fail accordingly (using the test harness's
Expect/require/t.Fatalf as used elsewhere in this file) so downstream assertions
only run when WaitImmediateForUpdatedStatus() succeeded, then log via
logger.Infof("OK!\n") after the check.
- Around line 252-259: Unwrap the exit code into variables instead of passing
UnwrapExecCode(err) directly into Expect: call something like (code, unwrapErr)
:= UnwrapExecCode(err), assert unwrapErr is nil/DidNot(HaveOccurred()) before
asserting the numeric exit code with Expect(code).Replace both usages in the
prune tests (the lines using UnwrapExecCode(err) to check non-zero return code)
so the unwrap error is explicitly checked and the actual code is asserted
separately.
In `@test/extended-priv/testdata/files/create-pod.yaml`:
- Around line 23-29: The Pod-level securityContext currently includes
container-only fields (allowPrivilegeEscalation and capabilities) which are
ignored; move allowPrivilegeEscalation and capabilities into the container's
securityContext (the container spec for the pod's container), leaving
runAsNonRoot and seccompProfile (pod-level or keep under pod's securityContext
if intended) appropriately placed—update the manifest so the container entry has
a securityContext block containing allowPrivilegeEscalation: false and
capabilities: { drop: ["ALL"] } while leaving runAsNonRoot and seccompProfile
where you want pod-level behavior.
In `@test/extended-priv/util.go`:
- Around line 1321-1339: The timestamp parsing is brittle:
filterTimestampFromLogs uses an unescaped "." so it can match wrong separators
and getTimeDifferenceInMinute blindly indexes split results causing panics; fix
by updating the regex in filterTimestampFromLogs to use a literal dot (e.g.
"\.[0-9]{1,6}") and a stricter pattern for timestamps, and change
getTimeDifferenceInMinute to validate split lengths before indexing (check
len(oldTimeValues)>=3 and that splitting the seconds contains two parts), or
better yet use time.Parse with a matching layout to parse both oldTimestamp and
newTimestamp safely; also normalize fractional seconds to nanoseconds when
constructing time.Date so you don’t mis-handle 1–6 digit fractions.
---
Nitpick comments:
In `@test/extended-priv/util.go`:
- Around line 1301-1318: The type checks in IsExecShellError and UnwrapExecCode
only inspect one unwrap level; replace the manual unwrap logic with errors.As to
robustly detect *exec.ExitError across arbitrary wrapping. In IsExecShellError
use errors.As(err, &exitError) and return whether it matched; in UnwrapExecCode
use errors.As to obtain the *exec.ExitError and return exitError.ExitCode(),
otherwise return -1 and a formatted error mentioning the original err. Update
references to IsExecShellError and UnwrapExecCode accordingly.
🪄 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: a3281dcc-aa80-4b54-80e9-fdcf3bdaef06
📒 Files selected for processing (13)
test/extended-priv/const.gotest/extended-priv/controller.gotest/extended-priv/machineconfig.gotest/extended-priv/machineconfigpool.gotest/extended-priv/mco_drain.gotest/extended-priv/mco_machineconfigpool.gotest/extended-priv/mco_prune.gotest/extended-priv/node.gotest/extended-priv/testdata/files/add-mc-to-trigger-node-drain.yamltest/extended-priv/testdata/files/change-worker-ign-version.yamltest/extended-priv/testdata/files/create-pod.yamltest/extended-priv/testdata/files/pod-disruption-budget.yamltest/extended-priv/util.go
Comment on lines
+184
to
+189
| // GetMachineConfigCreatedByMCPs returns a list of the machineconfigs that were created by a MCP | ||
| func (mcl *MachineConfigList) GetMCPRenderedMachineConfigsOrFail() []*MachineConfig { | ||
| renderedMcList, err := mcl.GetRenderedMachineConfigForMaster() | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "Error getting the list of the machineconfigs that were created by a MCP ") | ||
| return renderedMcList | ||
| } |
There was a problem hiding this comment.
GetMCPRenderedMachineConfigsOrFail returns the wrong data set.
This wrapper still delegates to the master-only lookup, so it cannot return rendered MachineConfigs for other MCPs. Either rename it to match the current behavior or point it at an MCP-specific filter.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/extended-priv/machineconfig.go` around lines 184 - 189,
GetMCPRenderedMachineConfigsOrFail currently calls
GetRenderedMachineConfigForMaster, so it returns only master-rendered
MachineConfigs; change it to return MCP-specific rendered MachineConfigs by
either renaming the method to reflect the master-only behavior or, preferably,
implement and call an MCP-aware lookup (e.g., create/use
GetRenderedMachineConfigForMCP or a filter by MCP label/selector inside
GetMCPRenderedMachineConfigsOrFail) and ensure you pass the MCP identifier to
that lookup and update any callers accordingly; search for
GetMCPRenderedMachineConfigsOrFail and GetRenderedMachineConfigForMaster to
locate the code to modify.
Comment on lines
+88
to
+94
| timestamps := filterTimestampFromLogs(podLogs, 3) | ||
| logger.Infof("Timestamps %s", timestamps) | ||
| // First 3 retries should be queued every 1 minute. We check 1 min < time < 2.7 min | ||
| o.Expect(getTimeDifferenceInMinute(timestamps[0], timestamps[1])).Should(o.BeNumerically("<=", 2.7)) | ||
| o.Expect(getTimeDifferenceInMinute(timestamps[0], timestamps[1])).Should(o.BeNumerically(">=", 1)) | ||
| o.Expect(getTimeDifferenceInMinute(timestamps[1], timestamps[2])).Should(o.BeNumerically("<=", 2.7)) | ||
| o.Expect(getTimeDifferenceInMinute(timestamps[1], timestamps[2])).Should(o.BeNumerically(">=", 1)) |
There was a problem hiding this comment.
Guard extracted timestamps before indexing.
This code assumes exactly 3 timestamp matches; if log format drifts, this will panic instead of producing a clear assertion failure.
💡 Proposed fix
timestamps := filterTimestampFromLogs(podLogs, 3)
+ o.Expect(timestamps).To(o.HaveLen(3), "Expected 3 timestamps in drain failure logs")
logger.Infof("Timestamps %s", timestamps)📝 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.
Suggested change
| timestamps := filterTimestampFromLogs(podLogs, 3) | |
| logger.Infof("Timestamps %s", timestamps) | |
| // First 3 retries should be queued every 1 minute. We check 1 min < time < 2.7 min | |
| o.Expect(getTimeDifferenceInMinute(timestamps[0], timestamps[1])).Should(o.BeNumerically("<=", 2.7)) | |
| o.Expect(getTimeDifferenceInMinute(timestamps[0], timestamps[1])).Should(o.BeNumerically(">=", 1)) | |
| o.Expect(getTimeDifferenceInMinute(timestamps[1], timestamps[2])).Should(o.BeNumerically("<=", 2.7)) | |
| o.Expect(getTimeDifferenceInMinute(timestamps[1], timestamps[2])).Should(o.BeNumerically(">=", 1)) | |
| timestamps := filterTimestampFromLogs(podLogs, 3) | |
| o.Expect(timestamps).To(o.HaveLen(3), "Expected 3 timestamps in drain failure logs") | |
| logger.Infof("Timestamps %s", timestamps) | |
| // First 3 retries should be queued every 1 minute. We check 1 min < time < 2.7 min | |
| o.Expect(getTimeDifferenceInMinute(timestamps[0], timestamps[1])).Should(o.BeNumerically("<=", 2.7)) | |
| o.Expect(getTimeDifferenceInMinute(timestamps[0], timestamps[1])).Should(o.BeNumerically(">=", 1)) | |
| o.Expect(getTimeDifferenceInMinute(timestamps[1], timestamps[2])).Should(o.BeNumerically("<=", 2.7)) | |
| o.Expect(getTimeDifferenceInMinute(timestamps[1], timestamps[2])).Should(o.BeNumerically(">=", 1)) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/extended-priv/mco_drain.go` around lines 88 - 94, The test currently
assumes filterTimestampFromLogs returns at least 3 timestamps and indexes
timestamps[0..2], which can cause a panic if the log format changes; update the
test around filterTimestampFromLogs and timestamps to first assert the slice
length (e.g., require or o.Expect(len(timestamps)).To(BeNumerically(">=", 3)))
before accessing timestamps[0], timestamps[1], timestamps[2], and then perform
the existing getTimeDifferenceInMinute checks so failures surface as test
assertions rather than panics.
Comment on lines
+99
to
+102
| mcpOut, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("mcp/" + mcpName).Output() | ||
| o.Expect(err).Should(o.HaveOccurred()) | ||
| o.Expect(mcpOut).Should(o.ContainSubstring("NotFound")) | ||
| logger.Infof("Custom mcp is deleted successfully!") |
There was a problem hiding this comment.
NotFound check is reading stdout instead of stderr.
Run(...).Output() returns stdout; for failed oc get, the NotFound message is typically in stderr, so this assertion can fail even when behavior is correct.
💡 Proposed fix
- mcpOut, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("mcp/" + mcpName).Output()
+ _, mcpErrOut, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("mcp/" + mcpName).Outputs()
o.Expect(err).Should(o.HaveOccurred())
- o.Expect(mcpOut).Should(o.ContainSubstring("NotFound"))
+ o.Expect(mcpErrOut).Should(o.ContainSubstring("NotFound"))📝 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.
Suggested change
| mcpOut, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("mcp/" + mcpName).Output() | |
| o.Expect(err).Should(o.HaveOccurred()) | |
| o.Expect(mcpOut).Should(o.ContainSubstring("NotFound")) | |
| logger.Infof("Custom mcp is deleted successfully!") | |
| _, mcpErrOut, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("mcp/" + mcpName).Outputs() | |
| o.Expect(err).Should(o.HaveOccurred()) | |
| o.Expect(mcpErrOut).Should(o.ContainSubstring("NotFound")) | |
| logger.Infof("Custom mcp is deleted successfully!") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/extended-priv/mco_machineconfigpool.go` around lines 99 - 102, The test
is checking for "NotFound" in stdout but
oc.AsAdmin().WithoutNamespace().Run("get").Args("mcp/"+mcpName).Output() only
returns stdout; change the call to use a method that captures stderr (e.g.,
CombinedOutput()) and assert against that result (update the mcpOut variable
usage) so the "NotFound" message emitted on stderr is detected; locate the call
to oc.AsAdmin().WithoutNamespace().Run("get").Args("mcp/"+mcpName).Output() and
replace it with the CombinedOutput-equivalent and assert Combined output
contains "NotFound".
Comment on lines
+39
to
+40
| mMcp.WaitImmediateForUpdatedStatus() | ||
| logger.Infof("OK!\n") |
There was a problem hiding this comment.
Check the MCP wait result instead of ignoring it.
If this wait fails, later assertions run against unstable state and can produce misleading failures.
💡 Proposed fix
- mMcp.WaitImmediateForUpdatedStatus()
+ o.Expect(mMcp.WaitImmediateForUpdatedStatus()).To(o.Succeed(), "Master MCP did not reach Updated status")📝 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.
Suggested change
| mMcp.WaitImmediateForUpdatedStatus() | |
| logger.Infof("OK!\n") | |
| o.Expect(mMcp.WaitImmediateForUpdatedStatus()).To(o.Succeed(), "Master MCP did not reach Updated status") | |
| logger.Infof("OK!\n") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/extended-priv/mco_prune.go` around lines 39 - 40, The call to
mMcp.WaitImmediateForUpdatedStatus() is currently ignored; capture its return
value and fail the test if it indicates a timeout/error before logging "OK!";
specifically, replace the bare call to mMcp.WaitImmediateForUpdatedStatus() with
code that assigns the result (e.g., ok, err, or bool) and assert/fail
accordingly (using the test harness's Expect/require/t.Fatalf as used elsewhere
in this file) so downstream assertions only run when
WaitImmediateForUpdatedStatus() succeeded, then log via logger.Infof("OK!\n")
after the check.
Comment on lines
+50
to
+53
| sortedRenderedMCs := mcList.GetMCPRenderedMachineConfigsOrFail() | ||
| logger.Infof(" %s", sortedRenderedMCs) | ||
|
|
||
| sortedMCListMaster := mcList.GetRenderedMachineConfigForMasterOrFail() // to get master rendered machine config |
There was a problem hiding this comment.
This rendered-MC validation currently misses worker-rendered configs.
GetMCPRenderedMachineConfigsOrFail() currently calls the master-only helper (GetRenderedMachineConfigForMaster) per test/extended-priv/machineconfig.go:185-189, so worker prune coverage here is incomplete.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/extended-priv/mco_prune.go` around lines 50 - 53, The current
rendered-MC collection misses worker configs because
GetMCPRenderedMachineConfigsOrFail() delegates to the master-only helper
GetRenderedMachineConfigForMaster; update GetMCPRenderedMachineConfigsOrFail()
to iterate all MachineConfigPools and for each pool call the appropriate helper
based on pool role (use GetRenderedMachineConfigForMasterOrFail() for master
pools and the worker equivalent helper for worker pools, or a unified helper
that returns rendered config for any pool), then return the combined
master+worker rendered configs so the call site
(mcList.GetMCPRenderedMachineConfigsOrFail() in mco_prune.go) receives both
master and worker rendered machineconfigs.
Comment on lines
+23
to
+29
| securityContext: | ||
| allowPrivilegeEscalation: false | ||
| capabilities: | ||
| drop: ["ALL"] | ||
| runAsNonRoot: true | ||
| seccompProfile: | ||
| type: "RuntimeDefault" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n test/extended-priv/testdata/files/create-pod.yamlRepository: openshift/machine-config-operator
Length of output: 1091
🌐 Web query:
Kubernetes securityContext allowPrivilegeEscalation capabilities pod vs container level
💡 Result:
In Kubernetes, securityContext is defined at both pod level (spec.securityContext as PodSecurityContext) and container level (spec.containers[].securityContext as SecurityContext). Container-level settings override pod-level settings for overlapping fields. allowPrivilegeEscalation (boolean, controls no_new_privs flag to prevent child processes gaining more privileges than parent): - Container level only: spec.containers[].securityContext.allowPrivilegeEscalation - Pod level: Not available (PodSecurityContext lacks this field) - Defaults to true unless explicitly set to false. Always true if container is privileged or has CAP_SYS_ADMIN. - Examples in official docs show it only in container securityContext. capabilities (add/drop Linux capabilities): - Container level only: spec.containers[].securityContext.capabilities - Pod level: Not available (PodSecurityContext lacks capabilities field) - PodSecurityContext has pod-wide fields like runAsUser, runAsGroup, fsGroup, runAsNonRoot, seccompProfile, seLinuxOptions, but not allowPrivilegeEscalation or capabilities. PodSecurity Standards (Restricted profile) enforce must be false/empty at container level for these fields, confirming container-level application. Best practice: Set allowPrivilegeEscalation: false and drop ALL capabilities (add only necessary) at container level for security hardening.
Citations:
- 1: https://www.kubernetes.io/docs/concepts/security/pod-security-standards
- 2: https://www.k8s.io/docs/concepts/workloads/pods/advanced-pod-config/
- 3: https://kubernetes.io/docs/tasks/configure-pod-container/security-context/
- 4: https://www.k8s.io/docs/concepts/security/pod-security-standards/
- 5: https://kubernetes.io/docs/concepts/security/pod-security-standards/
- 6: https://kubernetes.io/docs/reference/kubernetes-api/workload-resources/pod-v1
- 7: https://kubernetes.io/docs/tasks/configure-pod-container/security-context
- 8: https://www.manifests.io/kubernetes/1.31/io.k8s.api.core.v1.SecurityContext?linked=Pod.spec.containers.securityContext
Move container-only security settings under the container spec.
allowPrivilegeEscalation and capabilities are container-level fields only in Kubernetes; the PodSecurityContext does not support them. Placing them at the pod level (lines 24-26) means they are silently ignored, and the fixture does not actually enforce the restrictive container security posture it appears to define.
Suggested fix
spec:
containers:
- args:
- sleep
- 4h
image: quay.io/openshifttest/busybox@sha256:c5439d7db88ab5423999530349d327b04279ad3161d7596d2126dfb5b02bfd1f
imagePullPolicy: Always
name: "${NAME}"
+ securityContext:
+ allowPrivilegeEscalation: false
+ capabilities:
+ drop: ["ALL"]
nodeSelector:
kubernetes.io/hostname: "${HOSTNAME}"
restartPolicy: Never
securityContext:
- allowPrivilegeEscalation: false
- capabilities:
- drop: ["ALL"]
runAsNonRoot: true
seccompProfile:
type: "RuntimeDefault"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/extended-priv/testdata/files/create-pod.yaml` around lines 23 - 29, The
Pod-level securityContext currently includes container-only fields
(allowPrivilegeEscalation and capabilities) which are ignored; move
allowPrivilegeEscalation and capabilities into the container's securityContext
(the container spec for the pod's container), leaving runAsNonRoot and
seccompProfile (pod-level or keep under pod's securityContext if intended)
appropriately placed—update the manifest so the container entry has a
securityContext block containing allowPrivilegeEscalation: false and
capabilities: { drop: ["ALL"] } while leaving runAsNonRoot and seccompProfile
where you want pod-level behavior.
Migrated 17 test cases from openshift-tests-private/test/extended/mco/ to machine-config-operator/test/extended-priv/: - mco_prune.go: 3 tests for prune renderedmachineconfigs functionality (73148, 73155, 74606) - mco_machineconfigpool.go: 10 tests for MCP operations (43048, 43064, 56131, 77354, 42390, 45318, 52373, 56123, 70125, 72007, 75149, 76108, 85073) - mco_drain.go: 4 tests for node drain behavior (43245, 51381, 49568, 49672) Added supporting helper functions: - MachineConfig: NewMachineConfigList, GetRenderedMachineConfigForMaster, GetRenderedMachineConfigForMasterOrFail, GetMCPRenderedMachineConfigsOrFail - MachineConfigPool: SetMaxUnavailable, RemoveMaxUnavailable, GetSortedUpdatedNodes, IsOCL, GetAllApplicableExtensionsToMCPOrFail - Controller: GetLogsAsList, GetFilteredLogsAsList - Node: FilterSchedulableNodesOrFail - Util: IsSNO, IsExecShellError, UnwrapExecCode, getTimeDifferenceInMinute, filterTimestampFromLogs, AddToAllMachineSets, checkUpdatedLists Added template files: - change-worker-ign-version.yaml - pod-disruption-budget.yaml - create-pod.yaml - add-mc-to-trigger-node-drain.yaml All tests build successfully and appear in test listing. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
ptalgulk01
force-pushed
the
migrate-prune-and-mcp-tests
branch
from
c1c47d6 to
f8ce746
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
test/extended-priv/mco_drain.go (1)
88-94:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard timestamp slice length before indexed access.
On Line 88–94,
timestamps[0..2]is accessed without asserting length. If log format changes, this panics instead of failing as an assertion.Proposed fix
timestamps := filterTimestampFromLogs(podLogs, 3) + o.Expect(timestamps).To(o.HaveLen(3), "Expected 3 timestamps in drain failure logs") logger.Infof("Timestamps %s", timestamps)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended-priv/mco_drain.go` around lines 88 - 94, The test accesses timestamps[0..2] without checking length; update the block after calling filterTimestampFromLogs to assert there are at least 3 entries (e.g., o.Expect(len(timestamps)).Should(o.BeNumerically(">=", 3))) before accessing timestamps[0], timestamps[1], timestamps[2]; keep the existing logger.Infof("Timestamps %s", timestamps) and then perform the existing getTimeDifferenceInMinute assertions only after that guard so the test fails with an assertion instead of panicking.test/extended-priv/mco_machineconfigpool.go (1)
99-102:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCheck
NotFoundfrom stderr, not stdout.On Line 99–102, the assertion inspects stdout (
Output()), butoc getfailures usually emitNotFoundon stderr.Proposed fix
- mcpOut, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("mcp/" + mcpName).Output() + _, mcpErrOut, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("mcp/" + mcpName).Outputs() o.Expect(err).Should(o.HaveOccurred()) - o.Expect(mcpOut).Should(o.ContainSubstring("NotFound")) + o.Expect(mcpErrOut).Should(o.ContainSubstring("NotFound"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended-priv/mco_machineconfigpool.go` around lines 99 - 102, The test is checking for "NotFound" on stdout (mcpOut) but oc get writes that message to stderr; replace the call to oc.AsAdmin().WithoutNamespace().Run("get").Args("mcp/"+mcpName).Output() with a call that captures stderr (for example CombinedOutput() or the library method that returns combined stdout+stderr) and assert the combined output (or stderr) contains "NotFound" instead of inspecting mcpOut; keep the existing expectation that an error occurred (err) and update the variable name if needed (e.g., combinedOut) when asserting the substring.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/extended-priv/mco_drain.go`:
- Around line 77-78: The code is currently ignoring errors returned by
mcc.GetFilteredLogsAsList when polling for drain failures; update the poll logic
around calls to GetFilteredLogsAsList (the calls that assign to variables named
logs and the second call near the later poll) to capture the returned error,
check it, and surface it (e.g., fail the test or log the error via
t.Fatalf/t.Errorf or the test logger) instead of discarding it; ensure you
reference mcc and workerNode.GetName() when forming the error message so
failures from GetFilteredLogsAsList are visible and cause the poll to stop with
the real error rather than a generic timeout.
- Around line 52-62: The test defers call pod.Delete(oc) twice which can cause
flaky teardown; remove the duplicate defer so the pod is deleted only once (keep
the first defer that immediately follows pod creation), and ensure the
MachineConfig cleanup still uses mc.DeleteWithWait() and mc.create() as-is;
locate the duplicate defer wrapped around pod.Delete(oc) near where mc is
created and delete that second defer line.
---
Duplicate comments:
In `@test/extended-priv/mco_drain.go`:
- Around line 88-94: The test accesses timestamps[0..2] without checking length;
update the block after calling filterTimestampFromLogs to assert there are at
least 3 entries (e.g., o.Expect(len(timestamps)).Should(o.BeNumerically(">=",
3))) before accessing timestamps[0], timestamps[1], timestamps[2]; keep the
existing logger.Infof("Timestamps %s", timestamps) and then perform the existing
getTimeDifferenceInMinute assertions only after that guard so the test fails
with an assertion instead of panicking.
In `@test/extended-priv/mco_machineconfigpool.go`:
- Around line 99-102: The test is checking for "NotFound" on stdout (mcpOut) but
oc get writes that message to stderr; replace the call to
oc.AsAdmin().WithoutNamespace().Run("get").Args("mcp/"+mcpName).Output() with a
call that captures stderr (for example CombinedOutput() or the library method
that returns combined stdout+stderr) and assert the combined output (or stderr)
contains "NotFound" instead of inspecting mcpOut; keep the existing expectation
that an error occurred (err) and update the variable name if needed (e.g.,
combinedOut) when asserting the substring.
🪄 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: efae0253-1a61-4846-b692-fe236a3879d2
📒 Files selected for processing (13)
test/extended-priv/const.gotest/extended-priv/controller.gotest/extended-priv/machineconfig.gotest/extended-priv/machineconfigpool.gotest/extended-priv/mco_drain.gotest/extended-priv/mco_machineconfigpool.gotest/extended-priv/mco_prune.gotest/extended-priv/node.gotest/extended-priv/testdata/files/add-mc-to-trigger-node-drain.yamltest/extended-priv/testdata/files/change-worker-ign-version.yamltest/extended-priv/testdata/files/create-pod.yamltest/extended-priv/testdata/files/pod-disruption-budget.yamltest/extended-priv/util.go
✅ Files skipped from review due to trivial changes (5)
- test/extended-priv/const.go
- test/extended-priv/testdata/files/change-worker-ign-version.yaml
- test/extended-priv/testdata/files/pod-disruption-budget.yaml
- test/extended-priv/testdata/files/add-mc-to-trigger-node-drain.yaml
- test/extended-priv/testdata/files/create-pod.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- test/extended-priv/machineconfig.go
- test/extended-priv/mco_prune.go
- test/extended-priv/util.go
- test/extended-priv/controller.go
Comment on lines
+52
to
+62
| defer func() { o.Expect(pod.Delete(oc)).NotTo(o.HaveOccurred()) }() | ||
| pod.Create(oc) | ||
|
|
||
| exutil.By("Create new mc to add new file on the node and trigger node drain") | ||
| mcName := "test-file" | ||
| mcTemplate := "add-mc-to-trigger-node-drain.yaml" | ||
| mc := NewMachineConfig(oc.AsAdmin(), mcName, MachineConfigPoolWorker).SetMCOTemplate(mcTemplate) | ||
| mc.skipWaitForMcp = true | ||
| defer mc.DeleteWithWait() | ||
| defer func() { o.Expect(pod.Delete(oc)).NotTo(o.HaveOccurred()) }() | ||
| mc.create() |
There was a problem hiding this comment.
Avoid double-deleting the same pod in defers.
Line 52 and Line 61 both defer pod.Delete(oc) with Expect(...).NotTo(HaveOccurred()). The second cleanup can fail after the first succeeds, producing teardown flakes.
Proposed fix
- defer func() { o.Expect(pod.Delete(oc)).NotTo(o.HaveOccurred()) }()
mc.create()📝 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.
Suggested change
| defer func() { o.Expect(pod.Delete(oc)).NotTo(o.HaveOccurred()) }() | |
| pod.Create(oc) | |
| exutil.By("Create new mc to add new file on the node and trigger node drain") | |
| mcName := "test-file" | |
| mcTemplate := "add-mc-to-trigger-node-drain.yaml" | |
| mc := NewMachineConfig(oc.AsAdmin(), mcName, MachineConfigPoolWorker).SetMCOTemplate(mcTemplate) | |
| mc.skipWaitForMcp = true | |
| defer mc.DeleteWithWait() | |
| defer func() { o.Expect(pod.Delete(oc)).NotTo(o.HaveOccurred()) }() | |
| mc.create() | |
| defer func() { o.Expect(pod.Delete(oc)).NotTo(o.HaveOccurred()) }() | |
| pod.Create(oc) | |
| exutil.By("Create new mc to add new file on the node and trigger node drain") | |
| mcName := "test-file" | |
| mcTemplate := "add-mc-to-trigger-node-drain.yaml" | |
| mc := NewMachineConfig(oc.AsAdmin(), mcName, MachineConfigPoolWorker).SetMCOTemplate(mcTemplate) | |
| mc.skipWaitForMcp = true | |
| defer mc.DeleteWithWait() | |
| mc.create() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/extended-priv/mco_drain.go` around lines 52 - 62, The test defers call
pod.Delete(oc) twice which can cause flaky teardown; remove the duplicate defer
so the pod is deleted only once (keep the first defer that immediately follows
pod creation), and ensure the MachineConfig cleanup still uses
mc.DeleteWithWait() and mc.create() as-is; locate the duplicate defer wrapped
around pod.Delete(oc) near where mc is created and delete that second defer
line.
Comment on lines
+77
to
+78
| logs, _ := mcc.GetFilteredLogsAsList(workerNode.GetName() + ".*Drain failed") | ||
| if len(logs) > 2 { |
There was a problem hiding this comment.
Don’t discard controller log read errors during polling.
On Line 77 and Line 98, errors from GetFilteredLogsAsList are ignored. That masks root causes and turns actionable failures into generic poll timeouts.
Proposed fix
- logs, _ := mcc.GetFilteredLogsAsList(workerNode.GetName() + ".*Drain failed")
+ logs, err := mcc.GetFilteredLogsAsList(workerNode.GetName() + ".*Drain failed")
+ if err != nil {
+ logger.Infof("Error getting filtered controller logs: %v", err)
+ return false, nil
+ }
...
- logs, _ := mcc.GetFilteredLogsAsList(workerNode.GetName() + ".*Drain has been failing for more than 10 minutes. Waiting 5 minutes")
+ logs, err := mcc.GetFilteredLogsAsList(workerNode.GetName() + ".*Drain has been failing for more than 10 minutes. Waiting 5 minutes")
+ if err != nil {
+ logger.Infof("Error getting filtered controller logs: %v", err)
+ return false, nil
+ }Also applies to: 98-99
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/extended-priv/mco_drain.go` around lines 77 - 78, The code is currently
ignoring errors returned by mcc.GetFilteredLogsAsList when polling for drain
failures; update the poll logic around calls to GetFilteredLogsAsList (the calls
that assign to variables named logs and the second call near the later poll) to
capture the returned error, check it, and surface it (e.g., fail the test or log
the error via t.Fatalf/t.Errorf or the test logger) instead of discarding it;
ensure you reference mcc and workerNode.GetName() when forming the error message
so failures from GetFilteredLogsAsList are visible and cause the poll to stop
with the real error rather than a generic timeout.
Contributor
Author
Contributor
|
@ptalgulk01: 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. |
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.
Migrated 17 test cases from openshift-tests-private/test/extended/mco/ to machine-config-operator/test/extended-priv/:
Added supporting helper functions:
Added template files:
All tests build successfully and appear in test listing.
- What I did
- How to verify it
- Description for the changelog
Summary by CodeRabbit
Tests
Chores