WIP: ⚠️ Remove Metal3Machine owner reference from BMH

baremetal/metal3machine_manager.go

@@ -244,6 +244,7 @@ func (m *MachineManager) RemovePauseAnnotation(ctx context.Context) error {
 		if _, ok := annotations[bmov1alpha1.PausedAnnotation]; ok {
 			if m.Cluster.Name == host.Labels[clusterv1.ClusterNameLabel] && annotations[bmov1alpha1.PausedAnnotation] == PausedAnnotationKey {
 				// Removing BMH Paused Annotation Since Owner Cluster is not paused.
+				m.Log.Info("Removing Pause Annotation.")
 				delete(host.Annotations, bmov1alpha1.PausedAnnotation)
 			} else if m.Cluster.Name == host.Labels[clusterv1.ClusterNameLabel] && annotations[bmov1alpha1.PausedAnnotation] != PausedAnnotationKey {
 				m.Log.Info("BMH is paused by user. Not removing Pause Annotation")
@@ -271,8 +272,10 @@ func (m *MachineManager) SetPauseAnnotation(ctx context.Context) error {
 
 	if annotations != nil {
 		if _, ok := annotations[bmov1alpha1.PausedAnnotation]; ok {
-			m.Log.Info("BaremetalHost is already paused")
-			return nil
+			if annotations[bmov1alpha1.PausedAnnotation] == PausedAnnotationKey {
+				m.Log.Info("BaremetalHost is already paused by CAPM3. No action needed.")
+				return nil
+			}
 		}
 	} else {
 		host.Annotations = make(map[string]string)
@@ -469,6 +472,7 @@ func (m *MachineManager) Delete(ctx context.Context) error {
 		if !consumerRefMatches(host.Spec.ConsumerRef, m.Metal3Machine) {
 			m.Log.Info("host already associated with another metal3 machine",
 				"host", host.Name)
+			// The following removal of ownerreference code will be removed in v1.11
 			// Remove the ownerreference to this machine, even if the consumer ref
 			// references another machine.
 			host.OwnerReferences, err = m.DeleteOwnerRef(host.OwnerReferences)
@@ -663,6 +667,7 @@ func (m *MachineManager) Delete(ctx context.Context) error {
 
 		host.Spec.ConsumerRef = nil
 
+		// The following removal of ownerreference code will be removed in v1.11
 		// Remove the ownerreference to this machine.
 		host.OwnerReferences, err = m.DeleteOwnerRef(host.OwnerReferences)
 		if err != nil {
@@ -1128,13 +1133,6 @@ func (m *MachineManager) setHostConsumerRef(_ context.Context, host *bmov1alpha1
 		APIVersion: m.Metal3Machine.APIVersion,
 	}
 
-	// Set OwnerReferences.
-	hostOwnerReferences, err := m.SetOwnerRef(host.OwnerReferences, true)
-	if err != nil {
-		return err
-	}
-	host.OwnerReferences = hostOwnerReferences
-
 	// Delete nodeReuseLabelName from host.
 	m.Log.Info("Deleting nodeReuseLabelName from host, if any")
 

baremetal/metal3machine_manager_test.go

@@ -1223,8 +1223,6 @@ var _ = Describe("Metal3Machine manager", func() {
 			Expect(tc.Host.Spec.ConsumerRef.Namespace).
 				To(Equal(m3mconfig.Namespace))
 			Expect(tc.Host.Spec.ConsumerRef.Kind).To(Equal("Metal3Machine"))
-			_, err = machineMgr.FindOwnerRef(tc.Host.OwnerReferences)
-			Expect(err).NotTo(HaveOccurred())
 
 			if tc.expectNodeReuseLabelDeleted {
 				Expect(tc.Host.Labels[nodeReuseLabelName]).To(Equal(""))
@@ -2732,7 +2730,6 @@ var _ = Describe("Metal3Machine manager", func() {
 		Data               *infrav1.Metal3Data
 		ExpectRequeue      bool
 		ExpectClusterLabel bool
-		ExpectOwnerRef     bool
 	}
 
 	DescribeTable("Test Associate function",
@@ -2783,12 +2780,7 @@ var _ = Describe("Metal3Machine manager", func() {
 				&savedHost,
 			)
 			Expect(err).NotTo(HaveOccurred())
-			_, err = machineMgr.FindOwnerRef(savedHost.OwnerReferences)
-			if tc.ExpectOwnerRef {
-				Expect(err).NotTo(HaveOccurred())
-			} else {
-				Expect(err).To(HaveOccurred())
-			}
+
 			if tc.ExpectClusterLabel {
 				// get the BMC credential
 				savedCred := corev1.Secret{}
@@ -2813,8 +2805,7 @@ var _ = Describe("Metal3Machine manager", func() {
 				Host: newBareMetalHost(baremetalhostName, nil, bmov1alpha1.StateNone, nil,
 					false, "metadata", false, "",
 				),
-				ExpectRequeue:  false,
-				ExpectOwnerRef: true,
+				ExpectRequeue: false,
 			},
 		),
 		Entry("Associate empty machine, Metal3 machine spec set",
@@ -2826,9 +2817,8 @@ var _ = Describe("Metal3Machine manager", func() {
 				Host: newBareMetalHost(baremetalhostName, bmhSpecBMC(), bmov1alpha1.StateNone, nil,
 					false, "metadata", false, "",
 				),
-				BMCSecret:      newBMCSecret("mycredentials", false),
-				ExpectRequeue:  false,
-				ExpectOwnerRef: true,
+				BMCSecret:     newBMCSecret("mycredentials", false),
+				ExpectRequeue: false,
 			},
 		),
 		Entry("Associate empty machine, host empty, Metal3 machine spec set",
@@ -2837,9 +2827,8 @@ var _ = Describe("Metal3Machine manager", func() {
 				M3Machine: newMetal3Machine(metal3machineName, m3mSpecAll(), nil,
 					m3mObjectMetaWithValidAnnotations(),
 				),
-				Host:           newBareMetalHost("", nil, bmov1alpha1.StateNone, nil, false, "metadata", false, ""),
-				ExpectRequeue:  true,
-				ExpectOwnerRef: false,
+				Host:          newBareMetalHost("", nil, bmov1alpha1.StateNone, nil, false, "metadata", false, ""),
+				ExpectRequeue: true,
 			},
 		),
 		Entry("Associate machine, host nil, Metal3 machine spec set, requeue",
@@ -2860,7 +2849,6 @@ var _ = Describe("Metal3Machine manager", func() {
 				BMCSecret:          newBMCSecret("mycredentials", false),
 				ExpectClusterLabel: true,
 				ExpectRequeue:      false,
-				ExpectOwnerRef:     true,
 			},
 		),
 		Entry("Associate machine with DataTemplate missing",
@@ -2887,7 +2875,6 @@ var _ = Describe("Metal3Machine manager", func() {
 				BMCSecret:          newBMCSecret("mycredentials", false),
 				ExpectClusterLabel: true,
 				ExpectRequeue:      true,
-				ExpectOwnerRef:     true,
 			},
 		),
 		Entry("Associate machine with DataTemplate and Data ready",
@@ -2933,7 +2920,6 @@ var _ = Describe("Metal3Machine manager", func() {
 				},
 				ExpectClusterLabel: true,
 				ExpectRequeue:      false,
-				ExpectOwnerRef:     true,
 			},
 		),
 	)

controllers/metal3machine_controller.go

@@ -176,10 +176,9 @@ func (r *Metal3MachineReconciler) Reconcile(ctx context.Context, req ctrl.Reques
 	}
 
 	// Check pause annotation on associated bmh (if any)
-	setPause := cluster.Spec.Paused != nil && *cluster.Spec.Paused
-	if setPause {
-		// set pause annotation on associated bmh (if any)
-		err := machineMgr.SetPauseAnnotation(ctx)
+	if annotations.IsPaused(cluster, capm3Machine) {
+		annotations.AddAnnotations(capm3Machine, map[string]string{"clusterctl.cluster.x-k8s.io/block-move": ""})
+		err = machineMgr.SetPauseAnnotation(ctx)
 		if err != nil {
 			machineLog.Info("failed to set pause annotation on associated bmh")
 			v1beta1conditions.MarkFalse(capm3Machine, infrav1.AssociateBMHCondition, infrav1.PauseAnnotationSetFailedReason, clusterv1beta1.ConditionSeverityInfo, "")
@@ -192,29 +191,6 @@ func (r *Metal3MachineReconciler) Reconcile(ctx context.Context, req ctrl.Reques
 			})
 			return ctrl.Result{}, nil
 		}
-	} else {
-		err := machineMgr.RemovePauseAnnotation(ctx)
-		if err != nil {
-			machineLog.Info("failed to check pause annotation on associated bmh")
-			v1beta1conditions.MarkFalse(capm3Machine, infrav1.AssociateBMHCondition, infrav1.PauseAnnotationRemoveFailedReason, clusterv1beta1.ConditionSeverityInfo, "")
-			message := "Failed to remove pause annotation on associated BareMetalHost. Error: " + err.Error()
-			v1beta2conditions.Set(capm3Machine, metav1.Condition{
-				Type:    clusterv1beta1.PausedV1Beta2Condition,
-				Status:  metav1.ConditionFalse,
-				Reason:  infrav1.BareMetalHostPauseAnnotationRemoveFailedV1Beta2Reason,
-				Message: message,
-			})
-			return ctrl.Result{}, nil
-		}
-		v1beta2conditions.Set(capm3Machine, metav1.Condition{
-			Type:   clusterv1beta1.PausedV1Beta2Condition,
-			Status: metav1.ConditionTrue,
-			Reason: clusterv1beta1.NotPausedV1Beta2Reason,
-		})
-	}
-
-	// Return early if the M3Machine or Cluster is paused.
-	if annotations.IsPaused(cluster, capm3Machine) {
 		machineLog.Info("reconciliation is paused for this object")
 		v1beta1conditions.MarkFalse(capm3Machine, infrav1.AssociateBMHCondition, infrav1.Metal3MachinePausedReason, clusterv1beta1.ConditionSeverityInfo, "")
 		v1beta2conditions.Set(capm3Machine, metav1.Condition{
@@ -224,6 +200,12 @@ func (r *Metal3MachineReconciler) Reconcile(ctx context.Context, req ctrl.Reques
 		})
 		return ctrl.Result{Requeue: true, RequeueAfter: requeueAfter}, nil
 	}
+	err = machineMgr.RemovePauseAnnotation(ctx)
+	if err != nil {
+		machineLog.Info("failed to remove pause annotation on associated bmh")
+		v1beta1conditions.MarkFalse(capm3Machine, infrav1.AssociateBMHCondition, infrav1.PauseAnnotationRemoveFailedReason, clusterv1beta1.ConditionSeverityInfo, "")
+		return ctrl.Result{}, nil
+	}
 
 	// Handle deleted machines
 	if !capm3Machine.ObjectMeta.DeletionTimestamp.IsZero() {

test/e2e/pivoting.go

@@ -149,6 +149,7 @@ func pivoting(ctx context.Context, inputGetter func() PivotingInput) {
 
 	By("Add labels to BMO CRDs")
 	labelBMOCRDs(ctx, input.BootstrapClusterProxy)
+
 	By("Add Labels to hardwareData CRDs")
 	labelHDCRDs(ctx, input.BootstrapClusterProxy)
 
@@ -202,6 +203,24 @@ func pivoting(ctx context.Context, inputGetter func() PivotingInput) {
 		return input.TargetCluster.GetClient().Get(ctx, client.ObjectKey{Name: "kube-system"}, kubeSystem)
 	}, "5s", "100ms").Should(Succeed(), "Failed to assert target API server stability")
 
+	Logf("Dump the target cluster resources before pivoting")
+	framework.DumpAllResources(ctx, framework.DumpAllResourcesInput{
+		Lister:               input.TargetCluster.GetClient(),
+		Namespace:            input.Namespace,
+		LogPath:              filepath.Join(input.ArtifactFolder, "clusters", "target-cluster-before-pivot", "resources"),
+		KubeConfigPath:       input.TargetCluster.GetKubeconfigPath(),
+		ClusterctlConfigPath: input.ClusterctlConfigPath,
+	})
+
+	By("Fetch logs from target cluster before pivoting")
+	err = FetchClusterLogs(input.TargetCluster, filepath.Join(input.ArtifactFolder, "clusters", "target-cluster-before-pivot", "resources"))
+	if err != nil {
+		Logf("Error: %v", err)
+	}
+
+	By("Add paused annotation to BMHs")
+	addPausedAnnotation(ctx, input.BootstrapClusterProxy)
+
 	By("Moving the cluster to self hosted")
 	clusterctl.Move(ctx, clusterctl.MoveInput{
 		LogFolder:            filepath.Join(input.ArtifactFolder, "clusters", input.ClusterName+"-bootstrap"),
@@ -212,6 +231,24 @@ func pivoting(ctx context.Context, inputGetter func() PivotingInput) {
 	})
 	LogFromFile(filepath.Join(input.ArtifactFolder, "clusters", input.ClusterName+"-bootstrap", "logs", input.Namespace, "clusterctl-move.log"))
 
+	By("Fetch logs from target cluster after pivoting")
+	err = FetchClusterLogs(input.TargetCluster, filepath.Join(input.ArtifactFolder, "clusters", "target-cluster-after-pivot", "resources"))
+	if err != nil {
+		Logf("Error: %v", err)
+	}
+
+	Logf("Dump the target cluster resources after pivoting")
+	framework.DumpAllResources(ctx, framework.DumpAllResourcesInput{
+		Lister:               input.TargetCluster.GetClient(),
+		Namespace:            input.Namespace,
+		LogPath:              filepath.Join(input.ArtifactFolder, "clusters", "target-cluster-after-pivot", "resources"),
+		KubeConfigPath:       input.TargetCluster.GetKubeconfigPath(),
+		ClusterctlConfigPath: input.ClusterctlConfigPath,
+	})
+
+	By("Remove paused annotation from BMH")
+	removePausedAnnotation(ctx, input.TargetCluster)
+
 	By("Remove BMO deployment from the source cluster")
 	RemoveDeployment(ctx, func() RemoveDeploymentInput {
 		return RemoveDeploymentInput{
@@ -220,6 +257,7 @@ func pivoting(ctx context.Context, inputGetter func() PivotingInput) {
 			Name:              input.E2EConfig.MustGetVariable(NamePrefix) + "-controller-manager",
 		}
 	})
+
 	pivotingCluster := framework.DiscoveryAndWaitForCluster(ctx, framework.DiscoveryAndWaitForClusterInput{
 		Getter:    input.TargetCluster.GetClient(),
 		Namespace: input.Namespace,
@@ -332,18 +370,40 @@ func RemoveDeployment(ctx context.Context, inputGetter func() RemoveDeploymentIn
 }
 
 func labelBMOCRDs(ctx context.Context, targetCluster framework.ClusterProxy) {
+	bmhs, err := GetAllBmhs(ctx, targetCluster.GetClient(), "metal3")
+	Expect(err).ToNot(HaveOccurred(), "Cannot fetch BMHs")
 	labels := map[string]string{}
 	labels[clusterctlv1.ClusterctlLabel] = ""
-	labels[clusterv1.ProviderNameLabel] = "metal3"
+	labels[clusterv1.ProviderNameLabel] = "metal3" //nolint:goconst
 	crdName := "baremetalhosts.metal3.io"
-	err := LabelCRD(ctx, targetCluster.GetClient(), crdName, labels)
+	err = LabelCRD(ctx, targetCluster.GetClient(), crdName, labels)
 	Expect(err).ToNot(HaveOccurred(), "Cannot label BMH CRDs")
+	for _, bmh := range bmhs {
+		// Merge new labels with existing labels
+		if bmh.ObjectMeta.Labels == nil {
+			bmh.ObjectMeta.Labels = map[string]string{}
+		}
+
+		bmh.ObjectMeta.Labels[clusterctlv1.ClusterctlLabel] = ""
+		bmh.ObjectMeta.Labels[clusterctlv1.ClusterctlMoveLabel] = ""
+		bmh.ObjectMeta.Labels[clusterctlv1.ClusterctlMoveHierarchyLabel] = ""
+		bmh.ObjectMeta.Labels[clusterv1.ProviderNameLabel] = "metal3"
+
+		err = targetCluster.GetClient().Update(ctx, &bmh)
+		if err != nil {
+			Logf("Cannot label BMH %s: %v", bmh.Name, err)
+		}
+		Logf("After adding the labels")
+		Logf(fmt.Sprintf("BMH metadata: %v", bmh.ObjectMeta.Labels))
+	}
+	Expect(err).ToNot(HaveOccurred(), "Cannot label BMHs")
 }
 
 func labelHDCRDs(ctx context.Context, targetCluster framework.ClusterProxy) {
 	labels := map[string]string{}
 	labels[clusterctlv1.ClusterctlLabel] = ""
-	labels[clusterctlv1.ClusterctlMoveLabel] = ""
+	labels[clusterctlv1.ClusterctlMoveHierarchyLabel] = ""
+	labels[clusterv1.ProviderNameLabel] = "metal3"
 	crdName := "hardwaredata.metal3.io"
 	err := LabelCRD(ctx, targetCluster.GetClient(), crdName, labels)
 	Expect(err).ToNot(HaveOccurred(), "Cannot label HD CRDs")
@@ -446,6 +506,9 @@ func rePivoting(ctx context.Context, inputGetter func() RePivotingInput) {
 		return input.BootstrapClusterProxy.GetClient().Get(ctx, client.ObjectKey{Name: "kube-system"}, kubeSystem)
 	}, "5s", "100ms").Should(Succeed(), "Failed to assert bootstrap API server stability")
 
+	By("Add paused annotation to BMHs")
+	addPausedAnnotation(ctx, input.TargetCluster)
+
 	By("Move back to bootstrap cluster")
 	clusterctl.Move(ctx, clusterctl.MoveInput{
 		LogFolder:            filepath.Join(input.ArtifactFolder, "clusters", input.ClusterName+"-pivot"),
@@ -472,6 +535,9 @@ func rePivoting(ctx context.Context, inputGetter func() RePivotingInput) {
 	})
 	Expect(controlPlane).ToNot(BeNil())
 
+	By("Remove paused annotation from BMHs")
+	removePausedAnnotation(ctx, input.BootstrapClusterProxy)
+
 	By("Check that BMHs are in provisioned state")
 	WaitForNumBmhInState(ctx, bmov1alpha1.StateProvisioned, WaitForNumInput{
 		Client:    input.BootstrapClusterProxy.GetClient(),
@@ -532,3 +598,38 @@ func fetchContainerLogs(containerNames *[]string, folder string, containerComman
 		Expect(writeErr).ToNot(HaveOccurred())
 	}
 }
+
+func removePausedAnnotation(ctx context.Context, targetCluster framework.ClusterProxy) {
+	bmhs, err := GetAllBmhs(ctx, targetCluster.GetClient(), "metal3")
+	Expect(err).ToNot(HaveOccurred(), "Cannot fetch BMHs")
+	for _, bmh := range bmhs {
+		if bmh.ObjectMeta.Annotations != nil {
+			if _, ok := bmh.ObjectMeta.Annotations[bmov1alpha1.PausedAnnotation]; ok {
+				delete(bmh.ObjectMeta.Annotations, bmov1alpha1.PausedAnnotation)
+				err = targetCluster.GetClient().Update(ctx, &bmh)
+				if err != nil {
+					Logf("Cannot remove paused annotation from BMH %s: %v", bmh.Name, err)
+				}
+				Logf("Removed paused annotation from BMH %s", bmh.Name)
+			}
+		}
+	}
+	Expect(err).ToNot(HaveOccurred(), "Cannot remove paused annotation from BMHs")
+}
+
+func addPausedAnnotation(ctx context.Context, targetCluster framework.ClusterProxy) {
+	bmhs, err := GetAllBmhs(ctx, targetCluster.GetClient(), "metal3")
+	Expect(err).ToNot(HaveOccurred(), "Cannot fetch BMHs")
+	for _, bmh := range bmhs {
+		if bmh.ObjectMeta.Annotations == nil {
+			bmh.ObjectMeta.Annotations = map[string]string{}
+		}
+		bmh.ObjectMeta.Annotations[bmov1alpha1.PausedAnnotation] = "manual-pivoting"
+		err = targetCluster.GetClient().Update(ctx, &bmh)
+		if err != nil {
+			Logf("Cannot add paused annotation to BMH %s: %v", bmh.Name, err)
+		}
+		Logf("Added paused annotation to BMH %s", bmh.Name)
+	}
+	Expect(err).ToNot(HaveOccurred(), "Cannot add paused annotation to BMHs")
+}