Skip to content
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

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

savitrilh
Copy link

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 savitrilh@in.ibm.com

@savitrilh savitrilh requested a review from a team as a code owner October 14, 2024 09:55
Add podvm instance cpu and mem size support for libvirt

Fixes: confidential-containers#1650

Signed-off-by : SAVITRI HUNASHEEKATTI <savitrilh@in.ibm.com>
@savitrilh savitrilh marked this pull request as draft October 14, 2024 15:06
Add podvm instance cpu and mem size support for libvirt

Fixes: confidential-containers#1650

Signed-off-by : SAVITRI HUNASHEEKATTI <savitrilh@in.ibm.com>
Add podvm instance cpu and mem size support for libvirt

Fixes: confidential-containers#1650

Signed-off-by : SAVITRI HUNASHEEKATTI <savitrilh@in.ibm.com>
@savitrilh savitrilh force-pushed the libvirt-resource branch 2 times, most recently from ca6e52a to 0bf4a2d Compare January 8, 2025 09:17
Removed extra closing bracket

Signed-off-by : SAVITRI HUNASHEEKATTI <savitrilh@in.ibm.com>
Copy link
Member

@stevenhorsman stevenhorsman left a 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

@savitrilh savitrilh force-pushed the libvirt-resource branch 2 times, most recently from bb190bd to f17c97b Compare February 17, 2025 07:42
Use io.katacontainers.config.hypervisor.default_vcpus and io.katacontainers.config.hypervisor.default_memory annotations to set the libvirt podvm instance.

Signed-off-by: savitrilh <138098272+savitrilh@users.noreply.github.com>
provider.DefaultToEnv(&memoryStr, "LIBVIRT_MEMORY", defaultMemory)
if memoryStr != "" {
memory, _ := strconv.ParseUint(memoryStr, 10, 64)
libvirtcfg.Memory = uint(memory)

Check failure

Code scanning / CodeQL

Incorrect conversion between integer types High

Incorrect conversion of an unsigned 64-bit integer from
strconv.ParseUint
to a lower bit size type uint without an upper bound check.
provider.DefaultToEnv(&processorsStr, "LIBVIRT_CPU", defaultCPU)
if processorsStr != "" {
cpu, _ := strconv.ParseUint(processorsStr, 10, 64)
libvirtcfg.CPU = uint(cpu)

Check failure

Code scanning / CodeQL

Incorrect conversion between integer types High

Incorrect conversion of an unsigned 64-bit integer from
strconv.ParseUint
to a lower bit size type uint without an upper bound check.
set -x
exec cloud-api-adaptor libvirt \
-pods-dir "${PEER_PODS_DIR}" \
-socket "${REMOTE_HYPERVISOR_ENDPOINT}" \
-uri "${LIBVIRT_URI}" \
-data-dir /opt/data-dir \
-network-name "${LIBVIRT_NET:-default}" \
-pool-name "${LIBVIRT_POOL:-default}" \
-pool-name "${LIBVIRT_POOL:-default}"
Copy link
Member

Choose a reason for hiding this comment

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

I think this needs the \, otherwise all the optionals are treated like a new line?

Comment on lines +59 to +60
memory, _ := strconv.ParseUint(memoryStr, 10, 64)
libvirtcfg.Memory = uint(memory)
Copy link
Member

Choose a reason for hiding this comment

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

This is flagged up as a potentially unsafe conversion, maybe we should explicitly make libvirtcfg.Memory a uint64 type?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

libvirt: Add podvm instance cpu/mem size support for libvirt
2 participants