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

Enable IPV6 config on Sandbox creation on live-restore case #2043

Merged
merged 3 commits into from
Jan 2, 2018

Conversation

selansen
Copy link
Contributor

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]

@codecov-io
Copy link

codecov-io commented Dec 21, 2017

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2043   +/-   ##
=========================================
  Coverage          ?   40.12%           
=========================================
  Files             ?      138           
  Lines             ?    22132           
  Branches          ?        0           
=========================================
  Hits              ?     8881           
  Misses            ?    11955           
  Partials          ?     1296
Impacted Files Coverage Δ
osl/namespace_linux.go 35.31% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0eff162...76bfcb0. Read the comment docs.

@mavenugo
Copy link
Contributor

LGTM

Can you pls write a test case for this ?

@fcrisciani
Copy link

LGTM too, waiting for the test

// 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 {
Copy link
Contributor

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

Copy link
Member

Choose a reason for hiding this comment

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

ping @selansen ^^

@selansen
Copy link
Contributor Author

selansen commented Dec 27, 2017 via email

@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 "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]>
@selansen
Copy link
Contributor Author

Incorporated all comments and added new unit test case . Pls merge the commit into upstream.

@fcrisciani
Copy link

LGTM

@abhi abhi changed the title Enable IPV6 config on Sandbaox creation on live-restore case Enable IPV6 config on Sandbox creation on live-restore case Jan 2, 2018
Copy link
Contributor

@abhi abhi left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants