diff --git a/pkg/controller/vsphere/reconciler.go b/pkg/controller/vsphere/reconciler.go index 7075de326..1dd4ea3e9 100644 --- a/pkg/controller/vsphere/reconciler.go +++ b/pkg/controller/vsphere/reconciler.go @@ -27,8 +27,10 @@ import ( "github.com/vmware/govmomi/vim25/types" corev1 "k8s.io/api/core/v1" + storagev1 "k8s.io/api/storage/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/fields" apimachinerytypes "k8s.io/apimachinery/pkg/types" apimachineryutilerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/component-base/featuregate" @@ -41,6 +43,8 @@ import ( machinecontroller "github.com/openshift/machine-api-operator/pkg/controller/machine" "github.com/openshift/machine-api-operator/pkg/controller/vsphere/session" "github.com/openshift/machine-api-operator/pkg/metrics" + + runtimeclient "sigs.k8s.io/controller-runtime/pkg/client" ) const ( @@ -55,6 +59,10 @@ const ( // Not all controllers support up to 30, but the maximum is 30. // xref: https://docs.vmware.com/en/VMware-vSphere/8.0/vsphere-vm-administration/GUID-5872D173-A076-42FE-8D0B-9DB0EB0E7362.html#:~:text=If%20you%20add%20a%20hard,values%20from%200%20to%2014. maxUnitNumber = 30 + // VSphereCSIDriverName is the CSI driver name for vSphere volumes. + VSphereCSIDriverName = "csi.vsphere.vmware.com" + // VSphereInTreePluginName is the in-tree plugin name for vSphere volumes. + VSphereInTreePluginName = "kubernetes.io/vsphere-volume" ) // These are the guestinfo variables used by Ignition. @@ -534,11 +542,11 @@ func (r *Reconciler) delete() error { return fmt.Errorf("destroying vm in progress, requeuing") } -// nodeHasVolumesAttached returns true if node status still have volumes attached -// pod deletion and volume detach happen asynchronously, so pod could be deleted before volume detached from the node -// this could cause issue for some storage provisioner, for example, vsphere-volume this is problematic -// because if the node is deleted before detach success, then the underline VMDK will be deleted together with the Machine -// so after node draining we need to check if all volumes are detached before deleting the node. +// nodeHasVolumesAttached returns true if node status still has vSphere-backed volumes attached. +// Pod deletion and volume detach happen asynchronously, so pod could be deleted before volume detached from the node. +// This is problematic for vSphere volumes because if the node is deleted before detach succeeds, +// the underlying VMDK will be deleted together with the Machine. +// Non-vSphere volumes (NFS, iSCSI, etc.) do not have this risk since vSphere Destroy_Task does not affect them. func (r *Reconciler) nodeHasVolumesAttached(ctx context.Context, nodeName string, machineName string) (bool, error) { node := &corev1.Node{} if err := r.apiReader.Get(ctx, apimachinerytypes.NamespacedName{Name: nodeName}, node); err != nil { @@ -549,7 +557,91 @@ func (r *Reconciler) nodeHasVolumesAttached(ctx context.Context, nodeName string return true, err } - return len(node.Status.VolumesAttached) != 0, nil + if len(node.Status.VolumesAttached) == 0 { + return false, nil + } + + klog.V(3).Infof("Machine %s: checking %d attached volumes on node %s for vSphere-backed volumes", machineName, len(node.Status.VolumesAttached), nodeName) + + var vsphereVolumes []string + var nonVSphereVolumes []string + var unknownVolumes []string + + for _, vol := range node.Status.VolumesAttached { + volName := string(vol.Name) + va, err := r.getVolumeAttachmentForAttachedVolume(ctx, volName, nodeName, machineName) + if err != nil { + klog.Warningf("Machine %s: failed to get VolumeAttachment for %s: %v, conservatively treating as vSphere-backed", machineName, volName, err) + vsphereVolumes = append(vsphereVolumes, volName) + continue + } + if va == nil { + klog.Warningf("Machine %s: VolumeAttachment for %s not found, conservatively treating as vSphere-backed", machineName, volName) + vsphereVolumes = append(vsphereVolumes, volName) + continue + } + + switch va.Spec.Attacher { + case VSphereCSIDriverName, VSphereInTreePluginName: + vsphereVolumes = append(vsphereVolumes, volName) + case "": + unknownVolumes = append(unknownVolumes, fmt.Sprintf("%s (attacher: empty)", volName)) + case "nfs.csi.k8s.io", "csi.nfs.io", "iscsi.csi.k8s.io", "cinder.csi.openstack.org", + "ebs.csi.aws.com", "pd.csi.storage.gke.io", "disk.csi.azure.com": + nonVSphereVolumes = append(nonVSphereVolumes, fmt.Sprintf("%s (attacher: %s)", volName, va.Spec.Attacher)) + default: + unknownVolumes = append(unknownVolumes, fmt.Sprintf("%s (attacher: %s)", volName, va.Spec.Attacher)) + } + } + + if len(vsphereVolumes) > 0 { + klog.Warningf("Machine %s: vSphere-backed volumes still attached on node %s: %v", machineName, nodeName, vsphereVolumes) + } + if len(nonVSphereVolumes) > 0 { + klog.V(3).Infof("Machine %s: non-vSphere volumes attached (safe to ignore): %v", machineName, nonVSphereVolumes) + } + if len(unknownVolumes) > 0 { + klog.Warningf("Machine %s on node %s: volumes with unknown type attached, conservatively blocking deletion: %v", machineName, nodeName, unknownVolumes) + } + + return len(vsphereVolumes) > 0 || len(unknownVolumes) > 0, nil +} + +// getVolumeAttachmentForAttachedVolume retrieves the VolumeAttachment corresponding to an attached volume +// on a node. It first tries a direct lookup by the volume name, then falls back to listing and +// correlating VolumeAttachments for the node (needed for CSI volumes with hashed names like +// csi-). +func (r *Reconciler) getVolumeAttachmentForAttachedVolume(ctx context.Context, volName, nodeName, machineName string) (*storagev1.VolumeAttachment, error) { + va := &storagev1.VolumeAttachment{} + if err := r.apiReader.Get(ctx, apimachinerytypes.NamespacedName{Name: volName}, va); err != nil { + if !apierrors.IsNotFound(err) { + return nil, err + } + klog.V(4).Infof("Machine %s: VolumeAttachment %s not found by name, attempting correlation via list", machineName, volName) + } else { + return va, nil + } + + fieldSelector, err := fields.ParseSelector("spec.nodeName=" + nodeName) + if err != nil { + return nil, err + } + + vaList := &storagev1.VolumeAttachmentList{} + if err := r.apiReader.List(ctx, vaList, &runtimeclient.ListOptions{FieldSelector: fieldSelector}); err != nil { + return nil, err + } + + for i := range vaList.Items { + va := &vaList.Items[i] + if va.Name == volName { + klog.V(4).Infof("Machine %s: correlated VolumeAttachment %s for node %s", machineName, va.Name, nodeName) + return va, nil + } + } + + klog.V(4).Infof("Machine %s: no VolumeAttachment found for %s on node %s after listing %d attachments", machineName, volName, nodeName, len(vaList.Items)) + return nil, nil } // reconcileMachineWithCloudState reconcile machineSpec and status with the latest cloud state diff --git a/pkg/controller/vsphere/reconciler_test.go b/pkg/controller/vsphere/reconciler_test.go index 3fc67d993..ec96cc047 100644 --- a/pkg/controller/vsphere/reconciler_test.go +++ b/pkg/controller/vsphere/reconciler_test.go @@ -25,6 +25,7 @@ import ( "reflect" "strings" "testing" + "time" . "github.com/onsi/gomega" @@ -36,7 +37,9 @@ import ( "github.com/vmware/govmomi/vim25/types" corev1 "k8s.io/api/core/v1" + storagev1 "k8s.io/api/storage/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + apimachineryruntime "k8s.io/apimachinery/pkg/runtime" apimachinerytypes "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/kubernetes/scheme" vsphere "k8s.io/cloud-provider-vsphere/pkg/common/config" @@ -2449,6 +2452,535 @@ func TestDelete(t *testing.T) { } } +func TestDeleteWithVolumeTypeFiltering(t *testing.T) { + type vCenterSimConfig struct { + secret *corev1.Secret + configMap *corev1.ConfigMap + featureGate *configv1.FeatureGate + host string + port string + username string + pwd string + simServer *simulator.Server + } + + namespace := "test" + nodeName := "somenodename" + instanceUUID := "5001d986-65e4-5598-93d4-6b86b37d4415" + + getVcenterSimParams := func(server *simulator.Server, ns string) (*vCenterSimConfig, error) { + host, port, err := net.SplitHostPort(server.URL.Host) + if err != nil { + return nil, err + } + unameKey := fmt.Sprintf("%s.username", host) + pwdKey := fmt.Sprintf("%s.password", host) + + password, _ := server.URL.User.Password() + + credentialsSecretName := "test" + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: credentialsSecretName, + Namespace: ns, + }, + Data: map[string][]byte{ + unameKey: []byte(server.URL.User.Username()), + pwdKey: []byte(password), + }, + } + + testConfig := fmt.Sprintf(testConfigFmt, port, credentialsSecretName, ns) + configMap := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: OpenshiftConfigManagedConfigMap, + Namespace: openshiftConfigNamespaceForTest, + }, + Data: map[string]string{ + OpenshiftConfigManagedCloudConfigKey: testConfig, + }, + } + + featureGate := &configv1.FeatureGate{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster", + }, + } + + return &vCenterSimConfig{ + secret: secret, + configMap: configMap, + host: host, + port: port, + username: server.URL.User.Username(), + pwd: password, + simServer: server, + featureGate: featureGate, + }, nil + } + + getMachineWithStatus := func(t *testing.T, status machinev1.MachineStatus, simHost string) *machinev1.Machine { + providerSpec := machinev1.VSphereMachineProviderSpec{ + CredentialsSecret: &corev1.LocalObjectReference{ + Name: "test", + }, + Workspace: &machinev1.Workspace{ + Server: simHost, + }, + } + raw, err := RawExtensionFromProviderSpec(&providerSpec) + if err != nil { + t.Fatal(err) + } + return &machinev1.Machine{ + TypeMeta: metav1.TypeMeta{ + Kind: "Machine", + }, + ObjectMeta: metav1.ObjectMeta{ + UID: apimachinerytypes.UID(instanceUUID), + Name: "defaultFolder", + Namespace: namespace, + }, + Spec: machinev1.MachineSpec{ + ProviderSpec: machinev1.ProviderSpec{ + Value: raw, + }, + }, + Status: status, + } + } + + getNodeWithConditions := func(conditions []corev1.NodeCondition) *corev1.Node { + return &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: nodeName, + Namespace: metav1.NamespaceNone, + }, + TypeMeta: metav1.TypeMeta{ + Kind: "Node", + }, + Status: corev1.NodeStatus{ + Conditions: conditions, + }, + } + } + + addDiskToVm := func(ctx context.Context, simVm *simulator.VirtualMachine, diskName string, simClient *vim25.Client) error { + managedObjRef := simVm.VirtualMachine.Reference() + vmObj := object.NewVirtualMachine(simClient, managedObjRef) + devices, err := vmObj.Device(ctx) + if err != nil { + return err + } + scsi := devices.SelectByType((*types.VirtualSCSIController)(nil))[0] + + additionalDisk := &types.VirtualDisk{ + VirtualDevice: types.VirtualDevice{ + Backing: &types.VirtualDiskFlatVer2BackingInfo{ + DiskMode: string(types.VirtualDiskModePersistent), + ThinProvisioned: types.NewBool(true), + VirtualDeviceFileBackingInfo: types.VirtualDeviceFileBackingInfo{ + FileName: fmt.Sprintf("[LocalDS_0] %s/%s.vmdk", simVm.Name, diskName), + Datastore: &simVm.Datastore[0], + }, + }, + }, + } + additionalDisk.CapacityInKB = 1024 + devices.AssignController(additionalDisk, scsi.(types.BaseVirtualController)) + + err = vmObj.AddDevice(ctx, additionalDisk) + if err != nil { + return err + } + return nil + } + + volumeTypeFilteringTestCases := []struct { + name string + machine func(t *testing.T, simServerHost string) *machinev1.Machine + node func(t *testing.T) *corev1.Node + volumeAttachments []runtimeclient.Object + attachDisks bool + secondReconcileError string + }{ + { + name: "NFS volumes attached, deletion proceeds", + machine: func(t *testing.T, simServerHost string) *machinev1.Machine { + return getMachineWithStatus(t, machinev1.MachineStatus{ + NodeRef: &corev1.ObjectReference{ + Name: nodeName, + }, + }, simServerHost) + }, + node: func(t *testing.T) *corev1.Node { + node := getNodeWithConditions([]corev1.NodeCondition{ + { + Type: corev1.NodeReady, + Status: corev1.ConditionUnknown, + }, + }) + node.Status.VolumesAttached = []corev1.AttachedVolume{ + { + Name: "csi-nfs-123", + DevicePath: "/dev/sda", + }, + } + return node + }, + volumeAttachments: []runtimeclient.Object{ + &storagev1.VolumeAttachment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "csi-nfs-123", + }, + Spec: storagev1.VolumeAttachmentSpec{ + Attacher: "nfs.csi.k8s.io", + NodeName: nodeName, + }, + }, + }, + attachDisks: false, + secondReconcileError: "destroying vm in progress, requeuing", + }, + { + name: "vSphere CSI volumes attached, deletion blocked", + machine: func(t *testing.T, simServerHost string) *machinev1.Machine { + return getMachineWithStatus(t, machinev1.MachineStatus{ + NodeRef: &corev1.ObjectReference{ + Name: nodeName, + }, + }, simServerHost) + }, + node: func(t *testing.T) *corev1.Node { + node := getNodeWithConditions([]corev1.NodeCondition{ + { + Type: corev1.NodeReady, + Status: corev1.ConditionUnknown, + }, + }) + node.Status.VolumesAttached = []corev1.AttachedVolume{ + { + Name: "csi-vsphere-456", + DevicePath: "/dev/sdb", + }, + } + return node + }, + volumeAttachments: []runtimeclient.Object{ + &storagev1.VolumeAttachment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "csi-vsphere-456", + }, + Spec: storagev1.VolumeAttachmentSpec{ + Attacher: VSphereCSIDriverName, + NodeName: nodeName, + }, + }, + }, + attachDisks: true, + secondReconcileError: "node somenodename has attached volumes, requeuing", + }, + { + name: "vSphere in-tree volumes attached, deletion blocked", + machine: func(t *testing.T, simServerHost string) *machinev1.Machine { + return getMachineWithStatus(t, machinev1.MachineStatus{ + NodeRef: &corev1.ObjectReference{ + Name: nodeName, + }, + }, simServerHost) + }, + node: func(t *testing.T) *corev1.Node { + node := getNodeWithConditions([]corev1.NodeCondition{ + { + Type: corev1.NodeReady, + Status: corev1.ConditionUnknown, + }, + }) + node.Status.VolumesAttached = []corev1.AttachedVolume{ + { + Name: "vsphere-in-tree-789", + DevicePath: "/dev/sdc", + }, + } + return node + }, + volumeAttachments: []runtimeclient.Object{ + &storagev1.VolumeAttachment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "vsphere-in-tree-789", + }, + Spec: storagev1.VolumeAttachmentSpec{ + Attacher: VSphereInTreePluginName, + NodeName: nodeName, + }, + }, + }, + attachDisks: true, + secondReconcileError: "node somenodename has attached volumes, requeuing", + }, + { + name: "Mixed volumes (NFS + vSphere), deletion blocked", + machine: func(t *testing.T, simServerHost string) *machinev1.Machine { + return getMachineWithStatus(t, machinev1.MachineStatus{ + NodeRef: &corev1.ObjectReference{ + Name: nodeName, + }, + }, simServerHost) + }, + node: func(t *testing.T) *corev1.Node { + node := getNodeWithConditions([]corev1.NodeCondition{ + { + Type: corev1.NodeReady, + Status: corev1.ConditionUnknown, + }, + }) + node.Status.VolumesAttached = []corev1.AttachedVolume{ + { + Name: "csi-nfs-123", + DevicePath: "/dev/sda", + }, + { + Name: "csi-vsphere-456", + DevicePath: "/dev/sdb", + }, + } + return node + }, + volumeAttachments: []runtimeclient.Object{ + &storagev1.VolumeAttachment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "csi-nfs-123", + }, + Spec: storagev1.VolumeAttachmentSpec{ + Attacher: "nfs.csi.k8s.io", + NodeName: nodeName, + }, + }, + &storagev1.VolumeAttachment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "csi-vsphere-456", + }, + Spec: storagev1.VolumeAttachmentSpec{ + Attacher: VSphereCSIDriverName, + NodeName: nodeName, + }, + }, + }, + attachDisks: true, + secondReconcileError: "node somenodename has attached volumes, requeuing", + }, + { + name: "Non-vSphere attacher, deletion proceeds", + machine: func(t *testing.T, simServerHost string) *machinev1.Machine { + return getMachineWithStatus(t, machinev1.MachineStatus{ + NodeRef: &corev1.ObjectReference{ + Name: nodeName, + }, + }, simServerHost) + }, + node: func(t *testing.T) *corev1.Node { + node := getNodeWithConditions([]corev1.NodeCondition{ + { + Type: corev1.NodeReady, + Status: corev1.ConditionUnknown, + }, + }) + node.Status.VolumesAttached = []corev1.AttachedVolume{ + { + Name: "csi-iscsi-789", + DevicePath: "/dev/sdc", + }, + } + return node + }, + volumeAttachments: []runtimeclient.Object{ + &storagev1.VolumeAttachment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "csi-iscsi-789", + }, + Spec: storagev1.VolumeAttachmentSpec{ + Attacher: "iscsi.csi.k8s.io", + NodeName: nodeName, + }, + }, + }, + attachDisks: false, + secondReconcileError: "destroying vm in progress, requeuing", + }, + { + name: "Volume attached but no VolumeAttachment, deletion blocked conservatively", + machine: func(t *testing.T, simServerHost string) *machinev1.Machine { + return getMachineWithStatus(t, machinev1.MachineStatus{ + NodeRef: &corev1.ObjectReference{ + Name: nodeName, + }, + }, simServerHost) + }, + node: func(t *testing.T) *corev1.Node { + node := getNodeWithConditions([]corev1.NodeCondition{ + { + Type: corev1.NodeReady, + Status: corev1.ConditionUnknown, + }, + }) + node.Status.VolumesAttached = []corev1.AttachedVolume{ + { + Name: "csi-missing-va", + DevicePath: "/dev/sdd", + }, + } + return node + }, + volumeAttachments: []runtimeclient.Object{}, + attachDisks: true, + secondReconcileError: "node somenodename has attached volumes, requeuing", + }, + { + name: "VolumeAttachment with empty attacher, deletion blocked conservatively", + machine: func(t *testing.T, simServerHost string) *machinev1.Machine { + return getMachineWithStatus(t, machinev1.MachineStatus{ + NodeRef: &corev1.ObjectReference{ + Name: nodeName, + }, + }, simServerHost) + }, + node: func(t *testing.T) *corev1.Node { + node := getNodeWithConditions([]corev1.NodeCondition{ + { + Type: corev1.NodeReady, + Status: corev1.ConditionUnknown, + }, + }) + node.Status.VolumesAttached = []corev1.AttachedVolume{ + { + Name: "csi-empty-attacher", + DevicePath: "/dev/sde", + }, + } + return node + }, + volumeAttachments: []runtimeclient.Object{ + &storagev1.VolumeAttachment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "csi-empty-attacher", + }, + Spec: storagev1.VolumeAttachmentSpec{ + Attacher: "", + NodeName: nodeName, + }, + }, + }, + attachDisks: true, + secondReconcileError: "node somenodename has attached volumes, requeuing", + }, + { + name: "VolumeAttachment with unrecognized attacher, deletion blocked conservatively", + machine: func(t *testing.T, simServerHost string) *machinev1.Machine { + return getMachineWithStatus(t, machinev1.MachineStatus{ + NodeRef: &corev1.ObjectReference{ + Name: nodeName, + }, + }, simServerHost) + }, + node: func(t *testing.T) *corev1.Node { + node := getNodeWithConditions([]corev1.NodeCondition{ + { + Type: corev1.NodeReady, + Status: corev1.ConditionUnknown, + }, + }) + node.Status.VolumesAttached = []corev1.AttachedVolume{ + { + Name: "csi-unknown-attacher", + DevicePath: "/dev/sdf", + }, + } + return node + }, + volumeAttachments: []runtimeclient.Object{ + &storagev1.VolumeAttachment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "csi-unknown-attacher", + }, + Spec: storagev1.VolumeAttachmentSpec{ + Attacher: "unknown.custom.driver.io", + NodeName: nodeName, + }, + }, + }, + attachDisks: true, + secondReconcileError: "node somenodename has attached volumes, requeuing", + }, + } + for _, tc := range volumeTypeFilteringTestCases { + t.Run(tc.name, func(t *testing.T) { + g := NewWithT(t) + + model, sess, srv := initSimulator(t) + defer func() { + model.Remove() + srv.Close() + }() + simParams, err := getVcenterSimParams(srv, namespace) + g.Expect(err).NotTo(HaveOccurred()) + + vm := model.Map().Any("VirtualMachine").(*simulator.VirtualMachine) + vm.Config.InstanceUuid = instanceUUID + + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + + if tc.attachDisks { + simClient := sess.Client.Client + g.Expect(addDiskToVm(ctx, vm, fmt.Sprintf("%s-%s", vm.Name, tc.name), simClient)).To(Succeed()) + } + + nodeNameIndexExtractor := func(rawObj runtimeclient.Object) []string { + pod := rawObj.(*corev1.Pod) + return []string{pod.Spec.NodeName} + } + + vaNodeNameIndexExtractor := func(rawObj runtimeclient.Object) []string { + va := rawObj.(*storagev1.VolumeAttachment) + return []string{va.Spec.NodeName} + } + + var objects []apimachineryruntime.Object + objects = append(objects, + simParams.secret, + tc.machine(t, simParams.host), + simParams.configMap, + tc.node(t), + ) + for _, va := range tc.volumeAttachments { + objects = append(objects, va) + } + + cl := fake.NewClientBuilder().WithScheme( + scheme.Scheme, + ).WithIndex(&corev1.Pod{}, "spec.nodeName", nodeNameIndexExtractor).WithIndex(&storagev1.VolumeAttachment{}, "spec.nodeName", vaNodeNameIndexExtractor).WithRuntimeObjects( + objects..., + ).Build() + mScope, err := newMachineScope(machineScopeParams{ + client: cl, + Context: ctx, + machine: tc.machine(t, simParams.host), + apiReader: cl, + openshiftConfigNameSpace: openshiftConfigNamespaceForTest, + }) + g.Expect(err).NotTo(HaveOccurred()) + + reconciler := newReconciler(mScope) + + // First call powers off the VM + g.Expect(reconciler.delete()).To(MatchError(ContainSubstring("powering off vm is in progress, requeuing"))) + + // Second reconciliation should behave according to volume types + g.Expect(reconciler.delete()).To(MatchError(ContainSubstring(tc.secondReconcileError))) + }) + } +} + func TestCreate(t *testing.T) { model, session, server := initSimulator(t) defer model.Remove()