diff --git a/internal/controllers/topology/cluster/scope/state.go b/internal/controllers/topology/cluster/scope/state.go index b2a20c7f1da2..00ebc04bbbb5 100644 --- a/internal/controllers/topology/cluster/scope/state.go +++ b/internal/controllers/topology/cluster/scope/state.go @@ -18,16 +18,14 @@ package scope import ( "context" - "fmt" "github.com/pkg/errors" - corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "sigs.k8s.io/controller-runtime/pkg/client" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" expv1 "sigs.k8s.io/cluster-api/exp/api/v1beta1" + "sigs.k8s.io/cluster-api/internal/topology/check" ) // ClusterState holds all the objects representing the state of a managed Cluster topology. @@ -102,31 +100,7 @@ type MachineDeploymentState struct { // A machine deployment is considered upgrading if at least one of the Machines of this // MachineDeployment has a different version. func (md *MachineDeploymentState) IsUpgrading(ctx context.Context, c client.Reader) (bool, error) { - // If the MachineDeployment has no version there is no definitive way to check if it is upgrading. Therefore, return false. - // Note: This case should not happen. - if md.Object.Spec.Template.Spec.Version == nil { - return false, nil - } - selectorMap, err := metav1.LabelSelectorAsMap(&md.Object.Spec.Selector) - if err != nil { - return false, errors.Wrapf(err, "failed to check if MachineDeployment %s is upgrading: failed to convert label selector to map", md.Object.Name) - } - machines := &clusterv1.MachineList{} - if err := c.List(ctx, machines, client.InNamespace(md.Object.Namespace), client.MatchingLabels(selectorMap)); err != nil { - return false, errors.Wrapf(err, "failed to check if MachineDeployment %s is upgrading: failed to list Machines", md.Object.Name) - } - mdVersion := *md.Object.Spec.Template.Spec.Version - // Check if the versions of the all the Machines match the MachineDeployment version. - for i := range machines.Items { - machine := machines.Items[i] - if machine.Spec.Version == nil { - return false, fmt.Errorf("failed to check if MachineDeployment %s is upgrading: Machine %s has no version", md.Object.Name, machine.Name) - } - if *machine.Spec.Version != mdVersion { - return true, nil - } - } - return false, nil + return check.IsMachineDeploymentUpgrading(ctx, c, md.Object) } // MachinePoolsStateMap holds a collection of MachinePool states. @@ -164,21 +138,5 @@ type MachinePoolState struct { // A machine pool is considered upgrading if at least one of the Machines of this // MachinePool has a different version. func (mp *MachinePoolState) IsUpgrading(ctx context.Context, c client.Reader) (bool, error) { - // If the MachinePool has no version there is no definitive way to check if it is upgrading. Therefore, return false. - // Note: This case should not happen. - if mp.Object.Spec.Template.Spec.Version == nil { - return false, nil - } - mpVersion := *mp.Object.Spec.Template.Spec.Version - // Check if the kubelet versions of the MachinePool noderefs match the MachinePool version. - for _, nodeRef := range mp.Object.Status.NodeRefs { - node := &corev1.Node{} - if err := c.Get(ctx, client.ObjectKey{Name: nodeRef.Name}, node); err != nil { - return false, fmt.Errorf("failed to check if MachinePool %s is upgrading: failed to get Node %s", mp.Object.Name, nodeRef.Name) - } - if mpVersion != node.Status.NodeInfo.KubeletVersion { - return true, nil - } - } - return false, nil + return check.IsMachinePoolUpgrading(ctx, c, mp.Object) } diff --git a/internal/controllers/topology/cluster/scope/state_test.go b/internal/controllers/topology/cluster/scope/state_test.go index 40a7a9d97a6c..b76e0d21ef51 100644 --- a/internal/controllers/topology/cluster/scope/state_test.go +++ b/internal/controllers/topology/cluster/scope/state_test.go @@ -29,92 +29,6 @@ import ( "sigs.k8s.io/cluster-api/internal/test/builder" ) -func TestIsUpgrading(t *testing.T) { - g := NewWithT(t) - scheme := runtime.NewScheme() - g.Expect(clusterv1.AddToScheme(scheme)).To(Succeed()) - - tests := []struct { - name string - md *clusterv1.MachineDeployment - machines []*clusterv1.Machine - want bool - wantErr bool - }{ - { - name: "should return false if all the machines of MachineDeployment have the same version as the MachineDeployment", - md: builder.MachineDeployment("ns", "md1"). - WithClusterName("cluster1"). - WithVersion("v1.2.3"). - Build(), - machines: []*clusterv1.Machine{ - builder.Machine("ns", "machine1"). - WithClusterName("cluster1"). - WithVersion("v1.2.3"). - Build(), - builder.Machine("ns", "machine2"). - WithClusterName("cluster1"). - WithVersion("v1.2.3"). - Build(), - }, - want: false, - wantErr: false, - }, - { - name: "should return true if at least one of the machines of MachineDeployment has a different version", - md: builder.MachineDeployment("ns", "md1"). - WithClusterName("cluster1"). - WithVersion("v1.2.3"). - Build(), - machines: []*clusterv1.Machine{ - builder.Machine("ns", "machine1"). - WithClusterName("cluster1"). - WithVersion("v1.2.3"). - Build(), - builder.Machine("ns", "machine2"). - WithClusterName("cluster1"). - WithVersion("v1.2.2"). - Build(), - }, - want: true, - wantErr: false, - }, - { - name: "should return false if the MachineDeployment has no machines (creation phase)", - md: builder.MachineDeployment("ns", "md1"). - WithClusterName("cluster1"). - WithVersion("v1.2.3"). - Build(), - machines: []*clusterv1.Machine{}, - want: false, - wantErr: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - g := NewWithT(t) - ctx := context.Background() - objs := []client.Object{} - objs = append(objs, tt.md) - for _, m := range tt.machines { - objs = append(objs, m) - } - fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(objs...).Build() - mdState := &MachineDeploymentState{ - Object: tt.md, - } - got, err := mdState.IsUpgrading(ctx, fakeClient) - if tt.wantErr { - g.Expect(err).To(HaveOccurred()) - } else { - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(got).To(Equal(tt.want)) - } - }) - } -} - func TestUpgrading(t *testing.T) { g := NewWithT(t) scheme := runtime.NewScheme() diff --git a/internal/webhooks/cluster.go b/internal/webhooks/cluster.go index c2a265968de6..bc2f3a4088a8 100644 --- a/internal/webhooks/cluster.go +++ b/internal/webhooks/cluster.go @@ -41,7 +41,6 @@ import ( expv1 "sigs.k8s.io/cluster-api/exp/api/v1beta1" "sigs.k8s.io/cluster-api/feature" "sigs.k8s.io/cluster-api/internal/contract" - "sigs.k8s.io/cluster-api/internal/controllers/topology/cluster/scope" "sigs.k8s.io/cluster-api/internal/topology/check" "sigs.k8s.io/cluster-api/internal/topology/variables" "sigs.k8s.io/cluster-api/util/conditions" @@ -525,7 +524,7 @@ func validateTopologyMachineDeploymentVersions(ctx context.Context, fldPath *fie for i := range mds.Items { md := &mds.Items[i] - upgrading, err := (&scope.MachineDeploymentState{Object: md}).IsUpgrading(ctx, ctrlClient) + upgrading, err := check.IsMachineDeploymentUpgrading(ctx, ctrlClient, md) if err != nil { return errors.Wrap(err, "failed to list upgrading MachinePools") } @@ -571,7 +570,8 @@ func validateTopologyMachinePoolVersions(ctx context.Context, fldPath *field.Pat for i := range mps.Items { mp := &mps.Items[i] - upgrading, err := (&scope.MachinePoolState{Object: mp}).IsUpgrading(ctx, wlClient) + + upgrading, err := check.IsMachinePoolUpgrading(ctx, wlClient, mp) if err != nil { return errors.Wrap(err, "failed to list upgrading MachinePools") }