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

Add macvlan support for declarative containers #20935

Merged
merged 1 commit into from Dec 12, 2016

Conversation

montag451
Copy link
Contributor

Motivation for this change
Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@mention-bot
Copy link

@montag451, thanks for your PR! By analyzing the history of the files in this pull request, we identified @kampfschlaefer, @edolstra and @wlhlm to be potential reviewers.

@montag451 montag451 changed the title Add macvlan support for declarative container Add macvlan support for declarative containers Dec 6, 2016
Copy link
Contributor

@kampfschlaefer kampfschlaefer left a comment

Choose a reason for hiding this comment

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

Any chance you could add a test to prove that this works as intended and doesn't break anytime soon?

@montag451
Copy link
Contributor Author

I use nixos-container run for a test but it's not reliable as long as #21044 is not solved

@kampfschlaefer
Copy link
Contributor

I like that!

One additional suggestion: is it also possible (and) testable to ping the containers from machine1? Or is that not how macvlans works?

@montag451
Copy link
Contributor Author

montag451 commented Dec 11, 2016

@kampfschlaefer Yes I could ping from the host but I would have to create another macvlan (on the host) on the same network than eth1. So it would complicate the test because I would end up with two interfaces on the same network. Do you know if it's possible to prevent the test framework to automatically assign an IP to eth1? By the way, thank for you review, it's really valuable for newcomer like me!

@montag451
Copy link
Contributor Author

montag451 commented Dec 11, 2016

@kampfschlaefer Finally I found a way to ping containers from host (see 6cedee9e21fe8b00cbfc910c451a1f00a34e09ca)

@kampfschlaefer
Copy link
Contributor

@montag451 I think the contribution guidelines want these commits squashed, maybe the two test commits can be together?

Otherwise 👍

(sadly I am not yet a member and can not merge this for you)

@montag451
Copy link
Contributor Author

@kampfschlaefer I squashed all the commits into one. Thanks again for your help 👍

@Mic92 Mic92 merged commit 4889c27 into NixOS:master Dec 12, 2016
@Mic92
Copy link
Member

Mic92 commented Dec 12, 2016

Thanks!

@montag451 montag451 deleted the container-macvlan branch December 12, 2016 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants