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.

Sign up for GitHub

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

Jump to bottom

hotunplug - reverse phase1 #9737

Merged
merged 8 commits into from Jun 16, 2023
Merged

hotunplug - reverse phase1 #9737

merged 8 commits into from Jun 16, 2023

Conversation

AlonaKaplan
Copy link
Member

@AlonaKaplan AlonaKaplan commented May 15, 2023 •
edited

What this PR does / why we need it:
The purpose of the PR is on hotunplug - to remove the interfaces created by phase1 (bridge, tap, dummy nic) and the caches (file and volatile).
Hotunplug is detected when the spec interfaces/networks doesn't contain a network that is stored in the configState cache.
In such case unplug phase1 will try to clean the bridge, tap, dummy nic and the caches in case they exist.

The PR doesn't include -
In case a network has caches but the preparation for some reason didn't start, hotunplug won't remove the cached since the indicator to the cleanup is that the configState is PodIfaceNetworkPreparationStarted or PodIfaceNetworkPreparationFinished.

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:

On hotunplug - remove bridge, tap and dummy interface from virt-launcher and the caches (file and volatile) from the node.

@kubevirt-bot
Copy link
Contributor

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

@kubevirt-bot kubevirt-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels May 15, 2023
@AlonaKaplan AlonaKaplan force-pushed the phase1_cleanup branch 3 times, most recently from c8a26a3 to 11e467a Compare May 16, 2023 10:02
@kubevirt-bot kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 21, 2023
@kubevirt-bot kubevirt-bot added size/XXL and removed size/XL needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 22, 2023
@AlonaKaplan AlonaKaplan force-pushed the phase1_cleanup branch 2 times, most recently from b567323 to 7029d2c Compare May 23, 2023 15:26
@kubevirt-bot kubevirt-bot added size/XXL kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API and removed size/XL labels May 23, 2023
@AlonaKaplan AlonaKaplan force-pushed the phase1_cleanup branch 2 times, most recently from 946e040 to ea7241e Compare May 24, 2023 10:40
@AlonaKaplan AlonaKaplan changed the title hotunplug - remove bridge, tap and dummy interface from virt-launcher hotunplug - reverse phase1 May 30, 2023
@kubevirt-bot kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 14, 2023
@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 15, 2023
@ormergi
Copy link
Contributor

ormergi commented Jun 15, 2023

/lgtm

@AlonaKaplan
Copy link
Member Author

/approve
The pr was lgtmed by three network team members.

@kubevirt-bot
Copy link
Contributor

[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:

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

@kubevirt-bot kubevirt-bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 15, 2023
@AlonaKaplan
hotunplug nic: invoke phase1 cleanup in case of interface hotunplug
faffa05 
This commit detects whether the ConfigStateCache contains pod interfaces
that no longer have corresponing network in the VMI spec.
If such networks are detected phase1 cleanup is invoked for each one of
them.

Future commits will implement the cleanup itself which will contain
removal of the bridge, tap device and caches created by phase1.

Signed-off-by: Alona Paz <alkaplan@redhat.com>
@AlonaKaplan
hotunplug, netstat: clean volatile cache
992383f 
Remove from the volatile cache entries of interfaces that have `absent`
state.

Note: the file cache is not cleaned here, a follow-up commit will clean it
during phase1 cleanup.

Signed-off-by: Alona Paz <alkaplan@redhat.com>
@AlonaKaplan
hotunplug, unpluggedPodNic: implement reverse phase1
feb9705 
The commit includes removal of bridge, tap and dummy nic if they exist.
Follow up commits will take care of the caches files deletion.

Notice:
1. The cleanup is general and not binding specific. Any link created
by whatecer binding should be removed (although we curretnly support only
bridge binding hotunplug).
2. Error removing one of the links doesn't block removal on the others.

Signed-off-by: Alona Paz <alkaplan@redhat.com>
@AlonaKaplan
hotunplug, cleanup caches: domain interface and DHCP cache
568df1f 
This commit takes care for cleaning the dmain interface and DHCP caches
during UnplugPhase1.

The PodInterface cache should be cleaned last.
Since it is used as the indicator whether the cleanup should be
done/not over yet.

Signed-off-by: Alona Paz <alkaplan@redhat.com>
@AlonaKaplan
configStateCache: Delete should clean the volatile cache only
f1fe544 
ConfigStateCache is just one of the frotends to the PodInterfaceCache file.
PodInterfaceCache contains not only the state data but other data as well.
Therefore, removal of a ConfigState entry shouldn't cause removal of the
whole PodInterfaceCache file.

Signed-off-by: Alona Paz <alkaplan@redhat.com>
@AlonaKaplan
hotunplug, virt-handler: don't unplug nics with ordinal name scheme
c249c1e 
Signed-off-by: Alona Paz <alkaplan@redhat.com>
@AlonaKaplan
virt-handler, podIfaceName discovery: support not found links
1c5cf68 
The VM can hotplugged interface that weren't yet plugged to the pod, it
is a valid scenario.

Signed-off-by: Alona Paz <alkaplan@redhat.com>
@AlonaKaplan
hotunplug: combine hotunplug flow with the plug
b574ebd 
This commit fixes several review comments.
1. combine hotunplug flow with the plug
2. remove form ConfigState the podIfaceName cache. On each hotunplug,
configState will enter the virt-launcher net namespace.
The expesive case is when unplugging an ordinal interface, then on each
reconcile of the virt-handler the virt-launcher's net ns will be entered.
But it is an edge case that doesn't worth such a complex solution.
In a follow-up we should block/remove Absent state from such interfaces.

Signed-off-by: Alona Paz <alkaplan@redhat.com>
@kubevirt-bot kubevirt-bot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 15, 2023
@AlonaKaplan
Copy link
Member Author

rebased
/lgtm

@kubevirt-bot
Copy link
Contributor

@AlonaKaplan: you cannot LGTM your own PR.

In response to this:

rebased
/lgtm

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 kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 15, 2023
@AlonaKaplan
Copy link
Member Author

/retest

@kubevirt-bot kubevirt-bot merged commit d250711 into kubevirt:main Jun 16, 2023
35 checks passed
@AlonaKaplan
Copy link
Member Author

/cherry-pick release-1.0

@kubevirt-bot
Copy link
Contributor

@AlonaKaplan: #9737 failed to apply on top of branch "release-1.0":

Applying: hotunplug nic: invoke phase1 cleanup in case of interface hotunplug
Applying: hotunplug, netstat: clean volatile cache
Applying: hotunplug, unpluggedPodNic: implement reverse phase1
Applying: hotunplug, cleanup caches: domain interface and DHCP cache
Applying: configStateCache: Delete should clean the volatile cache only
Applying: hotunplug, virt-handler: don't unplug nics with ordinal name scheme
Using index info to reconstruct a base tree...
M	pkg/network/setup/netconf_test.go
M	pkg/network/setup/network.go
M	pkg/virt-handler/vm.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/virt-handler/vm.go
Auto-merging pkg/network/setup/network.go
Auto-merging pkg/network/setup/netconf_test.go
CONFLICT (content): Merge conflict in pkg/network/setup/netconf_test.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0006 hotunplug, virt-handler: don't unplug nics with ordinal name scheme
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

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.

@AlonaKaplan
Copy link
Member Author

/cherry-pick release-1.0

@kubevirt-bot
Copy link
Contributor

@AlonaKaplan: new pull request created: #9946

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 kubevirt-bot mentioned this pull request Jun 20, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ormergi ormergi ormergi approved these changes

@orelmisan orelmisan orelmisan approved these changes

@prnaraya prnaraya prnaraya left review comments

@EdDev EdDev EdDev approved these changes

@xpivarc xpivarc Awaiting requested review from xpivarc

@stu-gott stu-gott Awaiting requested review from stu-gott

Assignees

@EdDev EdDev

@ormergi ormergi

@orelmisan orelmisan

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/network size/XXL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@AlonaKaplan @kubevirt-bot @ormergi @EdDev @orelmisan @prnaraya