SPLAT-1211: recognize topology and failure domains represented by the infrastructure resource by rvanderp3 · Pull Request #250 · openshift/cluster-control-plane-machine-set-operator

rvanderp3 · 2023-10-13T21:38:22Z

This PR introduces the infrastructure resource to aid in the resolution of topology and failure domains for platforms such as vSphere.

rvanderp3 · 2023-10-13T21:58:25Z

I would expect this lint error since there isn't a provider using the infrastructure resource until Nutanix or vSphere providers are implemented.

pkg/machineproviders/providers/openshift/machine/v1beta1/providerconfig/providerconfig.go:115:138: `newProviderConfigFromProviderSpec` - `infrastructure` is unused (unparam)
func newProviderConfigFromProviderSpec(logger logr.Logger, providerSpec machinev1beta1.ProviderSpec, platformType configv1.PlatformType, infrastructure *configv1.Infrastructure) (ProviderConfig, error) {
                                                                                                                                         ^
pkg/controllers/controlplanemachinesetgenerator/utils.go:170:122: `buildFailureDomains` - `infrastructure` always receives `nil` (unparam)
func buildFailureDomains(logger logr.Logger, machineSets []machinev1beta1.MachineSet, machines []machinev1beta1.Machine, infrastructure *configv1.Infrastructure) (*machinev1builder.FailureDomainsApplyConfiguration, error) {

JoelSpeed · 2023-10-16T11:54:10Z

Should we add the appropriate no lints for now so that this doesn't give a false signal on other PRs? Then they can be removed when they are no longer required?

I assume the motivation here is to share the commonly needed parts for the Nutanix and vSphere failure domain implementations and get them merged in chunks?

rvanderp3 · 2023-10-16T12:20:04Z

Should we add the appropriate no lints for now so that this doesn't give a false signal on other PRs? Then they can be removed when they are no longer required?

I assume the motivation here is to share the commonly needed parts for the Nutanix and vSphere failure domain implementations and get them merged in chunks?

Yes, that makes sense, I'll add the no lint. Correct, the motivation was to break out the common pieces to get them merged in chunks.

infrastructure resource

yanhua121

Should add the below line at L149 of machine/v1beta1/provider.go:
o := &openshiftMachineProvider{
...
infrastructure: infrastructure,
}

infrastructure resource

jcpowermac · 2023-10-25T17:12:19Z

/lgtm

elmiko

this makes sense to me, i just have a question and request.

elmiko · 2023-10-25T18:57:35Z

pkg/controllers/controlplanemachineset/suite_test.go


+	infrastructure := configv1builder.Infrastructure().AsAWS("test", "eu-west-2").WithName(util.InfrastructureName).Build()
+	Expect(k8sClient.Create(ctx, infrastructure)).To(Succeed())
+


are there any new tests associated with the change, or should the old test just keep working?

i'm wondering if we need a test for the specific logic changes.

The old tests should just continue to work. I'm expecting vSphere and Nutanix to implement unit tests that specifically exercise the consumption of the infrastructure resource.

elmiko · 2023-10-25T19:08:37Z

pkg/controllers/controlplanemachinesetgenerator/utils.go

 //
-//nolint:cyclop
-func buildFailureDomains(logger logr.Logger, machineSets []machinev1beta1.MachineSet, machines []machinev1beta1.Machine) (*machinev1builder.FailureDomainsApplyConfiguration, error) {
+//nolint:cyclop,unparam


since we are adding the unparam here, i think we should add a comment mentioning that the infra is a TODO or something, waiting for the nutanix and vsphere impls.

good call out, will handle that

elmiko · 2023-10-25T19:08:53Z

pkg/machineproviders/providers/openshift/machine/v1beta1/providerconfig/providerconfig.go

 }

-func newProviderConfigFromProviderSpec(logger logr.Logger, providerSpec machinev1beta1.ProviderSpec, platformType configv1.PlatformType) (ProviderConfig, error) {
+//nolint:unparam


similar comment on unparam

…ure resource Co-Authored-By: vr4manta <vr4manta@gmail.com>

openshift-ci-robot · 2023-10-25T21:21:45Z

@rvanderp3: This pull request references SPLAT-1211 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.15.0" version, but no target version was set.

Details

In response to this:

This PR introduces the infrastructure resource to aid in the resolution of topology and failure domains for platforms such as vSphere.

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-ci · 2023-10-25T22:05:00Z

@rvanderp3: 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-nutanix-ovn	`5c2ad1a`	link	false	`/test e2e-nutanix-ovn`
ci/prow/e2e-gcp-ovn-etcd-scaling	`5c2ad1a`	link	false	`/test e2e-gcp-ovn-etcd-scaling`
ci/prow/e2e-vsphere-operator	`5c2ad1a`	link	false	`/test e2e-vsphere-operator`
ci/prow/e2e-vsphere-multi-zone-operator	`5c2ad1a`	link	false	`/test e2e-vsphere-multi-zone-operator`

Full PR test history. Your PR dashboard.

Details

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.

JoelSpeed · 2023-10-26T11:24:51Z

/approve
/lgtm

IIUC the tests that are currently failing are expected to fail. I think @elmiko's comments have been resolved so we can continue

JoelSpeed · 2023-10-26T16:22:01Z

/approve
/lgtm

openshift-ci · 2023-10-26T16:26:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoelSpeed

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [JoelSpeed]

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

openshift-ci bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 13, 2023

openshift-ci bot requested review from JoelSpeed and odvarkadaniel October 13, 2023 21:39

rvanderp3 force-pushed the SPLAT-1211 branch from d6a6f32 to 81f28d1 Compare October 16, 2023 13:08

rvanderp3 mentioned this pull request Oct 16, 2023

SPLAT-1129: adding failure domain awareness for vSphere #228

Merged

6 tasks

rvanderp3 added a commit to rvanderp3/cluster-control-plane-machine-set-operator that referenced this pull request Oct 16, 2023

openshift#250 recognize topology and failure domains represented by the

85cd554

infrastructure resource

yanhua121 reviewed Oct 17, 2023

View reviewed changes

rvanderp3 force-pushed the SPLAT-1211 branch from 81f28d1 to e34feb2 Compare October 18, 2023 18:42

rvanderp3 added a commit to rvanderp3/cluster-control-plane-machine-set-operator that referenced this pull request Oct 23, 2023

openshift#250 recognize topology and failure domains represented by the

6941ee2

infrastructure resource

yanhua121 mentioned this pull request Oct 25, 2023

SPLAT-1272: Add support for Nutanix failure domains #258

Merged

openshift-ci bot assigned jcpowermac Oct 25, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 25, 2023

elmiko reviewed Oct 25, 2023

View reviewed changes

rvanderp3 force-pushed the SPLAT-1211 branch from e34feb2 to 56cdeed Compare October 25, 2023 19:31

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 25, 2023

recognize topology and failure domains represented by the infrastruct…

5c2ad1a

…ure resource Co-Authored-By: vr4manta <vr4manta@gmail.com>

rvanderp3 force-pushed the SPLAT-1211 branch from 56cdeed to 5c2ad1a Compare October 25, 2023 19:32

rvanderp3 changed the title ~~recognize topology and failure domains represented by the infrastructure resource~~ SPLAT-1211: recognize topology and failure domains represented by the infrastructure resource Oct 25, 2023

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Oct 25, 2023

rvanderp3 changed the title ~~SPLAT-1211: recognize topology and failure domains represented by the infrastructure resource~~ SPLAT-1211: recognize topology and failure domains represented by the infrastructure resource Oct 26, 2023

openshift-ci bot assigned JoelSpeed Oct 26, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 26, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 26, 2023

openshift-ci bot merged commit 8cf08ea into openshift:main Oct 26, 2023


		infrastructure := configv1builder.Infrastructure().AsAWS("test", "eu-west-2").WithName(util.InfrastructureName).Build()
		Expect(k8sClient.Create(ctx, infrastructure)).To(Succeed())

Comments

Conversation

rvanderp3 commented Oct 13, 2023

Uh oh!

rvanderp3 commented Oct 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JoelSpeed commented Oct 16, 2023

Uh oh!

rvanderp3 commented Oct 16, 2023

Uh oh!

yanhua121 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcpowermac commented Oct 25, 2023

Uh oh!

elmiko left a comment

Choose a reason for hiding this comment

Uh oh!

elmiko Oct 25, 2023

Choose a reason for hiding this comment

Uh oh!

rvanderp3 Oct 25, 2023

Choose a reason for hiding this comment

Uh oh!

elmiko Oct 25, 2023

Choose a reason for hiding this comment

Uh oh!

rvanderp3 Oct 25, 2023

Choose a reason for hiding this comment

Uh oh!

elmiko Oct 25, 2023

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Oct 25, 2023 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci bot commented Oct 25, 2023

Uh oh!

JoelSpeed commented Oct 26, 2023

Uh oh!

JoelSpeed commented Oct 26, 2023

Uh oh!

openshift-ci bot commented Oct 26, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

rvanderp3 commented Oct 13, 2023 •

edited

Loading

yanhua121 left a comment •

edited

Loading

openshift-ci-robot commented Oct 25, 2023 •

edited by openshift-ci bot

Loading