agent: Simplify tearing down the network #225

sboeuf · 2018-04-26T22:04:40Z

This PR addresses the issues related to the way the agent currently tears down the network.
First it fixes an issue due to a modification of a list containing pointers instead of copies, while at the same time, this list is being parsed.

Then, with the idea of simplifying the way the network is removed, it introduces a commit removing the network removal code that was called when the sandbox was destroyed. Indeed, such a case means the VM is going to be shut down, leading to the removal of the network by the VM anyway.

Fixes #226
Fixes #230

Signed-off-by: Sebastien Boeuf [email protected]

amshinde · 2018-04-26T23:00:11Z

network.go

 	for _, route := range s.network.routes {
-		if err := s.removeRoute(netHandle, route); err != nil {
+		routeList = append(routeList, *route)


@sboeuf Where else in the code are we updating this list?
Why not just store objects instead of pointers in the list? We can always modify the objects itself when a slice is passed to a function.

This involves a bigger change regarding how things are handled if we move from pointer to object. I am fine with any solution but I want some feedback before to go ahead and implement it.
@bergwolf @jodh-intel @egernst @grahamwhaley PTAL

This is because a slice is not really a good data structure for dynamical element removal, - you cannot avoid data copy if you want to remove within a loop.

OTOH, if we add an ID to pb.Route to uniquely identify a route for a sandbox, we can use a map for it and it is perfectly safe to do

for _, r := range routes { delete(routes, r.ID) }

The same applies to pb.Interface where we can use interface.Name to uniquely identify an interface and thus use a map instead of a slice to store it.

codecov · 2018-04-27T00:01:28Z

Codecov Report

Merging #225 into master will increase coverage by 0.32%.
The diff coverage is 50%.

@@            Coverage Diff            @@
##           master    #225      +/-   ##
=========================================
+ Coverage   40.88%   41.2%   +0.32%     
=========================================
  Files          12      12              
  Lines        1815    1803      -12     
=========================================
+ Hits          742     743       +1     
+ Misses        970     958      -12     
+ Partials      103     102       -1

Impacted Files	Coverage Δ
grpc.go	`20.64% <0%> (-0.05%)`	⬇️
network.go	`49.16% <66.66%> (+2.92%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 74e720e...4bcacdc. Read the comment docs.

sboeuf · 2018-04-27T22:58:54Z

@egernst @amshinde I have simplified the network removal, PTAL.

amshinde

lgtm

sboeuf · 2018-04-28T00:35:48Z

@bergwolf @grahamwhaley PTAL

bergwolf · 2018-04-28T01:58:28Z

network.go

@@ -25,10 +25,10 @@ import (
 // related information.
 type network struct {
 	ifacesLock sync.Mutex
-	ifaces     []*pb.Interface
+	ifaces     []pb.Interface


As I mentioned in https://github.com/kata-containers/agent/pull/225/files#r184835917, we can use pb.Interface.Name to uniquely identify an interface and thus use a map instead of a slice to store it. Then it is perfectly safe to call delete in a loop.

Either way is fine with me. Do you really want that we introduce maps here, or you're okay with this way ?

Please change to use a map for it. It has a few advantages:

suitable for delete within a loop

ensuring each interface is unique

Ok, it works in this case because the name is already what we use to identify what we need to remove.

bergwolf · 2018-04-28T02:02:37Z

grpc.go

@@ -892,10 +892,6 @@ func (a *agentGRPC) DestroySandbox(ctx context.Context, req *pb.DestroySandboxRe
 	}
 	a.sandbox.Unlock()

-	if err := a.sandbox.removeNetwork(); err != nil {


Based on the same logic, can we simplify DestroySandbox by removing removeMounts and teardownSharedPidNs as well? Assuming that we do not reuse a sandbox after destroying it, the only thing must being done here is necessary pre-condition check and syscall.Sync().

We need to discuss about this, but if it's done, it's gonna be done in a follow up PR. This PR is more about the network.

Yes, agreed.

bergwolf · 2018-04-28T03:49:51Z

grpc.go

@@ -892,10 +892,6 @@ func (a *agentGRPC) DestroySandbox(ctx context.Context, req *pb.DestroySandboxRe
 	}
 	a.sandbox.Unlock()

-	if err := a.sandbox.removeNetwork(); err != nil {


Yes, agreed.

bergwolf · 2018-04-28T03:51:29Z

network.go

@@ -25,10 +25,10 @@ import (
 // related information.
 type network struct {
 	ifacesLock sync.Mutex
-	ifaces     []*pb.Interface
+	ifaces     []pb.Interface


Please change to use a map for it. It has a few advantages:

suitable for delete within a loop

ensuring each interface is unique

bergwolf · 2018-04-28T03:52:21Z

network.go


 	routesLock sync.Mutex
-	routes     []*pb.Route
+	routes     []pb.Route


We should be able to use a map here as well, by introducing a name/id field for each route rule. Then we can delete the rule in a loop safely. But it's ok if you want to defer it to a later PR.

Yes, this is in the scope for another PR here, because we don't have such thing as route ID, and we actually check for the entire route fields to detect if this is the same route.

By storing the network information such as routes as pointers, we end up having issues if we try to remove some elements from the list as at the same time this list is parsed through by a loop. For interfaces, we can use maps instead, because we have a way to keep track of them based on their names. Maps are safer and allow to keep pointers in this case. In both cases, routes and interfaces list can be modified properly while we loop over every items. Fixes kata-containers#226 Signed-off-by: Sebastien Boeuf <[email protected]>

sboeuf · 2018-04-30T18:25:31Z

@bergwolf PR updated PTAL

sboeuf · 2018-04-30T18:28:20Z

@egernst PTAL

sboeuf · 2018-04-30T19:55:21Z

@egernst PR updated.

egernst · 2018-04-30T20:32:02Z

@sboeuf - I think for paranoia sake it would be wise to keep just the interface removal part (I don't see need for removing routes/dns).

egernst

Can we still remove the interfaces? In the event that the VM disconnects the network first (not necessarily the host side), I'd rather explicitly shut it down so we know when the packets stop sending out of the container...

sboeuf · 2018-04-30T20:35:08Z

@egernst I have updated the PR. I am no longer removing the code removing the interfaces.
Do you want me to remove the code removing DNS ?

egernst · 2018-04-30T21:43:04Z

@sboeuf - yes, please remove that as well. LGTM otherwise - I'm happy with merging one you make this update.

sboeuf · 2018-04-30T22:38:16Z

@egernst done !

The network routes will always be removed when the associated interfaces are going to be removed. Same thing for DNS. It is safer to leave this to each interface removal. Fixes kata-containers#230 Signed-off-by: Sebastien Boeuf <[email protected]>

sboeuf · 2018-05-01T05:44:36Z

@grahamwhaley PTAL

grahamwhaley

lgtm

sboeuf added the review label Apr 26, 2018

sboeuf force-pushed the fix_k8s_routes branch 2 times, most recently from 1a653c9 to de7a8c0 Compare April 26, 2018 22:14

amshinde reviewed Apr 26, 2018

View reviewed changes

sboeuf mentioned this pull request Apr 26, 2018

tests: Add initial openshift testing kata-containers/tests#221

Merged

sboeuf force-pushed the fix_k8s_routes branch from de7a8c0 to 721b5b9 Compare April 27, 2018 22:50

sboeuf changed the title ~~agent: network: Don't loop over a list being potentially modified~~ agent: Simplify tearing down the network Apr 27, 2018

sboeuf force-pushed the fix_k8s_routes branch from 721b5b9 to 2b30978 Compare April 27, 2018 22:58

amshinde approved these changes Apr 27, 2018

View reviewed changes

sboeuf mentioned this pull request Apr 28, 2018

virtcontainers: Properly remove the container when shim gets killed kata-containers/runtime#275

Merged

bergwolf reviewed Apr 28, 2018

View reviewed changes

sboeuf force-pushed the fix_k8s_routes branch from 2b30978 to 4ea1906 Compare April 30, 2018 18:25

sboeuf force-pushed the fix_k8s_routes branch from 4ea1906 to ba8af33 Compare April 30, 2018 19:54

egernst suggested changes Apr 30, 2018

View reviewed changes

sboeuf force-pushed the fix_k8s_routes branch from ba8af33 to 013072d Compare April 30, 2018 22:35

sboeuf force-pushed the fix_k8s_routes branch from 013072d to 254aa22 Compare May 1, 2018 04:01

sboeuf force-pushed the fix_k8s_routes branch from 254aa22 to 4bcacdc Compare May 1, 2018 04:18

egernst approved these changes May 1, 2018

View reviewed changes

grahamwhaley approved these changes May 1, 2018

View reviewed changes

grahamwhaley merged commit c1af46a into kata-containers:master May 1, 2018

grahamwhaley removed the review label May 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

agent: Simplify tearing down the network #225

agent: Simplify tearing down the network #225

sboeuf commented Apr 26, 2018 •

edited

Loading

amshinde Apr 26, 2018

sboeuf Apr 26, 2018

bergwolf Apr 28, 2018

codecov bot commented Apr 27, 2018 •

edited

Loading

sboeuf commented Apr 27, 2018

amshinde left a comment

sboeuf commented Apr 28, 2018

bergwolf Apr 28, 2018

sboeuf Apr 28, 2018

bergwolf Apr 28, 2018

sboeuf Apr 30, 2018

bergwolf Apr 28, 2018

sboeuf Apr 28, 2018

bergwolf Apr 28, 2018

bergwolf Apr 28, 2018

bergwolf Apr 28, 2018

bergwolf Apr 28, 2018

sboeuf Apr 30, 2018

sboeuf commented Apr 30, 2018

sboeuf commented Apr 30, 2018

sboeuf commented Apr 30, 2018

egernst commented Apr 30, 2018

egernst left a comment

sboeuf commented Apr 30, 2018

egernst commented Apr 30, 2018

sboeuf commented Apr 30, 2018

sboeuf commented May 1, 2018

grahamwhaley left a comment

agent: Simplify tearing down the network #225

agent: Simplify tearing down the network #225

Conversation

sboeuf commented Apr 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 27, 2018 • edited Loading

Codecov Report

sboeuf commented Apr 27, 2018

amshinde left a comment

Choose a reason for hiding this comment

sboeuf commented Apr 28, 2018

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

sboeuf commented Apr 30, 2018

sboeuf commented Apr 30, 2018

sboeuf commented Apr 30, 2018

egernst commented Apr 30, 2018

egernst left a comment

Choose a reason for hiding this comment

sboeuf commented Apr 30, 2018

egernst commented Apr 30, 2018

sboeuf commented Apr 30, 2018

sboeuf commented May 1, 2018

grahamwhaley left a comment

Choose a reason for hiding this comment

sboeuf commented Apr 26, 2018 •

edited

Loading

codecov bot commented Apr 27, 2018 •

edited

Loading