Skip to content

fix(core): run firmware upgrade migration only for running vmi #2162

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

Merged
danilrwx merged 7 commits into from
Mar 27, 2026
Merged

fix(core): run firmware upgrade migration only for running vmi #2162

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
379000b
fix(core): run firmware upgrade migration only for running vmi
danilrwx Mar 26, 2026
414af67
fix(workload-updater): handle missing KVVMI in firmware handler
danilrwx Mar 26, 2026
400c809
test(workload-updater): extend firmware handler branch coverage
danilrwx Mar 26, 2026
dad8d79
test(workload-updater): cover nodeplacement edge branches
danilrwx Mar 26, 2026
ff45f17
test(workload-updater): remove unparam warning in firmware tests
danilrwx Mar 27, 2026
246cd65
fix(core): gate firmware outdated state by running kvvmi
danilrwx Mar 27, 2026
6d098c7
test(workload-updater): remove obsolete kvvmi checks from firmware tests
danilrwx Mar 27, 2026
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
2 changes: 1 addition & 1 deletion images/virtualization-artifact/pkg/controller/vm/internal/firmware.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ func (h *FirmwareHandler) syncFirmwareUpToDate(vm *v1alpha2.VirtualMachine, kvvm
return
}

upToDate := kvvmi == nil || kvvmi.Status.LauncherContainerImageVersion == "" || kvvmi.Status.LauncherContainerImageVersion == h.image
upToDate := kvvmi == nil || kvvmi.Status.Phase == virtv1.Succeeded || kvvmi.Status.Phase == virtv1.Failed || kvvmi.Status.LauncherContainerImageVersion == "" || kvvmi.Status.LauncherContainerImageVersion == h.image

cb := conditions.NewConditionBuilder(vmcondition.TypeFirmwareUpToDate).Generation(vm.GetGeneration())
defer func() {
Expand Down
43 changes: 24 additions & 19 deletions images/virtualization-artifact/pkg/controller/vm/internal/firmware_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,9 +58,10 @@ var _ = Describe("TestFirmwareHandler", func() {
return vmbuilder.NewEmpty(name, namespace)
}

newKVVMI := func(image string) *virtv1.VirtualMachineInstance {
newKVVMI := func(image string, phase virtv1.VirtualMachineInstancePhase) *virtv1.VirtualMachineInstance {
kvvmi := newEmptyKVVMI(name, namespace)
kvvmi.Status.LauncherContainerImageVersion = image
kvvmi.Status.Phase = phase
return kvvmi
}

Expand Down Expand Up @@ -88,13 +89,15 @@ var _ = Describe("TestFirmwareHandler", func() {
Expect(upToDate.Reason).To(Equal(expectedReason.String()))
}
},
Entry("Should be up to date", newVM(), newKVVMI(expectedImage), metav1.ConditionTrue, vmcondition.ReasonFirmwareUpToDate, false),
Entry("Should be up to date", newVM(), newKVVMI(expectedImage, virtv1.Running), metav1.ConditionTrue, vmcondition.ReasonFirmwareUpToDate, false),
Entry("Should be up to date because kvvmi is not exists", newVM(), nil, metav1.ConditionTrue, vmcondition.ReasonFirmwareUpToDate, false),
Entry("Should be out of date 1", newVM(), newKVVMI("other-image-1"), metav1.ConditionFalse, vmcondition.ReasonFirmwareOutOfDate, true),
Entry("Should be out of date 2", newVM(), newKVVMI("other-image-2"), metav1.ConditionFalse, vmcondition.ReasonFirmwareOutOfDate, true),
Entry("Should be out of date 3", newVM(), newKVVMI("other-image-3"), metav1.ConditionFalse, vmcondition.ReasonFirmwareOutOfDate, true),
Entry("Should be out of date 4", newVM(), newKVVMI("other-image-4"), metav1.ConditionFalse, vmcondition.ReasonFirmwareOutOfDate, true),
Entry("Should be out of date 5", newVM(), newKVVMI("other-image-5"), metav1.ConditionFalse, vmcondition.ReasonFirmwareOutOfDate, true),
Entry("Should be up to date because kvvmi is succeeded", newVM(), newKVVMI("other-image", virtv1.Succeeded), metav1.ConditionTrue, vmcondition.ReasonFirmwareUpToDate, false),
Entry("Should be up to date because kvvmi is failed", newVM(), newKVVMI("other-image", virtv1.Failed), metav1.ConditionTrue, vmcondition.ReasonFirmwareUpToDate, false),
Entry("Should be out of date 1", newVM(), newKVVMI("other-image-1", virtv1.Running), metav1.ConditionFalse, vmcondition.ReasonFirmwareOutOfDate, true),
Entry("Should be out of date 2", newVM(), newKVVMI("other-image-2", virtv1.Running), metav1.ConditionFalse, vmcondition.ReasonFirmwareOutOfDate, true),
Entry("Should be out of date 3", newVM(), newKVVMI("other-image-3", virtv1.Running), metav1.ConditionFalse, vmcondition.ReasonFirmwareOutOfDate, true),
Entry("Should be out of date 4", newVM(), newKVVMI("other-image-4", virtv1.Running), metav1.ConditionFalse, vmcondition.ReasonFirmwareOutOfDate, true),
Entry("Should be out of date 5", newVM(), newKVVMI("other-image-5", virtv1.Running), metav1.ConditionFalse, vmcondition.ReasonFirmwareOutOfDate, true),
)

DescribeTable("Condition TypeFirmwareUpToDate should be in the expected state considering the VM phase",
Expand All @@ -111,22 +114,24 @@ var _ = Describe("TestFirmwareHandler", func() {
Expect(upToDate.Status).To(Equal(expectedStatus))
}
},
Entry("Running phase, condition should not be set", newVM(), v1alpha2.MachineRunning, newKVVMI(expectedImage), metav1.ConditionUnknown, false),
Entry("Running phase, condition should be set", newVM(), v1alpha2.MachineRunning, newKVVMI("other-image-1"), metav1.ConditionFalse, true),
Entry("Running phase, condition should not be set", newVM(), v1alpha2.MachineRunning, newKVVMI(expectedImage, virtv1.Running), metav1.ConditionUnknown, false),
Entry("Running phase, condition should be set", newVM(), v1alpha2.MachineRunning, newKVVMI("other-image-1", virtv1.Running), metav1.ConditionFalse, true),
Entry("Running phase, condition should not be set for succeeded kvvmi", newVM(), v1alpha2.MachineRunning, newKVVMI("other-image-1", virtv1.Succeeded), metav1.ConditionUnknown, false),
Entry("Running phase, condition should not be set for failed kvvmi", newVM(), v1alpha2.MachineRunning, newKVVMI("other-image-1", virtv1.Failed), metav1.ConditionUnknown, false),

Entry("Migrating phase, condition should not be set", newVM(), v1alpha2.MachineMigrating, newKVVMI(expectedImage), metav1.ConditionUnknown, false),
Entry("Migrating phase, condition should be set", newVM(), v1alpha2.MachineMigrating, newKVVMI("other-image-1"), metav1.ConditionFalse, true),
Entry("Migrating phase, condition should not be set", newVM(), v1alpha2.MachineMigrating, newKVVMI(expectedImage, virtv1.Running), metav1.ConditionUnknown, false),
Entry("Migrating phase, condition should be set", newVM(), v1alpha2.MachineMigrating, newKVVMI("other-image-1", virtv1.Running), metav1.ConditionFalse, true),

Entry("Stopping phase, condition should not be set", newVM(), v1alpha2.MachineStopping, newKVVMI(expectedImage), metav1.ConditionUnknown, false),
Entry("Stopping phase, condition should be set", newVM(), v1alpha2.MachineStopping, newKVVMI("other-image-1"), metav1.ConditionFalse, true),
Entry("Stopping phase, condition should not be set", newVM(), v1alpha2.MachineStopping, newKVVMI(expectedImage, virtv1.Running), metav1.ConditionUnknown, false),
Entry("Stopping phase, condition should be set", newVM(), v1alpha2.MachineStopping, newKVVMI("other-image-1", virtv1.Running), metav1.ConditionFalse, true),

Entry("Pending phase, condition should not be set", newVM(), v1alpha2.MachinePending, newKVVMI(expectedImage), metav1.ConditionUnknown, false),
Entry("Pending phase, condition should not be set", newVM(), v1alpha2.MachinePending, newKVVMI("other-image-1"), metav1.ConditionUnknown, false),
Entry("Pending phase, condition should not be set", newVM(), v1alpha2.MachinePending, newKVVMI(expectedImage, virtv1.Running), metav1.ConditionUnknown, false),
Entry("Pending phase, condition should not be set", newVM(), v1alpha2.MachinePending, newKVVMI("other-image-1", virtv1.Running), metav1.ConditionUnknown, false),

Entry("Starting phase, condition should not be set", newVM(), v1alpha2.MachineStarting, newKVVMI(expectedImage), metav1.ConditionUnknown, false),
Entry("Starting phase, condition should not be set", newVM(), v1alpha2.MachineStarting, newKVVMI("other-image-1"), metav1.ConditionUnknown, false),
Entry("Starting phase, condition should not be set", newVM(), v1alpha2.MachineStarting, newKVVMI(expectedImage, virtv1.Running), metav1.ConditionUnknown, false),
Entry("Starting phase, condition should not be set", newVM(), v1alpha2.MachineStarting, newKVVMI("other-image-1", virtv1.Running), metav1.ConditionUnknown, false),

Entry("Stopped phase, condition should not be set", newVM(), v1alpha2.MachineStopped, newKVVMI(expectedImage), metav1.ConditionUnknown, false),
Entry("Stopped phase, condition should not be set", newVM(), v1alpha2.MachineStopped, newKVVMI("other-image-1"), metav1.ConditionUnknown, false),
Entry("Stopped phase, condition should not be set", newVM(), v1alpha2.MachineStopped, newKVVMI(expectedImage, virtv1.Running), metav1.ConditionUnknown, false),
Entry("Stopped phase, condition should not be set", newVM(), v1alpha2.MachineStopped, newKVVMI("other-image-1", virtv1.Running), metav1.ConditionUnknown, false),
)
})
8 changes: 6 additions & 2 deletions images/virtualization-artifact/pkg/controller/workload-updater/internal/handler/firmware.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,11 @@ import (

const firmwareHandler = "FirmwareHandler"

func NewFirmwareHandler(client client.Client, migration OneShotMigration, firmwareImage, namespace, virtControllerName string) *FirmwareHandler {
type firmwareMigration interface {
OnceMigrate(ctx context.Context, vm *v1alpha2.VirtualMachine, annotationKey, annotationExpectedValue string) (bool, error)
}

func NewFirmwareHandler(client client.Client, migration firmwareMigration, firmwareImage, namespace, virtControllerName string) *FirmwareHandler {
return &FirmwareHandler{
client: client,
oneShotMigration: migration,
Expand All @@ -48,7 +52,7 @@ func NewFirmwareHandler(client client.Client, migration OneShotMigration, firmwa

type FirmwareHandler struct {
client client.Client
oneShotMigration OneShotMigration
oneShotMigration firmwareMigration
firmwareImage string
namespace string
virtControllerName string
Expand Down
218 changes: 204 additions & 14 deletions ...virtualization-artifact/pkg/controller/workload-updater/internal/handler/firmware_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,14 @@ import (
"github.com/deckhouse/virtualization/api/core/v1alpha2/vmcondition"
)

type firmwareMigrationStub struct {
onceMigrate func(ctx context.Context, vm *v1alpha2.VirtualMachine, annotationKey, annotationExpectedValue string) (bool, error)
}

func (s *firmwareMigrationStub) OnceMigrate(ctx context.Context, vm *v1alpha2.VirtualMachine, annotationKey, annotationExpectedValue string) (bool, error) {
return s.onceMigrate(ctx, vm, annotationKey, annotationExpectedValue)
}

var _ = Describe("TestFirmwareHandler", func() {
const (
name = "vm-firmware"
Expand All @@ -54,19 +62,31 @@ var _ = Describe("TestFirmwareHandler", func() {
fakeClient = nil
})

newVM := func(needMigrate bool) *v1alpha2.VirtualMachine {
newVM := func(firmwareUpToDateStatus metav1.ConditionStatus) *v1alpha2.VirtualMachine {
vm := vmbuilder.NewEmpty(name, namespace)
status := metav1.ConditionTrue
if needMigrate {
status = metav1.ConditionFalse
}
vm.Status.Conditions = append(vm.Status.Conditions, metav1.Condition{
Type: vmcondition.TypeFirmwareUpToDate.String(),
Status: status,
Status: firmwareUpToDateStatus,
})
return vm
}

newVMNeedMigrate := func() *v1alpha2.VirtualMachine {
return newVM(metav1.ConditionFalse)
}

newVMUpToDate := func() *v1alpha2.VirtualMachine {
return newVM(metav1.ConditionTrue)
}

setupFirmwareEnvironment := func(vm *v1alpha2.VirtualMachine, objs ...client.Object) client.Client {
allObjects := []client.Object{vm}
allObjects = append(allObjects, objs...)
fakeClient, err := testutil.NewFakeClientWithObjects(allObjects...)
Expect(err).NotTo(HaveOccurred())
return fakeClient
}

newVirtController := func(ready bool, launcherImage string) *appsv1.Deployment {
deploy := &appsv1.Deployment{
TypeMeta: metav1.TypeMeta{
Expand All @@ -90,20 +110,20 @@ var _ = Describe("TestFirmwareHandler", func() {
},
},
}
deploy.Status.Replicas = 1
if ready {
deploy.Status.ReadyReplicas = 1
deploy.Status.Replicas = 1
}

return deploy
}

DescribeTable("FirmwareHandler should return serviceCompleteErr if migration executed",
func(vm *v1alpha2.VirtualMachine, deploy *appsv1.Deployment, needMigrate bool) {
fakeClient = setupEnvironment(vm, deploy)
fakeClient = setupFirmwareEnvironment(vm, deploy)

mockMigration := &OneShotMigrationMock{
OnceMigrateFunc: func(ctx context.Context, vm *v1alpha2.VirtualMachine, annotationKey, annotationExpectedValue string) (bool, error) {
mockMigration := &firmwareMigrationStub{
onceMigrate: func(ctx context.Context, vm *v1alpha2.VirtualMachine, annotationKey, annotationExpectedValue string) (bool, error) {
return true, serviceCompleteErr
},
}
Expand All @@ -118,9 +138,179 @@ var _ = Describe("TestFirmwareHandler", func() {
Expect(err).NotTo(HaveOccurred())
}
},
Entry("Migration should be executed", newVM(true), newVirtController(true, firmwareImage), true),
Entry("Migration not should be executed", newVM(false), newVirtController(true, firmwareImage), false),
Entry("Migration not should be executed because virt-controller not ready", newVM(false), newVirtController(false, firmwareImage), false),
Entry("Migration not should be executed because virt-controller ready but has wrong image", newVM(false), newVirtController(true, "wrong-image"), false),
Entry("Migration should be executed", newVMNeedMigrate(), newVirtController(true, firmwareImage), true),
Entry("Migration not should be executed", newVMUpToDate(), newVirtController(true, firmwareImage), false),
Entry("Migration not should be executed because virt-controller not ready", newVMNeedMigrate(), newVirtController(false, firmwareImage), false),
Entry("Migration not should be executed because virt-controller ready but has wrong image", newVMNeedMigrate(), newVirtController(true, "wrong-image"), false),
)

It("should call migration when update is needed and virt-controller is ready", func() {
vm := newVMNeedMigrate()
deploy := newVirtController(true, firmwareImage)
fakeClient = setupFirmwareEnvironment(vm, deploy)

migrationCalled := false
h := NewFirmwareHandler(fakeClient, &firmwareMigrationStub{
onceMigrate: func(ctx context.Context, vm *v1alpha2.VirtualMachine, annotationKey, annotationExpectedValue string) (bool, error) {
migrationCalled = true
return false, nil
},
}, firmwareImage, virtControllerNamespace, virtControllerName)

_, err := h.Handle(ctx, vm)
Expect(err).NotTo(HaveOccurred())
Expect(migrationCalled).To(BeTrue())
})

It("should return nil when vm is nil", func() {
h := NewFirmwareHandler(nil, nil, firmwareImage, virtControllerNamespace, virtControllerName)

_, err := h.Handle(ctx, nil)
Expect(err).NotTo(HaveOccurred())
})

It("should return nil when vm has deletion timestamp", func() {
vm := newVMNeedMigrate()
now := metav1.Now()
vm.DeletionTimestamp = &now

h := NewFirmwareHandler(nil, nil, firmwareImage, virtControllerNamespace, virtControllerName)
_, err := h.Handle(ctx, vm)
Expect(err).NotTo(HaveOccurred())
})

It("should return error when virt-controller deployment get fails", func() {
vm := newVMNeedMigrate()
fakeClient = setupFirmwareEnvironment(vm)

h := NewFirmwareHandler(fakeClient, &firmwareMigrationStub{
onceMigrate: func(ctx context.Context, vm *v1alpha2.VirtualMachine, annotationKey, annotationExpectedValue string) (bool, error) {
return false, nil
},
}, firmwareImage, virtControllerNamespace, virtControllerName)

_, err := h.Handle(ctx, vm)
Expect(err).To(HaveOccurred())
})

Describe("needUpdate", func() {
buildVMWithConditions := func(conditions []metav1.Condition) *v1alpha2.VirtualMachine {
vm := vmbuilder.NewEmpty(name, namespace)
vm.Status.Conditions = conditions
return vm
}

DescribeTable("should evaluate FirmwareUpToDate condition",
func(vm *v1alpha2.VirtualMachine, expected bool) {
h := NewFirmwareHandler(nil, nil, firmwareImage, virtControllerNamespace, virtControllerName)
Expect(h.needUpdate(vm)).To(Equal(expected))
},
Entry("FirmwareUpToDate is False", buildVMWithConditions([]metav1.Condition{{
Type: vmcondition.TypeFirmwareUpToDate.String(),
Status: metav1.ConditionFalse,
}}), true),
Entry("FirmwareUpToDate is True", buildVMWithConditions([]metav1.Condition{{
Type: vmcondition.TypeFirmwareUpToDate.String(),
Status: metav1.ConditionTrue,
}}), false),
Entry("FirmwareUpToDate is absent", buildVMWithConditions([]metav1.Condition{{
Type: "SomeOtherCondition",
Status: metav1.ConditionTrue,
}}), false),
)
})

Describe("isVirtControllerUpToDate", func() {
It("should return error when deployment is not found", func() {
h := NewFirmwareHandler(setupFirmwareEnvironment(newVMNeedMigrate()), nil, firmwareImage, virtControllerNamespace, virtControllerName)

ready, err := h.isVirtControllerUpToDate(ctx)
Expect(err).To(HaveOccurred())
Expect(ready).To(BeFalse())
})

DescribeTable("should evaluate deployment state and launcher image",
func(deploy *appsv1.Deployment, expectedReady bool) {
h := NewFirmwareHandler(setupFirmwareEnvironment(newVMNeedMigrate(), deploy), nil, firmwareImage, virtControllerNamespace, virtControllerName)

ready, err := h.isVirtControllerUpToDate(ctx)
Expect(err).NotTo(HaveOccurred())
Expect(ready).To(Equal(expectedReady))
},
Entry("deployment is not ready", newVirtController(false, firmwareImage), false),
Entry("deployment is ready but image differs", newVirtController(true, "other-image"), false),
Entry("deployment is ready and image matches", newVirtController(true, firmwareImage), true),
)
})

Describe("getVirtLauncherImage", func() {
It("should return empty when virt-controller container is absent", func() {
deploy := &appsv1.Deployment{
Spec: appsv1.DeploymentSpec{
Template: corev1.PodTemplateSpec{
Spec: corev1.PodSpec{
Containers: []corev1.Container{{
Name: "sidecar",
Args: []string{"--launcher-image", firmwareImage},
}},
},
},
},
}
Expect(getVirtLauncherImage(deploy)).To(BeEmpty())
})

It("should return empty when launcher image argument is absent", func() {
deploy := &appsv1.Deployment{
Spec: appsv1.DeploymentSpec{
Template: corev1.PodTemplateSpec{
Spec: corev1.PodSpec{
Containers: []corev1.Container{{
Name: "virt-controller",
Args: []string{"--some-flag", "value"},
}},
},
},
},
}
Expect(getVirtLauncherImage(deploy)).To(BeEmpty())
})

It("should return launcher image from --launcher-image=<value>", func() {
deploy := &appsv1.Deployment{
Spec: appsv1.DeploymentSpec{
Template: corev1.PodTemplateSpec{
Spec: corev1.PodSpec{
Containers: []corev1.Container{{
Name: "virt-controller",
Args: []string{"--launcher-image=" + firmwareImage},
}},
},
},
},
}
Expect(getVirtLauncherImage(deploy)).To(Equal(firmwareImage))
})

It("should return launcher image from command arguments", func() {
deploy := &appsv1.Deployment{
Spec: appsv1.DeploymentSpec{
Template: corev1.PodTemplateSpec{
Spec: corev1.PodSpec{
Containers: []corev1.Container{{
Name: "virt-controller",
Command: []string{"virt-controller", "--launcher-image", firmwareImage},
}},
},
},
},
}
Expect(getVirtLauncherImage(deploy)).To(Equal(firmwareImage))
})
})

It("should return firmware handler name", func() {
h := NewFirmwareHandler(nil, nil, firmwareImage, virtControllerNamespace, virtControllerName)
Expect(h.Name()).To(Equal(firmwareHandler))
})
})
Loading
Loading