MULTIARCH-4096: PowerVS: Generate the cluster assets #8116

hamzy · 2024-03-07T13:04:07Z

Generate the cluster assets for the PowerVS CAPI provider.

This has no included code written over two weeks.

openshift-ci-robot · 2024-03-07T13:04:12Z

@hamzy: This pull request references MULTIARCH-4096 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 sub-task to target the "4.16.0" version, but no target version was set.

In response to this:

Generate the cluster assets for the PowerVS CAPI provider.

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.

hamzy · 2024-03-19T03:29:10Z

/retest-required

openshift-ci-robot · 2024-03-20T22:38:45Z

@hamzy: This pull request references MULTIARCH-4096 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 sub-task to target the "4.16.0" version, but no target version was set.

In response to this:

Generate the cluster assets for the PowerVS CAPI provider.

This has no included code written over two weeks.

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.

pkg/asset/manifests/clusterapi/cluster.go

r4f4 · 2024-03-20T22:11:29Z

pkg/asset/manifests/powervs/cluster.go

+	if installConfig.Config.Platform.PowerVS.PowerVSResourceGroup != "" {
+		// @TODO is it a name or id?
+		resourceGroupName = installConfig.Config.Platform.PowerVS.PowerVSResourceGroup
+	}


Suggested change

if installConfig.Config.Platform.PowerVS.PowerVSResourceGroup != "" {

// @TODO is it a name or id?

resourceGroupName = installConfig.Config.Platform.PowerVS.PowerVSResourceGroup

}

The TODO means future work/investigation. I am trying to get this PR in so I don't have to juggle code for a month.

pkg/asset/manifests/powervs/cluster.go

r4f4 · 2024-03-20T22:15:12Z

pkg/asset/manifests/powervs/cluster.go

+	default:
+		return nil, fmt.Errorf("GenerateClusterAssets: Region is empty")


I'm guessing this is here just for completeness and you're already validating the region during the installconfig validation stage?

Yes, for completeness and to stop someone else from asking why no default.

r4f4 · 2024-03-20T22:16:32Z

pkg/asset/manifests/powervs/cluster.go

+			ServiceInstance: &service,
+			Zone:            &installConfig.Config.Platform.PowerVS.Zone,
+			ResourceGroup: &capibm.IBMPowerVSResourceReference{
+				Name: &resourceGroupName,


Suggested change

Name: &resourceGroupName,

Name: &installConfig.Config.Platform.PowerVS.PowerVSResourceGroup,

This is part of the future TODO

pkg/asset/manifests/powervs/cluster.go

pkg/asset/installconfig/gcp/mock/usertags_mock.go

pkg/asset/manifests/cloudproviderconfig.go

pkg/asset/manifests/powervs/cluster.go

mjturek · 2024-03-23T17:35:56Z

/lgtm

mjturek · 2024-03-25T13:24:35Z

/lgtm

mjturek · 2024-03-25T13:27:37Z

/remove-lgtm

mjturek · 2024-03-25T15:59:17Z

/lgtm

This adds a controller to run for the PowerVS provider.

Generate the cluster assets for the PowerVS CAPI provider.

Fix machine manifest generation and add a bootstrap machine.

When the CAPI infrastructure is ready, 1) Create the DNS entries 2) Add security group rules for necessary ports

The PowerVS CAPI controller creates three networks for each of the endpoints. Therefore, account for this change.

r4f4 · 2024-03-25T23:05:22Z

pkg/asset/manifests/powervs/cluster.go

+	// Avoid empty manifest elements
+	logrus.Debugf("GenerateClusterAssets: len(VPCSubnets) = %d", len(installConfig.Config.Platform.PowerVS.VPCSubnets))
+	if len(installConfig.Config.Platform.PowerVS.VPCSubnets) > 0 {
+		powerVSCluster.Spec.VPCSubnets = make([]capibm.Subnet, 0)


Nit: can preallocate the array here

Suggested change

powerVSCluster.Spec.VPCSubnets = make([]capibm.Subnet, 0)

powerVSCluster.Spec.VPCSubnets = make([]capibm.Subnet, 0, len(installConfig.Config.Platform.PowerVS.VPCSubnets)

r4f4 · 2024-03-25T23:08:40Z

pkg/asset/manifests/powervs/cluster.go

+		for i := range installConfig.Config.Platform.PowerVS.VPCSubnets {
+			// We cannot get the string in the loop and add it as it appears duplicated in the generated file
+			powerVSCluster.Spec.VPCSubnets = append(powerVSCluster.Spec.VPCSubnets,
+				capibm.Subnet{
+					ID: &installConfig.Config.Platform.PowerVS.VPCSubnets[i],
+				})
+		}


Nit: not that it matters, but you can get the string if you do:

Suggested change

for i := range installConfig.Config.Platform.PowerVS.VPCSubnets {

// We cannot get the string in the loop and add it as it appears duplicated in the generated file

powerVSCluster.Spec.VPCSubnets = append(powerVSCluster.Spec.VPCSubnets,

capibm.Subnet{

ID: &installConfig.Config.Platform.PowerVS.VPCSubnets[i],

})

}

for _, subnet := range installConfig.Config.Platform.PowerVS.VPCSubnets {

subnet := subnet

powerVSCluster.Spec.VPCSubnets = append(powerVSCluster.Spec.VPCSubnets,

capibm.Subnet{

ID: &subnet,

})

}

r4f4 · 2024-03-25T23:15:11Z

pkg/infrastructure/powervs/clusterapi/powervs.go

+		logrus.Debugf("in.InfraID = %s", in.InfraID)
+		idx = strings.LastIndex(in.InfraID, "-")
+		logrus.Debugf("idx = %d", idx)
+		substr = in.InfraID[idx:]
+		logrus.Debugf("substr = %s", substr)
+		infraID = strings.ReplaceAll(in.InfraID, substr, "")
+		logrus.Debugf("infraID = %s", infraID)


Suggested change

logrus.Debugf("in.InfraID = %s", in.InfraID)

idx = strings.LastIndex(in.InfraID, "-")

logrus.Debugf("idx = %d", idx)

substr = in.InfraID[idx:]

logrus.Debugf("substr = %s", substr)

infraID = strings.ReplaceAll(in.InfraID, substr, "")

logrus.Debugf("infraID = %s", infraID)

logrus.Debugf("in.InfraID = %s", in.InfraID)

infraID := in.InfraID[:strings.LastIndex(in.InfraID, "-")]

logrus.Debugf("infraID = %s", infraID)

r4f4 · 2024-03-25T23:36:50Z

pkg/infrastructure/powervs/clusterapi/powervs.go

+	switch {
+	case refServiceInstance.ID != nil:
+		logrus.Debugf("PostProvision: CreateSSHKey: si id = %s, key = %s",
+			*refServiceInstance.ID,
+			in.InstallConfig.Config.SSHKey)
+
+		backoff := wait.Backoff{
+			Duration: 15 * time.Second,
+			Factor:   1.1,
+			Cap:      leftInContext(ctx),
+			Steps:    math.MaxInt32}
+		err = wait.ExponentialBackoffWithContext(ctx, backoff, func(context.Context) (bool, error) {
+			err2 := client.CreateSSHKey(ctx,
+				*refServiceInstance.ID,
+				*powerVSMachine.Status.Zone,
+				sshKeyName,
+				in.InstallConfig.Config.SSHKey)
+			if err2 == nil {
+				return true, nil
+			}
+			return false, err2
+		})
+		if err != nil {
+			return fmt.Errorf("failed to add SSH key for the workers(ID): %w", err)
+		}
+	case refServiceInstance.Name != nil:
+		logrus.Debugf("PostProvision: CreateSSHKey: si name = %s, key = %s",
+			*refServiceInstance.Name,
+			in.InstallConfig.Config.SSHKey)
+
+		vpc, err := client.GetVPCByName(ctx, *refServiceInstance.Name)
+		if err != nil {
+			return fmt.Errorf("failed to find id for VPC name %s: %w",
+				*refServiceInstance.Name,
+				err)
+		}
+
+		backoff := wait.Backoff{
+			Duration: 15 * time.Second,
+			Factor:   1.1,
+			Cap:      leftInContext(ctx),
+			Steps:    math.MaxInt32}
+		err = wait.ExponentialBackoffWithContext(ctx, backoff, func(context.Context) (bool, error) {
+			err2 := client.CreateSSHKey(ctx,
+				*vpc.ID,
+				*powerVSMachine.Status.Zone,
+				sshKeyName,
+				in.InstallConfig.Config.SSHKey)
+			if err2 == nil {
+				return true, nil
+			}
+			return false, err2
+		})
+		if err != nil {
+			return fmt.Errorf("failed to add SSH key for the workers(Name): %w", err)
+		}
+	default:
+		return fmt.Errorf("could not handle powerVSMachine.Spec.ServiceInstance")
+	}
+


Nit:

Suggested change

switch {

case refServiceInstance.ID != nil:

logrus.Debugf("PostProvision: CreateSSHKey: si id = %s, key = %s",

*refServiceInstance.ID,

in.InstallConfig.Config.SSHKey)

backoff := wait.Backoff{

Duration: 15 * time.Second,

Factor: 1.1,

Cap: leftInContext(ctx),

Steps: math.MaxInt32}

err = wait.ExponentialBackoffWithContext(ctx, backoff, func(context.Context) (bool, error) {

err2 := client.CreateSSHKey(ctx,

*refServiceInstance.ID,

*powerVSMachine.Status.Zone,

sshKeyName,

in.InstallConfig.Config.SSHKey)

if err2 == nil {

return true, nil

}

return false, err2

})

if err != nil {

return fmt.Errorf("failed to add SSH key for the workers(ID): %w", err)

}

case refServiceInstance.Name != nil:

logrus.Debugf("PostProvision: CreateSSHKey: si name = %s, key = %s",

*refServiceInstance.Name,

in.InstallConfig.Config.SSHKey)

vpc, err := client.GetVPCByName(ctx, *refServiceInstance.Name)

if err != nil {

return fmt.Errorf("failed to find id for VPC name %s: %w",

*refServiceInstance.Name,

err)

}

backoff := wait.Backoff{

Duration: 15 * time.Second,

Factor: 1.1,

Cap: leftInContext(ctx),

Steps: math.MaxInt32}

err = wait.ExponentialBackoffWithContext(ctx, backoff, func(context.Context) (bool, error) {

err2 := client.CreateSSHKey(ctx,

*vpc.ID,

*powerVSMachine.Status.Zone,

sshKeyName,

in.InstallConfig.Config.SSHKey)

if err2 == nil {

return true, nil

}

return false, err2

})

if err != nil {

return fmt.Errorf("failed to add SSH key for the workers(Name): %w", err)

}

default:

return fmt.Errorf("could not handle powerVSMachine.Spec.ServiceInstance")

}

var instanceID *string

var fieldType string

switch {

case refServiceInstance.ID != nil:

logrus.Debugf("PostProvision: CreateSSHKey: si id = %s, key = %s",

*refServiceInstance.ID,

in.InstallConfig.Config.SSHKey)

instanceID = refServiceInstance.ID

fieldType = "ID"

case refServiceInstance.Name != nil:

logrus.Debugf("PostProvision: CreateSSHKey: si name = %s, key = %s",

*refServiceInstance.Name,

in.InstallConfig.Config.SSHKey)

vpc, err := client.GetVPCByName(ctx, *refServiceInstance.Name)

if err != nil {

return fmt.Errorf("failed to find id for VPC name %s: %w",

*refServiceInstance.Name,

err)

}

instanceID = vpc.ID

fieldType = "Name"

default:

return fmt.Errorf("could not handle powerVSMachine.Spec.ServiceInstance")

}

backoff := wait.Backoff{

Duration: 15 * time.Second,

Factor: 1.1,

Cap: leftInContext(ctx),

Steps: math.MaxInt32,

}

err = wait.ExponentialBackoffWithContext(ctx, backoff, func(context.Context) (bool, error) {

err2 := client.CreateSSHKey(ctx, *instanceID, *powerVSMachine.Status.Zone, sshKeyName, in.InstallConfig.Config.SSHKey)

if err2 == nil {

return true, nil

}

return false, err2

})

if err != nil {

return fmt.Errorf("failed to add SSH key for the workers(%s): %w", fieldType, err)

}

mjturek · 2024-03-26T13:13:13Z

@r4f4 can we address these in a follow up patch? We have other work that is based on this that we do not want to break.

mjturek · 2024-03-26T13:13:36Z

/lgtm

r4f4 · 2024-03-26T13:17:40Z

/approve

openshift-ci · 2024-03-26T13:19:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: r4f4

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:

~~OWNERS~~ [r4f4]

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

openshift-ci · 2024-03-26T16:14:16Z

@hamzy: 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-ibmcloud-ovn	`fe29e84`	link	false	`/test e2e-ibmcloud-ovn`
ci/prow/okd-e2e-aws-ovn-upgrade	`fe29e84`	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-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Mar 7, 2024

openshift-ci bot requested review from r4f4 and sadasu March 7, 2024 13:07

hamzy force-pushed the PowerVS-MULTIARCH-4096 branch 5 times, most recently from 3ffb4ce to ad5f3ac Compare March 13, 2024 19:59

hamzy force-pushed the PowerVS-MULTIARCH-4096 branch 5 times, most recently from 5f81298 to 014741f Compare March 18, 2024 20:43

hamzy force-pushed the PowerVS-MULTIARCH-4096 branch 3 times, most recently from ba9e604 to b3b23a0 Compare March 20, 2024 21:55

r4f4 reviewed Mar 20, 2024

View reviewed changes

hamzy force-pushed the PowerVS-MULTIARCH-4096 branch from b3b23a0 to 106ad0c Compare March 22, 2024 10:21

openshift-ci bot assigned mjturek Mar 23, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 23, 2024

hamzy force-pushed the PowerVS-MULTIARCH-4096 branch from 106ad0c to 2479ed6 Compare March 23, 2024 20:27

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 23, 2024

hamzy force-pushed the PowerVS-MULTIARCH-4096 branch 3 times, most recently from 0e224a7 to 6479343 Compare March 25, 2024 10:57

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

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

hamzy force-pushed the PowerVS-MULTIARCH-4096 branch from 6479343 to 1fc7115 Compare March 25, 2024 14:25

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

hamzy force-pushed the PowerVS-MULTIARCH-4096 branch from 1fc7115 to b013ab1 Compare March 25, 2024 20:29

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

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 25, 2024

hamzy added 4 commits March 25, 2024 15:39

PowerVS: Add controller for CAPI

ea5033d

This adds a controller to run for the PowerVS provider.

MULTIARCH-4096: PowerVS: Add GenerateClusterAssets

2a44e60

Generate the cluster assets for the PowerVS CAPI provider.

Update PowerVS machine manifests for CAPI

e6376a5

Fix machine manifest generation and add a bootstrap machine.

PowerVS: Include latest ibmcloud-infrastructure-components.yaml

4a2e695

hamzy force-pushed the PowerVS-MULTIARCH-4096 branch from b013ab1 to 94f54c2 Compare March 25, 2024 20:55

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 25, 2024

hamzy added 4 commits March 25, 2024 16:49

PowerVS: Add CAPI support for InfraReady

c8c4431

When the CAPI infrastructure is ready, 1) Create the DNS entries 2) Add security group rules for necessary ports

PowerVS: Fix CloudProviderConfig for CAPI

50ce0ab

The PowerVS CAPI controller creates three networks for each of the endpoints. Therefore, account for this change.

PowerVS: Vendoring go.* changes

f9ef2ad

PowerVS: Vendoring the rest changes

fe29e84

hamzy force-pushed the PowerVS-MULTIARCH-4096 branch from 94f54c2 to fe29e84 Compare March 25, 2024 21:55

r4f4 reviewed Mar 25, 2024

View reviewed changes

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

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

openshift-merge-bot bot merged commit 0645657 into openshift:master Mar 26, 2024
25 of 27 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MULTIARCH-4096: PowerVS: Generate the cluster assets #8116

MULTIARCH-4096: PowerVS: Generate the cluster assets #8116

hamzy commented Mar 7, 2024 •

edited

openshift-ci-robot commented Mar 7, 2024 •

edited by openshift-ci bot

hamzy commented Mar 19, 2024

openshift-ci-robot commented Mar 20, 2024 •

edited by openshift-ci bot

r4f4 Mar 20, 2024

hamzy Mar 21, 2024

r4f4 Mar 20, 2024

hamzy Mar 21, 2024

r4f4 Mar 20, 2024

hamzy Mar 21, 2024

mjturek commented Mar 23, 2024

mjturek commented Mar 25, 2024

mjturek commented Mar 25, 2024

mjturek commented Mar 25, 2024

r4f4 Mar 25, 2024

r4f4 Mar 25, 2024

r4f4 Mar 25, 2024

r4f4 Mar 25, 2024

mjturek commented Mar 26, 2024

mjturek commented Mar 26, 2024

r4f4 commented Mar 26, 2024

openshift-ci bot commented Mar 26, 2024

openshift-ci bot commented Mar 26, 2024

		default:
		return nil, fmt.Errorf("GenerateClusterAssets: Region is empty")

	Name: &resourceGroupName,
	Name: &installConfig.Config.Platform.PowerVS.PowerVSResourceGroup,

	powerVSCluster.Spec.VPCSubnets = make([]capibm.Subnet, 0)
	powerVSCluster.Spec.VPCSubnets = make([]capibm.Subnet, 0, len(installConfig.Config.Platform.PowerVS.VPCSubnets)

MULTIARCH-4096: PowerVS: Generate the cluster assets #8116

MULTIARCH-4096: PowerVS: Generate the cluster assets #8116

Conversation

hamzy commented Mar 7, 2024 • edited

openshift-ci-robot commented Mar 7, 2024 • edited by openshift-ci bot

hamzy commented Mar 19, 2024

openshift-ci-robot commented Mar 20, 2024 • edited by openshift-ci bot

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjturek commented Mar 23, 2024

mjturek commented Mar 25, 2024

mjturek commented Mar 25, 2024

mjturek commented Mar 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjturek commented Mar 26, 2024

mjturek commented Mar 26, 2024

r4f4 commented Mar 26, 2024

openshift-ci bot commented Mar 26, 2024

openshift-ci bot commented Mar 26, 2024

hamzy commented Mar 7, 2024 •

edited

openshift-ci-robot commented Mar 7, 2024 •

edited by openshift-ci bot

openshift-ci-robot commented Mar 20, 2024 •

edited by openshift-ci bot