Skip to content

Handle the error case when a container reattaches to the same network#39137

Merged
thaJeztah merged 2 commits intomoby:masterfrom
arkodg:attach-to-existing-network-error
Jun 12, 2019
Merged

Handle the error case when a container reattaches to the same network#39137
thaJeztah merged 2 commits intomoby:masterfrom
arkodg:attach-to-existing-network-error

Conversation

@arkodg
Copy link
Contributor

@arkodg arkodg commented Apr 24, 2019 •
edited by kolyshkin
Loading

fixes docker/for-linux#632

Signed-off-by: Arko Dasgupta [email protected]

- What I did

Made sure the Networks context is manipulated in the scenario when a container attempts to attach to an a network it is already connected to

- How I did it
When attaching to a network in findAndAttachNetwork make sure you return with an error when that network is already part of container.NetworkSettings.Networks

- How to verify it
Followed the same steps in the issue and made sure it is resolved

- Description for the changelog
Fix an issue where connecting a container to a network it's already connected to removes its IP address.

- A picture of a cute animal (not mandatory but encouraged)

@arkodg
Copy link
Contributor Author

arkodg commented Apr 24, 2019

@mavenugo PTAL

@arkodg arkodg force-pushed the attach-to-existing-network-error branch from 34c89c3 to 925e18c Compare April 24, 2019 23:24
@codecov
Copy link

codecov bot commented Apr 24, 2019 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@619df5a). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master   #39137   +/-   ##
=========================================
  Coverage          ?   37.01%           
=========================================
  Files             ?      612           
  Lines             ?    45434           
  Branches          ?        0           
=========================================
  Hits              ?    16816           
  Misses            ?    26333           
  Partials          ?     2285

@arkodg arkodg force-pushed the attach-to-existing-network-error branch from 925e18c to 438f910 Compare April 25, 2019 17:54
@thaJeztah
Copy link
Member

thaJeztah commented Apr 25, 2019

Wondering; should we actually error, or treat this as a no-op (i.e., make network connect be idempotent)?

Gracefully take care of the error case when a container
871acb1 
retries to attach to a network, it is already connected to

Fixes - docker/for-linux#632

Signed-off-by: Arko Dasgupta <[email protected]>
@arkodg arkodg force-pushed the attach-to-existing-network-error branch from 438f910 to 871acb1 Compare April 26, 2019 23:00
thaJeztah
thaJeztah requested changes Apr 28, 2019
View reviewed changes
@arkodg arkodg force-pushed the attach-to-existing-network-error branch from e47c9c4 to 52ed152 Compare May 3, 2019 05:08
@arkodg arkodg force-pushed the attach-to-existing-network-error branch from 52ed152 to b5cbf94 Compare May 3, 2019 21:27
@arkodg arkodg force-pushed the attach-to-existing-network-error branch from b5cbf94 to 31e8fcc Compare May 3, 2019 22:59
@arkodg
Copy link
Contributor Author

arkodg commented May 6, 2019

Derek add label: rebuild/*

@derek derek bot added the rebuild/* label May 6, 2019
@arkodg
Copy link
Contributor Author

arkodg commented May 6, 2019 •
edited
Loading

Derek add label: rebuild/*

@derek derek bot added the rebuild/* label May 6, 2019
@cpuguy83
Copy link
Member

cpuguy83 commented May 9, 2019

Wow, this is weird.
We are already erroring out here, I'm wondering why network settings is being modified in that case.

cpuguy83
cpuguy83 approved these changes May 9, 2019
View reviewed changes
Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

LGTM

@cpuguy83
Copy link
Member

cpuguy83 commented May 9, 2019

The issue seems to be that the way the code is structured we are modifying the container object before things have even happened.
I think this is a good validation and happy to get it in... sadly it seems like the core logic deeper in the stack is flawed and difficult to work with.

@andrewhsu
Copy link
Contributor

andrewhsu commented May 9, 2019

Looked at this at the maintainers meeting with @arkodg and @cpuguy83 and this PR looks like it will help catch an error case but need bigger refactor to address the private function behaviours of moby/daemon/container_operations.go which will be tracked in a new issue.

@cpuguy83 cpuguy83 mentioned this pull request May 9, 2019
Open
@cpuguy83
Copy link
Member

cpuguy83 commented Jun 8, 2019

Ping need another reviewer.

@arkodg
Copy link
Contributor Author

arkodg commented Jun 8, 2019

@thaJeztah PTAL

thaJeztah
thaJeztah approved these changes Jun 12, 2019
View reviewed changes
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

whoops, my bad, thought I left mine already

LGTM

@thaJeztah thaJeztah merged commit 04ff4a2 into moby:master Jun 12, 2019
This was referenced Jun 12, 2019
Merged
Merged
@tiborvass
Copy link
Contributor

tiborvass commented Jun 12, 2019

CI passed 5 days ago, but merging this broke master because of #39332

@tiborvass tiborvass mentioned this pull request Jun 12, 2019
Merged
@thaJeztah thaJeztah mentioned this pull request Aug 27, 2019
Closed
3 tasks
@thaJeztah thaJeztah added this to the 20.03.0 milestone Apr 2, 2020
@akerouanton akerouanton mentioned this pull request Sep 18, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cpuguy83 cpuguy83 cpuguy83 approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

Assignees

@MHBauer MHBauer

Labels

area/networking Networking impact/changelog process/cherry-picked status/2-code-review

Projects

None yet

Milestone

20.10.0

Development

Successfully merging this pull request may close these issues.

Running docker container has no ipAddress

8 participants

@arkodg @thaJeztah @cpuguy83 @andrewhsu @tiborvass @kolyshkin @GordonTheTurtle @MHBauer