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.

Sign up for GitHub

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

Jump to bottom

Clean up --config-only networks after --config-from networks have ungracefully exited #2373

Merged
merged 1 commit into from
Jun 4, 2019
Merged

Clean up --config-only networks after --config-from networks have ungracefully exited #2373

merged 1 commit into from
Jun 4, 2019

Conversation

arkodg
Copy link
Contributor

@arkodg arkodg commented May 7, 2019

Only increment Endpoint count for --config-only networks when the respective --config-from inherited network has been successfully created

Fixes - moby/moby#35101 (comment)

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

@arkodg
Copy link
Contributor Author

arkodg commented May 7, 2019

PTAL @thaJeztah @mavenugo

@arkodg arkodg force-pushed the ungraceful-exit-config-from-net-clean branch from 3f4d080 to c6a77b4 Compare May 7, 2019 22:56
euanh
euanh reviewed May 8, 2019
View reviewed changes
controller.go Outdated Show resolved Hide resolved
@thaJeztah
Copy link
Member

ping @euanh PTAL

@arkodg arkodg force-pushed the ungraceful-exit-config-from-net-clean branch from c6a77b4 to a10c81b Compare May 14, 2019 15:19
@euanh
Copy link
Collaborator

euanh commented May 14, 2019

@arkodg The fix looks good.

Please redraft the commit message (git commit --amend):

  • The commit title should explain what the commit does e.g. Correctly clean up --config-only networks
  • The commit message body should have a reference to the bug (Fixes: Error response from daemon: configuration network 8-2 is in use moby#35101)
  • Please include some detail about the change you are making - mention that the variable shadowing was causing the deferred function not to run, and so you are changing the scope of the variables.

We have this information in the pull request discussion, but it's better to have it in the commit message as well so someone working on this code in future can easily figure out what your change is doing.

@thaJeztah
Copy link
Member

We have this information in the pull request discussion, but it's better to have it in the commit message as well so someone working on this code in future can easily figure out what your change is doing.

👍 git history is what's being preserved; GitHub description will get lost

@thaJeztah
Copy link
Member

Might want to double check the error-checks in defer, as they might not do what you expect them to do if they're expected to act on the actual error returned; https://play.golang.org/p/GWUmmR52Nk5

@euanh
Copy link
Collaborator

euanh commented May 14, 2019 •
edited
Loading

Might want to double check the error-checks in defer, as they might not do what you expect them to do if they're expected to act on the actual error returned; https://play.golang.org/p/GWUmmR52Nk5

👍 This is the retErr pattern I was meaning. This whole bug is because the err checks were not doing what was expected (not obvious without a close reading of the code).

@arkodg arkodg force-pushed the ungraceful-exit-config-from-net-clean branch 2 times, most recently from ef6ee87 to 9753b22 Compare May 14, 2019 16:34
@thaJeztah
Copy link
Member

Thanks; yes, I saw your comment, so had a quick peek at the changes; didn't have time to go through the whole logic, so thought a quick example to illustrate potential problems would be helpful

Been bit by this more than once 😂

Correctly clean up --config-only networks
eacb56d 
The endpoint count for --config-only networks
was being incremented even when the respective --config-from
inherited network failed to create a network

This was due to a variable shadowing problem with err causing
the deferred function to not execute correctly.

Using the same err variable across the entire function fixes
the issue

Fixes: moby/moby#35101

Signed-off-by: Arko Dasgupta <[email protected]>
@arkodg arkodg force-pushed the ungraceful-exit-config-from-net-clean branch from 9753b22 to eacb56d Compare May 14, 2019 17:21
@arkodg
Copy link
Contributor Author

arkodg commented May 14, 2019

@thaJeztah @euanh I went through all the defer functions and they all seem to be operating on errwhich is now visible and same across the function (and every err seems to be legit and causes the function to exit immediately), and then operating on another local err to further execute logic

@euanh
Copy link
Collaborator

euanh commented May 15, 2019

@arkodg Yes, I think your commit fixes this problem correctly, but it's useful to be aware of the problem and the retErr pattern for the future.

thaJeztah
thaJeztah approved these changes May 23, 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.

LGTM (but still would love to see a follow up for the retErr 😇

ping @mavenugo @joeabbey PTAL

@thaJeztah thaJeztah mentioned this pull request May 23, 2019
Closed
@mdbraber
Copy link

mdbraber commented Jun 3, 2019

+1 for getting this merged for the next version! Tnx @arkodg @thaJeztah

@tiborvass tiborvass merged commit fc5a7d9 into moby:master Jun 4, 2019
@tiborvass tiborvass mentioned this pull request Jun 7, 2019
Merged
2 tasks
This was referenced Jun 7, 2019
Merged
Merged
@arkodg arkodg deleted the ungraceful-exit-config-from-net-clean branch June 14, 2019 14:51
thaJeztah added a commit to thaJeztah/docker that referenced this pull request Jul 23, 2019
@thaJeztah
bump libnetwork 55685ba49593e67f5e1c8180539379b16736c25e (18.09 branch)
0a3767c 
full diff: moby/libnetwork@e7933d4...55685ba

changes included:

- moby/libnetwork#2382 Backporting PR 2069 to bump_18.09
  - backport of https://github.com/docker/libnetwork#2069 Rolling back the port configs if failed to programIngress()
- moby/libnetwork#2363 [18.09] align dependencies with engine 18.09
- moby/libnetwork#2400 [18.09 backport] Fix TestValidRemoteDriver GetCapabilities errors
- moby/libnetwork#2391 [18.09 backport] Correctly clean up --config-only networks
  - backport of moby/libnetwork#2373
  - fixes moby#35101
- moby/libnetwork#2392 [18.09 backport] remove gosimple - package is gone and it's not important

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah mentioned this pull request Jul 23, 2019
Merged
docker-jenkins pushed a commit to docker-archive/docker-ce that referenced this pull request Jul 25, 2019
@thaJeztah
bump libnetwork 55685ba49593e67f5e1c8180539379b16736c25e (18.09 branch)
c9d036b 
full diff: moby/libnetwork@e7933d4...55685ba

changes included:

- moby/libnetwork#2382 Backporting PR 2069 to bump_18.09
  - backport of https://github.com/docker/libnetwork#2069 Rolling back the port configs if failed to programIngress()
- moby/libnetwork#2363 [18.09] align dependencies with engine 18.09
- moby/libnetwork#2400 [18.09 backport] Fix TestValidRemoteDriver GetCapabilities errors
- moby/libnetwork#2391 [18.09 backport] Correctly clean up --config-only networks
  - backport of moby/libnetwork#2373
  - fixes moby/moby#35101
- moby/libnetwork#2392 [18.09 backport] remove gosimple - package is gone and it's not important

Signed-off-by: Sebastiaan van Stijn <[email protected]>
Upstream-commit: 0a3767c7e9803f0a595a07b0548e99d60e861062
Component: engine
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@euanh euanh euanh approved these changes

@tiborvass tiborvass tiborvass approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@arkodg @thaJeztah @euanh @mdbraber @tiborvass