Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CORS-3741: Nutanix enhancement: allow multiple NICs #1711

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

yanhua121
Copy link

@yanhua121 yanhua121 commented Nov 5, 2024

CORS-3741:
Nutanix machine-api enhancements: Multi-NICs for OCP Cluster Nodes

The enhancement PR for this:
#1711

@openshift-ci-robot
Copy link

openshift-ci-robot commented Nov 5, 2024

@yanhua121: This pull request references CORS-3741 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 "4.18.0" version, but no target version was set.

In response to this:

CORS-3741:
Nutanix machine-api enhancements: Multi-NICs for OCP Cluster Nodes

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.

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Nov 5, 2024
@openshift-ci-robot
Copy link

openshift-ci-robot commented Nov 5, 2024

@yanhua121: This pull request references CORS-3741 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 "4.18.0" version, but no target version was set.

In response to this:

CORS-3741:
Nutanix machine-api enhancements: Multi-NICs for OCP Cluster Nodes

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.

@yanhua121
Copy link
Author

/assign @JoelSpeed

Copy link
Contributor

openshift-ci bot commented Nov 6, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from joelspeed. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Comment on lines +3 to +6
authors:
reviewers:
approvers:
api-approvers:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These need to be filled out, I can be assigned as review and api-approver. Probably this needs some installer input so @patrickdillon probably wants to be the approver here, unless he wants to delegate to another

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I can be a reviewer. Also @rvanderp3 or someone from splat

Copy link
Contributor

@patrickdillon patrickdillon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As presented, this looks pretty straight forward. The API fields already exist in the install config and also machine API. So it is mostly a matter of relaxing validation.

Are NICs handled transparently? i.e. all that needs to happen is more subnets are specified in the API and then NICs are created automagically for those subnets?

There is no reference to CAPI support. The CAPI manifests used by the installer to create the control plane would likely need to be updated as well.

Comment on lines +3 to +6
authors:
reviewers:
approvers:
api-approvers:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I can be a reviewer. Also @rvanderp3 or someone from splat

@openshift-ci-robot
Copy link

openshift-ci-robot commented Nov 7, 2024

@yanhua121: This pull request references CORS-3741 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 "4.18.0" version, but no target version was set.

In response to this:

CORS-3741:
Nutanix machine-api enhancements: Multi-NICs for OCP Cluster Nodes

The enhancement PR for this:
#1711

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.


### API Extensions

Currently, the “subnets” fields in both Machine/MachineSet’s Nutanix providerSpec and Nutanix FailureDomain are already array type. The only change for the api is to relax the validation rule for the “subnets” fields to allow multiple values and to ensure no duplication values are configured.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we now use CAPI during the installation process, do we also need to make changes to the CAPI nutanix provider?

Copy link
Author

@yanhua121 yanhua121 Nov 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The nutanix-capi already supports multi-subnets. And in the OCP installer code currently, the mutli-subnets as array type already handled properly when generating the nutanix-capi manifests and also the MAPI manifests. This time, the installer change is only to relax the validation (previously limit to only one subnet). See the PR openshift/installer#9133. My manual integration test with multi-subnets passed.

@JoelSpeed @patrickdillon

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there anything we need to be careful with software side when adding multiple subnets, any networking configuration?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each subnet used for the OCP cluster nodes should already exist in the Prism Element (PE) cluster of the PC. This is already documented in OCP documentation. Same requirement as before: before is limited to just one existing subnet and now can configure more than one existing subnets per PE (FailureDomain).

@yanhua121 yanhua121 force-pushed the CORS-3741 branch 2 times, most recently from 188e565 to 1c57780 Compare November 8, 2024 17:47
Copy link
Contributor

openshift-ci bot commented Nov 8, 2024

@yanhua121: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/markdownlint 780d14e link true /test markdownlint

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@JoelSpeed
Copy link
Contributor

Apart from my outstanding question about software and the markdownlint being sad, I'm ok with what's written here, thanks @huali9

@yanhua121
Copy link
Author

/assign @rvanderp3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
jira/valid-reference Indicates that this PR references a valid Jira ticket of any type.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants