fix(cvi,vi): unlock pending vi/cvi from vd ref #416

LopatinDmitr · 2024-10-02T20:11:05Z

Description

Add conditon 'InUse" for VirtualDisk. This condition indicate:
- that the VirtualDisk may be used for create ClusterVirtualImage or VirtualImage
- that the VirtualDisk may be attached to running VirtualMachine

Why do we need it, and what problem does it solve?

Fix problem with lock VirtualImage and ClusterVirtual image in phase 'Pending' after stopping or deleting VirtualMachine, when used needed VirtualDisk in them.

What is the expected result?

Checklist

The code is covered by unit tests.
e2e tests passed.
Documentation updated according to the changes.
Changes were tested in the Kubernetes cluster manually.

images/virtualization-artifact/pkg/controller/cvi/internal/source/object_ref_vd.go

images/virtualization-artifact/pkg/controller/cvi/internal/datasource_ready.go

images/virtualization-artifact/pkg/controller/cvi/internal/source/errors.go

images/virtualization-artifact/pkg/controller/vd/internal/attachee.go

images/virtualization-artifact/pkg/controller/vi/internal/source/object_ref_vd.go

api/core/v1alpha2/vdcondition/condition.go

images/virtualization-artifact/pkg/controller/vd/internal/attachee.go

images/virtualization-artifact/pkg/controller/vd/internal/inuse.go

images/virtualization-artifact/pkg/controller/vd/vd_reconciler.go

images/virtualization-artifact/pkg/controller/vi/internal/source/object_ref_vd.go

images/virtualization-artifact/pkg/controller/vm/internal/block_device.go

images/virtualization-artifact/pkg/controller/vd/internal/inuse.go

images/virtualization-artifact/pkg/controller/vi/internal/source/errors.go

images/virtualization-artifact/pkg/controller/watchers/vd_enqueuer.go

images/virtualization-artifact/pkg/controller/cvi/internal/source/object_ref_vd.go

images/virtualization-artifact/pkg/controller/vd/internal/inuse.go

Signed-off-by: dmitry.lopatin <[email protected]>

Isteb4k · 2024-12-09T08:43:34Z

api/core/v1alpha2/vdcondition/condition.go

@@ -34,6 +34,8 @@ const (
 	SnapshottingType Type = "Snapshotting"
 	// StorageClassReadyType indicates whether the storage class is ready.
 	StorageClassReadyType Type = "StorageClassReady"
+	// InUseType indicates whether the VirtualDisk is attached to a running VirtualMachine or is being used in a process of a VirtualImage creation.


of an image creation

Isteb4k · 2024-12-09T08:56:55Z

images/virtualization-artifact/pkg/controller/vd/internal/inuse.go

+	allowedPhases := []virtv2.ImagePhase{virtv2.ImageProvisioning, virtv2.ImagePending}
+
+	for _, vi := range vis.Items {
+		if slices.Contains(allowedPhases, vi.Status.Phase) && vi.Spec.DataSource.Type == virtv2.DataSourceTypeObjectRef && vi.Spec.DataSource.ObjectRef != nil && vi.Spec.DataSource.ObjectRef.Kind == virtv2.VirtualDiskKind {


vi.Spec.DataSource.ObjectRef.Name == vd.Name

Isteb4k · 2024-12-09T08:56:58Z

images/virtualization-artifact/pkg/controller/vd/internal/inuse.go

+	allowedPhases := []virtv2.ImagePhase{virtv2.ImageProvisioning, virtv2.ImagePending}
+
+	for _, vi := range vis.Items {
+		if slices.Contains(allowedPhases, vi.Status.Phase) && vi.Spec.DataSource.Type == virtv2.DataSourceTypeObjectRef && vi.Spec.DataSource.ObjectRef != nil && vi.Spec.DataSource.ObjectRef.Kind == virtv2.VirtualDiskKind {


Too long line

Isteb4k · 2024-12-09T11:22:04Z

images/virtualization-artifact/pkg/controller/vd/internal/inuse.go

+	cb := conditions.NewConditionBuilder(vdcondition.InUseType)
+
+	switch {
+	case allowUseForVM && inUseCondition.Status == metav1.ConditionUnknown:


Looks like a little optimization is possible here: you dont have to list vi and cvi if allowUseForVM is true

Isteb4k · 2024-12-09T11:27:36Z

images/virtualization-artifact/pkg/controller/vd/internal/inuse.go

+			conditions.SetCondition(cb, &vd.Status.Conditions)
+		}
+	default:
+		needChange := false


Mb rename to setUnkown := false ?

Isteb4k · 2024-12-09T12:10:27Z

images/virtualization-artifact/pkg/controller/vd/internal/inuse_test.go

+import (
+	"context"
+
+	"github.com/onsi/ginkgo/v2"


Why dont use dot import alias?

Isteb4k · 2024-12-09T12:13:28Z

images/virtualization-artifact/pkg/controller/vd/internal/inuse_test.go

+	})
+
+	ginkgo.Context("when VirtualDisk is used by not ready VirtualMachine", func() {
+		ginkgo.It("must set status True and reason AllowedForVirtualMachineUsage", func() {


it sets Unknown

Isteb4k · 2024-12-09T12:14:20Z

images/virtualization-artifact/pkg/controller/vd/internal/inuse_test.go

+		})
+	})
+
+	ginkgo.Context("when VirtualDisk is not in use after create image", func() {


after image creation

Isteb4k · 2024-12-09T12:15:34Z

images/virtualization-artifact/pkg/controller/vd/internal/inuse_test.go

+		})
+	})
+
+	ginkgo.Context("when VirtualDisk is not in use after running VM", func() {


after VM deletion

Isteb4k · 2024-12-09T12:17:29Z

images/virtualization-artifact/pkg/controller/vd/internal/inuse.go

+					Namespace:     vm.GetNamespace(),
+					LabelSelector: labels.SelectorFromSet(map[string]string{virtv1.VirtualMachineNameLabel: vm.GetName()}),
+				})
+				if err != nil && !k8serrors.IsNotFound(err) {


List cannot return not found error

Signed-off-by: dmitry.lopatin <[email protected]>

github-actions bot assigned LopatinDmitr Oct 2, 2024

LopatinDmitr requested a review from Isteb4k October 7, 2024 15:28

LopatinDmitr marked this pull request as ready for review October 7, 2024 15:28

Isteb4k requested changes Oct 8, 2024

View reviewed changes

images/virtualization-artifact/pkg/controller/vi/internal/source/object_ref_vd.go Outdated Show resolved Hide resolved

LopatinDmitr force-pushed the fix/vi/unlock-pending-vi-from-vd-ref branch 2 times, most recently from 45552d1 to 8e5b4f1 Compare October 13, 2024 09:29

LopatinDmitr changed the title ~~fix(vi): unlock pending vi from vd ref~~ fix(cvi,vi): unlock pending vi/cvi from vd ref Oct 13, 2024

LopatinDmitr requested a review from Isteb4k October 14, 2024 20:37

Isteb4k requested changes Oct 15, 2024

View reviewed changes

Isteb4k self-requested a review October 15, 2024 16:29

Isteb4k requested changes Oct 15, 2024

View reviewed changes

images/virtualization-artifact/pkg/controller/vd/internal/inuse.go Outdated Show resolved Hide resolved

LopatinDmitr force-pushed the fix/vi/unlock-pending-vi-from-vd-ref branch from 523f4a4 to f8134b5 Compare October 21, 2024 08:36

LopatinDmitr requested review from Isteb4k and diafour and removed request for Isteb4k October 23, 2024 21:14

LopatinDmitr force-pushed the fix/vi/unlock-pending-vi-from-vd-ref branch from 70fd271 to 0f11fd4 Compare October 27, 2024 19:57

diafour reviewed Oct 28, 2024

View reviewed changes

images/virtualization-artifact/pkg/controller/vi/internal/source/errors.go Outdated Show resolved Hide resolved

diafour reviewed Oct 28, 2024

View reviewed changes

images/virtualization-artifact/pkg/controller/watchers/vd_enqueuer.go Show resolved Hide resolved

LopatinDmitr force-pushed the fix/vi/unlock-pending-vi-from-vd-ref branch 8 times, most recently from d37bad7 to 7b7bcd5 Compare October 29, 2024 20:42

LopatinDmitr marked this pull request as draft October 30, 2024 09:11

LopatinDmitr force-pushed the fix/vi/unlock-pending-vi-from-vd-ref branch from 7b7bcd5 to 11bbb7f Compare October 30, 2024 12:12

LopatinDmitr requested a review from Isteb4k November 22, 2024 08:25

LopatinDmitr marked this pull request as ready for review November 22, 2024 08:25

LopatinDmitr force-pushed the fix/vi/unlock-pending-vi-from-vd-ref branch 6 times, most recently from f5f5d24 to 2c03739 Compare November 22, 2024 13:16

Isteb4k previously approved these changes Nov 22, 2024

View reviewed changes

images/virtualization-artifact/pkg/controller/cvi/internal/source/object_ref_vd.go Outdated Show resolved Hide resolved

images/virtualization-artifact/pkg/controller/vd/internal/inuse.go Outdated Show resolved Hide resolved

LopatinDmitr dismissed Isteb4k’s stale review via 5bc2012 November 25, 2024 14:38

LopatinDmitr force-pushed the fix/vi/unlock-pending-vi-from-vd-ref branch 7 times, most recently from 8d2b638 to 8b0c8b6 Compare November 26, 2024 19:49

LopatinDmitr marked this pull request as draft November 27, 2024 08:47

LopatinDmitr force-pushed the fix/vi/unlock-pending-vi-from-vd-ref branch 2 times, most recently from 03a7ea4 to a959b5b Compare December 4, 2024 21:02

LopatinDmitr requested a review from Isteb4k December 5, 2024 06:41

LopatinDmitr marked this pull request as ready for review December 5, 2024 06:42

LopatinDmitr added 3 commits December 9, 2024 11:02

fix(cvi,vi): unlock pending image from VD ref

70ce517

Signed-off-by: dmitry.lopatin <[email protected]>

fix(api): add new logic allowed use for images and vms

2ccd21d

Signed-off-by: dmitry.lopatin <[email protected]>

fix(vd): add new handler

aa7a52b

Signed-off-by: dmitry.lopatin <[email protected]>

LopatinDmitr force-pushed the fix/vi/unlock-pending-vi-from-vd-ref branch from a959b5b to 3132202 Compare December 9, 2024 08:03

fix

c7c091c

Signed-off-by: dmitry.lopatin <[email protected]>

LopatinDmitr force-pushed the fix/vi/unlock-pending-vi-from-vd-ref branch from 3132202 to c7c091c Compare December 9, 2024 08:05

Isteb4k requested changes Dec 9, 2024

View reviewed changes

++

565da08

Signed-off-by: dmitry.lopatin <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(cvi,vi): unlock pending vi/cvi from vd ref #416

fix(cvi,vi): unlock pending vi/cvi from vd ref #416

LopatinDmitr commented Oct 2, 2024 •

edited

Loading

Isteb4k Dec 9, 2024

Isteb4k Dec 9, 2024

Isteb4k Dec 9, 2024

Isteb4k Dec 9, 2024

Isteb4k Dec 9, 2024

Isteb4k Dec 9, 2024

Isteb4k Dec 9, 2024 •

edited

Loading

Isteb4k Dec 9, 2024

Isteb4k Dec 9, 2024

Isteb4k Dec 9, 2024

fix(cvi,vi): unlock pending vi/cvi from vd ref #416

Are you sure you want to change the base?

fix(cvi,vi): unlock pending vi/cvi from vd ref #416

Conversation

LopatinDmitr commented Oct 2, 2024 • edited Loading

Description

Why do we need it, and what problem does it solve?

What is the expected result?

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Isteb4k Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LopatinDmitr commented Oct 2, 2024 •

edited

Loading

Isteb4k Dec 9, 2024 •

edited

Loading