MCO-2482: Revert "MCO-2470: Disable scale-up test support for AWS and vSphere" - #6356
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@proietfb: This pull request references MCO-2470 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. |
WalkthroughThe extended scale-up test now supports AWS. It discovers non-partition NVMe by-path devices from a temporary probe node, uses those paths in the MachineConfig, and selects a separate AWS test node. ChangesAWS NVMe scale-up testing
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ScaleUpTest
participant ProbeNode
participant discoverNVMeByPathDisks
participant MachineConfig
participant TestNode
ScaleUpTest->>ProbeNode: Provision temporary probe node
ScaleUpTest->>discoverNVMeByPathDisks: Discover NVMe by-path devices
discoverNVMeByPathDisks->>ProbeNode: Run node-local shell command
ProbeNode-->>discoverNVMeByPathDisks: Return sorted device paths
discoverNVMeByPathDisks-->>ScaleUpTest: Return two paths
ScaleUpTest->>MachineConfig: Create configuration with discovered paths
ScaleUpTest->>TestNode: Scale AWS nodes to two
ScaleUpTest->>TestNode: Select node distinct from probe node
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (12 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. Comment |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/extended-priv/mco_irreconcilablechanges.go`:
- Around line 80-96: The discoverNVMeByPathDisks function selects all
non-partition NVMe devices from the glob pattern, but AWS instance-store volumes
also match this pattern, causing the HaveLen(2) assertion to fail on instances
with local NVMe storage. Update the logic after collecting the by-path entries
to filter and map each candidate to the expected block-device mappings /dev/sdb
or /dev/sdc. Only return the two MachineSet EBS data disks by validating each
discovered path corresponds to one of these two expected block devices rather
than returning all matched NVMe devices.
🪄 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: Pro Plus
Run ID: 07048f52-9c5d-4f2e-a0cc-d6ac1ed7ff25
📒 Files selected for processing (1)
test/extended-priv/mco_irreconcilablechanges.go
| // discoverNVMeByPathDisks discovers the /dev/disk/by-path/ entries for non-boot NVMe | ||
| // devices on an AWS node. These PCI-based paths are stable per instance type, unlike | ||
| // /dev/nvmeXn1 which depends on device enumeration order. | ||
| func discoverNVMeByPathDisks(node *Node) []string { | ||
| script := `for p in /dev/disk/by-path/*nvme*; do [[ $p == *part* ]] && continue; ls ${p}-part* &>/dev/null && continue; echo "$p"; done | sort` | ||
| stdout, _, err := node.DebugNodeWithChrootStd("bash", "-c", script) | ||
| o.ExpectWithOffset(1, err).NotTo(o.HaveOccurred(), "Failed to discover NVMe by-path disks on node %s", node.GetName()) | ||
|
|
||
| var paths []string | ||
| for _, line := range strings.Split(strings.TrimSpace(stdout), "\n") { | ||
| line = strings.TrimSpace(line) | ||
| if line != "" { | ||
| paths = append(paths, line) | ||
| } | ||
| } | ||
| logger.Infof("Discovered %d non-boot NVMe by-path disks on node %s: %v", len(paths), node.GetName(), paths) | ||
| return paths |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Select only the two MachineSet EBS data disks.
The command selects every non-partition NVMe device. AWS can automatically enumerate NVMe instance-store volumes, and those devices also match this glob. The HaveLen(2) assertion then fails on supported instance types with local NVMe storage. (docs.aws.amazon.com)
Map each candidate to the expected /dev/sdb or /dev/sdc block-device mapping before returning its by-path entry. Do not use the total NVMe-device count as disk selection.
🤖 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/mco_irreconcilablechanges.go` around lines 80 - 96, The
discoverNVMeByPathDisks function selects all non-partition NVMe devices from the
glob pattern, but AWS instance-store volumes also match this pattern, causing
the HaveLen(2) assertion to fail on instances with local NVMe storage. Update
the logic after collecting the by-path entries to filter and map each candidate
to the expected block-device mappings /dev/sdb or /dev/sdc. Only return the two
MachineSet EBS data disks by validating each discovered path corresponds to one
of these two expected block devices rather than returning all matched NVMe
devices.
|
@proietfb: 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. |
|
/payload-job periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-metal-ipi-ovn-dualstack-mco-disruptive-techpreview-1of3 periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-vsphere-mco-disruptive-techpreview periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-azure-mco-disruptive-techpreview-1of3 periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-gcp-mco-disruptive-techpreview-1of3 periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-disruptive-techpreview-1of3 |
|
@proietfb: trigger 5 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/50db5c00-8cf7-11f1-97fe-1a7c1596ddc9-0 |
|
/test required |
|
@proietfb: The specified target(s) for The following commands are available to trigger optional jobs: Use 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. |
|
/pipeline required |
|
Pipeline controller notification No second-stage tests were triggered for this PR. This can happen when:
Use |
yuqi-zhang
changed the title
|
@proietfb: This pull request references MCO-2482 which is a valid jira issue. 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. |
|
/lgtm |
|
Pipeline controller notification No second-stage tests were triggered for this PR. This can happen when:
Use |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: proietfb, yuqi-zhang The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
3 participants
Reverts #6344
Summary by CodeRabbit