Skip to content

feat(vm): Add firmware handler #874

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

Jump to bottom
Closed
yaroslavborbat wants to merge 8 commits into from
Closed

feat(vm): Add firmware handler #874

yaroslavborbat wants to merge 8 commits into from

Conversation

@yaroslavborbat
Copy link
Member

@yaroslavborbat yaroslavborbat commented Mar 20, 2025 •
edited
Loading

Description

Add firmware handler

Add 2 new metric

  • d8_virtualization_virtualmachine_info
  • d8_virtualization_virtualmachine_firmware_up_to_date

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

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.

Changelog entries

section: vm
type: feature
summary: Add firmware version in virtual machine status

@yaroslavborbat yaroslavborbat added this to the v0.17.0 milestone Mar 20, 2025
@yaroslavborbat yaroslavborbat force-pushed the feat/vm/firmware-handler branch 2 times, most recently from ef812f6 to f4ad3ff Compare March 24, 2025 15:08
@yaroslavborbat yaroslavborbat marked this pull request as ready for review March 24, 2025 17:35
@yaroslavborbat yaroslavborbat force-pushed the feat/vm/firmware-handler branch from 13bb65f to 52c2ae2 Compare March 24, 2025 22:49
@yaroslavborbat
add firmware handler
61ea5b2 
Signed-off-by: Yaroslav Borbat <[email protected]>
@yaroslavborbat
add metric
f5b8c4b 
Signed-off-by: Yaroslav Borbat <[email protected]>
@yaroslavborbat yaroslavborbat force-pushed the feat/vm/firmware-handler branch from 30f5abc to f5b8c4b Compare March 24, 2025 23:05
@yaroslavborbat
fix
f7ee533 
Signed-off-by: Yaroslav Borbat <[email protected]>
@yaroslavborbat
fix
7749f12 
Signed-off-by: Yaroslav Borbat <[email protected]>
z9r5
z9r5 reviewed Mar 25, 2025
View reviewed changes
z9r5
z9r5 previously approved these changes Mar 25, 2025
View reviewed changes
@yaroslavborbat
fix
a2b1c2e 
Signed-off-by: Yaroslav Borbat <[email protected]>
Isteb4k
Isteb4k requested changes Mar 25, 2025
View reviewed changes
api/core/v1alpha2/vmcondition/condition.go Outdated
TypeFilesystemFrozen Type = "FilesystemFrozen"
TypeSizingPolicyMatched Type = "SizingPolicyMatched"
TypeSnapshotting Type = "Snapshotting"
TypeFirmwareNeedUpdate Type = "FirmwareNeedUpdate"
Copy link
Contributor

@Isteb4k Isteb4k Mar 25, 2025

Choose a reason for hiding this comment

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

The reason violates the rules of the English language.

Copy link
Member Author

@yaroslavborbat yaroslavborbat Mar 25, 2025

Choose a reason for hiding this comment

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

I replaced it with FirmwareUpdateRequired. Is that okay?
I don’t like how FirmwareUpdateNeeded sounds.

api/core/v1alpha2/vmcondition/condition.go Outdated
ReasonPodConditionMissing Reason = "PodConditionMissing"
ReasonGuestNotRunning Reason = "GuestNotRunning"

ReasonFirmwareNeedUpdate Reason = "FirmwareNeedUpdate"
Copy link
Contributor

@Isteb4k Isteb4k Mar 25, 2025

Choose a reason for hiding this comment

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

Cover with doc comments

Copy link
Member Author

@yaroslavborbat yaroslavborbat Mar 25, 2025

Choose a reason for hiding this comment

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

FirmwareUpdateRequired

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

if kvvmi == nil || kvvmi.Status.LauncherContainerImageVersion == f.firmwareImage {
// If kvvmi does not exist, update the firmware version,
// as any newly created kvvmi will use the currently available firmware version.
Copy link
Contributor

@Isteb4k Isteb4k Mar 25, 2025

Choose a reason for hiding this comment

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

"Can we guarantee this? What if the virt-controller has been updated, but the virt-operator hasn't yet? In that case, the virtual machine will start with the old firmware, but the version will already be the new one.

Copy link
Member Author

@yaroslavborbat yaroslavborbat Mar 25, 2025

Choose a reason for hiding this comment

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

You're right. Currently, we cannot guarantee that our controller and KubeVirt are using the same configuration. A possible solution could be to introduce a special wait for the readiness of KubeVirt components during startup.

images/virtualization-artifact/pkg/version/version.go Outdated
return edition
import "golang.org/x/mod/semver"

const MainVersion = "main"
Copy link
Contributor

@Isteb4k Isteb4k Mar 25, 2025

Choose a reason for hiding this comment

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

It's not clear why we need this const. Is it for debugging? Maybe it should be taken from firmware.yaml as well?

Copy link
Member Author

@yaroslavborbat yaroslavborbat Mar 25, 2025

Choose a reason for hiding this comment

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

main is valid version for firmware. This const used in IsValid and IsMain methods in Version struct

images/virtualization-artifact/pkg/version/firmware.yaml Outdated
@@ -0,0 +1,3 @@
# Don't touch! Generating in CI
Copy link
Contributor

@Isteb4k Isteb4k Mar 25, 2025

Choose a reason for hiding this comment

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

There are two firmware.yaml files in the repo. Do we need both of them? If yes, add comments why

Copy link
Member Author

@yaroslavborbat yaroslavborbat Mar 25, 2025

Choose a reason for hiding this comment

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

There are two version_map.yml files in the repo, and both are needed:
The first one is should be generated in CI and copied during the build.
The second one is required for local builds and to prevent Go from complaining about a missing embedded file.

Isteb4k
Isteb4k requested changes Mar 25, 2025
View reviewed changes
Copy link
Contributor

@Isteb4k Isteb4k left a comment

Choose a reason for hiding this comment

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

It's necessary to sync the PR with ADR.

@yaroslavborbat
fix
ea52fb8 
Signed-off-by: Yaroslav Borbat <[email protected]>
@yaroslavborbat
fix
dbe540a 
Signed-off-by: Yaroslav Borbat <[email protected]>
@yaroslavborbat yaroslavborbat force-pushed the feat/vm/firmware-handler branch from 54fd922 to dbe540a Compare March 25, 2025 18:22
@yaroslavborbat
fix
129ea22 
Signed-off-by: Yaroslav Borbat <[email protected]>
@yaroslavborbat
Copy link
Member Author

yaroslavborbat commented Mar 26, 2025

design changed

@yaroslavborbat yaroslavborbat marked this pull request as draft March 26, 2025 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Isteb4k Isteb4k Isteb4k requested changes

@z9r5 z9r5 z9r5 left review comments

@universal-itengineer universal-itengineer Awaiting requested review from universal-itengineer universal-itengineer is a code owner

@nevermarine nevermarine Awaiting requested review from nevermarine nevermarine is a code owner

@fl64 fl64 Awaiting requested review from fl64 fl64 is a code owner

@goganat goganat Awaiting requested review from goganat goganat is a code owner

Assignees

@yaroslavborbat yaroslavborbat

Labels

None yet

Projects

None yet

Milestone

v0.17.0

Development

Successfully merging this pull request may close these issues.

4 participants

@yaroslavborbat @z9r5 @Isteb4k