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.

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

Loopback plugin #121

Merged
merged 3 commits into from
Mar 2, 2016
Merged

Loopback plugin #121

merged 3 commits into from
Mar 2, 2016

Conversation

zachgersh
Copy link
Contributor

WRT #97

  • Introduces ginkgo / gomega style testing (you don't need to look at the diff for that, just dependencies).
  • Noop on delete (because you can just nuke a namespace with its lo
  • Depending on if you merge the better error handling PR first - I may need to tweak to remove the godeps bit
  • priv-net-run now doesn't up a loopback!

@zachgersh
Copy link
Contributor Author

So - this is red due to the fact that you need to be able to mkdir on /var/run/netns and you get permission denied :/

@zachgersh
Copy link
Contributor Author

I don't know how to make this go green on travis. Works fine inside of a docker image locally.

@steveej
Copy link
Member

steveej commented Feb 11, 2016

Thanks for this one! I think all you need to do is to ./test to sudo ./test in the travis.yml. Please see if that helps and I'll finish reviewing this.

@zachgersh
Copy link
Contributor Author

@steveej - thanks for the reminder that you could do that, been awhile since I have used travis!

Now, because we installed go without sudo you can't find it when sudo (hence the build fails again).

@zachgersh
Copy link
Contributor Author

@steveej - good to go now!

@steveej
Copy link
Member

steveej commented Feb 11, 2016

Thanks @zachgersh! The new dependencies look like a rather big addition, what exactly do we gain from these?

@zachgersh
Copy link
Contributor Author

Hey @steveej,

Good question!

Our team (CloudFoundry Container Networking) uses Ginkgo and Gomega to support BDD-style tests (e.g. our vxlan plugin and our runc / cni adapter). We like this style because test descriptions are in plain English, which helps us do test-driven development. The Ginkgo test runner is very mature (e.g. focusing, parallel testing, randomly permuting test order) and there are many Gomega matchers that simplify testing of common scenarios (e.g. MatchJSON).

Our other PR adds test coverage elsewhere, using the same approach.

For what it’s worth, Kubernetes uses Ginkgo and Gomega also.

Our team is actively using CNI and we expect to add more test coverage over time. Using Ginkgo and Gomega in CNI would make that work much easier for us.

@steveej
Copy link
Member

steveej commented Feb 12, 2016

Thank you @zachgersh for the explanation. I think we can leverage a full-featured testing framework in CNI. Personally I had not used anything else than Go's built-in testing package but ginkgo looks like a good choice.

Can I ask you to squash the commits before I do a full code review?

@zachgersh
Copy link
Contributor Author

@steveej look at for a squash shortly. I am going to squash everything except the addition of the ginkgo / gomega dependencies. Should be easier for you to do a code review that way.

@zachgersh
Copy link
Contributor Author

@steveej believe this should be good to go after the tests pass.

@zachgersh
Copy link
Contributor Author

@steveej anything else required here?

@steveej
Copy link
Member

steveej commented Feb 24, 2016

ping @zachgersh, this needs a rebase now ;)

Zachary Gershman and others added 3 commits February 29, 2016 12:27
- Believe we need sudo to create netns
- Use syscall instead of relying on ip netns
- Add sudo to .travis.yml
- Needs more -E
- Revert Godeps GoVersion to 1.4.2
- in travis, test command is run with all necessary env vars
- Loopback plugin only works on 'lo' interface
- Update README, add loopback plugin config
- note script dependency on jq

Signed-off-by: Gabe Rosenhouse <grosenhouse@pivotal.io>
- After creating new netns, switch back to main netns
- Lock thread during test and test setup
@zachgersh
Copy link
Contributor Author

@steveej - sorry for the delay, rebased. Could we try to get this merged today? I am available to help and could jump on IRC if needed.

@steveej
Copy link
Member

steveej commented Mar 2, 2016

I think this is good to go. I changed the "Fixes" in the PR comment. Strictly seen it doesn't fix the mentioned issue, which is about upping the loopback interface by default, while PR allows to do this with a designated plugin.

steveej added a commit that referenced this pull request Mar 2, 2016
@steveej steveej merged commit 68259e3 into containernetworking:master Mar 2, 2016
h0tw1r3 added a commit to h0tw1r3/nomad that referenced this pull request Jun 19, 2022
CNI changed how to bring up the interface in v0.2.0.
Support was moved to a new loopback plugin.

containernetworking/cni#121
h0tw1r3 added a commit to h0tw1r3/nomad that referenced this pull request Jun 19, 2022
CNI changed how to bring up the interface in v0.2.0.
Support was moved to a new loopback plugin.

containernetworking/cni#121

Fixes hashicorp#10014
tgross pushed a commit to hashicorp/nomad that referenced this pull request Jun 20, 2022
CNI changed how to bring up the interface in v0.2.0.
Support was moved to a new loopback plugin.

containernetworking/cni#121

Fixes #10014
tbehling pushed a commit to hashicorp/nomad that referenced this pull request Jun 29, 2022
CNI changed how to bring up the interface in v0.2.0.
Support was moved to a new loopback plugin.

containernetworking/cni#121

Fixes #10014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants