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?

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].

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(a)redhat.com&gt; > wrote: > > > 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]. > > 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.