-
Notifications
You must be signed in to change notification settings - Fork 880
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
Enable IPV6 config on Sandbox creation on live-restore case #2043
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2043 +/- ##
=========================================
Coverage ? 40.12%
=========================================
Files ? 138
Lines ? 22132
Branches ? 0
=========================================
Hits ? 8881
Misses ? 11955
Partials ? 1296
Continue to review full report at Codecov.
|
LGTM Can you pls write a test case for this ? |
LGTM too, waiting for the test |
osl/namespace_linux.go
Outdated
// In live-restore mode, IPV6 entries are getting cleaned up due to below code | ||
// We should retain IPV6 configrations in live-restore mode when Docker Daemon | ||
// comesback. It should work as it is on other cases | ||
if !isRestore { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please avoid the double indentation of the logic below by instead
if !isRestore && !n.isDefault {
Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @selansen ^^
I made the changes but trying to come up with unit test for this case and
its kind of tricky to recreate this scenario with unit test case. will
try to commit with basic unit test for live-restore true case while
creating sandbox and commit it.
…On Wed, Dec 27, 2017 at 8:05 AM, Sebastiaan van Stijn < ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In osl/namespace_linux.go
<#2043 (comment)>:
> @@ -220,15 +220,18 @@ func NewSandbox(key string, osCreate, isRestore bool) (Sandbox, error) {
if err != nil {
logrus.Warnf("Failed to set the timeout on the sandbox netlink handle sockets: %v", err)
}
-
- // As starting point, disable IPv6 on all interfaces
- if !n.isDefault {
- err = setIPv6(n.path, "all", false)
- if err != nil {
- logrus.Warnf("Failed to disable IPv6 on all interfaces on network namespace %q: %v", n.path, err)
+ // In live-restore mode, IPV6 entries are getting cleaned up due to below code
+ // We should retain IPV6 configrations in live-restore mode when Docker Daemon
+ // comesback. It should work as it is on other cases
+ if !isRestore {
ping @selansen <https://github.com/selansen> ^^
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2043 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AKEKnyYUb_epb7-Zk62CQVVtHsrYPvDPks5tEkCqgaJpZM4RKUCx>
.
|
Please sign your commits following these rules: $ git clone -b "master" [email protected]:selansen/libnetwork.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842354563768
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. |
In sandbox creation we disable IPV6 config. But this causes problem in live-restore case where all IPV6 configs are wiped out on running container. Hence extra check has been added take care of this issue. Signed-off-by: selansen <[email protected]>
Incorporated all comments and added new unit test case . Pls merge the commit into upstream. |
LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This is to address issue moby/moby#35757.
In sandbox creation we disable IPV6 config. But this causes problem in live-restore case
where all IPV6 configs are wiped out on running container. Hence extra check has been added
take care of this issue.
Signed-off-by: selansen [email protected]