Skip to content
This repository has been archived by the owner on May 12, 2021. It is now read-only.

Network interface removal and decoupling #1181

Merged
merged 2 commits into from
Jan 31, 2019
Merged

Network interface removal and decoupling #1181

merged 2 commits into from
Jan 31, 2019

Conversation

sameo
Copy link

@sameo sameo commented Jan 25, 2019

This PR does 2 things:

@sameo sameo self-assigned this Jan 25, 2019
@sameo sameo requested review from bergwolf and sboeuf January 25, 2019 13:54
@sameo sameo force-pushed the topic/network-interface branch from 28dffb0 to 0725738 Compare January 25, 2019 14:17
Samuel Ortiz added 2 commits January 25, 2019 15:25
virtcontainers: Remove the network interface
b39cb1d 
There's only one real implementer of the network interface and no real
need to implement anything else. We can just go ahead and remove this
abstraction.

Fixes: kata-containers#1179

Signed-off-by: Samuel Ortiz <[email protected]>
virtcontainers: Decouple the network API from the sandbox one
18dcd2c 
In order to fix kata-containers#1059, we want to create a hypervisor package. Some of
the hypervisor implementations (qemu) depend on the network and endpoint
interfaces. We can not have a virtcontainers -> hypervisor -> network,
endpoint -> virtcontainers cyclic dependency.
So before creating the hypervisor package, we need to decouple the
network API from the virtcontainers one.

Fixes: kata-containers#1180

Signed-off-by: Samuel Ortiz <[email protected]>
@sameo sameo force-pushed the topic/network-interface branch from 0725738 to 18dcd2c Compare January 25, 2019 14:28
@sameo
Copy link
Author

sameo commented Jan 25, 2019

/test

sboeuf
sboeuf reviewed Jan 25, 2019
View reviewed changes
Copy link

@sboeuf sboeuf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sameo I'm not against the removal of the Go interface since, as you mentioned it, we're only using one single implementation. But I'm concerned about the unit tests, because the noop implementation is pretty convenient to test a bunch of code that would be complex to test if we had to mimic the real network behavior. WDYT?

@sameo
Copy link
Author

sameo commented Jan 25, 2019

@sameo I'm not against the removal of the Go interface since, as you mentioned it, we're only using one single implementation. But I'm concerned about the unit tests, because the noop implementation is pretty convenient to test a bunch of code that would be complex to test if we had to mimic the real network behavior. WDYT?

FWIW all the unit tests are passing with the default network implementation. When first thinking about doing this I thought they will all fail but they don't. I get CI errors here but there not UT related.

@sameo
Copy link
Author

sameo commented Jan 25, 2019

@sboeuf @mcastelino b39cb1d#diff-bca2c2b5b04e0e74533b492fd23a3cc8R784 is what makes several unit tests pass with this PR.

mcastelino
mcastelino reviewed Jan 25, 2019
View reviewed changes
Copy link
Contributor

@mcastelino mcastelino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sameo just a couple of questions. LGTM otherwise.

virtcontainers/network.go
type Network struct {
}

func (n *Network) trace(ctx context.Context, name string) (opentracing.Span, context.Context) {
Copy link
Contributor

@mcastelino mcastelino Jan 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sameo This is code moved mostly unmodified from virtcontainers/default_network.go?

Were there any changes needed?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No functional changes, no.

virtcontainers/sandbox.go
@@ -206,7 +205,7 @@ type Sandbox struct {
hypervisor hypervisor
agent agent
storage resourceStorage
network network
network Network
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sameo why did this need to be made public?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm preparing for moving the network code into its own package.

@amshinde
Copy link
Member

@sameo Change looks good overall, its just the noop implementation has been useful in mocking our network behaviour in unit tests.

@sameo
Copy link
Author

sameo commented Jan 27, 2019

@sameo Change looks good overall, its just the noop implementation has been useful in mocking our network behaviour in unit tests.

noop is useful for sure, but:

  • It does not exercise much of the code path
  • We need to maintain an artificial abstraction for it
  • All unit tests pass without it

sboeuf
sboeuf approved these changes Jan 28, 2019
View reviewed changes
Copy link

@sboeuf sboeuf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough :)
LGTM

@amshinde
Copy link
Member

amshinde commented Jan 28, 2019 •
edited
Loading

@sameo If thats the case
lgtm

Approved with PullApprove

@sameo
Copy link
Author

sameo commented Jan 28, 2019

/retest

@bergwolf
Copy link
Member

bergwolf commented Jan 29, 2019 •
edited by amshinde
Loading

LGTM! It makes sense to simplify the code and CNM is not really CNM specific anyway.

Approved with PullApprove Approved with PullApprove

@bergwolf
Copy link
Member

/retest

@sboeuf
Copy link

sboeuf commented Jan 30, 2019

@chavafg which of those CI results can be ignored?

@GabyCT
Copy link
Contributor

GabyCT commented Jan 30, 2019

@sboeuf here it is the page where it says the CI known failures http://jenkins.katacontainers.io/view/CI%20Status/, what I see that is missing is the metrics job. @grahamwhaley could you please add that with the proper link to the issue. Thanks

@sboeuf
Copy link

sboeuf commented Jan 30, 2019

thanks @GabyCT
I've just restarted the fedora and metrics builds, let see how this goes.

@egernst egernst merged commit 29dae85 into kata-containers:master Jan 31, 2019
@egernst egernst mentioned this pull request Feb 26, 2019
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mcastelino mcastelino mcastelino left review comments

@sboeuf sboeuf sboeuf approved these changes

@amshinde amshinde amshinde approved these changes

@bergwolf bergwolf Awaiting requested review from bergwolf

Assignees

@sameo sameo

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@sameo @amshinde @bergwolf @sboeuf @GabyCT @mcastelino @egernst