Add pvc size validation

Add a validation for the pvc size that should be enough to contain the memory dump. The size should be the vm memory plus a hard coded overhead of 100Mi - that should be enough after some checking and testing examples. Also moved the pvc validation from virtctl to subresource. Signed-off-by: Shelly Kagan <[email protected]>
bnabholz · May 24, 2022 · 0e42243 · 0e42243
1 parent c593972
commit 0e42243
Show file tree

Hide file tree

Showing 9 changed files with 157 additions and 64 deletions.
diff --git a/manifests/generated/operator-csv.yaml.in b/manifests/generated/operator-csv.yaml.in
@@ -364,6 +364,12 @@ spec:
           - watch
           - patch
           - update
+        - apiGroups:
+          - ""
+          resources:
+          - persistentvolumeclaims
+          verbs:
+          - get
         - apiGroups:
           - kubevirt.io
           resources:

diff --git a/manifests/generated/rbac-operator.authorization.k8s.yaml.in b/manifests/generated/rbac-operator.authorization.k8s.yaml.in
@@ -266,6 +266,12 @@ rules:
   - watch
   - patch
   - update
+- apiGroups:
+  - ""
+  resources:
+  - persistentvolumeclaims
+  verbs:
+  - get
 - apiGroups:
   - kubevirt.io
   resources:

diff --git a/pkg/virt-api/rest/BUILD.bazel b/pkg/virt-api/rest/BUILD.bazel
@@ -40,6 +40,7 @@ go_library(
         "//vendor/k8s.io/api/core/v1:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/api/equality:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/api/errors:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/api/resource:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/fields:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/labels:go_default_library",
@@ -80,6 +81,7 @@ go_test(
         "//vendor/github.com/onsi/gomega/ghttp:go_default_library",
         "//vendor/k8s.io/api/core/v1:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/api/errors:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/api/resource:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/types:go_default_library",

diff --git a/pkg/virt-api/rest/subresource.go b/pkg/virt-api/rest/subresource.go
@@ -33,6 +33,7 @@ import (
 	v12 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/equality"
 	"k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/api/resource"
 	k8smetav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/fields"
 	"k8s.io/apimachinery/pkg/labels"
@@ -59,6 +60,8 @@ const (
 	vmiGuestAgentErr             = "VMI does not have guest agent connected"
 	prepConnectionErrFmt         = "Cannot prepare connection %s"
 	getRequestErrFmt             = "Cannot GET request %s"
+	pvcVolumeModeErr             = "pvc should be filesystem pvc"
+	pvcSizeErrFmt                = "pvc size should be bigger then vm memory:%s+%s"
 	defaultProfilerComponentPort = 8443
 )
 
@@ -1329,6 +1332,40 @@ func generateVMMemoryDumpRequestPatch(vm *v1.VirtualMachine, memoryDumpReq *v1.V
 	return patch, nil
 }
 
+func (app *SubresourceAPIApp) fetchPersistentVolumeClaim(name string, namespace string) (*v12.PersistentVolumeClaim, *errors.StatusError) {
+	pvc, err := app.virtCli.CoreV1().PersistentVolumeClaims(namespace).Get(context.TODO(), name, k8smetav1.GetOptions{})
+	if err != nil {
+		if errors.IsNotFound(err) {
+			return nil, errors.NewNotFound(v1.Resource("persistentvolumeclaim"), name)
+		}
+		return nil, errors.NewInternalError(fmt.Errorf("unable to retrieve pvc [%s]: %v", name, err))
+	}
+	return pvc, nil
+}
+
+func (app *SubresourceAPIApp) validateMemoryDumpClaim(vmi *v1.VirtualMachineInstance, claimName, namespace string) *errors.StatusError {
+	pvc, err := app.fetchPersistentVolumeClaim(claimName, namespace)
+	if err != nil {
+		return err
+	}
+	if pvc.Spec.VolumeMode != nil && *pvc.Spec.VolumeMode == v12.PersistentVolumeBlock {
+		return errors.NewConflict(v1.Resource("persistentvolumeclaim"), claimName, fmt.Errorf(pvcVolumeModeErr))
+	}
+
+	pvcSize := pvc.Spec.Resources.Requests.Storage()
+	scaledPvcSize := resource.NewScaledQuantity(pvcSize.ScaledValue(resource.Kilo), resource.Kilo)
+	domain := vmi.Spec.Domain
+	vmiMemoryReq := domain.Resources.Requests.Memory()
+	memOverhead := resource.MustParse("100Mi")
+	expectedPvcSize := resource.NewScaledQuantity(vmiMemoryReq.ScaledValue(resource.Kilo), resource.Kilo)
+	expectedPvcSize.Add(memOverhead)
+	if scaledPvcSize.Cmp(*expectedPvcSize) < 0 {
+		return errors.NewConflict(v1.Resource("persistentvolumeclaim"), claimName, fmt.Errorf(pvcSizeErrFmt, vmiMemoryReq.String(), memOverhead.String()))
+	}
+
+	return nil
+}
+
 func (app *SubresourceAPIApp) vmMemoryDumpRequestPatchStatus(name, namespace string, memoryDumpReq *v1.VirtualMachineMemoryDumpRequest, removeRequest bool) *errors.StatusError {
 	vm, statErr := app.fetchVirtualMachine(name, namespace)
 	if statErr != nil {
@@ -1344,6 +1381,10 @@ func (app *SubresourceAPIApp) vmMemoryDumpRequestPatchStatus(name, namespace str
 		if !vmi.IsRunning() {
 			return errors.NewConflict(v1.Resource("virtualmachineinstance"), name, fmt.Errorf(vmiNotRunning))
 		}
+
+		if err := app.validateMemoryDumpClaim(vmi, memoryDumpReq.ClaimName, namespace); err != nil {
+			return err
+		}
 	}
 
 	patch, err := generateVMMemoryDumpRequestPatch(vm, memoryDumpReq, removeRequest)

diff --git a/pkg/virt-api/rest/subresource_test.go b/pkg/virt-api/rest/subresource_test.go
@@ -51,6 +51,7 @@ import (
 
 	k8sv1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/api/resource"
 	k8smetav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/uuid"
 
@@ -1041,18 +1042,45 @@ var _ = Describe("VirtualMachineInstance Subresources", func() {
 	})
 
 	Context("Memory dump Subresource api", func() {
+		const (
+			fs          = false
+			block       = true
+			testPVCName = "testPVC"
+		)
 
 		newMemoryDumpBody := func(req *v1.VirtualMachineMemoryDumpRequest) io.ReadCloser {
 			reqJson, _ := json.Marshal(req)
 			return &readCloserWrapper{bytes.NewReader(reqJson)}
 		}
 
+		createTestPVC := func(size string, blockMode bool) *k8sv1.PersistentVolumeClaim {
+			quantity, _ := resource.ParseQuantity(size)
+			pvc := &k8sv1.PersistentVolumeClaim{
+				ObjectMeta: k8smetav1.ObjectMeta{
+					Name:      testPVCName,
+					Namespace: k8smetav1.NamespaceDefault,
+				},
+				Spec: k8sv1.PersistentVolumeClaimSpec{
+					Resources: k8sv1.ResourceRequirements{
+						Requests: k8sv1.ResourceList{
+							k8sv1.ResourceStorage: quantity,
+						},
+					},
+				},
+			}
+			if blockMode {
+				volumeMode := k8sv1.PersistentVolumeBlock
+				pvc.Spec.VolumeMode = &volumeMode
+			}
+			return pvc
+		}
+
 		BeforeEach(func() {
 			request.PathParameters()["name"] = testVMName
 			request.PathParameters()["namespace"] = k8smetav1.NamespaceDefault
 		})
 
-		DescribeTable("With memory dump request", func(memDumpReq *v1.VirtualMachineMemoryDumpRequest, statusCode int, enableGate bool, vmiRunning bool) {
+		DescribeTable("With memory dump request", func(memDumpReq *v1.VirtualMachineMemoryDumpRequest, statusCode int, enableGate bool, vmiRunning bool, pvc *k8sv1.PersistentVolumeClaim) {
 
 			if enableGate {
 				enableFeatureGate(virtconfig.HotplugVolumesGate)
@@ -1070,6 +1098,19 @@ var _ = Describe("VirtualMachineInstance Subresources", func() {
 			if vmiRunning {
 				vmi = api.NewMinimalVMI(testVMIName)
 				vmi.Status.Phase = v1.Running
+				vmi.Spec.Domain.Resources.Requests = k8sv1.ResourceList{
+					k8sv1.ResourceMemory: resource.MustParse("1Gi"),
+				}
+				kubeClient.Fake.PrependReactor("get", "persistentvolumeclaims", func(action testing.Action) (bool, runtime.Object, error) {
+					get, ok := action.(testing.GetAction)
+					Expect(ok).To(BeTrue())
+					Expect(get.GetNamespace()).To(Equal(k8smetav1.NamespaceDefault))
+					Expect(get.GetName()).To(Equal(testPVCName))
+					if pvc == nil {
+						return true, nil, errors.NewNotFound(v1.Resource("persistentvolumeclaim"), testPVCName)
+					}
+					return true, pvc, nil
+				})
 			}
 			vmiClient.EXPECT().Get(vm.Name, &k8smetav1.GetOptions{}).Return(vmi, nil).AnyTimes()
 			vmClient.EXPECT().PatchStatus(vm.Name, types.JSONPatchType, gomock.Any(), gomock.Any()).DoAndReturn(
@@ -1081,26 +1122,39 @@ var _ = Describe("VirtualMachineInstance Subresources", func() {
 			Expect(response.StatusCode()).To(Equal(statusCode))
 		},
 			Entry("VM with a valid memory dump request should succeed", &v1.VirtualMachineMemoryDumpRequest{
-				ClaimName: "vol1",
+				ClaimName: testPVCName,
 				Phase:     v1.MemoryDumpAssociating,
-			}, http.StatusAccepted, true, true),
+			}, http.StatusAccepted, true, true, createTestPVC("2Gi", fs)),
 			Entry("VM with an invalid memory dump request that's missing a claim name should fail", &v1.VirtualMachineMemoryDumpRequest{
 				Phase: v1.MemoryDumpAssociating,
-			}, http.StatusBadRequest, true, true),
+			}, http.StatusBadRequest, true, true, createTestPVC("2Gi", fs)),
 			Entry("VM with an invalid memory dump request that's missing a phase should fail", &v1.VirtualMachineMemoryDumpRequest{
-				ClaimName: "vol1",
-			}, http.StatusBadRequest, true, true),
+				ClaimName: testPVCName,
+			}, http.StatusBadRequest, true, true, createTestPVC("2Gi", fs)),
 			Entry("VM with an invalid memory dump request that's has phase different then 'Associating' should fail", &v1.VirtualMachineMemoryDumpRequest{
-				Phase: v1.MemoryDumpCompleted,
-			}, http.StatusBadRequest, true, true),
+				Phase:     v1.MemoryDumpCompleted,
+				ClaimName: testPVCName,
+			}, http.StatusBadRequest, true, true, createTestPVC("2Gi", fs)),
 			Entry("VM with a valid memory dump request but no feature gate should fail", &v1.VirtualMachineMemoryDumpRequest{
-				ClaimName: "vol1",
+				ClaimName: testPVCName,
 				Phase:     v1.MemoryDumpAssociating,
-			}, http.StatusBadRequest, false, true),
+			}, http.StatusBadRequest, false, true, createTestPVC("2Gi", fs)),
 			Entry("VM with a valid memory dump request vmi not running should fail", &v1.VirtualMachineMemoryDumpRequest{
-				ClaimName: "vol1",
+				ClaimName: testPVCName,
+				Phase:     v1.MemoryDumpAssociating,
+			}, http.StatusConflict, true, false, createTestPVC("2Gi", fs)),
+			Entry("VM with a memory dump request with a non existing PVC", &v1.VirtualMachineMemoryDumpRequest{
+				ClaimName: testPVCName,
+				Phase:     v1.MemoryDumpAssociating,
+			}, http.StatusNotFound, true, true, nil),
+			Entry("VM with a memory dump request pvc block mode should fail", &v1.VirtualMachineMemoryDumpRequest{
+				ClaimName: testPVCName,
+				Phase:     v1.MemoryDumpAssociating,
+			}, http.StatusConflict, true, true, createTestPVC("2Gi", block)),
+			Entry("VM with a memory dump request pvc size too small should fail", &v1.VirtualMachineMemoryDumpRequest{
+				ClaimName: testPVCName,
 				Phase:     v1.MemoryDumpAssociating,
-			}, http.StatusConflict, true, false),
+			}, http.StatusConflict, true, true, createTestPVC("1Gi", fs)),
 		)
 
 		DescribeTable("Should generate expected vm patch", func(memDumpReq *v1.VirtualMachineMemoryDumpRequest, existingMemDumpReq *v1.VirtualMachineMemoryDumpRequest, expectedPatch string, expectError bool, removeReq bool) {

diff --git a/pkg/virt-operator/resource/generate/rbac/apiserver.go b/pkg/virt-operator/resource/generate/rbac/apiserver.go
@@ -101,6 +101,17 @@ func newApiServerClusterRole() *rbacv1.ClusterRole {
 					"get", "list", "watch", "patch", "update",
 				},
 			},
+			{
+				APIGroups: []string{
+					"",
+				},
+				Resources: []string{
+					"persistentvolumeclaims",
+				},
+				Verbs: []string{
+					"get",
+				},
+			},
 			{
 				APIGroups: []string{
 					GroupName,

diff --git a/pkg/virtctl/vm/vm.go b/pkg/virtctl/vm/vm.go
@@ -415,18 +415,12 @@ func removeVolume(vmiName, volumeName, namespace string, virtClient kubecli.Kube
 }
 
 func memoryDump(vmName, claimName, namespace string, virtClient kubecli.KubevirtClient) error {
-	pvc, err := virtClient.CoreV1().PersistentVolumeClaims(namespace).Get(context.TODO(), claimName, metav1.GetOptions{})
-	if err != nil {
-		return fmt.Errorf("error dumping vm memory, failed to verify pvc %s, %v", claimName, err)
-	} else if pvc.Spec.VolumeMode != nil && *pvc.Spec.VolumeMode == k8sv1.PersistentVolumeBlock {
-		return fmt.Errorf("error dumping vm memory, pvc %s should be filesystem pvc", claimName)
-	}
 	memoryDumpRequest := &v1.VirtualMachineMemoryDumpRequest{
 		ClaimName: claimName,
 		Phase:     v1.MemoryDumpAssociating,
 	}
 
-	err = virtClient.VirtualMachine(namespace).MemoryDump(vmName, memoryDumpRequest)
+	err := virtClient.VirtualMachine(namespace).MemoryDump(vmName, memoryDumpRequest)
 	if err != nil {
 		return fmt.Errorf("error dumping vm memory, %v", err)
 	}

diff --git a/pkg/virtctl/vm/vm_test.go b/pkg/virtctl/vm/vm_test.go
@@ -567,19 +567,6 @@ var _ = Describe("VirtualMachine", func() {
 	})
 
 	Context("memory dump", func() {
-		var (
-			coreClient *fake.Clientset
-		)
-
-		createTestPVC := func() *corev1.PersistentVolumeClaim {
-			return &corev1.PersistentVolumeClaim{
-				ObjectMeta: k8smetav1.ObjectMeta{
-					Name: "testvolume",
-				},
-				Spec: corev1.PersistentVolumeClaimSpec{},
-			}
-		}
-
 		expectVMEndpointMemoryDump := func(vmName, claimName string) {
 			kubecli.MockKubevirtClientInstance.
 				EXPECT().
@@ -593,10 +580,6 @@ var _ = Describe("VirtualMachine", func() {
 			})
 		}
 
-		BeforeEach(func() {
-			coreClient = fake.NewSimpleClientset()
-		})
-
 		DescribeTable("should fail with missing required or invalid parameters", func(errorString string, args ...string) {
 			commandAndArgs := []string{"memory-dump"}
 			commandAndArgs = append(commandAndArgs, args...)
@@ -609,29 +592,7 @@ var _ = Describe("VirtualMachine", func() {
 			Entry("memorydump name, missing required claim-name", "required flag(s)", "testvm"),
 			Entry("memorydump name, invalid extra parameter", "unknown flag", "testvm", "--claim-name=blah", "--invalid=test"),
 		)
-		It("should fail memorydump when no pvc is found", func() {
-			kubecli.MockKubevirtClientInstance.EXPECT().CoreV1().Return(coreClient.CoreV1())
-			commandAndArgs := []string{"memory-dump", "testvm", "--claim-name=my-pvc"}
-			cmdAdd := clientcmd.NewRepeatableVirtctlCommand(commandAndArgs...)
-			res := cmdAdd()
-			Expect(res).ToNot(BeNil())
-			Expect(res.Error()).To(ContainSubstring("error dumping vm memory, failed to verify pvc"))
-		})
-		It("should fail memorydump when getting block pvc", func() {
-			kubecli.MockKubevirtClientInstance.EXPECT().CoreV1().Return(coreClient.CoreV1())
-			testPvc := createTestPVC()
-			volumeMode := corev1.PersistentVolumeBlock
-			testPvc.Spec.VolumeMode = &volumeMode
-			coreClient.CoreV1().PersistentVolumeClaims(k8smetav1.NamespaceDefault).Create(context.Background(), createTestPVC(), k8smetav1.CreateOptions{})
-			commandAndArgs := []string{"memory-dump", "testvm", "--claim-name=my-pvc"}
-			cmdAdd := clientcmd.NewRepeatableVirtctlCommand(commandAndArgs...)
-			res := cmdAdd()
-			Expect(res).ToNot(BeNil())
-			Expect(res.Error()).To(ContainSubstring("error dumping vm memory, failed to verify pvc"))
-		})
 		It("should call memory dump subresource", func() {
-			kubecli.MockKubevirtClientInstance.EXPECT().CoreV1().Return(coreClient.CoreV1())
-			coreClient.CoreV1().PersistentVolumeClaims(k8smetav1.NamespaceDefault).Create(context.Background(), createTestPVC(), k8smetav1.CreateOptions{})
 			expectVMEndpointMemoryDump("testvm", "testvolume")
 			commandAndArgs := []string{"memory-dump", "testvm", "--claim-name=testvolume"}
 			cmd := clientcmd.NewVirtctlCommand(commandAndArgs...)