MGMT-21836: Allow external platform with non-standard HA control plane#10088
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@yoavsc0302: This pull request references MGMT-21836 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 task to target the "4.22.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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (12)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (6)
WalkthroughAdds a new host validation Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
bot
added
the
size/L
openshift-ci
bot
added
the
api-review
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: yoavsc0302 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 |
openshift-ci
bot
added
the
approved
|
@yoavsc0302: This pull request references MGMT-21836 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 task to target the "4.22.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. |
|
@yoavsc0302: This pull request references MGMT-21836 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 task to target the "4.22.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. |
|
@yoavsc0302: This pull request references MGMT-21836 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 task to target the "4.22.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. |
![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 the current code and only fix it if needed.
Inline comments:
In `@internal/host/validations_test.go`:
- Around line 4204-4295: The tests only cover master behavior for
ControlPlaneCount == 4, missing the 5-CP master paths; add two tests mirroring
the 4-CP cases but with c.ControlPlaneCount = 5: one "bare metal master on
external platform with 5CP should pass" that sets h.Inventory =
generateInventoryWithVirtual(false), calls mockAndRefreshStatus(&h), then
asserts getValidationResult(h.ValidationsInfo, NonStandardHARequiresBareMetal)
returns found true and ValidationSuccess; and one "virtual master on external
platform with 5CP should fail" that sets h.Inventory =
generateInventoryWithVirtual(true), calls mockAndRefreshStatus(&h), then asserts
the validation is found, status is ValidationFailure and the message contains
"must be bare metal"; reuse existing helpers (hostutil.GenerateTestCluster,
hostutil.GenerateTestHostByKind, mockProviderRegistry.EXPECT(),
mockAndRefreshStatus, getValidationResult) to keep consistency.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3647510e-c98c-42e1-bed9-22820bd2c017
⛔ Files ignored due to path filters (3)
api/vendor/github.com/openshift/assisted-service/models/host_validation_id.gois excluded by!**/vendor/**client/vendor/github.com/openshift/assisted-service/models/host_validation_id.gois excluded by!**/vendor/**vendor/github.com/openshift/assisted-service/models/host_validation_id.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (12)
internal/featuresupport/feature_support_test.gointernal/featuresupport/features_misc.gointernal/featuresupport/features_platforms.gointernal/host/refresh_status_preprocessor.gointernal/host/statemachine.gointernal/host/transition_test.gointernal/host/validation_id.gointernal/host/validations_test.gointernal/host/validator.gomodels/host_validation_id.gorestapi/embedded_spec.goswagger.yaml
💤 Files with no reviewable changes (1)
- internal/featuresupport/features_platforms.go
|
@yoavsc0302: This pull request references MGMT-21836 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 task to target the "4.22.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. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #10088 +/- ##
=======================================
Coverage 44.17% 44.18%
=======================================
Files 416 416
Lines 72404 72422 +18
=======================================
+ Hits 31987 31999 +12
- Misses 37521 37525 +4
- Partials 2896 2898 +2
🚀 New features to boost your workflow:
|
Allow external platform (including OCI) to be used with 4/5 control plane nodes by adding it to the allowed platforms for the NonStandardHAControlPlane feature and removing it from the incompatible features list. Add a new host validation (non-standard-ha-requires-bare-metal) that checks control plane hosts are bare metal when using external platform with 4 or 5 control plane nodes. Virtual machine hosts are blocked from reaching known state.
yoavsc0302
force-pushed
the
MGMT-21836/Installation-with-4-5CP-should-be-allowed-with-external-platform-approach-2
branch
from
4b24a1d to
8110356
Compare
|
@yoavsc0302: This pull request references MGMT-21836 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 task to target the "4.22.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. |
|
/test ? |
|
/test edge-e2e-metal-assisted-5-control-planes-4-22 |
|
/test edge-e2e-oci-assisted-bm-iscsi-4-22 |
|
/test edge-e2e-oci-assisted-4-22 |
1 similar comment
|
/test edge-e2e-oci-assisted-4-22 |
|
@yoavsc0302: The following tests 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. |
|
/test ? |
1 similar comment
|
/test ? |
|
/test edge-e2e-oci-assisted-bm-4cp-4-22 |
2 participants
Allow external platform (including OCI) to be used with 4/5 control plane nodes by adding it to the allowed platforms for the NonStandardHAControlPlane feature and removing it from the incompatible features list.
Add a new host validation (non-standard-ha-requires-bare-metal) that checks control plane hosts are bare metal when using external platform with 4 or 5 control plane nodes. Virtual machine hosts are blocked from reaching known state.
What was done
1. Removed feature-level incompatibility between external platform and non-standard HA:
internal/featuresupport/features_misc.go— added PlatformTypeExternal to allowed platformsinternal/featuresupport/features_platforms.go— removed NonStandardHAControlPlane from OCI and External incompatible listsinternal/featuresupport/feature_support_test.go2. Added host-level bare metal validation for non-standard HA on external platform:
Added validation ID to swagger:
swagger.yaml— addednon-standard-ha-requires-bare-metalenum valueGenerated code (
skipper make generate):models/host_validation_id.go+ vendor copies +restapi/embedded_spec.goWired up the validation:
internal/host/validation_id.go— constant + "hardware" categoryinternal/host/validator.go— validation function (scoped to external platform + 4/5CP + master role)internal/host/refresh_status_preprocessor.go— registered the validationinternal/host/statemachine.go— added to hasMinRequiredHardwareTests:
internal/host/transition_test.gointernal/host/validations_test.goList all the issues related to this PR
What environments does this code impact?
How was this code tested?
Checklist
docs, README, etc)Reviewers Checklist