🐛 fastDeploy directory cleanup (#1476)

controllers/virtualmachineimagecache/virtualmachineimagecache_controller.go

@@ -13,7 +13,6 @@ import (
 	"strings"
 
 	"github.com/go-logr/logr"
-	"github.com/vmware/govmomi/crypto"
 	"github.com/vmware/govmomi/fault"
 	"github.com/vmware/govmomi/find"
 	"github.com/vmware/govmomi/object"
@@ -375,10 +374,7 @@ func (r *reconciler) reconcileLocations(
 			dstDatastores[spec.DatastoreID].mo.Info.
 				GetDatastoreInfo().SupportedVDiskFormats...)
 
-		cryptoSpec, err := r.getStorageProfileCrypto(
-			ctx,
-			vimClient,
-			spec.ProfileID)
+		isEnc, err := kubeutil.IsEncryptedStorageProfile(ctx, r.Client, spec.ProfileID)
 		if err != nil {
 			conditions = conditions.MarkError(
 				vmopv1.ReadyConditionType,
@@ -392,7 +388,7 @@ func (r *reconciler) reconcileLocations(
 		for i := range srcFiles {
 			srcFiles[i].DstProfileID = spec.ProfileID
 			srcFiles[i].DstDiskFormat = dstDiskFormat
-			srcFiles[i].CryptoSpec = cryptoSpec
+			srcFiles[i].IsDstProfileEncrypted = isEnc
 		}
 
 		cachedFiles, err := r.cacheFiles(
@@ -418,38 +414,6 @@ func (r *reconciler) reconcileLocations(
 	}
 }
 
-func (r *reconciler) getStorageProfileCrypto(
-	ctx context.Context,
-	vimClient *vim25.Client,
-	profileID string) (*vimtypes.CryptoSpecEncrypt, error) {
-
-	if profileID == "" {
-		return nil, nil
-	}
-
-	isEnc, err := kubeutil.IsEncryptedStorageProfile(ctx, r.Client, profileID)
-	if err != nil || !isEnc {
-		return nil, err
-	}
-
-	// TODO: We don't have an EncryptionClass but the default provider is
-	// optional, so what is the proper fallback?
-	m := crypto.NewManagerKmip(vimClient)
-	providerID, err := m.GetDefaultKmsClusterID(ctx, nil, true)
-	if err != nil {
-		return nil, fmt.Errorf("failed to get default key provider: %w", err)
-	}
-
-	return &vimtypes.CryptoSpecEncrypt{
-		CryptoKeyId: vimtypes.CryptoKeyId{
-			ProviderId: &vimtypes.KeyProviderId{
-				Id: providerID,
-			},
-			KeyId: "",
-		},
-	}, nil
-}
-
 func (r *reconciler) cacheFiles(
 	ctx context.Context,
 	vimClient *vim25.Client,

main.go

@@ -107,9 +107,9 @@ func main() {
 }
 
 // initFeatures updates our enabled/disabled features based on the capabilities.
-// The inability to get the capabilities should not prevent the container from
-// starting as the features will be processed later by the capabilities
-// controller.
+// The pod will exit if capabilities cannot be loaded, as running with incorrect
+// capabilities causes the controller to process requests incorrectly and
+// trigger unnecessary restarts.
 func initFeatures() {
 	setupLog.Info("Initial features from environment",
 		"features", pkgcfg.FromContext(ctx).Features)
@@ -120,9 +120,13 @@ func initFeatures() {
 
 	c, err := client.New(ctrl.GetConfigOrDie(), client.Options{Scheme: scheme})
 	if err != nil {
-		setupLog.Error(err, "Failed to create client for updating capabilities")
-	} else if _, err := capabilities.UpdateCapabilities(logr.NewContext(ctx, setupLog), c); err != nil {
-		setupLog.Error(err, "Failed to update capabilities")
+		setupLog.Error(err, "Failed to create client for updating capabilities, exiting")
+		os.Exit(1)
+	}
+
+	if _, err := capabilities.UpdateCapabilities(logr.NewContext(ctx, setupLog), c); err != nil {
+		setupLog.Error(err, "Failed to update capabilities, exiting")
+		os.Exit(1)
 	}
 
 	setupLog.Info("Initial features from capabilities",

pkg/constants/constants.go

@@ -256,4 +256,8 @@ const (
 	// NoUnmanagedVolumesRegisterAnnotationKey is the annotation to not create any CNSRegisterVolumes
 	// by skipping the unmanaged volume register reconcile.
 	NoUnmanagedVolumesRegisterAnnotationKey = "vmoperator.vmware.com/no-unmanaged-volumes-register"
+
+	// PVCEncryptionClassNameAnnotation specifies the name of an EncryptionClass
+	// on a PVC.
+	PVCEncryptionClassNameAnnotation = "csi.vsphere.encryption-class"
 )

pkg/providers/vsphere/constants/constants.go

@@ -77,6 +77,8 @@ const (
 	// EncryptionClassNameAnnotation specifies the name of an EncryptionClass
 	// resource. This is used by APIs that participate in BYOK but cannot modify
 	// their spec to do so, such as the PersistentVolumeClaim API.
+	// TODO (aruneshpa): Is this instance storage only? CSI appears to use
+	// PVCEncryptionClassNameAnnotation nowadays.
 	EncryptionClassNameAnnotation = "encryption.vmware.com/encryption-class-name"
 
 	// InstanceStoragePVCNamePrefix prefix of auto-generated PVC names.

pkg/providers/vsphere/vmlifecycle/create_fastdeploy.go

@@ -138,6 +138,55 @@ func fastDeploy(
 		"vmDirPath", vmDirPath,
 		"vmPathName", createArgs.ConfigSpec.Files.VmPathName)
 
+	// Register cleanup defer immediately after directory creation.
+	defer func() {
+		if retErr == nil {
+			// Do not delete the VM directory if this function succeeded.
+			return
+		}
+
+		// Use a context that is not cancelled when the parent is, so cleanup
+		// runs even if the request is cancelled. Preserves the VC opID so
+		// cleanup is correlated with the failed create in VC logs.
+		ctx := context.WithoutCancel(vmCtx.Context)
+
+		// Delete the VM directory and its contents.
+		// Always use FileManager.DeleteDatastoreFile() first as it can delete
+		// non-empty directories recursively. Note that DeleteDirectory() on a
+		// non-empty directory will return an error, which is why we delete the
+		// directory contents first using DeleteDatastoreFile().
+		t, err := fm.DeleteDatastoreFile(ctx, vmDirPath, datacenter)
+		if err != nil {
+			retErr = fmt.Errorf(
+				"failed to call delete api for vm dir %q: %w,%w",
+				vmDirPath, err, retErr)
+			return
+		}
+
+		// Wait for the delete call to return.
+		if err := t.Wait(ctx); err != nil &&
+			!fault.Is(err, &vimtypes.FileNotFound{}) {
+
+			retErr = fmt.Errorf(
+				"failed to delete vm dir %q: %w,%w",
+				vmDirPath, err, retErr)
+		}
+
+		// For non-TLD datastores, also clean up the namespace mapping.
+		if !createArgs.Datastores[0].TopLevelDirectoryCreateSupported {
+			if err := nm.DeleteDirectory(
+				ctx,
+				datacenter,
+				vmDirUUIDPath); err != nil &&
+				!fault.Is(err, &vimtypes.FileNotFound{}) {
+
+				retErr = fmt.Errorf(
+					"failed to delete vm dir namespace mapping %q: %w,%w",
+					vmDirUUIDPath, err, retErr)
+			}
+		}
+	}()
+
 	dstDiskPaths := make([]string, len(srcDiskPaths))
 	for i := 0; i < len(dstDiskPaths); i++ {
 		dstDiskPaths[i] = fmt.Sprintf("%s/disk-%d.vmdk", vmDirPath, i)
@@ -226,62 +275,6 @@ func fastDeploy(
 		}
 	}
 
-	// If any error occurs after this point, the newly created VM directory and
-	// its contents need to be cleaned up.
-	defer func() {
-		if retErr == nil {
-			// Do not delete the VM directory if this function was successful.
-			return
-		}
-
-		// Use a new context to ensure cleanup happens even if the context
-		// is cancelled.
-		ctx := context.Background()
-
-		// Delete the VM directory and its contents.
-		if createArgs.Datastores[0].TopLevelDirectoryCreateSupported {
-			t, err := fm.DeleteDatastoreFile(ctx, vmDirPath, datacenter)
-			if err != nil {
-				err = fmt.Errorf(
-					"failed to call delete api for vm dir %q: %w",
-					vmDirPath, err)
-				if retErr == nil {
-					retErr = err
-				} else {
-					retErr = fmt.Errorf("%w,%w", err, retErr)
-				}
-				return
-			}
-
-			// Wait for the delete call to return.
-			if err := t.Wait(ctx); err != nil {
-				if !fault.Is(err, &vimtypes.FileNotFound{}) {
-					err = fmt.Errorf("failed to delete vm dir %q: %w",
-						vmDirPath, err)
-					if retErr == nil {
-						retErr = err
-					} else {
-						retErr = fmt.Errorf("%w,%w", err, retErr)
-					}
-				}
-			}
-		} else if err := nm.DeleteDirectory(
-			ctx,
-			datacenter,
-			vmDirUUIDPath); err != nil {
-
-			if !fault.Is(err, &vimtypes.FileNotFound{}) {
-				err = fmt.Errorf("failed to delete vm dir %q: %w",
-					vmDirUUIDPath, err)
-				if retErr == nil {
-					retErr = err
-				} else {
-					retErr = fmt.Errorf("%w,%w", err, retErr)
-				}
-			}
-		}
-	}()
-
 	folder := object.NewFolder(vimClient, vimtypes.ManagedObjectReference{
 		Type:  "Folder",
 		Value: createArgs.FolderMoID,