Merge pull request #845 from openshift-cherrypick-robot/cherry-pick-8…

…15-to-release-4.6

[release-4.6] Bug 1947813: vSphere, detach virtual disks before virtual machine destroy if node not available

pkg/controller/vsphere/machine_scope.go

@@ -130,6 +130,44 @@ func (s *machineScope) GetSession() *session.Session {
 	return s.session
 }
 
+func (s *machineScope) isNodeLinked() bool {
+	return s.machine.Status.NodeRef != nil && s.machine.Status.NodeRef.Name != ""
+}
+
+func (s *machineScope) getNode() (*apicorev1.Node, error) {
+	var node apicorev1.Node
+	if !s.isNodeLinked() {
+		return nil, fmt.Errorf("NodeRef empty, unable to get related Node")
+	}
+	nodeName := s.machine.Status.NodeRef.Name
+	objectKey := runtimeclient.ObjectKey{
+		Name: nodeName,
+	}
+	if err := s.apiReader.Get(s.Context, objectKey, &node); err != nil {
+		if apimachineryerrors.IsNotFound(err) {
+			klog.V(2).Infof("Node %q not found", nodeName)
+			return nil, err
+		}
+		klog.Errorf("Failed to get node %q: %v", nodeName, err)
+		return nil, err
+	}
+
+	return &node, nil
+}
+
+func (s *machineScope) checkNodeReachable() (bool, error) {
+	node, err := s.getNode()
+	if err != nil {
+		return false, err
+	}
+	for _, condition := range node.Status.Conditions {
+		if condition.Type == apicorev1.NodeReady && condition.Status == apicorev1.ConditionUnknown {
+			return false, nil
+		}
+	}
+	return true, nil
+}
+
 // GetUserData fetches the user-data from the secret referenced in the Machine's
 // provider spec, if one is set.
 func (s *machineScope) GetUserData() ([]byte, error) {

pkg/controller/vsphere/machine_scope_test.go

@@ -539,3 +539,266 @@ func TestPatchMachine(t *testing.T) {
 		})
 	}
 }
+
+func TestNodeGetter(t *testing.T) {
+	g := NewWithT(t)
+	ctx := context.Background()
+
+	testEnv := &envtest.Environment{
+		CRDDirectoryPaths: []string{
+			filepath.Join("..", "..", "..", "install"),
+			filepath.Join("..", "..", "..", "vendor", "github.com", "openshift", "api", "config", "v1")},
+	}
+
+	cfg, err := testEnv.Start()
+	g.Expect(err).ToNot(HaveOccurred())
+	g.Expect(cfg).ToNot(BeNil())
+	defer func() {
+		g.Expect(testEnv.Stop()).To(Succeed())
+	}()
+
+	k8sClient, err := client.New(cfg, client.Options{})
+	g.Expect(err).ToNot(HaveOccurred())
+
+	nodeName := "somenodename"
+	node := &corev1.Node{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      nodeName,
+			Namespace: metav1.NamespaceNone,
+		},
+		TypeMeta: metav1.TypeMeta{
+			Kind: "Node",
+		},
+		Status: corev1.NodeStatus{
+			Conditions: []corev1.NodeCondition{
+				{
+					Type:   corev1.NodeReady,
+					Status: corev1.ConditionTrue,
+				},
+			},
+		},
+	}
+	g.Expect(k8sClient.Create(ctx, node)).To(Succeed())
+	defer func() {
+		g.Expect(k8sClient.Delete(ctx, node)).To(Succeed())
+	}()
+
+	testNamespace := &corev1.Namespace{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: TestNamespace,
+		},
+	}
+	g.Expect(k8sClient.Create(ctx, testNamespace)).To(Succeed())
+	defer func() {
+		g.Expect(k8sClient.Delete(ctx, testNamespace)).To(Succeed())
+	}()
+
+	machineName := "somemachine"
+	machineKey := types.NamespacedName{Namespace: TestNamespace, Name: machineName}
+	machine := &machinev1.Machine{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      machineName,
+			Namespace: TestNamespace,
+			Labels:    map[string]string{},
+		},
+		TypeMeta: metav1.TypeMeta{
+			Kind:       "Machine",
+			APIVersion: "machine.openshift.io/v1beta1",
+		},
+	}
+	g.Expect(k8sClient.Create(ctx, machine)).To(Succeed())
+	defer func() {
+		g.Expect(k8sClient.Delete(ctx, machine)).To(Succeed())
+	}()
+
+	// Ensure the machine has synced to the cache
+	getMachine := func() error {
+		return k8sClient.Get(ctx, machineKey, machine)
+	}
+	g.Eventually(getMachine, 10*time.Second).Should(Succeed())
+
+	machineScope := &machineScope{
+		Context:      ctx,
+		client:       k8sClient,
+		apiReader:    k8sClient,
+		machine:      machine,
+		providerSpec: &vspherev1.VSphereMachineProviderSpec{},
+	}
+
+	resetStatuses := func(node *corev1.Node, machine *machinev1.Machine) {
+		machine.Status = machinev1.MachineStatus{}
+		node.Status = corev1.NodeStatus{
+			Conditions: []corev1.NodeCondition{
+				{
+					Type:   corev1.NodeReady,
+					Status: corev1.ConditionTrue,
+				},
+			},
+		}
+	}
+
+	////////////// getNode test cases
+
+	nodeGetterTestCases := []struct {
+		name        string
+		err         error
+		node        *corev1.Node
+		setStatuses func(node *corev1.Node, machine *machinev1.Machine)
+	}{
+		{
+			name:        "getNode: No node linked",
+			err:         fmt.Errorf("NodeRef empty, unable to get related Node"),
+			node:        nil,
+			setStatuses: func(node *corev1.Node, machine *machinev1.Machine) {},
+		},
+		{
+			name: "getNode: Node linked",
+			err:  nil,
+			node: node,
+			setStatuses: func(node *corev1.Node, machine *machinev1.Machine) {
+				machine.Status = machinev1.MachineStatus{
+					NodeRef: &corev1.ObjectReference{
+						Name: nodeName,
+					},
+				}
+			},
+		},
+	}
+	for _, tc := range nodeGetterTestCases {
+		t.Run(tc.name, func(t *testing.T) {
+			gs := NewWithT(t)
+			tc.setStatuses(node, machine)
+			defer resetStatuses(node, machine)
+
+			node, err := machineScope.getNode()
+			if tc.err != nil {
+				gs.Expect(err).To(Equal(tc.err))
+			} else {
+				gs.Expect(err).To(BeNil())
+			}
+			gs.Expect(node).To(Equal(tc.node))
+		})
+	}
+
+	////////////// isNodeLinked test cases
+
+	isNodeLinkedTestCases := []struct {
+		name        string
+		expected    bool
+		setStatuses func(node *corev1.Node, machine *machinev1.Machine)
+	}{
+		{
+			name:        "isNodeLinked: No node linked",
+			expected:    false,
+			setStatuses: func(node *corev1.Node, machine *machinev1.Machine) {},
+		},
+		{
+			name:     "isNodeLinked: Node name empty",
+			expected: false,
+			setStatuses: func(node *corev1.Node, machine *machinev1.Machine) {
+				machine.Status = machinev1.MachineStatus{
+					NodeRef: &corev1.ObjectReference{
+						Name: "",
+					},
+				}
+			},
+		},
+		{
+			name:     "isNodeLinked: Node ref filled",
+			expected: true,
+			setStatuses: func(node *corev1.Node, machine *machinev1.Machine) {
+				machine.Status = machinev1.MachineStatus{
+					NodeRef: &corev1.ObjectReference{
+						Name: nodeName,
+					},
+				}
+			},
+		},
+	}
+	for _, tc := range isNodeLinkedTestCases {
+		t.Run(tc.name, func(t *testing.T) {
+			gs := NewWithT(t)
+
+			tc.setStatuses(node, machine)
+			defer resetStatuses(node, machine)
+
+			isNodeLinked := machineScope.isNodeLinked()
+
+			gs.Expect(isNodeLinked).To(Equal(tc.expected))
+		})
+	}
+
+	////////////// checkNodeReachable test cases
+
+	checkNodeReachableTestCases := []struct {
+		name        string
+		err         error
+		expected    bool
+		setStatuses func(node *corev1.Node, machine *machinev1.Machine)
+	}{
+		{
+			name:        "checkNodeReachable: node not linked",
+			err:         fmt.Errorf("NodeRef empty, unable to get related Node"),
+			expected:    false,
+			setStatuses: func(node *corev1.Node, machine *machinev1.Machine) {},
+		},
+		{
+			name:     "checkNodeReachable: node reachable",
+			err:      nil,
+			expected: true,
+			setStatuses: func(node *corev1.Node, machine *machinev1.Machine) {
+				machine.Status = machinev1.MachineStatus{
+					NodeRef: &corev1.ObjectReference{
+						Name: nodeName,
+					},
+				}
+				node.Status = corev1.NodeStatus{
+					Conditions: []corev1.NodeCondition{
+						{
+							Type:   corev1.NodeReady,
+							Status: corev1.ConditionTrue,
+						},
+					},
+				}
+				g.Expect(k8sClient.Status().Update(ctx, node)).To(Succeed())
+			},
+		},
+		{
+			name:     "checkNodeReachable: node unreachable",
+			err:      nil,
+			expected: false,
+			setStatuses: func(node *corev1.Node, machine *machinev1.Machine) {
+				machine.Status = machinev1.MachineStatus{
+					NodeRef: &corev1.ObjectReference{
+						Name: nodeName,
+					},
+				}
+				node.Status = corev1.NodeStatus{
+					Conditions: []corev1.NodeCondition{
+						{
+							Type:   corev1.NodeReady,
+							Status: corev1.ConditionUnknown,
+						},
+					},
+				}
+				g.Expect(k8sClient.Status().Update(ctx, node)).To(Succeed())
+			},
+		},
+	}
+	for _, tc := range checkNodeReachableTestCases {
+		t.Run(tc.name, func(t *testing.T) {
+			gs := NewWithT(t)
+			tc.setStatuses(node, machine)
+			defer resetStatuses(node, machine)
+
+			isNodeAvailable, err := machineScope.checkNodeReachable()
+
+			if tc.err != nil {
+				gs.Expect(err).To(Equal(tc.err))
+			} else {
+				gs.Expect(err).To(BeNil())
+			}
+			gs.Expect(isNodeAvailable).To(Equal(tc.expected))
+		})
+	}
+}

pkg/controller/vsphere/reconciler.go

@@ -7,6 +7,8 @@ import (
 	"fmt"
 	"strings"
 
+	apimachineryutilerrors "k8s.io/apimachinery/pkg/util/errors"
+
 	"github.com/google/uuid"
 	machinev1 "github.com/openshift/machine-api-operator/pkg/apis/machine/v1beta1"
 	vspherev1 "github.com/openshift/machine-api-operator/pkg/apis/vsphereprovider/v1beta1"
@@ -228,6 +230,19 @@ func (r *Reconciler) delete() error {
 		Ref:     vmRef,
 	}
 
+	if r.machineScope.isNodeLinked() {
+		isNodeReachable, err := r.machineScope.checkNodeReachable()
+		if err != nil {
+			return fmt.Errorf("%v: Can't check node status before vm destroy: %w", r.machine.GetName(), err)
+		}
+		if !isNodeReachable {
+			klog.Infof("%v: node not ready, kubelet unreachable for some reason. Detaching disks before vm destroy.", r.machine.GetName())
+			if err := vm.detachPVDisks(); err != nil {
+				return fmt.Errorf("%v: Failed to detach virtual disks related to pvs: %w", r.machine.GetName(), err)
+			}
+		}
+	}
+
 	if _, err := vm.powerOffVM(); err != nil {
 		return err
 	}
@@ -1002,6 +1017,55 @@ func (vm *virtualMachine) getNetworkStatusList(client *vim25.Client) ([]NetworkS
 	return networkStatusList, nil
 }
 
+type attachedDisk struct {
+	device   *types.VirtualDisk
+	fileName string
+	diskMode string
+}
+
+func (vm *virtualMachine) getAttachedDisks() ([]attachedDisk, error) {
+	var attachedDiskList []attachedDisk
+	devices, err := vm.Obj.Device(vm.Context)
+	if err != nil {
+		return nil, err
+	}
+
+	for _, disk := range devices.SelectByType((*types.VirtualDisk)(nil)) {
+		backingInfo := disk.GetVirtualDevice().Backing.(types.BaseVirtualDeviceFileBackingInfo).(*types.VirtualDiskFlatVer2BackingInfo)
+		attachedDiskList = append(attachedDiskList, attachedDisk{
+			device:   disk.(*types.VirtualDisk),
+			fileName: backingInfo.FileName,
+			diskMode: backingInfo.DiskMode,
+		})
+	}
+
+	return attachedDiskList, nil
+}
+
+func (vm *virtualMachine) detachPVDisks() error {
+	var errList []error
+	disks, err := vm.getAttachedDisks()
+	if err != nil {
+		return err
+	}
+	for _, disk := range disks {
+		// TODO (dmoiseev): should be enough, but maybe worth to check if its PV for sure
+		if disk.diskMode != string(types.VirtualDiskModePersistent) {
+			klog.V(3).Infof("Detaching disk associated with file %v", disk.fileName)
+			if err := vm.Obj.RemoveDevice(vm.Context, true, disk.device); err != nil {
+				errList = append(errList, err)
+				klog.Errorf("Failed to detach disk associated with file %v ", disk.fileName)
+			} else {
+				klog.V(3).Infof("Disk associated with file %v have been detached", disk.fileName)
+			}
+		}
+	}
+	if len(errList) > 0 {
+		return apimachineryutilerrors.NewAggregate(errList)
+	}
+	return nil
+}
+
 // IgnitionConfig returns a slice of option values that set the given data as
 // the guest's ignition config.
 func IgnitionConfig(data []byte) []types.BaseOptionValue {