Skip to content
This repository has been archived by the owner on Dec 7, 2023. It is now read-only.

CNI networking cleanup support, Docker client robustness improvements #111

Merged
merged 7 commits into from
Jul 22, 2019

Conversation

twelho
Copy link
Contributor

@twelho twelho commented Jul 9, 2019

This PR includes a bunch of fixes and improvements around container and cache handling:

  1. Remove the CNI networking of VMs post-run (52aa28d)
  2. Make the cache exclude newly generated Objects without UID (872dcb8)
  3. Improve the Docker client's parallel robusness by introducing fences (b58702c)
    -> you can now rm -f right after start -d without Ignite crashing

@luxas
Copy link
Contributor

luxas commented Jul 9, 2019

Thanks, will test in a moment

@luxas luxas changed the title v0.4.0 cleanup 3, CNI networking removal support CNI networking cleanup support Jul 9, 2019
@luxas luxas added this to the v0.5.0 milestone Jul 9, 2019
@luxas
Copy link
Contributor

luxas commented Jul 17, 2019

This PR currently depends on:

a) .status.containerID being added to the API #210
b) Patch support in the client #186

@luxas luxas closed this Jul 17, 2019
@luxas luxas reopened this Jul 17, 2019
@luxas
Copy link
Contributor

luxas commented Jul 17, 2019

Oops, didn't mean to close it.

@twelho twelho added the do-not-merge/wip The PR is still work in progress label Jul 17, 2019
@luxas
Copy link
Contributor

luxas commented Jul 19, 2019

This got unblocked with #225.
You can now read the containerID using the v1alpha1.ignite.weave.works.containerID annotation for all
VMs started with ignite that is built off master.

In other words, you can now run the cleanup func client-side in Ignite when removing a VM using CNI networking, as you can read the containerID from the annotation.

twelho added 4 commits July 22, 2019 14:33
# Conflicts:
#	api/ignite.md
#	pkg/metadata/vmmd/vm.go
#	pkg/operations/start.go
Also add a safety check for ensuring that cached Objects have their
UID set.
This is to ensure no race conditions occur during e.g. `rm -f`, where
a VM started with `-d` would still be in the process of being killed
when remove is issued, leading to a crash.
@twelho twelho changed the title CNI networking cleanup support CNI networking cleanup support, Docker client robustness improvements Jul 22, 2019
@twelho
Copy link
Contributor Author

twelho commented Jul 22, 2019

This now includes the CNI networking cleanup work in 52aa28d, making the cache exclude newly generated Objects without UID in 872dcb8 and improving the Docker client's parallel robusness by introducing fences in b58702c (so you can now rm -f right after start -d without crashing).

@twelho
Copy link
Contributor Author

twelho commented Jul 22, 2019

You can now read the containerID using the v1alpha1.ignite.weave.works.containerID annotation for all

I decided to go for implementing an InspectContainer method in the client, which returns the ID among other details. As this method is used to check for container existence in RemoveVMContainer, I just took the ID from its result to remove the CNI networking with. IMO this is a more robust solution, as we just need a single lookup this way (container removal also uses this ID) and we eliminate the chance of the user accidentally editing the ID in the annotations. Check 52aa28d for the details.

@twelho
Copy link
Contributor Author

twelho commented Jul 22, 2019

@luxas is out of office, so as this is ready, going ahead and merging.

@twelho twelho merged commit a5d0d2a into master Jul 22, 2019
@twelho twelho deleted the v0.4.0_cleanup_3 branch July 22, 2019 15:21
@luxas luxas removed the do-not-merge/wip The PR is still work in progress label Jul 22, 2019
@luxas luxas added the kind/enhancement Categorizes issue or PR as related to improving an existing feature. label Aug 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind/enhancement Categorizes issue or PR as related to improving an existing feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants