diff --git a/api/v1beta1/common_types.go b/api/v1beta1/common_types.go index db826ba0c1f6..947ce108c15e 100644 --- a/api/v1beta1/common_types.go +++ b/api/v1beta1/common_types.go @@ -53,6 +53,10 @@ const ( // will not be completed until the annotation is removed and all MachineDeployments are upgraded. ClusterTopologyDeferUpgradeAnnotation = "topology.cluster.x-k8s.io/defer-upgrade" + // ClusterTopologyUpgradeConcurrencyAnnotation can be set as top-level annotation on the Cluster object of + // a classy Cluster to define the maximum concurrency while upgrading MachineDeployments. + ClusterTopologyUpgradeConcurrencyAnnotation = "topology.cluster.x-k8s.io/upgrade-concurrency" + // ClusterTopologyUnsafeUpdateClassNameAnnotation can be used to disable the webhook check on // update that disallows a pre-existing Cluster to be populated with Topology information and Class. ClusterTopologyUnsafeUpdateClassNameAnnotation = "unsafe.topology.cluster.x-k8s.io/disable-update-class-name-check" diff --git a/docs/book/src/reference/labels_and_annotations.md b/docs/book/src/reference/labels_and_annotations.md index cdbb764b3f7c..615703802847 100644 --- a/docs/book/src/reference/labels_and_annotations.md +++ b/docs/book/src/reference/labels_and_annotations.md @@ -40,6 +40,7 @@ | topology.cluster.x-k8s.io/defer-upgrade | It can be used to defer the Kubernetes upgrade of a single MachineDeployment topology. If the annotation is set on a MachineDeployment topology in Cluster.spec.topology.workers, the Kubernetes upgrade for this MachineDeployment topology is deferred. It doesn't affect other MachineDeployment topologies. | | topology.cluster.x-k8s.io/dry-run | It is an annotation that gets set on objects by the topology controller only during a server side dry run apply operation. It is used for validating update webhooks for objects which get updated by template rotation (e.g. InfrastructureMachineTemplate). When the annotation is set and the admission request is a dry run, the webhook should deny validation due to immutability. By that the request will succeed (without any changes to the actual object because it is a dry run) and the topology controller will receive the resulting object. | | topology.cluster.x-k8s.io/hold-upgrade-sequence | It can be used to hold the entire MachineDeployment upgrade sequence. If the annotation is set on a MachineDeployment topology in Cluster.spec.topology.workers, the Kubernetes upgrade for this MachineDeployment topology and all subsequent ones is deferred. | +| topology.cluster.x-k8s.io/upgrade-concurrency | It can be used to configure the maximum concurrency while upgrading MachineDeployments of a classy Cluster. It is set as a top level annotation on the Cluster object. The value should be >= 1. If unspecified the upgrade concurrency will default to 1. | | machine.cluster.x-k8s.io/certificates-expiry | It captures the expiry date of the machine certificates in RFC3339 format. It is used to trigger rollout of control plane machines before certificates expire. It can be set on BootstrapConfig and Machine objects. The value set on Machine object takes precedence. The annotation is only used by control plane machines. | | machine.cluster.x-k8s.io/exclude-node-draining | It explicitly skips node draining if set. | | machine.cluster.x-k8s.io/exclude-wait-for-node-volume-detach | It explicitly skips the waiting for node volume detaching if set. | diff --git a/internal/controllers/topology/cluster/desired_state.go b/internal/controllers/topology/cluster/desired_state.go index acc7fe46f621..aae24f8c42ce 100644 --- a/internal/controllers/topology/cluster/desired_state.go +++ b/internal/controllers/topology/cluster/desired_state.go @@ -841,14 +841,6 @@ func computeMachineDeploymentVersion(s *scope.Scope, machineDeploymentTopology c return currentVersion, nil } - // At this point the control plane is stable (not scaling, not upgrading, not being upgraded). - // Checking to see if the machine deployments are also stable. - // If any of the MachineDeployments is rolling out, do not upgrade the machine deployment yet. - if s.Current.MachineDeployments.IsAnyRollingOut() { - s.UpgradeTracker.MachineDeployments.MarkPendingUpgrade(currentMDState.Object.Name) - return currentVersion, nil - } - // Control plane and machine deployments are stable. // Ready to pick up the topology version. s.UpgradeTracker.MachineDeployments.MarkRollingOut(currentMDState.Object.Name) diff --git a/internal/controllers/topology/cluster/desired_state_test.go b/internal/controllers/topology/cluster/desired_state_test.go index e33da3c3b718..0e81f0ed25a6 100644 --- a/internal/controllers/topology/cluster/desired_state_test.go +++ b/internal/controllers/topology/cluster/desired_state_test.go @@ -1635,23 +1635,25 @@ func TestComputeMachineDeployment(t *testing.T) { }). Build() - machineDeploymentStable := builder.MachineDeployment("test-namespace", "md-1"). + machineDeploymentStable := builder.MachineDeployment("test-namespace", "md-stable"). WithGeneration(1). WithReplicas(2). WithStatus(clusterv1.MachineDeploymentStatus{ ObservedGeneration: 2, Replicas: 2, + ReadyReplicas: 2, UpdatedReplicas: 2, AvailableReplicas: 2, }). Build() - machineDeploymentRollingOut := builder.MachineDeployment("test-namespace", "md-1"). + machineDeploymentRollingOut := builder.MachineDeployment("test-namespace", "md-rolling"). WithGeneration(1). WithReplicas(2). WithStatus(clusterv1.MachineDeploymentStatus{ ObservedGeneration: 2, Replicas: 1, + ReadyReplicas: 1, UpdatedReplicas: 1, AvailableReplicas: 1, }). @@ -1669,28 +1671,46 @@ func TestComputeMachineDeployment(t *testing.T) { name string machineDeploymentsState scope.MachineDeploymentsStateMap currentMDVersion *string + upgradeConcurrency string topologyVersion string expectedVersion string }{ { name: "use cluster.spec.topology.version if creating a new machine deployment", machineDeploymentsState: nil, + upgradeConcurrency: "1", currentMDVersion: nil, topologyVersion: "v1.2.3", expectedVersion: "v1.2.3", }, { - name: "use machine deployment's spec.template.spec.version if one of the machine deployments is rolling out", + name: "use machine deployment's spec.template.spec.version if one of the machine deployments is rolling out, concurrency limit reached", machineDeploymentsState: machineDeploymentsStateRollingOut, + upgradeConcurrency: "1", currentMDVersion: pointer.String("v1.2.2"), topologyVersion: "v1.2.3", expectedVersion: "v1.2.2", }, + { + name: "use cluster.spec.topology.version if one of the machine deployments is rolling out, concurrency limit not reached", + machineDeploymentsState: machineDeploymentsStateRollingOut, + upgradeConcurrency: "2", + currentMDVersion: pointer.String("v1.2.2"), + topologyVersion: "v1.2.3", + expectedVersion: "v1.2.3", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - s := scope.New(cluster) + + testCluster := cluster.DeepCopy() + if testCluster.Annotations == nil { + testCluster.Annotations = map[string]string{} + } + testCluster.Annotations[clusterv1.ClusterTopologyUpgradeConcurrencyAnnotation] = tt.upgradeConcurrency + + s := scope.New(testCluster) s.Blueprint = blueprint s.Blueprint.Topology.Version = tt.topologyVersion s.Blueprint.Topology.ControlPlane = clusterv1.ControlPlaneTopology{ @@ -1709,6 +1729,7 @@ func TestComputeMachineDeployment(t *testing.T) { WithStatus(clusterv1.MachineDeploymentStatus{ ObservedGeneration: 2, Replicas: 2, + ReadyReplicas: 2, UpdatedReplicas: 2, AvailableReplicas: 2, }). @@ -1722,6 +1743,7 @@ func TestComputeMachineDeployment(t *testing.T) { s.Current.ControlPlane = &scope.ControlPlaneState{ Object: controlPlaneStable123, } + s.UpgradeTracker.MachineDeployments.MarkRollingOut(s.Current.MachineDeployments.RollingOut()...) desiredControlPlaneState := &scope.ControlPlaneState{ Object: controlPlaneStable123, } @@ -1828,38 +1850,47 @@ func TestComputeMachineDeploymentVersion(t *testing.T) { // // A machine deployment is considered upgrading if any of the above conditions // is false. - machineDeploymentStable := builder.MachineDeployment("test-namespace", "md-1"). - WithGeneration(1). - WithReplicas(2). - WithStatus(clusterv1.MachineDeploymentStatus{ - ObservedGeneration: 2, - Replicas: 2, - UpdatedReplicas: 2, - AvailableReplicas: 2, - ReadyReplicas: 2, - UnavailableReplicas: 0, - }). - Build() - machineDeploymentRollingOut := builder.MachineDeployment("test-namespace", "md-2"). - WithGeneration(1). - WithReplicas(2). - WithStatus(clusterv1.MachineDeploymentStatus{ - ObservedGeneration: 2, - Replicas: 1, - UpdatedReplicas: 1, - AvailableReplicas: 1, - ReadyReplicas: 1, - UnavailableReplicas: 1, - }). - Build() + stableMachineDeployment := func(ns, name string) *clusterv1.MachineDeployment { + return builder.MachineDeployment(ns, name). + WithGeneration(1). + WithReplicas(2). + WithStatus(clusterv1.MachineDeploymentStatus{ + ObservedGeneration: 2, + Replicas: 2, + UpdatedReplicas: 2, + AvailableReplicas: 2, + ReadyReplicas: 2, + UnavailableReplicas: 0, + }). + Build() + } - machineDeploymentsStateStable := scope.MachineDeploymentsStateMap{ - "md1": &scope.MachineDeploymentState{Object: machineDeploymentStable}, - "md2": &scope.MachineDeploymentState{Object: machineDeploymentStable}, + rollingMachineDeployment := func(ns, name string) *clusterv1.MachineDeployment { + return builder.MachineDeployment(ns, name). + WithGeneration(1). + WithReplicas(2). + WithStatus(clusterv1.MachineDeploymentStatus{ + ObservedGeneration: 2, + Replicas: 1, + UpdatedReplicas: 1, + AvailableReplicas: 1, + ReadyReplicas: 1, + UnavailableReplicas: 1, + }). + Build() + } + + twoMachineDeploymentsStateStable := scope.MachineDeploymentsStateMap{ + "md1": &scope.MachineDeploymentState{Object: stableMachineDeployment("test1", "md1")}, + "md2": &scope.MachineDeploymentState{Object: stableMachineDeployment("test1", "md2")}, + } + oneStableOneRollingMachineDeploymentState := scope.MachineDeploymentsStateMap{ + "md1": &scope.MachineDeploymentState{Object: stableMachineDeployment("test1", "md1")}, + "md2": &scope.MachineDeploymentState{Object: rollingMachineDeployment("test1", "md2")}, } - machineDeploymentsStateRollingOut := scope.MachineDeploymentsStateMap{ - "md1": &scope.MachineDeploymentState{Object: machineDeploymentStable}, - "md2": &scope.MachineDeploymentState{Object: machineDeploymentRollingOut}, + twoRollingMachineDeploymentState := scope.MachineDeploymentsStateMap{ + "md1": &scope.MachineDeploymentState{Object: rollingMachineDeployment("test1", "md1")}, + "md2": &scope.MachineDeploymentState{Object: rollingMachineDeployment("test1", "md2")}, } tests := []struct { @@ -1867,6 +1898,7 @@ func TestComputeMachineDeploymentVersion(t *testing.T) { machineDeploymentTopology clusterv1.MachineDeploymentTopology currentMachineDeploymentState *scope.MachineDeploymentState machineDeploymentsStateMap scope.MachineDeploymentsStateMap + upgradeConcurrency int currentControlPlane *unstructured.Unstructured desiredControlPlane *unstructured.Unstructured topologyVersion string @@ -1889,16 +1921,7 @@ func TestComputeMachineDeploymentVersion(t *testing.T) { }, }, currentMachineDeploymentState: &scope.MachineDeploymentState{Object: builder.MachineDeployment("test1", "md-current").WithVersion("v1.2.2").Build()}, - machineDeploymentsStateMap: machineDeploymentsStateStable, - currentControlPlane: controlPlaneStable123, - desiredControlPlane: controlPlaneDesired, - topologyVersion: "v1.2.3", - expectedVersion: "v1.2.2", - }, - { - name: "should return machine deployment's spec.template.spec.version if any one of the machine deployments is rolling out", - currentMachineDeploymentState: &scope.MachineDeploymentState{Object: builder.MachineDeployment("test1", "md-current").WithVersion("v1.2.2").Build()}, - machineDeploymentsStateMap: machineDeploymentsStateRollingOut, + machineDeploymentsStateMap: twoMachineDeploymentsStateStable, currentControlPlane: controlPlaneStable123, desiredControlPlane: controlPlaneDesired, topologyVersion: "v1.2.3", @@ -1908,7 +1931,7 @@ func TestComputeMachineDeploymentVersion(t *testing.T) { // Control plane is considered upgrading if the control plane's spec.version and status.version is not equal. name: "should return machine deployment's spec.template.spec.version if control plane is upgrading", currentMachineDeploymentState: &scope.MachineDeploymentState{Object: builder.MachineDeployment("test1", "md-current").WithVersion("v1.2.2").Build()}, - machineDeploymentsStateMap: machineDeploymentsStateStable, + machineDeploymentsStateMap: twoMachineDeploymentsStateStable, currentControlPlane: controlPlaneUpgrading, topologyVersion: "v1.2.3", expectedVersion: "v1.2.2", @@ -1917,7 +1940,7 @@ func TestComputeMachineDeploymentVersion(t *testing.T) { // Control plane is considered ready to upgrade if spec.version of current and desired control planes are not equal. name: "should return machine deployment's spec.template.spec.version if control plane is ready to upgrade", currentMachineDeploymentState: &scope.MachineDeploymentState{Object: builder.MachineDeployment("test1", "md-current").WithVersion("v1.2.2").Build()}, - machineDeploymentsStateMap: machineDeploymentsStateStable, + machineDeploymentsStateMap: twoMachineDeploymentsStateStable, currentControlPlane: controlPlaneStable122, desiredControlPlane: controlPlaneDesired, topologyVersion: "v1.2.3", @@ -1927,7 +1950,7 @@ func TestComputeMachineDeploymentVersion(t *testing.T) { // Control plane is considered scaling if its spec.replicas is not equal to any of status.replicas, status.readyReplicas or status.updatedReplicas. name: "should return machine deployment's spec.template.spec.version if control plane is scaling", currentMachineDeploymentState: &scope.MachineDeploymentState{Object: builder.MachineDeployment("test1", "md-current").WithVersion("v1.2.2").Build()}, - machineDeploymentsStateMap: machineDeploymentsStateStable, + machineDeploymentsStateMap: twoMachineDeploymentsStateStable, currentControlPlane: controlPlaneScaling, topologyVersion: "v1.2.3", expectedVersion: "v1.2.2", @@ -1935,12 +1958,32 @@ func TestComputeMachineDeploymentVersion(t *testing.T) { { name: "should return cluster.spec.topology.version if the control plane is not upgrading, not scaling, not ready to upgrade and none of the machine deployments are rolling out", currentMachineDeploymentState: &scope.MachineDeploymentState{Object: builder.MachineDeployment("test1", "md-current").WithVersion("v1.2.2").Build()}, - machineDeploymentsStateMap: machineDeploymentsStateStable, + machineDeploymentsStateMap: twoMachineDeploymentsStateStable, currentControlPlane: controlPlaneStable123, desiredControlPlane: controlPlaneDesired, topologyVersion: "v1.2.3", expectedVersion: "v1.2.3", }, + { + name: "should return cluster.spec.topology.version if control plane is stable, other machine deployments are rolling out, concurrency limit not reached", + currentMachineDeploymentState: &scope.MachineDeploymentState{Object: builder.MachineDeployment("test1", "md-current").WithVersion("v1.2.2").Build()}, + machineDeploymentsStateMap: oneStableOneRollingMachineDeploymentState, + upgradeConcurrency: 2, + currentControlPlane: controlPlaneStable123, + desiredControlPlane: controlPlaneDesired, + topologyVersion: "v1.2.3", + expectedVersion: "v1.2.3", + }, + { + name: "should return machine deployment's spec.template.spec.version if control plane is stable, other machine deployments are rolling out, concurrency limit reached", + currentMachineDeploymentState: &scope.MachineDeploymentState{Object: builder.MachineDeployment("test1", "md-current").WithVersion("v1.2.2").Build()}, + machineDeploymentsStateMap: twoRollingMachineDeploymentState, + upgradeConcurrency: 2, + currentControlPlane: controlPlaneStable123, + desiredControlPlane: controlPlaneDesired, + topologyVersion: "v1.2.3", + expectedVersion: "v1.2.2", + }, } for _, tt := range tests { @@ -1959,9 +2002,10 @@ func TestComputeMachineDeploymentVersion(t *testing.T) { ControlPlane: &scope.ControlPlaneState{Object: tt.currentControlPlane}, MachineDeployments: tt.machineDeploymentsStateMap, }, - UpgradeTracker: scope.NewUpgradeTracker(), + UpgradeTracker: scope.NewUpgradeTracker(scope.MaxMDUpgradeConcurrency(tt.upgradeConcurrency)), } desiredControlPlaneState := &scope.ControlPlaneState{Object: tt.desiredControlPlane} + s.UpgradeTracker.MachineDeployments.MarkRollingOut(s.Current.MachineDeployments.RollingOut()...) version, err := computeMachineDeploymentVersion(s, tt.machineDeploymentTopology, desiredControlPlaneState, tt.currentMachineDeploymentState) g.Expect(err).NotTo(HaveOccurred()) g.Expect(version).To(Equal(tt.expectedVersion)) diff --git a/internal/controllers/topology/cluster/scope/scope.go b/internal/controllers/topology/cluster/scope/scope.go index 413470dc8173..272bfe69c27c 100644 --- a/internal/controllers/topology/cluster/scope/scope.go +++ b/internal/controllers/topology/cluster/scope/scope.go @@ -17,6 +17,8 @@ limitations under the License. package scope import ( + "strconv" + clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" ) @@ -45,12 +47,21 @@ func New(cluster *clusterv1.Cluster) *Scope { // enforce TypeMeta values in the Cluster object so we can assume it is always set during reconciliation. cluster.APIVersion = clusterv1.GroupVersion.String() cluster.Kind = "Cluster" + + // Determine the maximum upgrade concurrency from the annotation on the cluster. + maxMDUpgradeConcurrency := 1 + if concurrency, ok := cluster.Annotations[clusterv1.ClusterTopologyUpgradeConcurrencyAnnotation]; ok { + // The error can be ignored because the webhook ensures that the value is a positive integer. + maxMDUpgradeConcurrency, _ = strconv.Atoi(concurrency) + } return &Scope{ Blueprint: &ClusterBlueprint{}, Current: &ClusterState{ Cluster: cluster, }, - UpgradeTracker: NewUpgradeTracker(), + UpgradeTracker: NewUpgradeTracker( + MaxMDUpgradeConcurrency(maxMDUpgradeConcurrency), + ), HookResponseTracker: NewHookResponseTracker(), } } diff --git a/internal/controllers/topology/cluster/scope/scope_test.go b/internal/controllers/topology/cluster/scope/scope_test.go new file mode 100644 index 000000000000..2dd3a00cb85a --- /dev/null +++ b/internal/controllers/topology/cluster/scope/scope_test.go @@ -0,0 +1,59 @@ +/* +Copyright 2023 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package scope + +import ( + "testing" + + . "github.com/onsi/gomega" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" +) + +func TestNew(t *testing.T) { + t.Run("should set the right maxMachineDeploymentUpgradeConcurrency in UpgradeTracker from the cluster annotation", func(t *testing.T) { + tests := []struct { + name string + cluster *clusterv1.Cluster + want int + }{ + { + name: "if the annotation is not present the concurrency should default to 1", + cluster: &clusterv1.Cluster{}, + want: 1, + }, + { + name: "if the cluster has the annotation it should set the upgrade concurrency value", + cluster: &clusterv1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + clusterv1.ClusterTopologyUpgradeConcurrencyAnnotation: "2", + }, + }, + }, + want: 2, + }, + } + + for _, tt := range tests { + g := NewWithT(t) + s := New(tt.cluster) + g.Expect(s.UpgradeTracker.MachineDeployments.maxMachineDeploymentUpgradeConcurrency).To(Equal(tt.want)) + } + }) +} diff --git a/internal/controllers/topology/cluster/scope/upgradetracker.go b/internal/controllers/topology/cluster/scope/upgradetracker.go index 568d1cfaa583..f7fad8db0e30 100644 --- a/internal/controllers/topology/cluster/scope/upgradetracker.go +++ b/internal/controllers/topology/cluster/scope/upgradetracker.go @@ -18,8 +18,6 @@ package scope import "k8s.io/apimachinery/pkg/util/sets" -const maxMachineDeploymentUpgradeConcurrency = 1 - // UpgradeTracker is a helper to capture the upgrade status and make upgrade decisions. type UpgradeTracker struct { ControlPlane ControlPlaneUpgradeTracker @@ -46,19 +44,48 @@ type ControlPlaneUpgradeTracker struct { // MachineDeploymentUpgradeTracker holds the current upgrade status and makes upgrade // decisions for MachineDeployments. type MachineDeploymentUpgradeTracker struct { - pendingNames sets.Set[string] - deferredNames sets.Set[string] - rollingOutNames sets.Set[string] - holdUpgrades bool + pendingNames sets.Set[string] + deferredNames sets.Set[string] + rollingOutNames sets.Set[string] + holdUpgrades bool + maxMachineDeploymentUpgradeConcurrency int +} + +// UpgradeTrackerOptions contains the options for NewUpgradeTracker. +type UpgradeTrackerOptions struct { + maxMDUpgradeConcurrency int +} + +// UpgradeTrackerOption returns an option for the NewUpgradeTracker function. +type UpgradeTrackerOption interface { + ApplyToUpgradeTracker(options *UpgradeTrackerOptions) +} + +// MaxMDUpgradeConcurrency sets the upper limit for the number of Machine Deployments that can upgrade +// concurrently. +type MaxMDUpgradeConcurrency int + +// ApplyToUpgradeTracker applies the given UpgradeTrackerOptions. +func (m MaxMDUpgradeConcurrency) ApplyToUpgradeTracker(options *UpgradeTrackerOptions) { + options.maxMDUpgradeConcurrency = int(m) } // NewUpgradeTracker returns an upgrade tracker with empty tracking information. -func NewUpgradeTracker() *UpgradeTracker { +func NewUpgradeTracker(opts ...UpgradeTrackerOption) *UpgradeTracker { + options := &UpgradeTrackerOptions{} + for _, o := range opts { + o.ApplyToUpgradeTracker(options) + } + if options.maxMDUpgradeConcurrency < 1 { + // The concurrency should be at least 1. + options.maxMDUpgradeConcurrency = 1 + } return &UpgradeTracker{ MachineDeployments: MachineDeploymentUpgradeTracker{ - pendingNames: sets.Set[string]{}, - deferredNames: sets.Set[string]{}, - rollingOutNames: sets.Set[string]{}, + pendingNames: sets.Set[string]{}, + deferredNames: sets.Set[string]{}, + rollingOutNames: sets.Set[string]{}, + maxMachineDeploymentUpgradeConcurrency: options.maxMDUpgradeConcurrency, }, } } @@ -96,7 +123,7 @@ func (m *MachineDeploymentUpgradeTracker) AllowUpgrade() bool { if m.holdUpgrades { return false } - return m.rollingOutNames.Len() < maxMachineDeploymentUpgradeConcurrency + return m.rollingOutNames.Len() < m.maxMachineDeploymentUpgradeConcurrency } // MarkPendingUpgrade marks a machine deployment as in need of an upgrade. diff --git a/internal/controllers/topology/cluster/scope/upgradetracker_test.go b/internal/controllers/topology/cluster/scope/upgradetracker_test.go new file mode 100644 index 000000000000..49dfcb490c65 --- /dev/null +++ b/internal/controllers/topology/cluster/scope/upgradetracker_test.go @@ -0,0 +1,55 @@ +/* +Copyright 2023 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package scope + +import ( + "testing" + + . "github.com/onsi/gomega" +) + +func TestNewUpgradeTracker(t *testing.T) { + t.Run("should set the correct value for maxMachineDeploymentUpgradeConcurrency", func(t *testing.T) { + tests := []struct { + name string + options []UpgradeTrackerOption + want int + }{ + { + name: "should set a default value of 1 if not concurrency is not specified", + options: nil, + want: 1, + }, + { + name: "should set the value of 1 if given concurrency is less than 1", + options: []UpgradeTrackerOption{MaxMDUpgradeConcurrency(0)}, + want: 1, + }, + { + name: "should set the value to the given concurrency if the value is greater than 0", + options: []UpgradeTrackerOption{MaxMDUpgradeConcurrency(2)}, + want: 2, + }, + } + + for _, tt := range tests { + g := NewWithT(t) + got := NewUpgradeTracker(tt.options...) + g.Expect(got.MachineDeployments.maxMachineDeploymentUpgradeConcurrency).To(Equal(tt.want)) + } + }) +} diff --git a/internal/webhooks/cluster.go b/internal/webhooks/cluster.go index 0a3f255c1593..92815b3dcbb6 100644 --- a/internal/webhooks/cluster.go +++ b/internal/webhooks/cluster.go @@ -20,6 +20,7 @@ import ( "context" "fmt" "net" + "strconv" "strings" "time" @@ -246,6 +247,25 @@ func (webhook *Cluster) validateTopology(ctx context.Context, oldCluster, newClu ) } + // upgrade concurrency should be a numeric value. + if concurrency, ok := newCluster.Annotations[clusterv1.ClusterTopologyUpgradeConcurrencyAnnotation]; ok { + concurrencyAnnotationField := field.NewPath("metadata", "annotations", clusterv1.ClusterTopologyUpgradeConcurrencyAnnotation) + concurrencyInt, err := strconv.Atoi(concurrency) + if err != nil { + allErrs = append(allErrs, field.Invalid( + concurrencyAnnotationField, + concurrency, + errors.Wrap(err, "could not parse the value of the annotation").Error(), + )) + } else if concurrencyInt < 1 { + allErrs = append(allErrs, field.Invalid( + concurrencyAnnotationField, + concurrency, + "value cannot be less than 1", + )) + } + } + // Get the ClusterClass referenced in the Cluster. clusterClass, clusterClassPollErr := webhook.pollClusterClassForCluster(ctx, newCluster) if clusterClassPollErr != nil && diff --git a/internal/webhooks/cluster_test.go b/internal/webhooks/cluster_test.go index 9e9a2f8fd214..a9efcd6dedff 100644 --- a/internal/webhooks/cluster_test.go +++ b/internal/webhooks/cluster_test.go @@ -1206,6 +1206,45 @@ func TestClusterTopologyValidation(t *testing.T) { Build()). Build(), }, + { + name: "should return error when upgrade concurrency annotation value is < 1", + expectErr: true, + in: builder.Cluster("fooboo", "cluster1"). + WithAnnotations(map[string]string{ + clusterv1.ClusterTopologyUpgradeConcurrencyAnnotation: "-1", + }). + WithTopology(builder.ClusterTopology(). + WithClass("foo"). + WithVersion("v1.19.2"). + Build()). + Build(), + }, + { + name: "should return error when upgrade concurrency annotation value is not numeric", + expectErr: true, + in: builder.Cluster("fooboo", "cluster1"). + WithAnnotations(map[string]string{ + clusterv1.ClusterTopologyUpgradeConcurrencyAnnotation: "abc", + }). + WithTopology(builder.ClusterTopology(). + WithClass("foo"). + WithVersion("v1.19.2"). + Build()). + Build(), + }, + { + name: "should pass upgrade concurrency annotation value is >= 1", + expectErr: false, + in: builder.Cluster("fooboo", "cluster1"). + WithAnnotations(map[string]string{ + clusterv1.ClusterTopologyUpgradeConcurrencyAnnotation: "2", + }). + WithTopology(builder.ClusterTopology(). + WithClass("foo"). + WithVersion("v1.19.2"). + Build()). + Build(), + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) {