Remove hotplug VMI API #9958

AlonaKaplan · 2023-06-21T10:23:33Z

What this PR does / why we need it:
This PR removes the rest-api and virctlcommands for hotplugging/unplugging an interfaces to/from a VMI.
Interface hotplugging/unplugging will be supported via VMs only.

It was noticed that allowing users to access the VMI (through the virt-api) is introducing too many corner cases (for example interface hotplugged to a VMI didn't get MAC from kubemacpool) and complexity in the system. Users should have access only to their user facing API, which is the VM object. Allowing the system to apply the desired spec in the VM to the backend VMI.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Release note:

Disable network interface hotplug/unplug for VMIs. It will be supported for VMs only.

kubevirt-bot · 2023-06-21T10:23:34Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

AlonaKaplan · 2023-06-21T12:04:25Z

/test pull-kubevirt-e2e-k8s-1.26-sig-network-multus-v4
/test pull-kubevirt-e2e-k8s-1.26-sig-network

AlonaKaplan · 2023-06-21T12:14:07Z

/test pull-kubevirt-build
/test pull-kubevirt-build-arm64
/test pull-kubevirt-generate
/test pull-kubevirt-manifests
/test pull-kubevirt-unit-test

EdDev

Very nice, thanks!

virtctl: remove hotplug/ugnplug from VMI

hotplug/ugnplug will be allowed from VM only

Typo

EdDev · 2023-06-21T13:38:21Z

pkg/virt-api/rest/interfacehotplug_test.go

-				app.VMIAddInterfaceRequestHandler(request, response)
-			}
-		}
-
 		DescribeTable("Should succeed a dynamic interface request", func(addOpts *v1.AddInterfaceOptions, mockScenario func(addOpts *v1.AddInterfaceOptions)) {


No need for a table now.

EdDev · 2023-06-21T13:47:56Z

pkg/virt-controller/watch/vm.go

 	vmTemplateCopy := vm.Spec.Template.Spec.DeepCopy()
-	for i, ifaceRequest := range vm.Status.InterfaceRequests {
+	for i, _ := range vm.Status.InterfaceRequests {


The _ is redundant.

EdDev · 2023-06-21T13:48:26Z

pkg/virt-controller/watch/vm.go

-				}
-			}
-		}
+		vmiSpecCopy := vmi.Spec.DeepCopy()


Why a copy is used?

Since c.VMIInterfacesPatchcompares between the changed copy and the original VMI to know if there is a need to patch the VMI.

EdDev · 2023-06-21T13:53:38Z

pkg/virt-controller/watch/vm.go

 		}
 	}

 	vm.Spec.Template.Spec = *vmTemplateCopy
 	return nil
 }

+func (c *VMController) VMIInterfacesPatch(newVmiSpec *virtv1.VirtualMachineInstanceSpec, vmi *virtv1.VirtualMachineInstance) error {


I would suggest to split the patch preparation and the patching itself (the later can be done at the caller).
The patch preparation can be a simple function and not a method.

I prefer to keep it as is since it is aligned with other patches the controller has. e.g VMICPUsPatch.
Changed the method no to be exported.

How is this related to how others used it? I fail to understand.

I prefer you move the network patching to the network.go file, adding here stuff is bad. We should minimize it as much as possible, even if others do not care.

I will follow up on this and send a PR to fix it, it is not worth blocking on it.

EdDev · 2023-06-21T13:55:03Z

pkg/virt-controller/watch/vm.go

-					return err
-				}
-			}
+		if err := c.VMIInterfacesPatch(vmiSpecCopy, vmi); err != nil {


I think there is a need to update the VMI content (use the response of the patch operation).
Mutating the VMI in the middle of the flow, requires it to be updated so following usages have an up to date VMI object.

I think the VMI shouldn't be changed, it represents the original VMI. Other hot updates doesn't change it as well (volume/ cpu).

Well, once you patch it, it changed.
If the object is not updated, decisions taken after are incorrect, e.g. status updates.

Even in this patching you assume the object was not updated by anyone else with relations to interfaces/networks.

If others have not updated it when mutating it, we do not have to repeat the same bug.
But you can reason why we need to leave the controller with an non updated object.

It is ok that decisions will be made based on the old VMI object. The next reconcile it will be fixed.
This VMI object represents the existing VMI, that's how the rest of the code in the file treats it, I prefer to be consistent.

Hotplug unplug will be supported from VM only. Signed-off-by: Alona Paz <alkaplan@redhat.com>

EdDev · 2023-06-21T14:31:44Z

Very nice, thanks!

virtctl: remove hotplug/ugnplug from VMI
hotplug/ugnplug will be allowed from VM only

Typo

This has not been handled.

hotplug/unplug will be allowed from VM only Signed-off-by: Alona Paz <alkaplan@redhat.com>

hotplug/unplug will be allowed from VM only. Signed-off-by: Alona Paz <alkaplan@redhat.com>

It will be supported from VMs only. Signed-off-by: Alona Paz <alkaplan@redhat.com>

Signed-off-by: Alona Paz <alkaplan@redhat.com>

AlonaKaplan · 2023-06-21T15:13:03Z

/retest

AlonaKaplan · 2023-06-21T15:36:37Z

/cc @maiqueb

maiqueb · 2023-06-21T15:43:11Z

I don't think we can claim that the VM is only user facing API for a workload on KubeVirt.

Users should have access only to their user facing API, which is the VM object.

I think this is not correct.

Are we sure we don't have upstream users using/relying on this ? Wouldn't it make sense to send an email to the mailing list giving a heads-up of your intentions ?

@chinglinwen does this affect you in any way ?

maiqueb · 2023-06-21T15:48:05Z

Also, please be more specific.

allowing users to access the VMI (through the virt-api) is introducing too many corner cases

doesn't help understand the reasons behind breaking the API.

A list of corner cases would.

EdDev · 2023-06-21T16:51:16Z

I don't think we can claim that the VM is only user facing API for a workload on KubeVirt.

I think we can.

Users should have access only to their user facing API, which is the VM object.

I think this is not correct.

Are we sure we don't have upstream users using/relying on this ? Wouldn't it make sense to send an email to the mailing list giving a heads-up of your intentions ?

@chinglinwen does this affect you in any way ?

This API has not yet been released under a formal version.
We can and we should do this before the release is done to avoid deprecation notices.

In addition, the hotplug and unplug API/s are in tech-preview and this is how I suggest to treat it.
Equivalent to Kuberenetes alpha version if this helps give it a context.

The lesson learned from the period this API was in, is proof enough that we should fix the mistake we did sooner than later.

EdDev

Thank you!

chinglinwen · 2023-06-21T16:56:17Z

Users should have access only to their user facing API, which is the VM object.

I have an example that we depends on VM's pods k6t bridge device( to flood packet to do the packet mirror), it's internal, observable behavior will be depends on somehow ;).

@chinglinwen does this affect you in any way ?

@maiqueb for this particular PR, we don't using hot plug yet, we may using it later, so it currently doesn't affect us
I notice this change remove temporary hot plug feature, I'm not sure we will need it or not ( and I've work on something else currently, so I've lost some detail in my minds ).

EdDev · 2023-06-21T17:02:48Z

I have an example that we depends on VM's pods k6t bridge device( to flood packet to do the packet mirror), it's internal, observable behavior will be depends on somehow ;).

The existing code of hotplug has already broken the assumptions taken on the internal pod networking.
The names of the interfaces used in the pod have changed from ordinal to hashed.
But I guess this is the risk of any downstream that tweaks and depends on internals.

AlonaKaplan · 2023-06-21T17:09:19Z

@maiqueb for this particular PR, we don't using hot plug yet, we may using it later, so it currently doesn't affect us I notice this change remove temporary hot plug feature, I'm not sure we will need it or not ( and I've work on something else currently, so I've lost some detail in my minds ).

The PR is not removing the hotplug feature. It is still available via VMs. It just removes the option to hotplug directly to a VMI.

chinglinwen · 2023-06-21T17:10:43Z

I have an example that we depends on VM's pods k6t bridge device( to flood packet to do the packet mirror), it's internal, observable behavior will be depends on somehow ;).

The existing code of hotplug has already broken the assumptions taken on the internal pod networking.
The names of the interfaces used in the pod have changed from ordinal to hashed.
But I guess this is the risk of any downstream that tweaks and depends on internals.

Got it, thanks for the note.

AlonaKaplan · 2023-06-21T18:16:24Z

/retest

AlonaKaplan · 2023-06-22T07:05:44Z

/retest

AlonaKaplan · 2023-06-22T07:28:45Z

/approve

kubevirt-bot · 2023-06-22T07:29:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AlonaKaplan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [AlonaKaplan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

AlonaKaplan · 2023-06-22T07:30:42Z

Raising Eddy's lgtm to approve.

AlonaKaplan · 2023-06-22T07:57:57Z

/cherry-pick release-1.0

kubevirt-bot · 2023-06-22T07:57:59Z

@AlonaKaplan: once the present PR merges, I will cherry-pick it on top of release-1.0 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.0

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

kubevirt-bot · 2023-06-22T09:19:08Z

@AlonaKaplan: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubevirt-e2e-k8s-1.25-sig-compute	`b88c64d`	link	unknown	`/test pull-kubevirt-e2e-k8s-1.25-sig-compute`

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

kubevirt-bot · 2023-06-22T12:13:12Z

@AlonaKaplan: new pull request created: #9965

In response to this:

/cherry-pick release-1.0

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

rmohr · 2023-06-23T08:21:38Z

I don't think we can claim that the VM is only user facing API for a workload on KubeVirt.

Users should have access only to their user facing API, which is the VM object.

I think this is not correct.

I can only second @maiqueb. VMI, VM, VMIRS, VM Pools are all user-facing APIs. That the VMI is not user-facing is not correct. I would kindly ask to bring this to the mailing list and ask for broader approver commitment. Maybe they agree to start considering the VMI as non-userfacing in the future, but as it is right now that is not a correct assumption.

If this functionality is tech-preview it may be ok to keep it removed, but the decision that VMI is not user-facing can not be made here. There is in principle also no rule which says that every REST endpoint on a specific object (e.g. VM) needs to exist also on a VMI, this can be fine. Just the statement about VMI being not user-facing is critical to discuss.

kubevirt-bot requested review from lyarwood and qinqon June 21, 2023 10:23

AlonaKaplan force-pushed the removeHotplugVmiApi branch from 76a0ba5 to 2f17bca Compare June 21, 2023 10:29

kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jun 21, 2023

AlonaKaplan force-pushed the removeHotplugVmiApi branch from 2f17bca to 8ef0374 Compare June 21, 2023 12:02

EdDev approved these changes Jun 21, 2023

View reviewed changes

kubevirt-bot assigned EdDev Jun 21, 2023

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 21, 2023

virt-api: remove VMI hotplug/unplug

e1fdbb3

Hotplug unplug will be supported from VM only. Signed-off-by: Alona Paz <alkaplan@redhat.com>

AlonaKaplan force-pushed the removeHotplugVmiApi branch from 8ef0374 to 7e8a7dd Compare June 21, 2023 14:22

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 21, 2023

AlonaKaplan marked this pull request as ready for review June 21, 2023 14:23

kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 21, 2023

kubevirt-bot requested review from EdDev and mhenriks June 21, 2023 14:23

AlonaKaplan added 4 commits June 21, 2023 17:51

virtctl: remove hotplug/unplug from VMI

b132172

hotplug/unplug will be allowed from VM only Signed-off-by: Alona Paz <alkaplan@redhat.com>

api: remove hotplug/unplug from VMI

184b587

hotplug/unplug will be allowed from VM only. Signed-off-by: Alona Paz <alkaplan@redhat.com>

e2e: Remove Hotplug/unplug support from VMI

0b0996f

It will be supported from VMs only. Signed-off-by: Alona Paz <alkaplan@redhat.com>

hotplug, e2e: cleanup leftovers

b88c64d

Signed-off-by: Alona Paz <alkaplan@redhat.com>

AlonaKaplan force-pushed the removeHotplugVmiApi branch from 7e8a7dd to b88c64d Compare June 21, 2023 14:51

kubevirt-bot requested a review from maiqueb June 21, 2023 15:36

EdDev approved these changes Jun 21, 2023

View reviewed changes

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 21, 2023

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 22, 2023

kubevirt-bot merged commit f73d6ce into kubevirt:main Jun 22, 2023
35 of 36 checks passed

kubevirt-bot mentioned this pull request Jun 22, 2023

[release-1.0] Remove hotplug VMI API #9965

Merged

Remove hotplug VMI API #9958

Remove hotplug VMI API #9958

Conversation

AlonaKaplan commented Jun 21, 2023 • edited

kubevirt-bot commented Jun 21, 2023

AlonaKaplan commented Jun 21, 2023

AlonaKaplan commented Jun 21, 2023

EdDev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EdDev commented Jun 21, 2023

AlonaKaplan commented Jun 21, 2023

AlonaKaplan commented Jun 21, 2023

maiqueb commented Jun 21, 2023 • edited

maiqueb commented Jun 21, 2023

EdDev commented Jun 21, 2023

EdDev left a comment

Choose a reason for hiding this comment

chinglinwen commented Jun 21, 2023

EdDev commented Jun 21, 2023

AlonaKaplan commented Jun 21, 2023

chinglinwen commented Jun 21, 2023

AlonaKaplan commented Jun 21, 2023

AlonaKaplan commented Jun 22, 2023

AlonaKaplan commented Jun 22, 2023

kubevirt-bot commented Jun 22, 2023

AlonaKaplan commented Jun 22, 2023

AlonaKaplan commented Jun 22, 2023

kubevirt-bot commented Jun 22, 2023

kubevirt-bot commented Jun 22, 2023

kubevirt-bot commented Jun 22, 2023

rmohr commented Jun 23, 2023 • edited

AlonaKaplan commented Jun 21, 2023 •

edited

maiqueb commented Jun 21, 2023 •

edited

rmohr commented Jun 23, 2023 •

edited