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.

Sign up for GitHub

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

Jump to bottom

SPLAT-1272: Support Nutanix Failure Domains #7730

Merged
merged 2 commits into from
Nov 28, 2023
Merged

SPLAT-1272: Support Nutanix Failure Domains #7730

merged 2 commits into from
Nov 28, 2023

Conversation

yanhua121
Copy link
Contributor

Nutanix Failure Domains support

Depends on openshift/api#1578

@rvanderp3
Copy link
Contributor

/hold

openshift/api#1578 must merge and this must be revendored

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 15, 2023
@jcpowermac
Copy link
Contributor

I don't see any changes the platform spec validation

https://github.com/openshift/installer/blob/master/pkg/types/nutanix/validation/platform.go

@yanhua121 yanhua121 force-pushed the nutanix-failuredomains branch 2 times, most recently from 0c51b58 to 06963b0 Compare November 16, 2023 05:25
@yanhua121
Copy link
Contributor Author

I don't see any changes the platform spec validation
https://github.com/openshift/installer/blob/master/pkg/types/nutanix/validation/platform.go

@jcpowermac Added some validation for the failure domains configuration in this file.

@yanhua121
Copy link
Contributor Author

/retest-required

@jcpowermac
Copy link
Contributor

@yanhua121 you might want to start taking a look at golint since we are waiting on the api changes. There is a few items in there to fix.

@yanhua121 yanhua121 changed the title Support Nutanix Failure Domains SPLAT-1272: Support Nutanix Failure Domains Nov 21, 2023
@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 21, 2023
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Nov 21, 2023 •
edited by openshift-ci bot
Loading

@yanhua121: This pull request references SPLAT-1272 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 spike to target the "4.15.0" version, but no target version was set.

In response to this:

Nutanix Failure Domains support

Depends on openshift/api#1578

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/test-infra repository.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 22, 2023
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 22, 2023
@yanhua121
Copy link
Contributor Author

@jcpowermac The api dependency versioning is updated, and all your review comments are addressed. The go-lint test also passed. Could you take another look, thanks.

@yanhua121
Copy link
Contributor Author

/retest-required

@jcpowermac
Copy link
Contributor

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 22, 2023
@jcpowermac
Copy link
Contributor

/test e2e-nutanix-ovn

@jcpowermac
Copy link
Contributor

/assign @jhixson74

@jcpowermac
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 27, 2023
@r4f4
Copy link
Contributor

r4f4 commented Nov 27, 2023

Please separate vendor changes into their own commit.

@yanhua121
Copy link
Contributor Author

Please separate vendor changes into their own commit.

@r4f4 Should the go.mod and go.sum changes be included in the vendor commit?

@r4f4
Copy link
Contributor

r4f4 commented Nov 27, 2023

Please separate vendor changes into their own commit.

@r4f4 Should the go.mod and go.sum changes be included in the vendor commit?

There is no clear consensus on that. I know that @patrickdillon prefers separate commits (go.{mod,sum} in one commit and vendor/ in another).

@yanhua121
Copy link
Contributor Author

@r4f4 All your comments are addressed. Could you take another look?

@jcpowermac
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 27, 2023
@rvanderp3
Copy link
Contributor

/assign @patrickdillon

r4f4
r4f4 reviewed Nov 27, 2023
View reviewed changes
pkg/asset/machines/nutanix/machines.go Outdated Show resolved Hide resolved
pkg/asset/manifests/nutanix/infrastructure.go Outdated Show resolved Hide resolved
pkg/asset/manifests/nutanix/infrastructure.go Outdated Show resolved Hide resolved
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 27, 2023
@patrickdillon
Copy link
Contributor

There is no clear consensus on that. I know that @patrickdillon prefers separate commits (go.{mod,sum} in one commit and vendor/ in another).

Either way is fine. After learning about separating go.mod & go.sum into separate commits from the installer docs I have found it beneficial in my own workflow.

r4f4
r4f4 reviewed Nov 27, 2023
View reviewed changes
Copy link
Contributor

@r4f4 r4f4 left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 27, 2023
@yanhua121
Copy link
Contributor Author

/retest-required

1 similar comment
@jcpowermac
Copy link
Contributor

/retest-required

@patrickdillon
Copy link
Contributor

/approve

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Nov 27, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: patrickdillon

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

The pull request process is described 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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 27, 2023
@yanhua121
Copy link
Contributor Author

/retest-required

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Nov 28, 2023

@yanhua121: The following tests 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/e2e-aws-ovn-shared-vpc-localzones e72a6b5 link false /test e2e-aws-ovn-shared-vpc-localzones
ci/prow/e2e-azurestack e72a6b5 link false /test e2e-azurestack
ci/prow/e2e-aws-ovn-fips e72a6b5 link false /test e2e-aws-ovn-fips
ci/prow/e2e-openstack-sdn-parallel e72a6b5 link false /test e2e-openstack-sdn-parallel
ci/prow/e2e-azure-ovn-shared-vpc e72a6b5 link false /test e2e-azure-ovn-shared-vpc
ci/prow/e2e-gcp-ovn-xpn e72a6b5 link false /test e2e-gcp-ovn-xpn
ci/prow/e2e-aws-ovn-single-node e72a6b5 link false /test e2e-aws-ovn-single-node
ci/prow/openstack-manifests e72a6b5 link true /test openstack-manifests
ci/prow/e2e-gcp-secureboot e72a6b5 link false /test e2e-gcp-secureboot
ci/prow/e2e-openstack-proxy e72a6b5 link false /test e2e-openstack-proxy
ci/prow/e2e-openstack-ovn e72a6b5 link true /test e2e-openstack-ovn
ci/prow/e2e-gcp-ovn-shared-vpc e72a6b5 link false /test e2e-gcp-ovn-shared-vpc
ci/prow/e2e-aws-custom-security-groups e72a6b5 link false /test e2e-aws-custom-security-groups
ci/prow/e2e-aws-ovn-localzones e72a6b5 link false /test e2e-aws-ovn-localzones
ci/prow/e2e-aws-ovn-imdsv2 e72a6b5 link false /test e2e-aws-ovn-imdsv2
ci/prow/e2e-aws-ovn-shared-vpc e72a6b5 link false /test e2e-aws-ovn-shared-vpc
ci/prow/okd-e2e-aws-ovn-upgrade 5f01650 link false /test okd-e2e-aws-ovn-upgrade

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

@openshift-merge-bot openshift-merge-bot bot merged commit 8032847 into openshift:master Nov 28, 2023
19 of 24 checks passed
@openshift-bot
Copy link
Contributor

[ART PR BUILD NOTIFIER]

This PR has been included in build ose-installer-altinfra-container-v4.15.0-202311281631.p0.g8032847.assembly.stream for distgit ose-installer-altinfra.
All builds following this will include this PR.

@yanhua121 yanhua121 mentioned this pull request Dec 6, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcpowermac jcpowermac jcpowermac left review comments

@r4f4 r4f4 r4f4 left review comments

@andfasano andfasano Awaiting requested review from andfasano

@jhixson74 jhixson74 Awaiting requested review from jhixson74

Assignees

@r4f4 r4f4

@patrickdillon patrickdillon

@jcpowermac jcpowermac

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@yanhua121 @rvanderp3 @jcpowermac @openshift-ci-robot @r4f4 @patrickdillon @openshift-bot @openshift-merge-robot