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

2089 fix restart runtime error #2409

Merged
merged 2 commits into from
Nov 4, 2013

Conversation

pnasrat
Copy link
Contributor

@pnasrat pnasrat commented Oct 27, 2013

Tested with unittest and latest reproducer in bug.

root@precise64:/vagrant# ID=$(docker run -d busybox sleep 60)
root@precise64:/vagrant# invoke-rc.d docker restart
invoke-rc.d: unknown initscript, /etc/init.d/docker not found.
root@precise64:/vagrant# docker kill $ID
68cfb88cf047
root@precise64:/vagrant# docker start $ID
68cfb88cf047
root@precise64:/vagrant# docker ps
ID                  IMAGE               COMMAND             CREATED             STATUS              PORTS
68cfb88cf047        busybox:latest      sleep 60            10 seconds ago      Up 8 seconds

@pnasrat
Copy link
Contributor Author

pnasrat commented Oct 27, 2013

Issue #2089

@tianon
Copy link
Member

tianon commented Oct 27, 2013

Oops, I forgot that invoke-rc.d only works for init scripts (not upstart). If you replace that with service docker restart it will probably not error out and should give you a ghost like it's supposed to.

@pnasrat
Copy link
Contributor Author

pnasrat commented Oct 27, 2013

I actually just copied and pasted the wrong block (I reran with initctl stop). Let me just confirm that.

@pnasrat
Copy link
Contributor Author

pnasrat commented Oct 27, 2013

# ID=$(docker run -d busybox sleep 600)
Unable to find image 'busybox' (tag: latest) locally
Pulling repository busybox
e9aa60c60128: Download complete
# service docker restart
docker stop/waiting
docker start/running, process 10109
# docker kill $ID
8b4af5160c62
# docker inspect $ID | grep Ghost
        "Ghost": true
# docker start $ID
8b4af5160c62
# docker -v
Docker version 0.6.4-dev, build 877be37-dirty
# docker ps
ID                  IMAGE               COMMAND             CREATED              STATUS              PORTS
8b4af5160c62        busybox:latest      sleep 600           About a minute ago   Up 52 seconds

@tianon
Copy link
Member

tianon commented Oct 28, 2013

Cool! Major +1 from me for taking care of this! (although, IANTM)

@@ -932,7 +932,7 @@ func (container *Container) allocateNetwork() error {

var iface *NetworkInterface
var err error
if !container.State.Ghost {
if !container.State.Ghost || !container.State.Running {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure about the || ? the if is now always returning true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Meh I could have messed this up. I won't have time to look at until this evening.

I believe that if we have a non-running ghost we should create the network, let me add a test for the happy path and figure it out.

@pnasrat
Copy link
Contributor Author

pnasrat commented Oct 29, 2013

@vieux please take another look - I've made an explicit nil network ip check and both unit and manual test still passes.

@vieux
Copy link
Contributor

vieux commented Oct 31, 2013

LGTM, ping @crosbymichael

@pnasrat
Copy link
Contributor Author

pnasrat commented Nov 3, 2013

Need to rebase having some test failure issues. Will ping PR when fixed.

@pnasrat
Copy link
Contributor Author

pnasrat commented Nov 3, 2013

Have fixed up and my test passes with the signature change but getting failures

https://gist.github.com/pnasrat/d1fcd6bab3d2028c6c82

@ecnahc515 is also seeing the lxc-kill failures

@pnasrat
Copy link
Contributor Author

pnasrat commented Nov 3, 2013

Actually I misread the output and it passes after I updated the host docker to be the same version https://gist.github.com/pnasrat/d1fcd6bab3d2028c6c82

@crosbymichael should be ready for review.

@crosbymichael
Copy link
Contributor

LGTM

crosbymichael added a commit that referenced this pull request Nov 4, 2013
Fix restart runtime error with ghost container networking
@crosbymichael crosbymichael merged commit 35690e7 into moby:master Nov 4, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants