[Warn] Api change/removal due to k8s update

[Federico Fossemo]

Hello all,

Currently, in our API we are using k8s ResourceRequirements[1] as a type for
the SupportContainerResources.Resources[2] and InterfaceBindingPlugin.ComputeResourceOverhead[3] struct.

During the bump[4] of k8s dependency from 1.30 to 1.31, we noticed that this is causing a change in our API, because of the

addition of the `Claims` field to the `ResourceClaim` struct (used for DRA devices).

Actually, both SupportContainerResources.Resources and InterfaceBindingPlugin.ComputeResourceOverhead does not use

the `Claims` field of ResourceRequirements, but only the `Limits` and `Requests` ones.
Even if it could be used to consume DRA devices, it will have to be after DRA devices are supported in KubeVirt.
So, in order to increase the stability of our API, we decided to drop the direct dependency to the k8s ResourceRequirements,

by creating our `ResourceRequirementsWithoutClaims` struct that, as the name suggests, is a copy of the original struct

cleaned up of the `Claims` field.

Since the previous releases use the k8s ResourceRequirements this can cause a breaking change, even if the `Claims` field

is never consumed by KubeVirt in SupportContainerResources.Resources[2] and InterfaceBindingPlugin.ComputeResourceOverhead[3] struct.

Are there any concerns around this?

[1] https://github.com/kubernetes/kubernetes/blob/c75e83302660750dc61f1b4a967ca52fc767df3b/staging/src/k8s.io/api/core/v1/types.go#L2631

[2] https://github.com/kubevirt/kubevirt/blob/0a74910ead0d4b0a366cfcf0bc74c636881ebd81/staging/src/kubevirt.io/api/core/v1/types.go#L2591

[3] https://github.com/kubevirt/kubevirt/blob/0a74910ead0d4b0a366cfcf0bc74c636881ebd81/staging/src/kubevirt.io/api/core/v1/types.go#L2861

[4] https://github.com/kubevirt/kubevirt/pull/12729

--

Federico

[Varun Ramachandra Sekar]

Hi Federico,

Any reason you're not using the existing `ResourceRequirements` struct defined in kubevirt corev1? (https://github.com/kubevirt/kubevirt/blob/main/staging/src/kubevirt.io/api/core/v1/schema.go#L286)

If you use that you wouldn't need to define yet another struct

--

Varun

[Lee Yarwood]

On Tue, 1 Oct 2024 at 04:16, Federico Fossemo <ffos...@redhat.com> wrote:

Since the previous releases use the k8s ResourceRequirements this can cause a breaking change, even if the `Claims` field

is never consumed by KubeVirt in SupportContainerResources.Resources[2] and InterfaceBindingPlugin.ComputeResourceOverhead[3] struct.

Just to focus in on this point, how are we considering this a breaking change if it has never been implemented by KubeVirt? 

FWIW I agree with redefining the struct in our own API and removing the dependency on k8s I just don't understand how the removal of something that never worked could be considered a breaking change.

Cheers,

Lee

[Federico Fossemo]

Hey Lee,

The field is never used in any part of the KubeVirt project. 
BUT nothing was blocking users from filling that field, which is definitely a no-op.
The breaking change comes when during the upgrade, if for unknown reasons users have filled that field,

the KubeVirt CR will be rejected by the presence of the unknown field.

Thanks,

- Federico

[Lee Yarwood]

On Wed, 2 Oct 2024 at 17:47, Federico Fossemo <ffos...@redhat.com> wrote:

Hey Lee,

The field is never used in any part of the KubeVirt project. 
BUT nothing was blocking users from filling that field, which is definitely a no-op.
The breaking change comes when during the upgrade, if for unknown reasons users have filled that field,

the KubeVirt CR will be rejected by the presence of the unknown field.

Ah but rejected by what? The API should prune unknown fields by default [1] (see spec.template.spec.lyarwood):

$ oc apply -f - <<EOF
apiVersion: kubevirt.io/v1
kind: VirtualMachine
metadata:
  creationTimestamp: null
  name: lyarwood-test
spec:
  runStrategy: Always
  template:
    metadata:
      creationTimestamp: null
    spec:
      lyarwood:
        test: true
      domain:
        devices: {}
        memory:
          guest: 512Mi
        resources: {}
      terminationGracePeriodSeconds: 180
status: {}
EOF
virtualmachine.kubevirt.io/lyarwood-test created

$ oc get vms/lyarwood-test -o json | jq .spec.template.spec
{
  "architecture": "amd64",
  "domain": {
    "devices": {},
    "machine": {
      "type": "pc-q35-rhel9.4.0"
    },
    "memory": {
      "guest": "512Mi"
    },
    "resources": {}
  },
  "terminationGracePeriodSeconds": 180
}

In our use case I believe this behaviour will also lead to the stored KubeVirt CR dropping the unknown field when it is next updated [2] but I've not tested this.

Cheers,

Lee

[1] https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#field-pruning

[2] https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definition-versioning/#writing-reading-and-updating-versioned-customresourcedefinition-objects

[Federico Fossemo]

Actually, it's kind of weird that it is working for you.
We perform a schema validation[1] which checks the spec and rejects the request in our validating webhook.
The validate schema is used for almost all the resources (maybe without "almost").
I tried with your VM spec and I got this error:
fossedihelm@fedora ~/w/kubevirt (update-k8s-deps-to-1.31)> k create -f personal_utility/leevm.yaml
Error from server (BadRequest): error when creating "personal_utility/leevm.yaml": VirtualMachine in version "v1" cannot be handled as a VirtualMachine: strict decoding error: unknown field "spec.template.spec.lyarwood"

Same when I try to create a kv resource with an unknown field:
fossedihelm@fedora ~/w/kubevirt (update-k8s-deps-to-1.31) [1]> k create -f personal_utility/leevm.yaml
Error from server (BadRequest): error when creating "personal_utility/leevm.yaml": KubeVirt in version "v1" cannot be handled as a KubeVirt: strict decoding error: unknown field "spec.test"

Cheers,

- Federico

[1] https://github.com/kubevirt/kubevirt/blob/f99c707fc41b2ae30bc69c2959fd9e9ff1c2f8f9/pkg/util/openapi/openapi.go#L316

[Federico Fossemo]

Hey Varun,

Thank you for chiming in.

The main reason I did not use the v1.ResourceRequirements is that it contains the `OvercommitGuestOverhead` field, which is a boolean, and hence always comes with a value.

Also, the `OvercommitGuestOverhead` does not make much sense in the context of SupportContainerResources or 

ComputeResourceOverhead.

Looks like it is very specific for the guest resources.

Do you agree?

- Federico

--
You received this message because you are subscribed to the Google Groups "kubevirt-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to kubevirt-dev...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/kubevirt-dev/0c7a509f-d8c9-4e7b-80bc-bb3d27f86e73n%40googlegroups.com.

[Lee Yarwood]

On Wed, 2 Oct 2024 at 20:32, Federico Fossemo <ffos...@redhat.com> wrote:

Actually, it's kind of weird that it is working for you.
We perform a schema validation[1] which checks the spec and rejects the request in our validating webhook.
The validate schema is used for almost all the resources (maybe without "almost").
I tried with your VM spec and I got this error:
fossedihelm@fedora ~/w/kubevirt (update-k8s-deps-to-1.31)> k create -f personal_utility/leevm.yaml
Error from server (BadRequest): error when creating "personal_utility/leevm.yaml": VirtualMachine in version "v1" cannot be handled as a VirtualMachine: strict decoding error: unknown field "spec.template.spec.lyarwood"

Same when I try to create a kv resource with an unknown field:
fossedihelm@fedora ~/w/kubevirt (update-k8s-deps-to-1.31) [1]> k create -f personal_utility/leevm.yaml
Error from server (BadRequest): error when creating "personal_utility/leevm.yaml": KubeVirt in version "v1" cannot be handled as a KubeVirt: strict decoding error: unknown field "spec.test"

Cheers,

- Federico

[1] https://github.com/kubevirt/kubevirt/blob/f99c707fc41b2ae30bc69c2959fd9e9ff1c2f8f9/pkg/util/openapi/openapi.go#L316

The error you're seeing here is from k8s-apiserver server side validation of the resource that kubectl now asks for by default:

https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/2885-server-side-unknown-field-validation

I didn't see this as I was accidentally using a super old version of oc that doesn't request this server side validation by default:

$ oc version

Client Version: 4.11.18
Kustomize Version: v4.5.4
Server Version: 4.16.0-0.nightly-2024-10-03-033123
Kubernetes Version: v1.29.8+632b078

$ ./cluster-up/kubectl.sh apply -f lyarwood.yaml
selecting podman as container runtime
Error from server (BadRequest): error when creating "lyarwood.yaml": VirtualMachine in version "v1" cannot be handled as a VirtualMachine: strict decoding error: unknown field "spec.template.lyarwood", unknown field "spec.template.spec.lyarwood"
$ ./cluster-up/kubectl.sh apply --validate=false -f lyarwood.yaml
selecting podman as container runtime
virtualmachine.kubevirt.io/lyarwood created

This passes our internal schema validation check as the extra fields have been pruned. That really makes me question why we even bother with this extra check in our webhooks tbh.

So returning to the issue at hand, this will only be a breaking change where users are using a client requesting server side validation. For everyone else the unknown field will be pruned from the request and it should pass our internal schema checks in the webhooks.

Cheers,

Lee

 

[Federico Fossemo]

Thank you Lee!

We can start a discussion around this.

So returning to the issue at hand, this will only be a breaking change where users are using a client requesting server side validation. For everyone else the unknown field will be pruned from the request and it should pass our internal schema checks in the webhooks.

Better this way! :) 

Cheers,

Lee

 

[Lee Yarwood]

I've created https://github.com/kubevirt/kubevirt/issues/12998 to track this thanks!