Repository navigation
feat: add SKE managed cluster and node pool APIs - #36
Rana-Ashar wants to merge 3 commits into
Conversation
| // region is the STACKIT region in which infrastructure is created. | ||
| // +required | ||
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:Pattern=`^[a-z]{2}[0-9]{2}$` |
There was a problem hiding this comment.
is this an official pattern you got from our stackit api docs?
There was a problem hiding this comment.
This pattern is not coming from the SKE api docs. I reused the existing validation from StackitClusterSpec.Region.
| // providerIDList are the identification IDs of machine instances provided by | ||
| // the provider. This field must match the provider IDs as seen on the Node | ||
| // objects corresponding to a MachinePool's machine instances. | ||
| // +optional | ||
| // +listType=atomic | ||
| // +kubebuilder:validation:MaxItems=10000 | ||
| // +kubebuilder:validation:items:MinLength=1 | ||
| // +kubebuilder:validation:items:MaxLength=512 | ||
| ProviderIDList []string `json:"providerIDList,omitempty"` |
There was a problem hiding this comment.
this does not belong into the spec that is something that we retrieve from the stackit api and store inside the status
There was a problem hiding this comment.
or where did you get the idea from putting this inside the spec?
There was a problem hiding this comment.
This placement comes directly from the CAPI v1beta2 InfraMachinePool contract.
CAPI requires an InfraMachinePool to expose the instance provider IDs as spec.providerIDList. Core CAPI reads that field to match the MachinePool instances with Kubernetes Nodes and to track deletion.
So although this is controller-populated information rather than user desired state, it intentionally lives in spec for CAPI compatibility. I think we should keep it here for that reason.
https://cluster-api.sigs.k8s.io/developer/providers/contracts/infra-machinepool.html
| // ManagedClusterDesiredState holds the cluster-level fields needed by the SKE | ||
| // service layer. | ||
| type ManagedClusterDesiredState struct { | ||
| ProjectID string | ||
| Region string | ||
| ClusterName string | ||
| KubernetesVersion string | ||
| NetworkID string | ||
| Labels map[string]string | ||
| } | ||
|
|
||
| // ManagedNodePoolDesiredState holds the node-pool-level fields needed by the | ||
| // SKE service layer. | ||
| type ManagedNodePoolDesiredState struct { | ||
| Name string | ||
| AvailabilityZones []string | ||
| MachineType string | ||
| ImageName string | ||
| ImageVersion string | ||
| Minimum int32 | ||
| Maximum int32 | ||
| VolumeSize int32 | ||
| VolumeType string | ||
| AllowSystemComponents *bool | ||
| Labels map[string]string | ||
| Taints []ManagedNodePoolTaint | ||
| MaxSurge *int32 | ||
| MaxUnavailable *int32 | ||
| } |
There was a problem hiding this comment.
the desired state should be reflected by the CRs themselves why do we need these "shadow" structs that are essentially just code duplication?
Or was this added as prematurely for the actually API abstraction/implementation?
| // TODO(ske): This fallback version is time-sensitive as SKE rotates supported | ||
| // Flatcar images; replace with a durable configuration or provider-options | ||
| // selection strategy. | ||
| defaultNodePoolImageVersion = "4593.2.5-containerd2.1.9" |
There was a problem hiding this comment.
cannot be just leave this empty and SKE will automatically select the latest stable image version?
how does the terraform implementation deal with this?
| updateTriggered := false | ||
| if needsUpdate && state == skev2.CLUSTERSTATUSSTATE_STATE_HEALTHY { | ||
| log.Info("Updating SKE cluster node pools", "clusterName", s.scope.ClusterName(), "desiredPoolCount", len(desiredPools)) | ||
| payload := s.buildClusterPayload(desiredPools) | ||
| if _, updateErr := s.skeClient. | ||
| CreateOrUpdateCluster(ctx, s.scope.ProjectID(), s.scope.Region(), s.scope.ClusterName()). | ||
| CreateOrUpdateClusterPayload(payload). | ||
| Execute(); updateErr != nil { | ||
| updateErr = cloud.ClassifySDKError("update SKE cluster", updateErr) | ||
| if cloud.IsConflict(updateErr) { | ||
| log.V(1).Info("SKE cluster update already in progress", "clusterName", s.scope.ClusterName()) | ||
| updateTriggered = true | ||
| } else { | ||
| log.Error(updateErr, "Failed to update SKE cluster", "clusterName", s.scope.ClusterName()) | ||
| reason := "UpdateFailed" | ||
| if cloud.IsInvalidInput(updateErr) { | ||
| reason = ReasonInvalidNodePoolConfiguration | ||
| } | ||
| s.scope.SetNotReady(reason, updateErr.Error()) | ||
| s.projectNodePoolStatuses(cluster, observedByName, state, stateOK, false, reason, updateErr.Error()) | ||
| return ctrl.Result{}, updateErr | ||
| } | ||
| } else { | ||
| updateTriggered = true | ||
| } | ||
| } |
There was a problem hiding this comment.
this is way too much nesting
and why is the updateTriggered set in the else branch?
isn't the triggering happening in the if branch?
I guess the variable isn't precise enough it should probably be something like activeUpdate or isUpgradeActive
There was a problem hiding this comment.
for sake of maintainability i would put everything that is not strictly the reconcile and delete method into two separate files: cluster.go and nodepool.go
This PR adds the initial SKE managed cluster and node pool APIs to CAPSTK.
It introduces StackitManagedCluster and StackitManagedNodePool specifications, Cluster API v1beta2 contract fields, generated CRDs and RBAC, sample manifests, initial controllers, and tests.