Question about libvirtxml and kubernetes/kubevirt conventions
Hi everyone, I would like to bring up a possible issue with the libvirt-go-xml-module and request your advice. We are considering using libvirtxml [2] as an alternative of the custom types for the libvirt schema in KubeVirt [1]. Nonetheless, KubeVirt makes an effort to adhere to Kubernetes best guidelines. One thing I've noticed, though, is that libvirtml consistently uses uint or int rather than uint32 or int32. This is in opposition to the primitive types that Kubernetes [3] recommends; in fact, KubeVirt uses them in the schema.go [4]. Would it be possible to align the libvirtxml go library to the Kubernetes conventions? [1] https://github.com/kubevirt/kubevirt [2] https://github.com/kubevirt/kubevirt/issues/10844 [3] https://github.com/kubernetes/community/blob/master/contributors/devel/sig-a... [4] https://github.com/kubevirt/kubevirt/blob/main/pkg/virt-launcher/virtwrap/ap... Many thanks, Alice
On Wed, Feb 07, 2024 at 10:06:46AM +0100, Alice Frosi wrote:
Hi everyone,
I would like to bring up a possible issue with the libvirt-go-xml-module and request your advice.
We are considering using libvirtxml [2] as an alternative of the custom types for the libvirt schema in KubeVirt [1]. Nonetheless, KubeVirt makes an effort to adhere to Kubernetes best guidelines. One thing I've noticed, though, is that libvirtml consistently uses uint or int rather than uint32 or int32. This is in opposition to the primitive types that Kubernetes [3] recommends; in fact, KubeVirt uses them in the schema.go [4].
I sure spotted several uses of (u)int in there :)
Would it be possible to align the libvirtxml go library to the Kubernetes conventions?
Since this library is all about handling XML and doesn't talk at all with the underlying C library, I guess it would be possible to introduce explicit sizing for all struct members. I don't think we can do that without breaking API though. Another topic entirely would be whether the same is possible for the main binding. In that case, the underlying C library dictates the types, so I'm not sure we could change them at the Go level even if we wanted to. I know that you're not currently asking for this, I'm just trying to consider the full picture. Taking a step back, the guidelines say All public integer fields MUST use the Go int32 or Go int64 types, not int (which is ambiguously sized, depending on target platform). Internal types may use int. As long as KubeVirt doesn't use int for its own public types, it shouldn't matter what libvirtxml does, as it's only used internally. Is that not the case? -- Andrea Bolognani / Red Hat / Virtualization
On Wed, Feb 07, 2024 at 01:54:30AM -0800, Andrea Bolognani wrote:
On Wed, Feb 07, 2024 at 10:06:46AM +0100, Alice Frosi wrote:
Hi everyone,
I would like to bring up a possible issue with the libvirt-go-xml-module and request your advice.
We are considering using libvirtxml [2] as an alternative of the custom types for the libvirt schema in KubeVirt [1]. Nonetheless, KubeVirt makes an effort to adhere to Kubernetes best guidelines. One thing I've noticed, though, is that libvirtml consistently uses uint or int rather than uint32 or int32. This is in opposition to the primitive types that Kubernetes [3] recommends; in fact, KubeVirt uses them in the schema.go [4].
I sure spotted several uses of (u)int in there :)
Would it be possible to align the libvirtxml go library to the Kubernetes conventions?
Since this library is all about handling XML and doesn't talk at all with the underlying C library, I guess it would be possible to introduce explicit sizing for all struct members. I don't think we can do that without breaking API though.
It would also be a major effort to decide between int32 and int64 if we want to honour the other k8s rule that says to avoid int64 unless absolutely required.
Another topic entirely would be whether the same is possible for the main binding. In that case, the underlying C library dictates the types, so I'm not sure we could change them at the Go level even if we wanted to. I know that you're not currently asking for this, I'm just trying to consider the full picture.
In the C API we have been pretty careful to use 'unsigned long long' everywhere it was needed, which is mapped to int64 in the go bindings. There were just 2 APIs where we mistakenly used 'unsigned long' which varied in size, but we map them to int64 in Go too. In any case, while k8s might care about 32-bit, in the virtualization space almost no one cares anymore. i386 is basically dead and liable to be removed from QEMU. Arm7 is limping along, but its use of virt is larged in embedded / special scenarios - no one is going to be offering public cloud with arm7. IOW, I see no reason for kubevirt to care about 32-bit targets, and thus for all practical purposes any use of 'int' can be assumed to always be 64-bit [1].
Taking a step back, the guidelines say
All public integer fields MUST use the Go int32 or Go int64 types, not int (which is ambiguously sized, depending on target platform). Internal types may use int.
As long as KubeVirt doesn't use int for its own public types, it shouldn't matter what libvirtxml does, as it's only used internally. Is that not the case?
The guidelines quoted are specifically in relation to YAML/JSON docs K8s sends/receives. The libvirtxml mod is just used internally be kubevirt to convert the YAML/JSON representation of a users' VM into the XML needed to launch QEMU. The key pain point for kubevirt will data casts ? With regards, Daniel [1] until 128-bit platforms arrive at which point there's going to be alot of cursing and swearing at all levels of the OS / app stack. -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Wed, Feb 7, 2024 at 11:13 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
On Wed, Feb 07, 2024 at 10:06:46AM +0100, Alice Frosi wrote:
Hi everyone,
I would like to bring up a possible issue with the
and request your advice.
We are considering using libvirtxml [2] as an alternative of the custom types for the libvirt schema in KubeVirt [1]. Nonetheless, KubeVirt makes an effort to adhere to Kubernetes best guidelines. One thing I've noticed, though, is that libvirtml consistently uses uint or int rather than uint32 or int32. This is in opposition to
primitive types that Kubernetes [3] recommends; in fact, KubeVirt uses
On Wed, Feb 07, 2024 at 01:54:30AM -0800, Andrea Bolognani wrote: libvirt-go-xml-module the them
in the schema.go [4].
I sure spotted several uses of (u)int in there :)
Would it be possible to align the libvirtxml go library to the Kubernetes conventions?
Since this library is all about handling XML and doesn't talk at all with the underlying C library, I guess it would be possible to introduce explicit sizing for all struct members. I don't think we can do that without breaking API though.
It would also be a major effort to decide between int32 and int64 if we want to honour the other k8s rule that says to avoid int64 unless absolutely required.
Another topic entirely would be whether the same is possible for the main binding. In that case, the underlying C library dictates the types, so I'm not sure we could change them at the Go level even if we wanted to. I know that you're not currently asking for this, I'm just trying to consider the full picture.
In the C API we have been pretty careful to use 'unsigned long long' everywhere it was needed, which is mapped to int64 in the go bindings.
There were just 2 APIs where we mistakenly used 'unsigned long' which varied in size, but we map them to int64 in Go too.
In any case, while k8s might care about 32-bit, in the virtualization space almost no one cares anymore. i386 is basically dead and liable to be removed from QEMU. Arm7 is limping along, but its use of virt is larged in embedded / special scenarios - no one is going to be offering public cloud with arm7.
IOW, I see no reason for kubevirt to care about 32-bit targets, and thus for all practical purposes any use of 'int' can be assumed to always be 64-bit [1].
But this isn't clear if we keep using (u)int types. What I'm suggesting here is if it would be possible in a next version of the library to use more explicit types. My concern is that KubeVirt uses (u)int32 as much as possible and in this case, we might have a mismatch of the types.
On Fri, Feb 09, 2024 at 08:43:56AM +0100, Alice Frosi wrote:
On Wed, Feb 7, 2024 at 11:13 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
On Wed, Feb 07, 2024 at 10:06:46AM +0100, Alice Frosi wrote:
Hi everyone,
I would like to bring up a possible issue with the
and request your advice.
We are considering using libvirtxml [2] as an alternative of the custom types for the libvirt schema in KubeVirt [1]. Nonetheless, KubeVirt makes an effort to adhere to Kubernetes best guidelines. One thing I've noticed, though, is that libvirtml consistently uses uint or int rather than uint32 or int32. This is in opposition to
primitive types that Kubernetes [3] recommends; in fact, KubeVirt uses
On Wed, Feb 07, 2024 at 01:54:30AM -0800, Andrea Bolognani wrote: libvirt-go-xml-module the them
in the schema.go [4].
I sure spotted several uses of (u)int in there :)
Would it be possible to align the libvirtxml go library to the Kubernetes conventions?
Since this library is all about handling XML and doesn't talk at all with the underlying C library, I guess it would be possible to introduce explicit sizing for all struct members. I don't think we can do that without breaking API though.
It would also be a major effort to decide between int32 and int64 if we want to honour the other k8s rule that says to avoid int64 unless absolutely required.
Another topic entirely would be whether the same is possible for the main binding. In that case, the underlying C library dictates the types, so I'm not sure we could change them at the Go level even if we wanted to. I know that you're not currently asking for this, I'm just trying to consider the full picture.
In the C API we have been pretty careful to use 'unsigned long long' everywhere it was needed, which is mapped to int64 in the go bindings.
There were just 2 APIs where we mistakenly used 'unsigned long' which varied in size, but we map them to int64 in Go too.
In any case, while k8s might care about 32-bit, in the virtualization space almost no one cares anymore. i386 is basically dead and liable to be removed from QEMU. Arm7 is limping along, but its use of virt is larged in embedded / special scenarios - no one is going to be offering public cloud with arm7.
IOW, I see no reason for kubevirt to care about 32-bit targets, and thus for all practical purposes any use of 'int' can be assumed to always be 64-bit [1].
But this isn't clear if we keep using (u)int types. What I'm suggesting here is if it would be possible in a next version of the library to use more explicit types.
My concern is that KubeVirt uses (u)int32 as much as possible and in this case, we might have a mismatch of the types.
The issue is that this would be a change of API, and significant amount of work to do to audit each case, to solve a precision problem that doesn't appear to have any real world functional impact. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Fri, Feb 09, 2024 at 09:37:24AM +0000, Daniel P. Berrangé wrote:
On Fri, Feb 09, 2024 at 08:43:56AM +0100, Alice Frosi wrote:
On Wed, Feb 7, 2024 at 11:13 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
On Wed, Feb 07, 2024 at 10:06:46AM +0100, Alice Frosi wrote:
Hi everyone,
I would like to bring up a possible issue with the
and request your advice.
We are considering using libvirtxml [2] as an alternative of the custom types for the libvirt schema in KubeVirt [1]. Nonetheless, KubeVirt makes an effort to adhere to Kubernetes best guidelines. One thing I've noticed, though, is that libvirtml consistently uses uint or int rather than uint32 or int32. This is in opposition to
primitive types that Kubernetes [3] recommends; in fact, KubeVirt uses
On Wed, Feb 07, 2024 at 01:54:30AM -0800, Andrea Bolognani wrote: libvirt-go-xml-module the them
in the schema.go [4].
I sure spotted several uses of (u)int in there :)
Would it be possible to align the libvirtxml go library to the Kubernetes conventions?
Since this library is all about handling XML and doesn't talk at all with the underlying C library, I guess it would be possible to introduce explicit sizing for all struct members. I don't think we can do that without breaking API though.
It would also be a major effort to decide between int32 and int64 if we want to honour the other k8s rule that says to avoid int64 unless absolutely required.
Another topic entirely would be whether the same is possible for the main binding. In that case, the underlying C library dictates the types, so I'm not sure we could change them at the Go level even if we wanted to. I know that you're not currently asking for this, I'm just trying to consider the full picture.
In the C API we have been pretty careful to use 'unsigned long long' everywhere it was needed, which is mapped to int64 in the go bindings.
There were just 2 APIs where we mistakenly used 'unsigned long' which varied in size, but we map them to int64 in Go too.
In any case, while k8s might care about 32-bit, in the virtualization space almost no one cares anymore. i386 is basically dead and liable to be removed from QEMU. Arm7 is limping along, but its use of virt is larged in embedded / special scenarios - no one is going to be offering public cloud with arm7.
IOW, I see no reason for kubevirt to care about 32-bit targets, and thus for all practical purposes any use of 'int' can be assumed to always be 64-bit [1].
But this isn't clear if we keep using (u)int types. What I'm suggesting here is if it would be possible in a next version of the library to use more explicit types.
My concern is that KubeVirt uses (u)int32 as much as possible and in this case, we might have a mismatch of the types.
The issue is that this would be a change of API, and significant amount of work to do to audit each case, to solve a precision problem that doesn't appear to have any real world functional impact.
Yes, this would force us to declare a v2 API. Annoying, but less so for a Go library than a C one. And let's ignore the amount of work that would be necessary for a moment. I tend to agree with Alice that this would be good to have. Precisely-sized integer types are very commonly used in Go and Rust, and they make the API a lot better. Keep in mind that KubeVirt uses uint32 a lot for its public API (as recommended by the Kubernetes guidelines) and then, internally, needs to convert its structs to libvirtxml structs and back. So suppose that the some VM attribute is represented as uint32 in the KubeVirt API and as uint in the libvirtxml API. Going from the former to the latter works fine, as uint is always going to be at least as large as uint32; however, when going in the other direction (query information from libvirt, unmarshal the XML using libvirtxml and then copy the value over to KubeVirt's public struct) has to be done with care. Silly example to demonstrate the potential issue: $ cat uint.go package main import ( "fmt" "math" ) func main() { var k uint32 = math.MaxUint32 var l uint = uint(k) fmt.Println("k = ", k) fmt.Println("l = ", l) l = math.MaxUint32 + 1 k = uint32(l) fmt.Println("l = ", l) fmt.Println("k = ", k) } $ go run uint.go k = 4294967295 l = 4294967295 l = 4294967296 k = 0 Yes, casting a uint to uint32 without performing a check to ensure that the value fall within the acceptable range beforehand would be a bug in KubeVirt. But if libvirtxml used precisely-sized integers, such casts would need to be employed much less frequently, and attempts to carelessly cast uint64 to uint32 would hopefully be easier to spot and catch during review. -- Andrea Bolognani / Red Hat / Virtualization
participants (3)
-
Alice Frosi -
Andrea Bolognani -
Daniel P. Berrangé