Add PowerVS platform support #1225

mkumatag · 2022-04-04T13:07:39Z

What this PR does / why we need it:

Add PowerVS platform support, reference to the openshift enhancement proposal for platform addition.

Which issue(s) this PR fixes (optional, use fixes #<issue_number>(, fixes #<issue_number>, ...) format, where issue_number might be a GitHub issue, or a Jira story:
Fixes #

Checklist

Subject and description added to both, commit and PR.
Relevant issues have been referenced.
This change includes docs.
This change includes unit tests.

netlify · 2022-04-04T13:08:24Z

✅ Deploy Preview for hypershift-docs ready!

Name	Link
🔨 Latest commit	`c804844`
🔍 Latest deploy log	https://app.netlify.com/sites/hypershift-docs/deploys/624aed9d9c253b000b8451a3
😎 Deploy Preview	https://deploy-preview-1225--hypershift-docs.netlify.app/reference/api
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

openshift-ci · 2022-04-04T13:09:54Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mkumatag
To complete the pull request process, please assign alvaroaleman after the PR has been reviewed.
You can assign the PR to them by writing /assign @alvaroaleman in a comment when ready.

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

Needs approval from an approver in each of these files:

OWNERS

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

mkumatag · 2022-04-04T13:11:23Z

/cc @dharaneeshvrd

openshift-ci · 2022-04-04T13:11:24Z

@mkumatag: GitHub didn't allow me to request PR reviews from the following users: dharaneeshvrd.

Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @dharaneeshvrd

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.

Co-authored-by: Dharaneeshwaran Ravichandran <dharanish185@gmail.com>

mkumatag · 2022-04-05T12:21:44Z

/retest

openshift-ci · 2022-04-05T13:45:34Z

@mkumatag: all tests passed!

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.

mkumatag · 2022-04-07T06:28:39Z

/cc @enxebre

enxebre · 2022-04-08T20:01:31Z

/assign

enxebre · 2022-04-11T12:29:38Z

cmd/cluster/powervs/create.go

@@ -139,6 +139,7 @@ func applyPlatformSpecificsValues(ctx context.Context, exampleOptions *apifixtur
 			PowerVSCloudConnection: opts.PowerVSPlatform.PowerVSCloudConnection,


please conflate this commit with e10d67b

enxebre · 2022-04-11T12:34:16Z

api/v1alpha1/hostedcluster_types.go

+// PowerVSPlatformSpec defines IBMCloud PowerVS specific settings for components
+type PowerVSPlatformSpec struct {
+	// ResourceGroup is the IBMCloud Resource Group in which the cluster resides. If not
+	// specified then default resource group of the account will be used.


// specified then default resource group of the account will be used.

I don't see this default implemented in the controller so I assume is how ibm cloud behaves?
I see this is omitempty but Can we also add the +optional to be more explicit?

we initially thought will do this but this value is required for setting up the CCM, so will make this required.

unless marked as optional (omitempty / //optional) it's required so just need to change that and verify in the CRD generation.

enxebre · 2022-04-11T12:36:10Z

api/v1alpha1/hostedcluster_types.go

+	// Zone is the availability zone where control plane cloud resources are
+	// created.
+	//
+	// +immutable


there's omitempty here, can we add +optional tag too consistently everywhere is optional?

Again we need this value required as well for CCM code, will make this required.

enxebre · 2022-04-11T12:38:42Z

api/v1alpha1/hostedcluster_types.go

+	// Subnet is the subnet to use for control plane cloud resources.
+	//
+	// +optional
+	Subnet *PowerVSResourceReference `json:"subnet,omitempty"`


How can this be optional, what happens if it's not set?
Actually I can't see where this is consumed, is it used anywhere?

you are right, these can't be optional. This is not used anywhere because we are using Subnet from PowerVSNodePoolPlatform for the machines, but we need this in the future to create new nodepool(will submit a follow up PR)

enxebre · 2022-04-11T12:40:05Z

api/v1alpha1/hostedcluster_types.go

+	Subnet *PowerVSResourceReference `json:"subnet,omitempty"`
+
+	// ServiceInstanceID is the ServiceInstance to use for control plane cloud resources.
+	ServiceInstanceID string `json:"serviceInstanceID,omitempty"`


How can this be optional, what happens if it's not set?

Again, can't be optional.

enxebre · 2022-04-11T12:40:30Z

api/v1alpha1/hostedcluster_types.go

+	// plane.
+	//
+	// +immutable
+	VPC *PowerVSVPC `json:"vpc,omitempty"`


How can this be optional, what happens if it's not set?

can't be optional.

enxebre · 2022-04-11T12:43:27Z

api/v1alpha1/hostedcluster_types.go

+	// TODO(dan): document the "cloud controller policy"
+	//
+	// +immutable
+	KubeCloudControllerCreds corev1.LocalObjectReference `json:"kubeCloudControllerCreds"`


I brought this up in the past: I wonder if both KubeCloudControllerCreds and NodePoolManagementCreds should belong to the generic spec.

enxebre · 2022-04-11T12:49:19Z

api/v1alpha1/nodepool_types.go

+// on IBMCloud PowerVS platform.
+type PowerVSNodePoolPlatform struct {
+	// ServiceInstanceID is the ServiceInstance to use for control plane cloud resources.
+	ServiceInstanceID string `json:"serviceInstanceID"`


which "control plane cloud resources" does this refer to?

oops, this should be NodePool

enxebre · 2022-04-11T12:50:17Z

api/v1alpha1/nodepool_types.go

+
+	// ProcType (dedicated, shared, capped)
+	// +optional
+	ProcType string `json:"procType,omitempty"`


please add open api enum validation tag

How can this be optional, what happens if it's not set?

we have a web hook in the capibm controller to set some of these value default, rightnow I'm not deploying that part of the deployment(future plan is to add support).

So my next question is how are we dealing with other providers?

I don't think that should make any difference, I see webhooks as an additional UX layer. The API need to be solid even nevertheless. If a ProcType is necessary for the controller to work, then the field must be mark as required not optional (or alternative optional and use openAPI crd defaulting, but I rather do the former to avoid surprising opinions/assumptions for consumers). Then in the CLI we can give it a happy path/convenience value by default if we want to.

sure, I will make enum and default.

enxebre · 2022-04-11T12:54:52Z

api/v1alpha1/nodepool_types.go

+
+	// Memory specifies the amount of memory specified in GBs
+	// +optional
+	Memory string `json:"memory,omitempty"`


let's do this a resource quantity

How can this be optional, what happens if it's not set?

I would love to build api accepts value only in GiB format, I'm afraid that user won't be able to take advantage of resource quantity

enxebre · 2022-04-11T12:55:39Z

api/v1alpha1/nodepool_types.go

+	ProcType string `json:"procType,omitempty"`
+
+	// Processors specifies the number of processors allocated
+	// +optional


How can this be optional, what happens if it's not set?

again, setting defaults in the capibm webhook

enxebre · 2022-04-11T12:56:49Z

api/v1alpha1/nodepool_types.go

+
+	// Image used for deploying the nodes
+	// +optional
+	Image *PowerVSResourceReference `json:"image,omitempty"`


How can this be optional, what happens if it's not set?

In hypershift we create IBMPowerVSImage resource to onboard the image from release artefacts, but if user wants to test with a different image which is already onboarded into the service then this field is useful, place we are overriding it here: https://github.com/mkumatag/hypershift/blob/1e9207ea17296fca53960b1d5dd44b37c6b3ba45/hypershift-operator/controllers/nodepool/nodepool_controller.go#L519:L537

enxebre · 2022-04-11T12:57:08Z

api/v1alpha1/nodepool_types.go

+
+	// StorageType for the image and nodes
+	// +optional
+	StorageType string `json:"storageType,omitempty"`


is this enum type validation?

will add the enum and default

enxebre · 2022-04-11T12:57:31Z

api/v1alpha1/nodepool_types.go

+	// Subnet is the subnet to use for control plane cloud resources.
+	//
+	// +optional
+	Subnet *PowerVSResourceReference `json:"subnet,omitempty"`


which control plane resources does this refer to?
How can this be optional, what happens if it's not set?

typical copy/paste error, need to be required

enxebre · 2022-04-11T12:58:10Z

api/v1alpha1/nodepool_types.go

+	// DeletePolicy for the image
+	//
+	// +optional
+	DeletePolicy string `json:"deletePolicy,omitempty"`


is this enum type validation?
How can this be optional, what happens if it's not set?

will add the enum and default

enxebre · 2022-04-11T13:00:30Z

cmd/cluster/powervs/create.go

+func NewCreateCommand(opts *core.CreateOptions) *cobra.Command {
+	cmd := &cobra.Command{
+		Use:          "powervs",
+		Short:        "Creates basic functional HostedCluster resources on PowerVS PowerVS",


typo remove one "PowerVS"?

enxebre · 2022-04-11T13:01:38Z

cmd/cluster/powervs/create.go

+	}
+
+	cmd.Flags().StringVar(&opts.PowerVSPlatform.ResourceGroup, "resource-group", "", "IBM Cloud Resource group")
+	cmd.Flags().StringVar(&opts.PowerVSPlatform.PowerVSRegion, "powervs-region", opts.PowerVSPlatform.PowerVSRegion, "IBM Cloud PowerVS region")


none of this needs the "powervs-" prefix since this is the powervs subcommand already.

enxebre · 2022-04-11T13:12:16Z

hypershift-operator/controllers/hostedcluster/internal/platform/powervs/powervs.go

+	hcluster *hyperv1.HostedCluster,
+	controlPlaneNamespace string,
+	apiEndpoint hyperv1.APIEndpoint) (client.Object, error) {
+	if hcluster.Spec.Platform.IBMCloud != nil && hcluster.Spec.Platform.IBMCloud.ProviderType == configv1.IBMCloudProviderTypeUPI {


given IBMCloud.ProviderType is a discriminator union PowerVS definition should live there

type IBMCloudPlatformSpec struct { // ProviderType is a specific supported infrastructure provider within IBM Cloud. ProviderType configv1.IBMCloudProviderType `json:"providerType,omitempty"` PowerVS PowerVSPlatformSpec }

This is just a copy/paste error from ibmcloud, this is not required.

enxebre · 2022-04-11T13:16:00Z

hypershift-operator/controllers/nodepool/powervs.go

+	}
+}
+
+func ibmPowerVSMachineTemplateSpec(nodePool *hyperv1.NodePool, ami string) *capipowervs.IBMPowerVSMachineTemplateSpec {


what is an ami in IBM?

In PowerVS it is called BootImage or just Image

enxebre · 2022-04-11T13:20:31Z

Thanks @mkumatag! this is great work. For the sake of doing reasonable reviews and moving small chunks faster I suggest we split this in multiple PRs:
1 - API
2 - Controllers
3 - CLI

mkumatag · 2022-04-11T17:32:38Z

Thanks @mkumatag! this is great work. For the sake of doing reasonable reviews and moving small chunks faster I suggest we split this in multiple PRs:
1 - API
2 - Controllers
3 - CLI

API Changes: #1255
Vendor updates: #1254

enxebre · 2022-04-18T12:58:36Z

closing in favour of #1255
/close

openshift-ci · 2022-04-18T12:58:59Z

@enxebre: Closed this PR.

In response to this:

closing in favour of #1255
/close

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 bot requested review from csrwng and ironcladlou April 4, 2022 13:09

mkumatag force-pushed the powervs branch 2 times, most recently from e486d63 to b9c13ea Compare April 5, 2022 04:58

mkumatag and others added 4 commits April 5, 2022 10:33

powervs support

e10d67b

Co-authored-by: Dharaneeshwaran Ravichandran <dharanish185@gmail.com>

update vendors

5877371

fix validate

e696724

generated code

1e9207e

mkumatag force-pushed the powervs branch from b9c13ea to 1e9207e Compare April 5, 2022 05:08

openshift-ci bot requested a review from enxebre April 7, 2022 06:28

openshift-ci bot assigned enxebre Apr 8, 2022

enxebre reviewed Apr 11, 2022

View reviewed changes

mkumatag mentioned this pull request Apr 11, 2022

Introduce IBMPowerVS platform #1255

Merged

4 tasks

openshift-ci bot closed this Apr 18, 2022

		@@ -139,6 +139,7 @@ func applyPlatformSpecificsValues(ctx context.Context, exampleOptions *apifixtur
		PowerVSCloudConnection: opts.PowerVSPlatform.PowerVSCloudConnection,

Add PowerVS platform support #1225

Add PowerVS platform support #1225

Conversation

mkumatag commented Apr 4, 2022

netlify bot commented Apr 4, 2022

✅ Deploy Preview for hypershift-docs ready!

openshift-ci bot commented Apr 4, 2022

mkumatag commented Apr 4, 2022

openshift-ci bot commented Apr 4, 2022

mkumatag commented Apr 5, 2022

openshift-ci bot commented Apr 5, 2022

mkumatag commented Apr 7, 2022

enxebre commented Apr 8, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enxebre Apr 11, 2022 • edited Loading

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

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

enxebre Apr 11, 2022 • edited Loading

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

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

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

enxebre Apr 11, 2022 • edited Loading

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

enxebre commented Apr 11, 2022

mkumatag commented Apr 11, 2022

enxebre commented Apr 18, 2022

openshift-ci bot commented Apr 18, 2022

enxebre Apr 11, 2022 •

edited

Loading

enxebre Apr 11, 2022 •

edited

Loading

enxebre Apr 11, 2022 •

edited

Loading