From bb3e6668f7fc5876c515efff1eefb435276224c3 Mon Sep 17 00:00:00 2001 From: Isteb4k Date: Mon, 8 Jul 2024 17:55:30 +0200 Subject: [PATCH] fix(vd): synchronize PVC status changes with VD status updates * fix(vd): synchronize PVC status changes with VD status updates Signed-off-by: Isteb4k --- .../pkg/controller/service/condition.go | 10 ++ .../pkg/controller/service/disk_service.go | 16 ++- .../pkg/controller/vd/internal/resizing.go | 93 ++++++++-------- .../controller/vd/internal/resizing_test.go | 103 +++++++++--------- .../pkg/controller/vd/vd_controller.go | 2 +- .../pkg/controller/vd/vd_reconciler.go | 5 + 6 files changed, 125 insertions(+), 104 deletions(-) diff --git a/images/virtualization-artifact/pkg/controller/service/condition.go b/images/virtualization-artifact/pkg/controller/service/condition.go index e725cf07d..d9587f1cc 100644 --- a/images/virtualization-artifact/pkg/controller/service/condition.go +++ b/images/virtualization-artifact/pkg/controller/service/condition.go @@ -19,6 +19,7 @@ package service import ( "unicode" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" cdiv1 "kubevirt.io/containerized-data-importer-api/pkg/apis/core/v1beta1" @@ -70,3 +71,12 @@ func GetDataVolumeCondition(conditionType cdiv1.DataVolumeConditionType, conditi } return nil } + +func GetPersistentVolumeClaimCondition(conditionType corev1.PersistentVolumeClaimConditionType, conditions []corev1.PersistentVolumeClaimCondition) *corev1.PersistentVolumeClaimCondition { + for i, condition := range conditions { + if condition.Type == conditionType { + return &conditions[i] + } + } + return nil +} diff --git a/images/virtualization-artifact/pkg/controller/service/disk_service.go b/images/virtualization-artifact/pkg/controller/service/disk_service.go index a979d900c..0ea8de881 100644 --- a/images/virtualization-artifact/pkg/controller/service/disk_service.go +++ b/images/virtualization-artifact/pkg/controller/service/disk_service.go @@ -196,13 +196,17 @@ func (s DiskService) Resize(ctx context.Context, pvc *corev1.PersistentVolumeCla } curSize := pvc.Spec.Resources.Requests[corev1.ResourceStorage] - if newSize.Cmp(curSize) == 1 { - pvc.Spec.Resources.Requests[corev1.ResourceStorage] = newSize - err := s.client.Update(ctx, pvc) - if err != nil { - return fmt.Errorf("failed to increase pvc size: %w", err) - } + // newSize <= curSize + if newSize.Cmp(curSize) != 1 { + return fmt.Errorf("new pvc %s/%s size %s is too low: should be > %s", pvc.Namespace, pvc.Name, newSize.String(), curSize.String()) + } + + pvc.Spec.Resources.Requests[corev1.ResourceStorage] = newSize + + err := s.client.Update(ctx, pvc) + if err != nil { + return fmt.Errorf("failed to increase size for pvc %s/%s from %s to %s : %w", pvc.Namespace, pvc.Name, curSize.String(), newSize.String(), err) } return nil diff --git a/images/virtualization-artifact/pkg/controller/vd/internal/resizing.go b/images/virtualization-artifact/pkg/controller/vd/internal/resizing.go index 6c1ec5dc7..3657df027 100644 --- a/images/virtualization-artifact/pkg/controller/vd/internal/resizing.go +++ b/images/virtualization-artifact/pkg/controller/vd/internal/resizing.go @@ -18,11 +18,9 @@ package internal import ( "context" - "errors" - "fmt" + "log/slog" corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/reconcile" @@ -35,15 +33,19 @@ import ( type ResizingHandler struct { diskService DiskService + logger *slog.Logger } -func NewResizingHandler(diskService DiskService) *ResizingHandler { +func NewResizingHandler(logger *slog.Logger, diskService DiskService) *ResizingHandler { return &ResizingHandler{ + logger: logger, diskService: diskService, } } func (h ResizingHandler) Handle(ctx context.Context, vd *virtv2.VirtualDisk) (reconcile.Result, error) { + logger := h.logger.With("name", vd.Name, "ns", vd.Namespace) + condition, ok := service.GetCondition(vdcondition.ResizedType, vd.Status.Conditions) if !ok { condition = metav1.Condition{ @@ -61,18 +63,10 @@ func (h ResizingHandler) Handle(ctx context.Context, vd *virtv2.VirtualDisk) (re return reconcile.Result{}, nil } - vdSpecSize := vd.Spec.PersistentVolumeClaim.Size - if vdSpecSize == nil { - condition.Status = metav1.ConditionFalse - condition.Reason = vdcondition.NotRequested - condition.Message = "" - return reconcile.Result{}, nil - } - readyCondition, ok := service.GetCondition(vdcondition.ReadyType, vd.Status.Conditions) if !ok || readyCondition.Status != metav1.ConditionTrue { - condition.Status = metav1.ConditionFalse - condition.Reason = vdcondition.NotRequested + condition.Status = metav1.ConditionUnknown + condition.Reason = "" condition.Message = "" return reconcile.Result{}, nil } @@ -84,61 +78,66 @@ func (h ResizingHandler) Handle(ctx context.Context, vd *virtv2.VirtualDisk) (re } if pvc == nil { - return reconcile.Result{}, errors.New("pvc not found for ready virtual disk") + condition.Status = metav1.ConditionUnknown + condition.Reason = "" + condition.Message = "Underlying PersistentVolumeClaim not found: resizing is not possible." + return reconcile.Result{}, nil } - pvcSpecSize := pvc.Spec.Resources.Requests[corev1.ResourceStorage] - switch vdSpecSize.Cmp(pvcSpecSize) { - // Expected disk size is LESS THAN expected pvc size: no resize needed as resizing to a smaller size is not possible. - case -1: - condition.Status = metav1.ConditionFalse - condition.Reason = vdcondition.NotRequested - condition.Message = fmt.Sprintf("The virtual disk size is too low: should be >= %s.", pvcSpecSize.String()) + if pvc.Status.Phase != corev1.ClaimBound { + condition.Status = metav1.ConditionUnknown + condition.Reason = "" + condition.Message = "Underlying PersistentVolumeClaim not bound: resizing is not possible." return reconcile.Result{}, nil - // Expected disk size is GREATER THAN expected pvc size: resize needed, resizing to a larger size. - case 1: - err = h.diskService.Resize(ctx, pvc, *vdSpecSize) - if err != nil { - return reconcile.Result{}, err - } + } - vd.Status.Phase = virtv2.DiskResizing + vdSpecSize := vd.Spec.PersistentVolumeClaim.Size + pvcSpecSize := pvc.Spec.Resources.Requests[corev1.ResourceStorage] + pvcStatusSize := pvc.Status.Capacity[corev1.ResourceStorage] + + logger = logger.With("vdSpecSize", vdSpecSize, "pvcSpecSize", pvcSpecSize.String(), "pvcStatusSize", pvcStatusSize) + // Synchronize VirtualDisk size with PVC size. + vd.Status.Capacity = pvcStatusSize.String() + + pvcResizing := service.GetPersistentVolumeClaimCondition(corev1.PersistentVolumeClaimResizing, pvc.Status.Conditions) + if pvcResizing != nil && pvcResizing.Status == corev1.ConditionTrue { + logger.Info("Resizing is in progress", "msg", pvcResizing.Message) + + vd.Status.Phase = virtv2.DiskResizing condition.Status = metav1.ConditionFalse condition.Reason = vdcondition.InProgress - condition.Message = "The virtual disk resizing has started." + condition.Message = pvcResizing.Message return reconcile.Result{}, nil - // Expected disk size is EQUAL TO expected pvc size: cannot definitively say whether the resize has already happened or was not needed - perform additional checks. - case 0: } - var vdStatusSize resource.Quantity - vdStatusSize, err = resource.ParseQuantity(vd.Status.Capacity) - if err != nil { - return reconcile.Result{}, err - } + // Expected disk size is GREATER THAN expected pvc size: resize needed, resizing to a larger size. + if vdSpecSize != nil && vdSpecSize.Cmp(pvcSpecSize) == 1 { + err = h.diskService.Resize(ctx, pvc, *vdSpecSize) + if err != nil { + return reconcile.Result{}, err + } - pvcStatusSize := pvc.Status.Capacity[corev1.ResourceStorage] + logger.Info("The virtual disk resizing has started") - // Expected pvc size is GREATER THAN actual pvc size: resize has been requested and is in progress. - if pvcSpecSize.Cmp(pvcStatusSize) == 1 { vd.Status.Phase = virtv2.DiskResizing - condition.Status = metav1.ConditionFalse condition.Reason = vdcondition.InProgress - condition.Message = "The virtual disk is in the process of resizing." + condition.Message = "The virtual disk resizing has started." return reconcile.Result{}, nil } - // Virtual disk size DOES NOT MATCH pvc size: resize has completed, synchronize the virtual disk size. - if !vdStatusSize.Equal(pvcStatusSize) { - vd.Status.Capacity = pvcStatusSize.String() + // Expected disk size is NOT GREATER THAN expected pvc size: no resize needed since downsizing is not possible, and resizing to the same value makes no sense. + switch condition.Reason { + case vdcondition.InProgress, vdcondition.Resized: condition.Status = metav1.ConditionTrue condition.Reason = vdcondition.Resized condition.Message = "" - return reconcile.Result{Requeue: true}, nil + default: + condition.Status = metav1.ConditionFalse + condition.Reason = vdcondition.NotRequested + condition.Message = "" } - // Expected pvc size is NOT GREATER THAN actual PVC size AND virtual disk size MATCHES pvc size: keep previous status. return reconcile.Result{}, nil } diff --git a/images/virtualization-artifact/pkg/controller/vd/internal/resizing_test.go b/images/virtualization-artifact/pkg/controller/vd/internal/resizing_test.go index 665e7844b..e675434aa 100644 --- a/images/virtualization-artifact/pkg/controller/vd/internal/resizing_test.go +++ b/images/virtualization-artifact/pkg/controller/vd/internal/resizing_test.go @@ -18,6 +18,7 @@ package internal import ( "context" + "log/slog" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -36,11 +37,13 @@ var _ = Describe("Resizing handler Run", func() { var pvc *corev1.PersistentVolumeClaim var diskService *DiskServiceMock + size := resource.MustParse("10G") + BeforeEach(func() { vd = &virtv2.VirtualDisk{ Spec: virtv2.VirtualDiskSpec{ PersistentVolumeClaim: virtv2.VirtualDiskPersistentVolumeClaim{ - Size: new(resource.Quantity), + Size: &size, }, }, Status: virtv2.VirtualDiskStatus{ @@ -50,24 +53,29 @@ var _ = Describe("Resizing handler Run", func() { Status: metav1.ConditionTrue, }, }, - Capacity: "", + Capacity: size.String(), }, } pvc = &corev1.PersistentVolumeClaim{ Spec: corev1.PersistentVolumeClaimSpec{ Resources: corev1.VolumeResourceRequirements{ - Requests: make(corev1.ResourceList), + Requests: corev1.ResourceList{ + corev1.ResourceStorage: size, + }, }, }, Status: corev1.PersistentVolumeClaimStatus{ - Capacity: make(corev1.ResourceList), + Phase: corev1.ClaimBound, + Capacity: corev1.ResourceList{ + corev1.ResourceStorage: size, + }, }, } diskService = &DiskServiceMock{ GetPersistentVolumeClaimFunc: func(ctx context.Context, sup *supplements.Generator) (*corev1.PersistentVolumeClaim, error) { - return nil, nil + return pvc, nil }, ResizeFunc: func(ctx context.Context, pvc *corev1.PersistentVolumeClaim, newSize resource.Quantity) error { return nil @@ -75,28 +83,31 @@ var _ = Describe("Resizing handler Run", func() { } }) - It("Resize is not requested (vd.spec.size == nil)", func() { + It("Resizing is in progress", func() { vd.Spec.PersistentVolumeClaim.Size = nil + diskService.GetPersistentVolumeClaimFunc = func(ctx context.Context, sup *supplements.Generator) (*corev1.PersistentVolumeClaim, error) { + pvc.Status.Conditions = []corev1.PersistentVolumeClaimCondition{ + { + Type: corev1.PersistentVolumeClaimResizing, + Status: corev1.ConditionTrue, + }, + } + return pvc, nil + } - h := NewResizingHandler(diskService) + h := NewResizingHandler(slog.Default(), diskService) _, err := h.Handle(context.Background(), vd) Expect(err).To(BeNil()) - Expect(vd.Status.Conditions).To(ContainElement(metav1.Condition{ - Type: vdcondition.ResizedType, - Status: metav1.ConditionFalse, - Reason: vdcondition.NotRequested, - })) + resized, _ := service.GetCondition(vdcondition.ResizedType, vd.Status.Conditions) + Expect(resized.Status).To(Equal(metav1.ConditionFalse)) + Expect(resized.Reason).To(Equal(vdcondition.InProgress)) }) - It("Resize is not requested (vd.spec.size < pvc.spec.size)", func() { - *vd.Spec.PersistentVolumeClaim.Size = resource.MustParse("1G") - diskService.GetPersistentVolumeClaimFunc = func(ctx context.Context, sup *supplements.Generator) (*corev1.PersistentVolumeClaim, error) { - pvc.Spec.Resources.Requests[corev1.ResourceStorage] = resource.MustParse("2G") - return pvc, nil - } + It("Resize is not requested (vd.spec.size == nil)", func() { + vd.Spec.PersistentVolumeClaim.Size = nil - h := NewResizingHandler(diskService) + h := NewResizingHandler(slog.Default(), diskService) _, err := h.Handle(context.Background(), vd) Expect(err).To(BeNil()) @@ -105,61 +116,53 @@ var _ = Describe("Resizing handler Run", func() { Expect(resized.Reason).To(Equal(vdcondition.NotRequested)) }) - It("Resize has started (vd.spec.size > pvc.spec.size)", func() { - *vd.Spec.PersistentVolumeClaim.Size = resource.MustParse("2G") - diskService.GetPersistentVolumeClaimFunc = func(ctx context.Context, sup *supplements.Generator) (*corev1.PersistentVolumeClaim, error) { - pvc.Spec.Resources.Requests[corev1.ResourceStorage] = resource.MustParse("1G") - return pvc, nil - } + It("Resize is not requested (vd.spec.size < pvc.spec.size)", func() { + vd.Spec.PersistentVolumeClaim.Size.Sub(resource.MustParse("1G")) - h := NewResizingHandler(diskService) + h := NewResizingHandler(slog.Default(), diskService) _, err := h.Handle(context.Background(), vd) Expect(err).To(BeNil()) + resized, _ := service.GetCondition(vdcondition.ResizedType, vd.Status.Conditions) + Expect(resized.Status).To(Equal(metav1.ConditionFalse)) + Expect(resized.Reason).To(Equal(vdcondition.NotRequested)) + }) + It("Resize is not requested (vd.spec.size == pvc.spec.size)", func() { + h := NewResizingHandler(slog.Default(), diskService) + + _, err := h.Handle(context.Background(), vd) + Expect(err).To(BeNil()) resized, _ := service.GetCondition(vdcondition.ResizedType, vd.Status.Conditions) Expect(resized.Status).To(Equal(metav1.ConditionFalse)) - Expect(resized.Reason).To(Equal(vdcondition.InProgress)) + Expect(resized.Reason).To(Equal(vdcondition.NotRequested)) }) - It("Resize is in progress (vd.spec.size == pvc.spec.size, pvc.spec.size > pvc.status.size)", func() { - *vd.Spec.PersistentVolumeClaim.Size = resource.MustParse("2G") - q := resource.MustParse("1G") - vd.Status.Capacity = q.String() - diskService.GetPersistentVolumeClaimFunc = func(ctx context.Context, sup *supplements.Generator) (*corev1.PersistentVolumeClaim, error) { - pvc.Spec.Resources.Requests[corev1.ResourceStorage] = resource.MustParse("2G") - pvc.Status.Capacity[corev1.ResourceStorage] = resource.MustParse("1G") - return pvc, nil - } + It("Resize has started (vd.spec.size > pvc.spec.size)", func() { + vd.Spec.PersistentVolumeClaim.Size.Add(size) - h := NewResizingHandler(diskService) + h := NewResizingHandler(slog.Default(), diskService) _, err := h.Handle(context.Background(), vd) Expect(err).To(BeNil()) - resized, _ := service.GetCondition(vdcondition.ResizedType, vd.Status.Conditions) Expect(resized.Status).To(Equal(metav1.ConditionFalse)) Expect(resized.Reason).To(Equal(vdcondition.InProgress)) }) - It("Resized (vd.spec.size == pvc.spec.size, pvc.spec.size == pvc.status.size)", func() { - *vd.Spec.PersistentVolumeClaim.Size = resource.MustParse("2G") - q := resource.MustParse("1G") - vd.Status.Capacity = q.String() - diskService.GetPersistentVolumeClaimFunc = func(ctx context.Context, sup *supplements.Generator) (*corev1.PersistentVolumeClaim, error) { - pvc.Spec.Resources.Requests[corev1.ResourceStorage] = resource.MustParse("2G") - pvc.Status.Capacity[corev1.ResourceStorage] = resource.MustParse("2G") - return pvc, nil - } + It("Resize has completed", func() { + vd.Status.Conditions = append(vd.Status.Conditions, metav1.Condition{ + Type: vdcondition.ResizedType, + Status: metav1.ConditionFalse, + Reason: vdcondition.InProgress, + }) - h := NewResizingHandler(diskService) + h := NewResizingHandler(slog.Default(), diskService) _, err := h.Handle(context.Background(), vd) Expect(err).To(BeNil()) - resized, _ := service.GetCondition(vdcondition.ResizedType, vd.Status.Conditions) Expect(resized.Status).To(Equal(metav1.ConditionTrue)) Expect(resized.Reason).To(Equal(vdcondition.Resized)) - Expect(resource.MustParse(vd.Status.Capacity)).To(Equal(pvc.Status.Capacity[corev1.ResourceStorage])) }) }) diff --git a/images/virtualization-artifact/pkg/controller/vd/vd_controller.go b/images/virtualization-artifact/pkg/controller/vd/vd_controller.go index d1ccdb07d..5195d46b0 100644 --- a/images/virtualization-artifact/pkg/controller/vd/vd_controller.go +++ b/images/virtualization-artifact/pkg/controller/vd/vd_controller.go @@ -73,7 +73,7 @@ func NewController( logger, internal.NewDatasourceReadyHandler(blank, sources), internal.NewLifeCycleHandler(logger, blank, sources, mgr.GetClient()), - internal.NewResizingHandler(disk), + internal.NewResizingHandler(logger, disk), internal.NewDeletionHandler(sources), internal.NewAttacheeHandler(mgr.GetClient()), ) diff --git a/images/virtualization-artifact/pkg/controller/vd/vd_reconciler.go b/images/virtualization-artifact/pkg/controller/vd/vd_reconciler.go index 87b02b387..16663d342 100644 --- a/images/virtualization-artifact/pkg/controller/vd/vd_reconciler.go +++ b/images/virtualization-artifact/pkg/controller/vd/vd_reconciler.go @@ -188,6 +188,11 @@ func (r *Reconciler) SetupController(_ context.Context, mgr manager.Manager, ctr return true } + if service.GetPersistentVolumeClaimCondition(corev1.PersistentVolumeClaimResizing, oldPVC.Status.Conditions) != nil || + service.GetPersistentVolumeClaimCondition(corev1.PersistentVolumeClaimResizing, newPVC.Status.Conditions) != nil { + return true + } + return oldPVC.Status.Phase != newPVC.Status.Phase && newPVC.Status.Phase == corev1.ClaimBound }, },