moved updateToStore call in CreateEndPoint before we call addEndpoint#2024
moved updateToStore call in CreateEndPoint before we call addEndpoint#2024fcrisciani merged 1 commit intomoby:masterfrom
Conversation
|
Please sign your commits following these rules: $ git clone -b "master" git@github.com:selansen/libnetwork.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842354469720
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 -fAmending updates the existing PR. You DO NOT need to open a new one. |
selansen
changed the title
Codecov Report
@@ Coverage Diff @@
## master #2024 +/- ##
=========================================
Coverage ? 39.94%
=========================================
Files ? 137
Lines ? 22117
Branches ? 0
=========================================
Hits ? 8834
Misses ? 11980
Partials ? 1303
Continue to review full report at Codecov.
|
selansen
changed the title
|
|
||
| if err = n.getController().updateToStore(ep); err != nil { | ||
| //Commented out and calling same API before calling addEndPoint | ||
| /*if err = n.getController().updateToStore(ep); err != nil { |
There was a problem hiding this comment.
How about removing this code block instead of keeping it commented? Since git will store the previous version of the code, good comments alone may be sufficient. Commented code blocks are somewhat rendundant (and potentially confusing).
There was a problem hiding this comment.
ok. Will wait for other's comment and will incorporate all review comments together.
|
review comments are taken care. |
| }() | ||
|
|
||
| if err = n.addEndpoint(ep); err != nil { | ||
| //Moving updateToSTore before calling addEndpoint so that we shall clean up VETH interfaces in case |
There was a problem hiding this comment.
Very minor stylistic nit: I think typical convention for comments is to add a space between // and the comment line. So something like // Moving updateToStore would be ideal. Same applies to the line below.
ddebroy
left a comment
ddebroy
left a comment
There was a problem hiding this comment.
LGTM except for a very minor nit. Someone else should review too.
VETH interface was not cleaned up when DockerD got killed between addEndpoint and updateToStore calls. I have added logs and made sure calling updateToStore before addEndpoint contains same values. Hence moving up the call looks safer and VETH gets cleaned up even after DockerD gets killed in the middle. Signed-off-by: selansen <elango@docker.com>
|
All the code review comments incorporated. @fcrisciani could you pls merge it into main stream? |
|
LGTM |
5 participants
This is to address issue #1850 .
While creating NetworkEndpoint , if DockerD crashes before saving the object into store there will be Veth interface leak on the host. To address this issue, updateToStore call is moved up in the code flow before creating addEndpoint. In case of failure during addEndpoint, we delete from store with defer func. So it should be taken care in the event of failure in later point.
Signed-off-by: selansen elango.siva@docker.com