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

Fix internal macvlan network to work in swarm #2419

Merged
merged 2 commits into from
Feb 21, 2020
Merged

Conversation

lemrouch
Copy link
Contributor

Using dummy interface allows communication beween containers only if
they are running on the same node in swam.

Signed-off-by: Pavel Matěja pavel@verotel.cz

@GordonTheTurtle
Copy link

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "2418-fix" git@github.com:lemrouch/libnetwork.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842357810328
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

@selansen
Copy link
Collaborator

can you sign the commit with above instruction?

@lemrouch
Copy link
Contributor Author

lemrouch commented Jul 21, 2019

can you sign the commit with above instruction?

Hi, my impression was dco-signed — All commits are signed and All checks have passed means it's already done.
BTW: those instructions dont work.

@selansen
Copy link
Collaborator

you need to amend commit with -s option. I think that is what expected from the above message. if you have already done it, not sure why we received 'sing your commit' messages.

@lemrouch
Copy link
Contributor Author

you need to amend commit with -s option. I think that is what expected from the above message. if you have already done it, not sure why we received 'sing your commit' messages.

There was unsigned merge fixed by:
"lemrouch force-pushed the lemrouch:2418-fix branch from c7203f1 to da39847 5 days ago"
It should be all OK now.

@lemrouch
Copy link
Contributor Author

lemrouch commented Aug 8, 2019

What can I do to get this commit merged?

tle211212 pushed a commit to tle211212/libnetwork that referenced this pull request Jan 14, 2020
@arkodg
Copy link
Contributor

arkodg commented Feb 14, 2020

@lemrouch if the intent of this PR is to separate the notion of empty parent to create a dummy interface vs making sure internal networks can communicate in swarm, there might be more plumbing involved
https://github.com/docker/libnetwork/blob/da39847dba735007c79e104d53d4133616d5b00b/drivers/macvlan/macvlan_network.go#L65
and
https://github.com/docker/libnetwork/blob/da39847dba735007c79e104d53d4133616d5b00b/drivers/macvlan/macvlan_network.go#L93

@lemrouch
Copy link
Contributor Author

@arkodg intent is described in #2418 therefore I didn't really care about empty parent in other code paths as I have non-empty one filled by config-only network and all my MACVLAN networks are internal.
What do you suggest in other cases?

@arkodg
Copy link
Contributor

arkodg commented Feb 17, 2020

I see so both the above code flows are redundant for your case ( config-from networks ), have you tested these changes out ? Can you please share a working example

@lemrouch
Copy link
Contributor Author

Yes, I have tested them in combination with #2407
but I don't have access to the system right now. I hope I can provide example in a week or so.

@arkodg
Copy link
Contributor

arkodg commented Feb 18, 2020

Thanks, please share a snippet of the working commands when possible

@lemrouch
Copy link
Contributor Author

Example is in #2418
It assumes fix for default gateway as well.

@lemrouch
Copy link
Contributor Author

About lines 65 and 93 mentioned above:
I think someone can try the same thing with internal MACVLAN IPv6 only network.
As he would have same parent interfaces on all nodes and no need for IPv4 network split he would not need config-only networks.
But those lines are for nonexistent and empty parent so this fix should be enough. Am I correct?

Copy link
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

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

LGTM , thanks for sharing the working example in #2418 (comment)
ptal @selansen

@lemrouch
Copy link
Contributor Author

lemrouch commented Feb 20, 2020

Thanks!
Can you check #2407 for me as well as those are both required for this to work?

@arkodg
Copy link
Contributor

arkodg commented Feb 20, 2020

also do we need a similar change for ipvlan ?

@lemrouch
Copy link
Contributor Author

Frankly I don't know. I have not read nor tried that code. I can check it on Monday.

Copy link
Collaborator

@selansen selansen left a comment

Choose a reason for hiding this comment

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

LGTM

@arkodg
Copy link
Contributor

arkodg commented Feb 20, 2020

@arkodg
Copy link
Contributor

arkodg commented Feb 20, 2020

thanks, will merge on green

@lemrouch
Copy link
Contributor Author

Do you know what went wrong? I'm not familiar with your tests.

@arkodg
Copy link
Contributor

arkodg commented Feb 20, 2020

can you please rebase to the latest master, we fixed a bunch of flaky tests a while back

@arkodg
Copy link
Contributor

arkodg commented Feb 20, 2020

git remote add upstream git@github.com:docker/libnetwork.git
git checkout master
git merge upstream/master
git checkout 2418-fix
git rebase master

Using dummy interface allows communication beween containers only if
they are running on the same node in swam.

Signed-off-by: Pavel Matěja <pavel@verotel.cz>
Using dummy interface allows communication beween containers only if
they are running on the same node in swarm.

Signed-off-by: Pavel Matěja <pavel@verotel.cz>
@arkodg arkodg merged commit 027aede into moby:master Feb 21, 2020
@lemrouch lemrouch deleted the 2418-fix branch February 21, 2020 06:53
@lemrouch lemrouch restored the 2418-fix branch March 2, 2020 09:18
arkodg pushed a commit to arkodg/libnetwork that referenced this pull request Mar 3, 2020
moby#2419 and
moby#2407
attempted to seperate out empty parent and internal for
macvlan and ipvlan networks

However it didnt pass the integration tests in moby
moby/moby#40596 and exposed some
more plumbing that needed to be done to make sure
we seperate the two things

If the -o parent is empty we create a dummylink
and if internal is set we dont add a default gateway
and make sure north-south communication cannot take place
(only east-west / container-container can)

Signed-off-by: Arko Dasgupta <arko.dasgupta@docker.com>
arkodg pushed a commit to arkodg/libnetwork that referenced this pull request Mar 4, 2020
moby#2419 and
moby#2407
attempted to seperate out empty parent and internal for
macvlan and ipvlan networks

However it didnt pass the integration tests in moby
moby/moby#40596 and exposed some
more plumbing that needed to be done to make sure
we seperate the two things

If the -o parent is empty we create a dummylink
and if internal is set we dont add a default gateway
and make sure north-south communication cannot take place
(only east-west / container-container can)

Signed-off-by: Arko Dasgupta <arko.dasgupta@docker.com>
arkodg pushed a commit to arkodg/libnetwork that referenced this pull request Mar 4, 2020
moby#2419 and
moby#2407
attempted to seperate out empty parent and internal for
macvlan and ipvlan networks

However it didnt pass the integration tests in moby
moby/moby#40596 and exposed some
more plumbing that needed to be done to make sure
we separate the two things

If the -o parent is empty we create a dummylink
and if internal is set we dont add a default gateway
and make sure north-south communication cannot take place
(only east-west / container-container can)

Signed-off-by: Arko Dasgupta <arko.dasgupta@docker.com>
@lemrouch lemrouch deleted the 2418-fix branch March 5, 2020 21:02
tle211212 pushed a commit to tle211212/libnetwork that referenced this pull request Apr 6, 2020
moby#2419 and
moby#2407
attempted to seperate out empty parent and internal for
macvlan and ipvlan networks

However it didnt pass the integration tests in moby
moby/moby#40596 and exposed some
more plumbing that needed to be done to make sure
we separate the two things

If the -o parent is empty we create a dummylink
and if internal is set we dont add a default gateway
and make sure north-south communication cannot take place
(only east-west / container-container can)

Signed-off-by: Arko Dasgupta <arko.dasgupta@docker.com>
cpuguy83 pushed a commit to cpuguy83/docker that referenced this pull request May 25, 2021
moby/libnetwork#2419 and
moby/libnetwork#2407
attempted to seperate out empty parent and internal for
macvlan and ipvlan networks

However it didnt pass the integration tests in moby
moby#40596 and exposed some
more plumbing that needed to be done to make sure
we separate the two things

If the -o parent is empty we create a dummylink
and if internal is set we dont add a default gateway
and make sure north-south communication cannot take place
(only east-west / container-container can)

Signed-off-by: Arko Dasgupta <arko.dasgupta@docker.com>
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