Irreconcilable configs test suite#6085
Conversation
- MCO-1745: Scale up mixed scenario - MCO-1755: Feature disabling reporting consistency - MCO-1899: Scale up and revert scenario
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
WalkthroughThis PR adds a comprehensive test suite for irreconcilable validation overrides in machine configuration, including helper utilities, YAML test fixtures, four end-to-end test cases validating feature behavior across platform configurations and node lifecycle transitions, and cleanup of existing storage tests. ChangesIrreconcilable Validation Overrides Testing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 8 | ❌ 4❌ Failed checks (4 warnings)
✅ Passed checks (8 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: proietfb 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: 4
🤖 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-priv/irreconcilableChanges.go`:
- Around line 49-54: Add an explicit MicroShift skip guard to the suite setup:
inside the g.JustBeforeEach block (alongside PreChecks, skipIfNoTechPreview,
skipTestIfSupportedPlatformNotMatched, skipTestIfSNOCluster) call the existing
helper that skips MicroShift clusters (e.g., skipIfMicroShift(oc) or the
project’s equivalent), or if none exists add a short guard that detects
MicroShift and calls GinkgoT().Skip/By to skip the test; ensure the new call
runs before tests that rely on MachineSet scaling.
- Around line 339-344: The test is using AWS-only NVMe paths in mc2.Create which
will fail on GCE; replace the hardcoded "/dev/nvme3n1" and "/dev/nvme4n1"
parameters with platform-aware device paths by computing device1 and device2
based on the test environment (e.g., via an existing helper or function that
returns device paths for the current provider) and pass those variables into
mc2.Create along with newMcName and mcpool; update references around mc2.Create
to use the computed device variables so the test runs on both AWS and GCE.
- Around line 351-354: Remove the duplicate deletion by deleting the deferred
call and keeping the explicit deletion/wait: remove the defer
mc2.DeleteWithWait() line and leave the immediate mc2.DeleteWithWait() followed
by mcp.WaitForNotDegradedStatus(); this ensures the resource is deleted once
while preserving the existing wait logic (refer to mc2.DeleteWithWait() and
mcp.WaitForNotDegradedStatus()).
In `@test/extended-priv/util.go`:
- Around line 662-663: The call to IsSNOSafe(oc) currently discards the error
(sno, _ := IsSNOSafe(oc)) which can cause SNO checks to be bypassed; change the
call to capture the error, check it, and handle it appropriately (e.g., fail the
test, return the error, or skip the disruptive test) instead of proceeding when
detection fails — locate the IsSNOSafe call and the subsequent if sno { block in
test/extended-priv/util.go and replace the blank error discard with error
handling that logs or fails the test using the existing test framework/context
(use the same oc and surrounding helper functions to report the failure or
skip).
🪄 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: b0f1fe58-1928-43aa-9512-3f0acc39684f
📒 Files selected for processing (5)
test/extended-priv/irreconcilableChanges.gotest/extended-priv/machineconfiguration.gotest/extended-priv/mco_storage.gotest/extended-priv/testdata/files/extra-disks-with-files.yamltest/extended-priv/util.go
| g.JustBeforeEach(func() { | ||
| PreChecks(oc) | ||
| skipIfNoTechPreview(oc) | ||
| skipTestIfSupportedPlatformNotMatched(oc, AWSPlatform, GCPPlatform) | ||
| skipTestIfSNOCluster(oc) | ||
| }) |
There was a problem hiding this comment.
Add an explicit MicroShift skip guard in suite setup.
These new Ginkgo tests depend on MachineSet scaling and other features that are not available on MicroShift, but the suite setup only guards SNO/platform/feature gate.
Suggested guard
g.JustBeforeEach(func() {
PreChecks(oc)
skipIfNoTechPreview(oc)
skipTestIfSupportedPlatformNotMatched(oc, AWSPlatform, GCPPlatform)
+ if exutil.IsMicroShiftCluster(oc) {
+ g.Skip("skip test because current cluster is MicroShift")
+ }
skipTestIfSNOCluster(oc)
})📝 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.
| g.JustBeforeEach(func() { | |
| PreChecks(oc) | |
| skipIfNoTechPreview(oc) | |
| skipTestIfSupportedPlatformNotMatched(oc, AWSPlatform, GCPPlatform) | |
| skipTestIfSNOCluster(oc) | |
| }) | |
| g.JustBeforeEach(func() { | |
| PreChecks(oc) | |
| skipIfNoTechPreview(oc) | |
| skipTestIfSupportedPlatformNotMatched(oc, AWSPlatform, GCPPlatform) | |
| if exutil.IsMicroShiftCluster(oc) { | |
| g.Skip("skip test because current cluster is MicroShift") | |
| } | |
| skipTestIfSNOCluster(oc) | |
| }) |
🤖 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-priv/irreconcilableChanges.go` around lines 49 - 54, Add an
explicit MicroShift skip guard to the suite setup: inside the g.JustBeforeEach
block (alongside PreChecks, skipIfNoTechPreview,
skipTestIfSupportedPlatformNotMatched, skipTestIfSNOCluster) call the existing
helper that skips MicroShift clusters (e.g., skipIfMicroShift(oc) or the
project’s equivalent), or if none exists add a short guard that detects
MicroShift and calls GinkgoT().Skip/By to skip the test; ensure the new call
runs before tests that rely on MachineSet scaling.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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-priv/irreconcilableChanges.go`:
- Around line 34-40: The GCP case in platformBasedDisksNames returns root
partition paths (/dev/sda1–sda4) which can target the boot disk; change the GCP
mapping in the GCPPlatform branch (case GCPPlatform) to use non-root GCE device
names (e.g. /dev/sdb, /dev/sdc, /dev/sdd, /dev/sde) so tests reference
additional disks rather than the boot partitions.
In `@test/extended-priv/util.go`:
- Around line 662-664: The current check treats any IsSNOSafe error as a SNO and
silently skips tests; instead, call out the error when IsSNOSafe(oc) returns an
error and only skip when sno is true. Replace the combined condition using sno
and err so that after calling IsSNOSafe(oc) you first handle err (e.g., fail the
test or g.Fatalf/log the error with the error value from err) and only call
g.Skip("skip test because current cluster is a SNO cluster") when sno == true;
references: IsSNOSafe, sno, err, and g.Skip.
🪄 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: 314db668-ecaa-4a77-b3bf-4d64778c39aa
📒 Files selected for processing (2)
test/extended-priv/irreconcilableChanges.gotest/extended-priv/util.go
| case GCPPlatform: | ||
| return []string{ | ||
| "/dev/sda1", | ||
| "/dev/sda2", | ||
| "/dev/sda3", | ||
| "/dev/sda4", | ||
| } |
There was a problem hiding this comment.
Use non-root GCP device names in platformBasedDisksNames.
At Line 36-39, the GCP mapping points to /dev/sda* partitions, but these tests add extra non-boot disks. This can target the wrong devices and make GCE runs fail or behave unpredictably (downstream at Line 90 and Line 339-340).
Suggested fix
case GCPPlatform:
return []string{
- "/dev/sda1",
- "/dev/sda2",
- "/dev/sda3",
- "/dev/sda4",
+ "/dev/sdb",
+ "/dev/sdc",
+ "/dev/sdd",
+ "/dev/sde",
}📝 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.
| case GCPPlatform: | |
| return []string{ | |
| "/dev/sda1", | |
| "/dev/sda2", | |
| "/dev/sda3", | |
| "/dev/sda4", | |
| } | |
| case GCPPlatform: | |
| return []string{ | |
| "/dev/sdb", | |
| "/dev/sdc", | |
| "/dev/sdd", | |
| "/dev/sde", | |
| } |
🤖 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-priv/irreconcilableChanges.go` around lines 34 - 40, The GCP
case in platformBasedDisksNames returns root partition paths (/dev/sda1–sda4)
which can target the boot disk; change the GCP mapping in the GCPPlatform branch
(case GCPPlatform) to use non-root GCE device names (e.g. /dev/sdb, /dev/sdc,
/dev/sdd, /dev/sde) so tests reference additional disks rather than the boot
partitions.
| sno, err := IsSNOSafe(oc) | ||
| if sno || err != nil { | ||
| g.Skip("skip test because current cluster is a SNO cluster") |
There was a problem hiding this comment.
Don’t convert SNO-detection failures into silent skips.
At Line 663-664, err != nil is treated as “SNO” and skipped with a misleading reason. This can hide real failures and suppress test coverage.
Suggested fix
func skipTestIfSNOCluster(oc *exutil.CLI) {
sno, err := IsSNOSafe(oc)
- if sno || err != nil {
+ o.ExpectWithOffset(1, err).NotTo(o.HaveOccurred(), "Could not determine whether cluster is SNO")
+ if sno {
g.Skip("skip test because current cluster is a SNO cluster")
}
}📝 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.
| sno, err := IsSNOSafe(oc) | |
| if sno || err != nil { | |
| g.Skip("skip test because current cluster is a SNO cluster") | |
| func skipTestIfSNOCluster(oc *exutil.CLI) { | |
| sno, err := IsSNOSafe(oc) | |
| o.ExpectWithOffset(1, err).NotTo(o.HaveOccurred(), "Could not determine whether cluster is SNO") | |
| if sno { | |
| g.Skip("skip test because current cluster is a SNO cluster") | |
| } | |
| } |
🤖 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-priv/util.go` around lines 662 - 664, The current check treats
any IsSNOSafe error as a SNO and silently skips tests; instead, call out the
error when IsSNOSafe(oc) returns an error and only skip when sno is true.
Replace the combined condition using sno and err so that after calling
IsSNOSafe(oc) you first handle err (e.g., fail the test or g.Fatalf/log the
error with the error value from err) and only call g.Skip("skip test because
current cluster is a SNO cluster") when sno == true; references: IsSNOSafe, sno,
err, and g.Skip.
|
@proietfb: 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. |
1 participant
[MCO-1990] Test plan for irreconcilable changes feature
Implemented:
Summary by CodeRabbit