Skip to content

Commit

Permalink
handle clusterctl block-move annotation
Browse files Browse the repository at this point in the history
  • Loading branch information
nojnhuh committed Dec 14, 2023
1 parent 703a98b commit 27d8eff
Show file tree
Hide file tree
Showing 8 changed files with 142 additions and 14 deletions.
9 changes: 6 additions & 3 deletions controllers/azurecluster_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -207,9 +207,11 @@ func (acr *AzureClusterReconciler) reconcileNormal(ctx context.Context, clusterS
log.Info("Reconciling AzureCluster")
azureCluster := clusterScope.AzureCluster

// If the AzureCluster doesn't have our finalizer, add it.
if controllerutil.AddFinalizer(azureCluster, infrav1.ClusterFinalizer) {
// Register the finalizer immediately to avoid orphaning Azure resources on delete
// Register our finalizer immediately to avoid orphaning Azure resources on delete
needsPatch := controllerutil.AddFinalizer(azureCluster, infrav1.ClusterFinalizer)
// Register the block-move annotation immediately to avoid moving un-paused ASO resources
needsPatch = AddBlockMoveAnnotation(azureCluster) || needsPatch
if needsPatch {
if err := clusterScope.PatchObject(ctx); err != nil {
return reconcile.Result{}, err
}
Expand Down Expand Up @@ -275,6 +277,7 @@ func (acr *AzureClusterReconciler) reconcilePause(ctx context.Context, clusterSc
if err := acs.Pause(ctx); err != nil {
return reconcile.Result{}, errors.Wrap(err, "failed to pause cluster services")
}
RemoveBlockMoveAnnotation(clusterScope.AzureCluster)

return reconcile.Result{}, nil
}
Expand Down
7 changes: 7 additions & 0 deletions controllers/azurecluster_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import (
"sigs.k8s.io/cluster-api-provider-azure/internal/test"
"sigs.k8s.io/cluster-api-provider-azure/util/reconciler"
clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
clusterctlv1 "sigs.k8s.io/cluster-api/cmd/clusterctl/api/v1alpha3"
capierrors "sigs.k8s.io/cluster-api/errors"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
Expand Down Expand Up @@ -274,6 +275,9 @@ func TestAzureClusterReconcilePaused(t *testing.T) {
ObjectMeta: metav1.ObjectMeta{
Name: name,
Namespace: namespace,
Annotations: map[string]string{
clusterctlv1.BlockMoveAnnotation: "true",
},
OwnerReferences: []metav1.OwnerReference{
{
Kind: "Cluster",
Expand Down Expand Up @@ -311,6 +315,9 @@ func TestAzureClusterReconcilePaused(t *testing.T) {
g.Expect(result.RequeueAfter).To(BeZero())

g.Eventually(recorder.Events).Should(Receive(Equal("Normal ClusterPaused AzureCluster or linked Cluster is marked as paused. Won't reconcile normally")))

g.Expect(c.Get(ctx, client.ObjectKeyFromObject(instance), instance)).To(Succeed())
g.Expect(instance.GetAnnotations()).NotTo(HaveKey(clusterctlv1.BlockMoveAnnotation))
}

func TestAzureClusterReconcileDelete(t *testing.T) {
Expand Down
9 changes: 6 additions & 3 deletions controllers/azuremachine_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -247,9 +247,11 @@ func (amr *AzureMachineReconciler) reconcileNormal(ctx context.Context, machineS
return reconcile.Result{}, nil
}

// If the AzureMachine doesn't have our finalizer, add it.
if controllerutil.AddFinalizer(machineScope.AzureMachine, infrav1.MachineFinalizer) {
// Register the finalizer immediately to avoid orphaning Azure resources on delete
// Register our finalizer immediately to avoid orphaning Azure resources on delete
needsPatch := controllerutil.AddFinalizer(machineScope.AzureMachine, infrav1.MachineFinalizer)
// Register the block-move annotation immediately to avoid moving un-paused ASO resources
needsPatch = AddBlockMoveAnnotation(machineScope.AzureMachine) || needsPatch
if needsPatch {
if err := machineScope.PatchObject(ctx); err != nil {
return reconcile.Result{}, err
}
Expand Down Expand Up @@ -355,6 +357,7 @@ func (amr *AzureMachineReconciler) reconcilePause(ctx context.Context, machineSc
if err := ams.Pause(ctx); err != nil {
return reconcile.Result{}, errors.Wrap(err, "failed to pause azure machine services")
}
RemoveBlockMoveAnnotation(machineScope.AzureMachine)

return reconcile.Result{}, nil
}
Expand Down
7 changes: 5 additions & 2 deletions controllers/azuremanagedcontrolplane_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -231,11 +231,13 @@ func (amcpr *AzureManagedControlPlaneReconciler) reconcileNormal(ctx context.Con

log.Info("Reconciling AzureManagedControlPlane")

// Remove deprecated Cluster finalizer if it exists, if the AzureManagedControlPlane doesn't have our finalizer, add it.
// Remove deprecated Cluster finalizer if it exists
needsPatch := controllerutil.RemoveFinalizer(scope.ControlPlane, infrav1.ClusterFinalizer)
// Register our finalizer immediately to avoid orphaning Azure resources on delete
needsPatch = controllerutil.AddFinalizer(scope.ControlPlane, infrav1.ManagedClusterFinalizer) || needsPatch
// Register the block-move annotation immediately to avoid moving un-paused ASO resources
needsPatch = AddBlockMoveAnnotation(scope.ControlPlane) || needsPatch
if needsPatch {
// Register the finalizer immediately to avoid orphaning Azure resources on delete
if err := scope.PatchObject(ctx); err != nil {
amcpr.Recorder.Eventf(scope.ControlPlane, corev1.EventTypeWarning, "AzureManagedControlPlane unavailable", "failed to patch resource: %s", err)
return reconcile.Result{}, err
Expand Down Expand Up @@ -290,6 +292,7 @@ func (amcpr *AzureManagedControlPlaneReconciler) reconcilePause(ctx context.Cont
if err := svc.Pause(ctx); err != nil {
return reconcile.Result{}, errors.Wrap(err, "failed to pause control plane services")
}
RemoveBlockMoveAnnotation(scope.ControlPlane)

return reconcile.Result{}, nil
}
Expand Down
9 changes: 6 additions & 3 deletions controllers/azuremanagedmachinepool_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -251,9 +251,11 @@ func (ammpr *AzureManagedMachinePoolReconciler) reconcileNormal(ctx context.Cont

log.Info("Reconciling AzureManagedMachinePool")

// If the AzureManagedMachinePool doesn't have our finalizer, add it.
if controllerutil.AddFinalizer(scope.InfraMachinePool, infrav1.ClusterFinalizer) {
// Register the finalizer immediately to avoid orphaning Azure resources on delete
// Register the finalizer immediately to avoid orphaning Azure resources on delete
needsPatch := controllerutil.AddFinalizer(scope.InfraMachinePool, infrav1.ClusterFinalizer)
// Register the block-move annotation immediately to avoid moving un-paused ASO resources
needsPatch = AddBlockMoveAnnotation(scope.InfraMachinePool) || needsPatch
if needsPatch {
if err := scope.PatchObject(ctx); err != nil {
return reconcile.Result{}, err
}
Expand Down Expand Up @@ -311,6 +313,7 @@ func (ammpr *AzureManagedMachinePoolReconciler) reconcilePause(ctx context.Conte
if err := svc.Pause(ctx); err != nil {
return reconcile.Result{}, errors.Wrapf(err, "error pausing AzureManagedMachinePool %s/%s", scope.InfraMachinePool.Namespace, scope.InfraMachinePool.Name)
}
RemoveBlockMoveAnnotation(scope.InfraMachinePool)

return reconcile.Result{}, nil
}
Expand Down
27 changes: 27 additions & 0 deletions controllers/helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ import (
"sigs.k8s.io/cluster-api-provider-azure/util/reconciler"
"sigs.k8s.io/cluster-api-provider-azure/util/tele"
clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
clusterctlv1 "sigs.k8s.io/cluster-api/cmd/clusterctl/api/v1alpha3"
expv1 "sigs.k8s.io/cluster-api/exp/api/v1beta1"
capifeature "sigs.k8s.io/cluster-api/feature"
"sigs.k8s.io/cluster-api/util"
Expand Down Expand Up @@ -1120,3 +1121,29 @@ func GetClusterScoper(ctx context.Context, logger logr.Logger, c client.Client,

return nil, errors.Errorf("unsupported infrastructure type %q, should be AzureCluster or AzureManagedCluster", cluster.Spec.InfrastructureRef.Kind)
}

// AddBlockMoveAnnotation adds CAPI's block-move annotation and returns whether or not the annotation was added.
func AddBlockMoveAnnotation(obj metav1.Object) bool {
annotations := obj.GetAnnotations()

if _, exists := annotations[clusterctlv1.BlockMoveAnnotation]; exists {
return false
}

if annotations == nil {
annotations = make(map[string]string)
}

// this value doesn't mean anything, only the presence of the annotation matters.
annotations[clusterctlv1.BlockMoveAnnotation] = "true"
obj.SetAnnotations(annotations)

return true
}

// RemoveBlockMoveAnnotation removes CAPI's block-move annotation from the object.
func RemoveBlockMoveAnnotation(obj metav1.Object) {
azClusterAnnotations := obj.GetAnnotations()
delete(azClusterAnnotations, clusterctlv1.BlockMoveAnnotation)
obj.SetAnnotations(azClusterAnnotations)
}
81 changes: 81 additions & 0 deletions controllers/helpers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
"github.com/google/go-cmp/cmp"
. "github.com/onsi/gomega"
"go.uber.org/mock/gomock"
"golang.org/x/exp/maps"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/resource"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand All @@ -40,6 +41,7 @@ import (
"sigs.k8s.io/cluster-api-provider-azure/azure/scope"
"sigs.k8s.io/cluster-api-provider-azure/internal/test/mock_log"
clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
clusterctlv1 "sigs.k8s.io/cluster-api/cmd/clusterctl/api/v1alpha3"
expv1 "sigs.k8s.io/cluster-api/exp/api/v1beta1"
capifeature "sigs.k8s.io/cluster-api/feature"
"sigs.k8s.io/controller-runtime/pkg/client"
Expand Down Expand Up @@ -1517,3 +1519,82 @@ func TestClusterPauseChangeAndInfrastructureReady(t *testing.T) {
})
}
}

func TestAddBlockMoveAnnotation(t *testing.T) {
tests := []struct {
name string
annotations map[string]string
expectedAnnotations map[string]string
expected bool
}{
{
name: "annotation does not exist",
annotations: nil,
expectedAnnotations: map[string]string{clusterctlv1.BlockMoveAnnotation: "true"},
expected: true,
},
{
name: "annotation already exists",
annotations: map[string]string{clusterctlv1.BlockMoveAnnotation: "this value might be different but it doesn't matter"},
expectedAnnotations: map[string]string{clusterctlv1.BlockMoveAnnotation: "this value might be different but it doesn't matter"},
expected: false,
},
}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
obj := &metav1.ObjectMeta{
Annotations: test.annotations,
}
actual := AddBlockMoveAnnotation(obj)
if test.expected != actual {
t.Errorf("expected %v, got %v", test.expected, actual)
}
if !maps.Equal(test.expectedAnnotations, obj.GetAnnotations()) {
t.Errorf("expected %v, got %v", test.expectedAnnotations, obj.GetAnnotations())
}
})
}
}

func TestRemoveBlockMoveAnnotation(t *testing.T) {
tests := []struct {
name string
annotations map[string]string
expected map[string]string
}{
{
name: "nil",
annotations: nil,
expected: nil,
},
{
name: "annotation not present",
annotations: map[string]string{"another": "annotation"},
expected: map[string]string{"another": "annotation"},
},
{
name: "annotation present",
annotations: map[string]string{
clusterctlv1.BlockMoveAnnotation: "any value",
"another": "annotation",
},
expected: map[string]string{
"another": "annotation",
},
},
}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
obj := &metav1.ObjectMeta{
Annotations: maps.Clone(test.annotations),
}
RemoveBlockMoveAnnotation(obj)
actual := obj.GetAnnotations()
if !maps.Equal(test.expected, actual) {
t.Errorf("expected %v, got %v", test.expected, actual)
}
})
}
}
7 changes: 4 additions & 3 deletions exp/controllers/azuremachinepool_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -270,12 +270,12 @@ func (ampr *AzureMachinePoolReconciler) reconcileNormal(ctx context.Context, mac
return reconcile.Result{}, nil
}

// Add the finalizer and the InfrastructureMachineKind if it is not already set, and patch if either changed.

// Register the finalizer immediately to avoid orphaning Azure resources on delete
needsPatch := controllerutil.AddFinalizer(machinePoolScope.AzureMachinePool, expv1.MachinePoolFinalizer)
needsPatch = machinePoolScope.SetInfrastructureMachineKind() || needsPatch
// Register the block-move annotation immediately to avoid moving un-paused ASO resources
needsPatch = infracontroller.AddBlockMoveAnnotation(machinePoolScope.AzureMachinePool) || needsPatch

Check warning on line 277 in exp/controllers/azuremachinepool_controller.go

View check run for this annotation

Codecov / codecov/patch

exp/controllers/azuremachinepool_controller.go#L276-L277

Added lines #L276 - L277 were not covered by tests
if needsPatch {
// Register the finalizer immediately to avoid orphaning Azure resources on delete
if err := machinePoolScope.PatchObject(ctx); err != nil {
return reconcile.Result{}, err
}
Expand Down Expand Up @@ -368,6 +368,7 @@ func (ampr *AzureMachinePoolReconciler) reconcilePause(ctx context.Context, mach
if err := amps.Pause(ctx); err != nil {
return reconcile.Result{}, errors.Wrapf(err, "error deleting AzureMachinePool %s/%s", machinePoolScope.AzureMachinePool.Namespace, machinePoolScope.Name())
}
infracontroller.RemoveBlockMoveAnnotation(machinePoolScope.AzureMachinePool)

return reconcile.Result{}, nil
}
Expand Down

0 comments on commit 27d8eff

Please sign in to comment.