Skip to content

Add PowerVS platform support #1225

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

Closed
mkumatag wants to merge 4 commits into from
Closed

Add PowerVS platform support #1225

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e10d67b
powervs support
mkumatag Feb 4, 2022
5877371
update vendors
mkumatag Apr 4, 2022
e696724
fix validate
mkumatag Apr 4, 2022
1e9207e
generated code
mkumatag Apr 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
88 changes: 88 additions & 0 deletions api/fixtures/example.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ type ExampleOptions struct {
Agent *ExampleAgentOptions
Kubevirt *ExampleKubevirtOptions
Azure *ExampleAzureOptions
PowerVS *ExamplePowerVSOptions
NetworkType hyperv1.NetworkType
ControlPlaneAvailabilityPolicy hyperv1.AvailabilityPolicy
InfrastructureAvailabilityPolicy hyperv1.AvailabilityPolicy
Expand Down Expand Up @@ -385,6 +386,80 @@ web_identity_token_file = /var/run/secrets/openshift/serviceaccount/token
}
services = getIngressServicePublishingStrategyMapping()

case o.PowerVS != nil:
buildIBMCloudCreds := func(name, apikey string) *corev1.Secret {
return &corev1.Secret{
TypeMeta: metav1.TypeMeta{
Kind: "Secret",
APIVersion: corev1.SchemeGroupVersion.String(),
},
ObjectMeta: metav1.ObjectMeta{
Namespace: namespace.Name,
Name: name,
},
Data: map[string][]byte{
"ibm-credentials.env": []byte(fmt.Sprintf(`IBMCLOUD_AUTH_TYPE=iam
IBMCLOUD_APIKEY=%s
IBMCLOUD_AUTH_URL=https://iam.cloud.ibm.com
`, apikey)),
},
}
}
powerVSResources := &ExamplePowerVSResources{
buildIBMCloudCreds(o.Name+"-cloud-ctrl-creds", o.PowerVS.ApiKey),
buildIBMCloudCreds(o.Name+"-node-mgmt-creds", o.PowerVS.ApiKey),
buildIBMCloudCreds(o.Name+"-cpo-creds", o.PowerVS.ApiKey),
}
resources = powerVSResources.AsObjects()
platformSpec = hyperv1.PlatformSpec{
Type: hyperv1.PowerVSPlatform,
PowerVS: &hyperv1.PowerVSPlatformSpec{
ResourceGroup: o.PowerVS.ResourceGroup,
Region: o.PowerVS.PowerVSRegion,
Zone: o.PowerVS.PowerVSZone,
ServiceInstanceID: o.PowerVS.PowerVSCloudInstanceID,
VPC: &hyperv1.PowerVSVPC{
Name: o.PowerVS.Vpc,
Region: o.PowerVS.VpcRegion,
Subnet: o.PowerVS.VpcSubnet,
},
KubeCloudControllerCreds: corev1.LocalObjectReference{Name: powerVSResources.KubeCloudControllerPowerVSCreds.Name},
NodePoolManagementCreds: corev1.LocalObjectReference{Name: powerVSResources.NodePoolManagementPowerVSCreds.Name},
ControlPlaneOperatorCreds: corev1.LocalObjectReference{Name: powerVSResources.ControlPlaneOperatorPowerVSCreds.Name},
},
}
services = []hyperv1.ServicePublishingStrategyMapping{
{
Service: hyperv1.APIServer,
ServicePublishingStrategy: hyperv1.ServicePublishingStrategy{
Type: hyperv1.LoadBalancer,
},
},
{
Service: hyperv1.OAuthServer,
ServicePublishingStrategy: hyperv1.ServicePublishingStrategy{
Type: hyperv1.Route,
},
},
{
Service: hyperv1.OIDC,
ServicePublishingStrategy: hyperv1.ServicePublishingStrategy{
Type: hyperv1.S3,
},
},
{
Service: hyperv1.Konnectivity,
ServicePublishingStrategy: hyperv1.ServicePublishingStrategy{
Type: hyperv1.Route,
},
},
{
Service: hyperv1.Ignition,
ServicePublishingStrategy: hyperv1.ServicePublishingStrategy{
Type: hyperv1.Route,
},
},
}
default:
panic("no platform specified")
}
Expand Down Expand Up @@ -620,6 +695,19 @@ web_identity_token_file = /var/run/secrets/openshift/serviceaccount/token
}
nodePools = append(nodePools, nodePool)
}
case hyperv1.PowerVSPlatform:
nodePool := defaultNodePool(cluster.Name)
nodePool.Spec.Platform.PowerVS = &hyperv1.PowerVSNodePoolPlatform{
ServiceInstanceID: o.PowerVS.PowerVSCloudInstanceID,
SysType: o.PowerVS.SysType,
ProcType: o.PowerVS.ProcType,
Processors: o.PowerVS.Processors,
Memory: o.PowerVS.Memory,
Subnet: &hyperv1.PowerVSResourceReference{
ID: &o.PowerVS.PowerVSSubnetID,
},
}
nodePools = append(nodePools, nodePool)
default:
panic("Unsupported platform")
}
Expand Down
46 changes: 46 additions & 0 deletions api/fixtures/example_ibmcloud_powervs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
package fixtures

import (
corev1 "k8s.io/api/core/v1"
crclient "sigs.k8s.io/controller-runtime/pkg/client"
)

type ExamplePowerVSOptions struct {
ApiKey string
AccountID string
ResourceGroup string
PowerVSRegion string
PowerVSZone string
PowerVSCloudInstanceID string
PowerVSSubnetID string
PowerVSCloudConnection string
VpcRegion string
Vpc string
VpcSubnet string

// nodepool related options
SysType string
ProcType string
Processors string
Memory string
}

type ExamplePowerVSResources struct {
KubeCloudControllerPowerVSCreds *corev1.Secret
NodePoolManagementPowerVSCreds *corev1.Secret
ControlPlaneOperatorPowerVSCreds *corev1.Secret
}

func (o *ExamplePowerVSResources) AsObjects() []crclient.Object {
var objects []crclient.Object
if o.KubeCloudControllerPowerVSCreds != nil {
objects = append(objects, o.KubeCloudControllerPowerVSCreds)
}
if o.NodePoolManagementPowerVSCreds != nil {
objects = append(objects, o.NodePoolManagementPowerVSCreds)
}
if o.ControlPlaneOperatorPowerVSCreds != nil {
objects = append(objects, o.ControlPlaneOperatorPowerVSCreds)
}
return objects
}
116 changes: 115 additions & 1 deletion api/v1alpha1/hostedcluster_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -475,7 +475,7 @@ const (

// PlatformType is a specific supported infrastructure provider.
//
// +kubebuilder:validation:Enum=AWS;None;IBMCloud;Agent;KubeVirt;Azure
// +kubebuilder:validation:Enum=AWS;None;IBMCloud;Agent;KubeVirt;Azure;PowerVS
type PlatformType string

const (
Expand All @@ -496,6 +496,9 @@ const (

// AzurePlatform represents Azure infrastructure.
AzurePlatform PlatformType = "Azure"

// PowerVSPlatform represents PowerVS infrastructure.
PowerVSPlatform PlatformType = "PowerVS"
)

// PlatformSpec specifies the underlying infrastructure provider for the cluster
Expand Down Expand Up @@ -524,6 +527,12 @@ type PlatformSpec struct {

// Azure defines azure specific settings
Azure *AzurePlatformSpec `json:"azure,omitempty"`

// IBMCloud defines IBMCloud specific settings for components
//
// +optional
// +immutable
PowerVS *PowerVSPlatformSpec `json:"powervs,omitempty"`
}

// AgentPlatformSpec specifies configuration for agent-based installations.
Expand All @@ -538,6 +547,111 @@ type IBMCloudPlatformSpec struct {
ProviderType configv1.IBMCloudProviderType `json:"providerType,omitempty"`
}

// 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.
Copy link
Copy Markdown
Member

@enxebre enxebre Apr 11, 2022

Choose a reason for hiding this comment

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

// 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?

Copy link
Copy Markdown
Member Author

@mkumatag mkumatag Apr 11, 2022

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

@enxebre enxebre Apr 11, 2022

Choose a reason for hiding this comment

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

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

//
// +immutable
ResourceGroup string `json:"resourceGroup,omitempty"`

// Region is the IBMCloud region in which the cluster resides. This configures the
// OCP control plane cloud integrations, and is used by NodePool to resolve
// the correct boot image for a given release.
//
// +immutable
Region string `json:"region"`

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

@enxebre enxebre Apr 11, 2022

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

@mkumatag mkumatag Apr 11, 2022

Choose a reason for hiding this comment

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

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

Zone string `json:"zone,omitempty"`

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

@enxebre enxebre Apr 11, 2022

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Member Author

@mkumatag mkumatag Apr 11, 2022

Choose a reason for hiding this comment

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

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)


// ServiceInstanceID is the ServiceInstance to use for control plane cloud resources.
ServiceInstanceID string `json:"serviceInstanceID,omitempty"`
Copy link
Copy Markdown
Member

@enxebre enxebre Apr 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

@mkumatag mkumatag Apr 11, 2022

Choose a reason for hiding this comment

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

Again, can't be optional.


// VPC specifies IBM Cloud PowerVS Load Balancing configuration for the control
// plane.
//
// +immutable
VPC *PowerVSVPC `json:"vpc,omitempty"`
Copy link
Copy Markdown
Member

@enxebre enxebre Apr 11, 2022

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

@mkumatag mkumatag Apr 11, 2022

Choose a reason for hiding this comment

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

can't be optional.


// KubeCloudControllerCreds is a reference to a secret containing cloud
// credentials with permissions matching the cloud controller policy. The
// secret should have exactly one key, `credentials`, whose value is an AWS
// credentials file.
//
// TODO(dan): document the "cloud controller policy"
//
// +immutable
KubeCloudControllerCreds corev1.LocalObjectReference `json:"kubeCloudControllerCreds"`
Copy link
Copy Markdown
Member

@enxebre enxebre Apr 11, 2022

Choose a reason for hiding this comment

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

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


// NodePoolManagementCreds is a reference to a secret containing cloud
// credentials with permissions matching the node pool management policy. The
// secret should have exactly one key, `credentials`, whose value is an AWS
// credentials file.
//
// TODO(dan): document the "node pool management policy"
//
// +immutable
NodePoolManagementCreds corev1.LocalObjectReference `json:"nodePoolManagementCreds"`

// ControlPlaneOperatorCreds is a reference to a secret containing cloud
// credentials with permissions matching the control-plane-operator policy.
// The secret should have exactly one key, `credentials`, whose value is
// an AWS credentials file.
//
// TODO(dan): document the "control plane operator policy"
//
// +immutable
ControlPlaneOperatorCreds corev1.LocalObjectReference `json:"controlPlaneOperatorCreds"`
}

// PowerVSVPC specifies IBM Cloud PowerVS LoadBalancer configuration for the control
// plane.
type PowerVSVPC struct {
// Name for VPC to used for all the service load balancer.
// +immutable
Name string `json:"name"`

// Region is the IBMCloud region in which VPC gets created, this VPC used for all the ingress traffic
// into the OCP cluster.
//
// +immutable
Region string `json:"region"`

// Zone is the availability zone where load balancer cloud resources are
// created.
//
// +immutable
// +optional
Zone string `json:"zone,omitempty"`

// Subnet is the subnet to use for load balancer.
//
// +optional
Subnet string `json:"subnet,omitempty"`
}

// PowerVSResourceReference is a reference to a specific IBMCloud PowerVS resource by ID, or Name.
// Only one of ID, or Name may be specified. Specifying more than one will result in
// a validation error.
type PowerVSResourceReference struct {
// ID of resource
// +optional
ID *string `json:"id,omitempty"`

// Name of resource
// +optional
Name *string `json:"name,omitempty"`
}

// AWSCloudProviderConfig specifies AWS networking configuration.
type AWSCloudProviderConfig struct {
// Subnet is the subnet to use for control plane cloud resources.
Expand Down
46 changes: 46 additions & 0 deletions api/v1alpha1/nodepool_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,52 @@ type NodePoolPlatform struct {
Agent *AgentNodePoolPlatform `json:"agent,omitempty"`

Azure *AzureNodePoolPlatform `json:"azure,omitempty"`

// PowerVS specifies the configuration used when using IBMCloud PowerVS platform.
//
// +optional
PowerVS *PowerVSNodePoolPlatform `json:"powervs,omitempty"`
}

// PowerVSNodePoolPlatform specifies the configuration of a NodePool when operating
// on IBMCloud PowerVS platform.
type PowerVSNodePoolPlatform struct {
// ServiceInstanceID is the ServiceInstance to use for control plane cloud resources.
ServiceInstanceID string `json:"serviceInstanceID"`
Copy link
Copy Markdown
Member

@enxebre enxebre Apr 11, 2022

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

@mkumatag mkumatag Apr 11, 2022

Choose a reason for hiding this comment

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

oops, this should be NodePool


// SysType used to host the instance(e.g: s922, e980, e880)
// +optional
SysType string `json:"sysType,omitempty"`

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

@enxebre enxebre Apr 11, 2022

Choose a reason for hiding this comment

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

please add open api enum validation tag

Copy link
Copy Markdown
Member

@enxebre enxebre Apr 11, 2022

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

@mkumatag mkumatag Apr 11, 2022

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Member

@enxebre enxebre Apr 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member Author

@mkumatag mkumatag Apr 11, 2022

Choose a reason for hiding this comment

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

sure, I will make enum and default.


// Processors specifies the number of processors allocated
// +optional
Copy link
Copy Markdown
Member

@enxebre enxebre Apr 11, 2022

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

@mkumatag mkumatag Apr 11, 2022

Choose a reason for hiding this comment

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

again, setting defaults in the capibm webhook

Processors string `json:"processors,omitempty"`

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

@enxebre enxebre Apr 11, 2022

Choose a reason for hiding this comment

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

let's do this a resource quantity

Copy link
Copy Markdown
Member

@enxebre enxebre Apr 11, 2022

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

@mkumatag mkumatag Apr 11, 2022

Choose a reason for hiding this comment

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

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


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

@enxebre enxebre Apr 11, 2022

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

@mkumatag mkumatag Apr 11, 2022

Choose a reason for hiding this comment

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

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


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

@enxebre enxebre Apr 11, 2022

Choose a reason for hiding this comment

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

is this enum type validation?

Copy link
Copy Markdown
Member Author

@mkumatag mkumatag Apr 11, 2022

Choose a reason for hiding this comment

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

will add the enum and default


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

@enxebre enxebre Apr 11, 2022

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

@mkumatag mkumatag Apr 11, 2022

Choose a reason for hiding this comment

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

typical copy/paste error, need to be required


// DeletePolicy for the image
//
// +optional
DeletePolicy string `json:"deletePolicy,omitempty"`
Copy link
Copy Markdown
Member

@enxebre enxebre Apr 11, 2022

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

@mkumatag mkumatag Apr 11, 2022

Choose a reason for hiding this comment

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

will add the enum and default

}

// KubevirtNodePoolPlatform specifies the configuration of a NodePool when operating
Expand Down
Loading