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

✨ ROSA: Reconcile ROSAMachinePool fields #4804

Merged
merged 5 commits into from
Mar 1, 2024
Merged

✨ ROSA: Reconcile ROSAMachinePool fields #4804

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
006a40e
reconcile rosamachinepool fields
muraee Feb 29, 2024
9f69479
add /finalizers rbac
muraee Feb 29, 2024
a481de3
fixed spec.network nil pointer dereference
muraee Feb 29, 2024
610691b
added unit test
muraee Feb 29, 2024
3defa28
add validation webhook
muraee Mar 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions config/crd/bases/infrastructure.cluster.x-k8s.io_rosamachinepools.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,41 @@ spec:
type: array
subnet:
type: string
x-kubernetes-validations:
- message: subnet is immutable
rule: self == oldSelf
taints:
description: Taints specifies the taints to apply to the nodes of
the machine pool
items:
properties:
effect:
description: The effect of the taint on pods that do not tolerate
the taint. Valid effects are NoSchedule, PreferNoSchedule
and NoExecute.
enum:
- NoSchedule
- PreferNoSchedule
- NoExecute
type: string
key:
description: The taint key to be applied to a node.
type: string
value:
description: The taint value corresponding to the taint key.
pattern: ^(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?$
type: string
required:
- effect
- key
type: object
type: array
tuningConfigs:
description: TuningConfigs specifies the names of the tuning configs
to be applied to this MachinePool. Tuning configs must already exist.
items:
type: string
type: array
version:
description: Version specifies the penshift version of the nodes associated
with this machinepool. ROSAControlPlane version is used if not set.
Expand All @@ -102,6 +137,7 @@ spec:
- message: version must be a valid semantic version
rule: self.matches('^(0|[1-9]\\d*)\\.(0|[1-9]\\d*)\\.(0|[1-9]\\d*)$')
required:
- instanceType
- nodePoolName
type: object
status:
Expand Down
12 changes: 12 additions & 0 deletions config/rbac/role.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,12 @@ rules:
- get
- list
- watch
- apiGroups:
- controlplane.cluster.x-k8s.io
resources:
- rosacontrolplanes/finalizers
verbs:
- update
- apiGroups:
- controlplane.cluster.x-k8s.io
resources:
Expand Down Expand Up @@ -409,6 +415,12 @@ rules:
- patch
- update
- watch
- apiGroups:
- infrastructure.cluster.x-k8s.io
resources:
- rosamachinepools/finalizers
verbs:
- update
- apiGroups:
- infrastructure.cluster.x-k8s.io
resources:
Expand Down
1 change: 1 addition & 0 deletions controlplane/rosa/api/v1beta2/rosacontrolplane_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ type RosaControlPlaneSpec struct { //nolint: maligned
type NetworkSpec struct {
// IP addresses block used by OpenShift while installing the cluster, for example "10.0.0.0/16".
// +kubebuilder:validation:Format=cidr
// +optional
MachineCIDR string `json:"machineCIDR,omitempty"`

// IP address block from which to assign pod IP addresses, for example `10.128.0.0/14`.
Expand Down
64 changes: 34 additions & 30 deletions controlplane/rosa/controllers/rosacontrolplane_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ func (r *ROSAControlPlaneReconciler) SetupWithManager(ctx context.Context, mgr c
// +kubebuilder:rbac:groups=cluster.x-k8s.io,resources=machinepools,verbs=get;list;watch
// +kubebuilder:rbac:groups=controlplane.cluster.x-k8s.io,resources=rosacontrolplanes,verbs=get;list;watch;update;patch;delete
// +kubebuilder:rbac:groups=controlplane.cluster.x-k8s.io,resources=rosacontrolplanes/status,verbs=get;update;patch
// +kubebuilder:rbac:groups=controlplane.cluster.x-k8s.io,resources=rosacontrolplanes/finalizers,verbs=update
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is needed when OwnerReferencesPermissionEnforcement plugin is enabled

https://sdk.operatorframework.io/docs/faqs/#after-deploying-my-operator-why-do-i-see-errors-like-is-forbidden-cannot-set-blockownerdeletion-if-an-ownerreference-refers-to-a-resource-you-cant-set-finalizers-on-


// Reconcile will reconcile RosaControlPlane Resources.
func (r *ROSAControlPlaneReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl.Result, reterr error) {
Expand Down Expand Up @@ -267,7 +268,7 @@ func (r *ROSAControlPlaneReconciler) reconcileNormal(ctx context.Context, rosaSc
billingAccount = rosaScope.ControlPlane.Spec.BillingAccount
}

spec := ocm.Spec{
ocmClusterSpec := ocm.Spec{
DryRun: ptr.To(false),
Name: rosaScope.RosaClusterName(),
Region: *rosaScope.ControlPlane.Spec.Region,
Expand All @@ -281,7 +282,6 @@ func (r *ROSAControlPlaneReconciler) reconcileNormal(ctx context.Context, rosaSc

SubnetIds: rosaScope.ControlPlane.Spec.Subnets,
AvailabilityZones: rosaScope.ControlPlane.Spec.AvailabilityZones,
NetworkType: rosaScope.ControlPlane.Spec.Network.NetworkType,
IsSTS: true,
RoleARN: *rosaScope.ControlPlane.Spec.InstallerRoleARN,
SupportRoleARN: *rosaScope.ControlPlane.Spec.SupportRoleARN,
Expand All @@ -296,44 +296,48 @@ func (r *ROSAControlPlaneReconciler) reconcileNormal(ctx context.Context, rosaSc
AWSCreator: creator,
}

_, machineCIDR, err := net.ParseCIDR(rosaScope.ControlPlane.Spec.Network.MachineCIDR)
if err == nil {
spec.MachineCIDR = *machineCIDR
} else {
// TODO: expose in status
rosaScope.Error(err, "rosacontrolplane.spec.network.machineCIDR invalid", rosaScope.ControlPlane.Spec.Network.MachineCIDR)
return ctrl.Result{}, nil
}
if networkSpec := rosaScope.ControlPlane.Spec.Network; networkSpec != nil {
if networkSpec.MachineCIDR != "" {
_, machineCIDR, err := net.ParseCIDR(networkSpec.MachineCIDR)
if err != nil {
// TODO: expose in status
rosaScope.Error(err, "rosacontrolplane.spec.network.machineCIDR invalid", networkSpec.MachineCIDR)
return ctrl.Result{}, nil
Copy link
Member

Choose a reason for hiding this comment

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

Could we move this to a validation webhook, and just return the error here (backoff loop) or set a condition?

Copy link
Contributor

Choose a reason for hiding this comment

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

Returning an error when the data provided by the user is incorrect would only lead to infinite retry by the controller and not result in a good outcome. It seems universally correct to expose states like this in status so that the user knows that they need to update the object's .spec and so that the controller does not do extra reconciliation loops that cannot possibly result in a better outcome.

}
ocmClusterSpec.MachineCIDR = *machineCIDR
}

if rosaScope.ControlPlane.Spec.Network.PodCIDR != "" {
_, podCIDR, err := net.ParseCIDR(rosaScope.ControlPlane.Spec.Network.PodCIDR)
if err == nil {
spec.PodCIDR = *podCIDR
} else {
// TODO: expose in status.
rosaScope.Error(err, "rosacontrolplane.spec.network.podCIDR invalid", rosaScope.ControlPlane.Spec.Network.PodCIDR)
return ctrl.Result{}, nil
if networkSpec.PodCIDR != "" {
_, podCIDR, err := net.ParseCIDR(networkSpec.PodCIDR)
if err != nil {
// TODO: expose in status.
rosaScope.Error(err, "rosacontrolplane.spec.network.podCIDR invalid", networkSpec.PodCIDR)
return ctrl.Result{}, nil
}
ocmClusterSpec.PodCIDR = *podCIDR
}
}

if rosaScope.ControlPlane.Spec.Network.ServiceCIDR != "" {
_, serviceCIDR, err := net.ParseCIDR(rosaScope.ControlPlane.Spec.Network.ServiceCIDR)
if err == nil {
spec.ServiceCIDR = *serviceCIDR
} else {
// TODO: expose in status.
rosaScope.Error(err, "rosacontrolplane.spec.network.serviceCIDR invalid", rosaScope.ControlPlane.Spec.Network.ServiceCIDR)
return ctrl.Result{}, nil
if networkSpec.ServiceCIDR != "" {
_, serviceCIDR, err := net.ParseCIDR(networkSpec.ServiceCIDR)
if err != nil {
// TODO: expose in status.
rosaScope.Error(err, "rosacontrolplane.spec.network.serviceCIDR invalid", networkSpec.ServiceCIDR)
return ctrl.Result{}, nil
}
ocmClusterSpec.ServiceCIDR = *serviceCIDR
}

ocmClusterSpec.HostPrefix = networkSpec.HostPrefix
ocmClusterSpec.NetworkType = networkSpec.NetworkType
}

// Set autoscale replica
if rosaScope.ControlPlane.Spec.Autoscaling != nil {
spec.MaxReplicas = rosaScope.ControlPlane.Spec.Autoscaling.MaxReplicas
spec.MinReplicas = rosaScope.ControlPlane.Spec.Autoscaling.MinReplicas
ocmClusterSpec.MaxReplicas = rosaScope.ControlPlane.Spec.Autoscaling.MaxReplicas
ocmClusterSpec.MinReplicas = rosaScope.ControlPlane.Spec.Autoscaling.MinReplicas
}

cluster, err = ocmClient.CreateCluster(spec)
cluster, err = ocmClient.CreateCluster(ocmClusterSpec)
if err != nil {
// TODO: need to expose in status, as likely the spec is invalid
return ctrl.Result{}, fmt.Errorf("failed to create OCM cluster: %w", err)
Expand Down
38 changes: 33 additions & 5 deletions exp/api/v1beta2/rosamachinepool_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package v1beta2

import (
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
Expand Down Expand Up @@ -45,37 +46,64 @@ type RosaMachinePoolSpec struct {
// +optional
AvailabilityZone string `json:"availabilityZone,omitempty"`

// +kubebuilder:validation:XValidation:rule="self == oldSelf", message="subnet is immutable"
//
// +immutable
// +optional
Subnet string `json:"subnet,omitempty"`

// Labels specifies labels for the Kubernetes node objects
// +optional
Labels map[string]string `json:"labels,omitempty"`

// Taints specifies the taints to apply to the nodes of the machine pool
// +optional
Taints []RosaTaint `json:"taints,omitempty"`

// AutoRepair specifies whether health checks should be enabled for machines
// in the NodePool. The default is false.
// +optional
// +kubebuilder:default=false
AutoRepair bool `json:"autoRepair,omitempty"`

// +kubebuilder:validation:Required
//
// InstanceType specifies the AWS instance type
InstanceType string `json:"instanceType,omitempty"`
InstanceType string `json:"instanceType"`

// Autoscaling specifies auto scaling behaviour for this MachinePool.
// required if Replicas is not configured
// +optional
Autoscaling *RosaMachinePoolAutoScaling `json:"autoscaling,omitempty"`

// TODO(alberto): Enable and propagate this API input.
// Taints []*Taint `json:"taints,omitempty"`
// TuningConfigs []string `json:"tuningConfigs,omitempty"`
// Version *Version `json:"version,omitempty"`
// TuningConfigs specifies the names of the tuning configs to be applied to this MachinePool.
// Tuning configs must already exist.
// +optional
TuningConfigs []string `json:"tuningConfigs,omitempty"`

// ProviderIDList contain a ProviderID for each machine instance that's currently managed by this machine pool.
// +optional
ProviderIDList []string `json:"providerIDList,omitempty"`
}

type RosaTaint struct {
// +kubebuilder:validation:Required
//
// The taint key to be applied to a node.
Key string `json:"key"`
Copy link
Member

Choose a reason for hiding this comment

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

We usually have the +kubebuilder tags after the comment, like:

Suggested change
// +kubebuilder:validation:Required
//
// The taint key to be applied to a node.
Key string `json:"key"`
// The taint key to be applied to a node.
//
// +kubebuilder:validation:Required
Key string `json:"key"`

// +kubebuilder:validation:Pattern:=`^(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?$`
//
// The taint value corresponding to the taint key.
// +optional
Value string `json:"value,omitempty"`
// +kubebuilder:validation:Required
// +kubebuilder:validation:Enum=NoSchedule;PreferNoSchedule;NoExecute
//
// The effect of the taint on pods that do not tolerate the taint.
// Valid effects are NoSchedule, PreferNoSchedule and NoExecute.
Effect corev1.TaintEffect `json:"effect"`
}

// RosaMachinePoolAutoScaling specifies scaling options.
type RosaMachinePoolAutoScaling struct {
// +kubebuilder:validation:Minimum=1
Expand Down
25 changes: 25 additions & 0 deletions exp/api/v1beta2/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading