Skip to content

fix(vd): prevent VirtualDisk from stuck in WaitForFirstConsumer phase when VM is attached #1516

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

fix(vd): prevent VirtualDisk from stuck in WaitForFirstConsumer phase when VM is attached #1516

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
1909f13
wip
loktev-d Oct 3, 2025
d10efe4
Merge branch 'main' into fix/vd/wffc-stuck-in-waiting-phase
loktev-d Oct 3, 2025
6ceef9b
wip
loktev-d Oct 3, 2025
a79f8e9
wip
loktev-d Oct 3, 2025
99d306c
fix tests
loktev-d Oct 3, 2025
095eb4c
fix sourcery ai comments
loktev-d Oct 16, 2025
2f17c96
Merge branch 'main' into fix/vd/wffc-stuck-in-waiting-phase
loktev-d Oct 16, 2025
d910056
Merge branch 'main' into fix/vd/wffc-stuck-in-waiting-phase
loktev-d Oct 16, 2025
253200e
wip
loktev-d Oct 16, 2025
33dd5f1
wip
loktev-d Oct 16, 2025
d4cdc27
Revert "wip"
loktev-d Oct 16, 2025
99449cc
wip
loktev-d Oct 16, 2025
af28afe
wip
loktev-d Oct 16, 2025
01626ea
fix tests
loktev-d Oct 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions images/virtualization-artifact/pkg/controller/vd/internal/source/object_ref_cvi_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,13 @@ var _ = Describe("ObjectRef ClusterVirtualImage", func() {

It("waits for the first consumer", func() {
dv.Status.Phase = cdiv1.PendingPopulation
dv.Status.Conditions = []cdiv1.DataVolumeCondition{
{
Type: cdiv1.DataVolumeRunning,
Status: corev1.ConditionFalse,
Reason: "",
},
}
sc.VolumeBindingMode = ptr.To(storagev1.VolumeBindingWaitForFirstConsumer)
client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(pvc, dv, sc).Build()

Expand Down
7 changes: 7 additions & 0 deletions images/virtualization-artifact/pkg/controller/vd/internal/source/object_ref_vi_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,13 @@ var _ = Describe("ObjectRef VirtualImage", func() {

It("waits for the first consumer", func() {
dv.Status.Phase = cdiv1.PendingPopulation
dv.Status.Conditions = []cdiv1.DataVolumeCondition{
{
Type: cdiv1.DataVolumeRunning,
Status: corev1.ConditionFalse,
Reason: "",
},
}
sc.VolumeBindingMode = ptr.To(storagev1.VolumeBindingWaitForFirstConsumer)
client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(pvc, dv, sc).Build()

Expand Down
4 changes: 3 additions & 1 deletion images/virtualization-artifact/pkg/controller/vd/internal/source/sources.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,9 @@ func setPhaseConditionForPVCProvisioningDisk(
Message("Waiting for the pvc importer to be created")
return nil
}
if isStorageClassWFFC(sc) && (dv.Status.Phase == cdiv1.PendingPopulation || dv.Status.Phase == cdiv1.WaitForFirstConsumer) {

dvRunningCond, _ := conditions.GetDataVolumeCondition(conditions.DVRunningConditionType, dv.Status.Conditions)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (bug_risk): Missing nil check for dvRunningCond could lead to panics.

Add a nil check for dvRunningCond before accessing its fields to prevent potential panics.

if isStorageClassWFFC(sc) && (dv.Status.Phase == cdiv1.PendingPopulation || dv.Status.Phase == cdiv1.WaitForFirstConsumer) && dvRunningCond.Status == corev1.ConditionFalse && dvRunningCond.Reason == "" {
vd.Status.Phase = v1alpha2.DiskWaitForFirstConsumer
cb.
Status(metav1.ConditionFalse).
Expand Down
7 changes: 6 additions & 1 deletion images/virtualization-artifact/pkg/controller/vd/internal/source/step/wait_for_dv_step.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,13 +126,18 @@ func (s WaitForDVStep) setForProvisioning(vd *v1alpha2.VirtualDisk) (set bool) {
}

func (s WaitForDVStep) setForFirstConsumerIsAwaited(ctx context.Context, vd *v1alpha2.VirtualDisk) (set bool, err error) {
if vd.Status.StorageClassName == "" {
return false, nil
}

sc, err := object.FetchObject(ctx, types.NamespacedName{Name: vd.Status.StorageClassName}, s.client, &storagev1.StorageClass{})
if err != nil {
return false, fmt.Errorf("get sc: %w", err)
}

dvRunningCond, _ := conditions.GetDataVolumeCondition(conditions.DVRunningConditionType, s.dv.Status.Conditions)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (bug_risk): Consider checking for nil dvRunningCond before accessing its fields.

Accessing dvRunningCond.Status or dvRunningCond.Reason without a nil check may cause a panic if GetDataVolumeCondition returns nil. Please add a nil check to prevent this.

isWFFC := sc != nil && sc.VolumeBindingMode != nil && *sc.VolumeBindingMode == storagev1.VolumeBindingWaitForFirstConsumer
if isWFFC && (s.dv.Status.Phase == cdiv1.PendingPopulation || s.dv.Status.Phase == cdiv1.WaitForFirstConsumer) {
if isWFFC && (s.dv.Status.Phase == cdiv1.PendingPopulation || s.dv.Status.Phase == cdiv1.WaitForFirstConsumer) && dvRunningCond.Status == corev1.ConditionFalse && dvRunningCond.Reason == "" {
vd.Status.Phase = v1alpha2.DiskWaitForFirstConsumer
s.cb.
Status(metav1.ConditionFalse).
Expand Down
7 changes: 7 additions & 0 deletions images/virtualization-artifact/pkg/controller/vd/internal/watcher/datavolume_watcher.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,13 @@ func (w *DataVolumeWatcher) Watch(mgr manager.Manager, ctr controller.Controller
return true
}

oldDVRunning, _ := conditions.GetDataVolumeCondition(conditions.DVRunningConditionType, e.ObjectOld.Status.Conditions)
newDVRunning, _ := conditions.GetDataVolumeCondition(conditions.DVRunningConditionType, e.ObjectNew.Status.Conditions)

if oldDVRunning.Reason != newDVRunning.Reason {
Comment on lines +76 to +79
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (bug_risk): Potential nil dereference if oldDVRunning or newDVRunning is nil.

Add nil checks for oldDVRunning and newDVRunning before accessing their Reason fields to prevent panics.

return true
}

dvRunning := service.GetDataVolumeCondition(cdiv1.DataVolumeRunning, e.ObjectNew.Status.Conditions)
return dvRunning != nil && (dvRunning.Reason == "Error" || dvRunning.Reason == "ImagePullFailed")
},
Expand Down
16 changes: 14 additions & 2 deletions images/virtualization-artifact/pkg/controller/vm/internal/block_device_condition.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,11 @@ import (
"fmt"
"strings"

storagev1 "k8s.io/api/storage/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"

"github.com/deckhouse/virtualization-controller/pkg/common/object"
"github.com/deckhouse/virtualization-controller/pkg/controller/conditions"
"github.com/deckhouse/virtualization-controller/pkg/controller/vm/internal/state"
"github.com/deckhouse/virtualization-controller/pkg/logger"
Expand Down Expand Up @@ -54,8 +57,17 @@ func (h *BlockDeviceHandler) checkVirtualDisksToBeWFFC(ctx context.Context, s st
}

for _, vd := range vds {
if vd.Status.Phase == v1alpha2.DiskWaitForFirstConsumer {
return true, nil
scName := vd.Status.StorageClassName
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we check the SC type of the disk in VM? Why can't we trust the VD phase?

sc, err := object.FetchObject(ctx, types.NamespacedName{Name: scName}, h.client, &storagev1.StorageClass{})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: Consider handling the case where scName is empty before fetching the StorageClass.

A guard clause for empty scName would avoid unnecessary API calls and reduce confusion from attempting to fetch a StorageClass with no name.

if err != nil {
return false, fmt.Errorf("fetch storage class %s: %w", scName, err)
}

if sc != nil && sc.VolumeBindingMode != nil && *sc.VolumeBindingMode == storagev1.VolumeBindingWaitForFirstConsumer {
Comment on lines +60 to +66
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: Consider handling missing or empty StorageClassName more explicitly.

If vd.Status.StorageClassName is empty, FetchObject will try to fetch a storage class with an empty name, which could cause errors or unnecessary log entries. Consider adding a check to handle this case before calling FetchObject.

readyCondition, _ := conditions.GetCondition(vdcondition.ReadyType, vd.Status.Conditions)
if readyCondition.Status != metav1.ConditionTrue {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (bug_risk): Check for nil readyCondition before accessing Status.

Add a nil check for readyCondition before accessing its Status to prevent a potential panic.

return true, nil
}
}
}

Expand Down
18 changes: 16 additions & 2 deletions images/virtualization-artifact/pkg/controller/vm/internal/block_devices_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
corev1 "k8s.io/api/core/v1"
storagev1 "k8s.io/api/storage/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
apiruntime "k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"
Expand Down Expand Up @@ -58,6 +59,7 @@ var _ = Describe("Test BlockDeviceReady condition", func() {
v1alpha2.AddToScheme,
virtv1.AddToScheme,
corev1.AddToScheme,
storagev1.AddToScheme,
} {
err := f(scheme)
Expect(err).NotTo(HaveOccurred(), "failed to add scheme: %s", err)
Expand Down Expand Up @@ -203,7 +205,8 @@ var _ = Describe("Test BlockDeviceReady condition", func() {
Namespace: namespacedName.Namespace,
},
Status: v1alpha2.VirtualDiskStatus{
Phase: v1alpha2.DiskWaitForFirstConsumer,
Phase: v1alpha2.DiskWaitForFirstConsumer,
StorageClassName: "wffc-storage",
Target: v1alpha2.DiskTarget{
PersistentVolumeClaim: "testPvc",
},
Expand All @@ -222,8 +225,19 @@ var _ = Describe("Test BlockDeviceReady condition", func() {
}
}

getWFFCStorageClass := func() *storagev1.StorageClass {
bindingMode := storagev1.VolumeBindingWaitForFirstConsumer
return &storagev1.StorageClass{
ObjectMeta: metav1.ObjectMeta{
Name: "wffc-storage",
},
VolumeBindingMode: &bindingMode,
}
}

DescribeTable("One wffc disk", func(vd *v1alpha2.VirtualDisk, vm *v1alpha2.VirtualMachine, status metav1.ConditionStatus, msg string) {
fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(vm, vd).Build()
sc := getWFFCStorageClass()
fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(vm, vd, sc).Build()

vmResource := reconciler.NewResource(namespacedName, fakeClient, vmFactoryByVM(vm), vmStatusGetter)
err := vmResource.Fetch(ctx)
Expand Down
Loading