-
Notifications
You must be signed in to change notification settings - Fork 92
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.
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
libvirt: Add podvm instance cpu and mem size support for libvirt #2116
base: main
Are you sure you want to change the base?
libvirt: Add podvm instance cpu and mem size support for libvirt #2116
Conversation
628f293
to
5c98498
Compare
Add podvm instance cpu and mem size support for libvirt Fixes: confidential-containers#1650 Signed-off-by : SAVITRI HUNASHEEKATTI <[email protected]>
5c98498
to
951f560
Compare
Add podvm instance cpu and mem size support for libvirt Fixes: confidential-containers#1650 Signed-off-by : SAVITRI HUNASHEEKATTI <[email protected]>
Add podvm instance cpu and mem size support for libvirt Fixes: confidential-containers#1650 Signed-off-by : SAVITRI HUNASHEEKATTI <[email protected]>
ca6e52a
to
0bf4a2d
Compare
Removed extra closing bracket Signed-off-by : SAVITRI HUNASHEEKATTI <[email protected]>
0bf4a2d
to
36c06e3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left a couple of comments, but we also need the commits squashed together so it is more reviewable. Thanks
// Convert Memory size from MegaBytes to GigaBytes | ||
LIBVIRT_CONV_MEM = 1000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The kata annotation is in MiB (https://github.com/kata-containers/kata-containers/blob/main/docs/how-to/how-to-set-sandbox-config-kata.md#hypervisor-options) and the domainXML we create is in GiB (
Memory: &libvirtxml.DomainMemory{Value: cfg.mem, Unit: "GiB", DumpCore: "on"}, |
@@ -56,9 +56,12 @@ func (p *libvirtProvider) CreateInstance(ctx context.Context, podName, sandboxID | |||
return nil, err | |||
} | |||
|
|||
// TODO: Specify the maximum instance name length in Libvirt | |||
vm := &vmConfig{name: instanceName, userData: userData, firmware: p.serviceConfig.Firmware} | |||
// Convert the memory units in gigabytes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment need updating too.
[[ "${LIBVIRT_CPU}" ]] && optionals+="-CPU ${LIBVIRT_CPU} " | ||
[[ "${LIBVIRT_MEMORY}" ]] && optionals+="-Memory ${LIBVIRT_MEMORY} " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like convention is for the flags to be lowercase
@@ -28,6 +28,8 @@ configMapGenerator: | |||
- LIBVIRT_EFI_FIRMWARE="/usr/share/OVMF/OVMF_CODE_4M.fd" # Edit to change the EFI firmware path, or comment to unset, if not using EFI. | |||
#- LIBVIRT_LAUNCH_SECURITY="" #sev or s390-pv | |||
#- LIBVIRT_VOL_NAME="" # Uncomment and set if you want to use a specific volume name. Defaults to podvm-base.qcow2 | |||
#- LIBVIRT_CPU="2" | |||
#- LIBVIRT_Memory="800 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean 8196 here?
Use io.katacontainers.config.hypervisor.default_vcpus and io.katacontainers.config.hypervisor.default_memory annotations to set the libvirt podvm instance. Use the default values if no annotations are provided.
Fixes: #1650
Signed-off-by : savitrilh [email protected]