Skip to content

Reorder checks in Walk to avoid panics#332

Merged
mrunalp merged 1 commit into
opencontainers:masterfrom
LK4D4:fix_panic_in_getpids
Oct 13, 2015
Merged

Reorder checks in Walk to avoid panics#332
mrunalp merged 1 commit into
opencontainers:masterfrom
LK4D4:fix_panic_in_getpids

Conversation

@LK4D4

@LK4D4 LK4D4 commented Oct 13, 2015

Copy link
Copy Markdown
Contributor

@tiborvass

tiborvass commented Oct 13, 2015

Copy link
Copy Markdown
Contributor

This PR fixes the issues we were having with docker.

@LK4D4 LK4D4 force-pushed the fix_panic_in_getpids branch from ffe2c98 to 77cf4da Compare October 13, 2015 21:46
@mrunalp

mrunalp commented Oct 13, 2015

Copy link
Copy Markdown
Contributor

I am seeing some errors in other tests running locally

=== RUN TestEnter
--- FAIL: TestEnter (0.32s)
        exec_test.go:251: unexpected number of processes [] 395 401

@crosbymichael

crosbymichael commented Oct 13, 2015

Copy link
Copy Markdown
Member

Same error locally

@LK4D4

LK4D4 commented Oct 13, 2015

Copy link
Copy Markdown
Contributor Author

Yup, I see.

@LK4D4
Reorder checks in Walk to avoid panics
6c198ae 
Also added test for host PID namespace

Signed-off-by: Alexander Morozov <lk4d4@docker.com>
@LK4D4 LK4D4 force-pushed the fix_panic_in_getpids branch from 77cf4da to 6c198ae Compare October 13, 2015 22:07
@LK4D4

LK4D4 commented Oct 13, 2015

Copy link
Copy Markdown
Contributor Author

@mrunalp @crosbymichael fixed

@tiborvass tiborvass mentioned this pull request Oct 13, 2015
Merged
@mrunalp

mrunalp commented Oct 13, 2015

Copy link
Copy Markdown
Contributor

okay tests are passing for me now. @tiborvass @ibuildthecloud. Do you guys want to try the updated patch as well?

@ibuildthecloud

ibuildthecloud commented Oct 13, 2015

Copy link
Copy Markdown

I'll try tonight

@ibuildthecloud

ibuildthecloud commented Oct 13, 2015

Copy link
Copy Markdown

@imikushin can you test this too?

@tiborvass

tiborvass commented Oct 13, 2015

Copy link
Copy Markdown
Contributor

@mrunalp this is basically blocking docker 1.9 so if someone could test/review/merge I would greatly appreciate it :)

@crosbymichael

crosbymichael commented Oct 13, 2015

Copy link
Copy Markdown
Member

LGTM

@mrunalp

mrunalp commented Oct 13, 2015

Copy link
Copy Markdown
Contributor

@tiborvass I am good with it from runc perspective ;) LGTM

mrunalp pushed a commit that referenced this pull request Oct 13, 2015
Merge pull request #332 from LK4D4/fix_panic_in_getpids
872c4ac 
Reorder checks in Walk to avoid panics
@mrunalp mrunalp merged commit 872c4ac into opencontainers:master Oct 13, 2015
@LK4D4 LK4D4 deleted the fix_panic_in_getpids branch October 13, 2015 22:34
@crosbymichael

crosbymichael commented Oct 13, 2015

Copy link
Copy Markdown
Member

@tiborvass what is this docker u speak of?

@mrunalp

mrunalp commented Oct 13, 2015

Copy link
Copy Markdown
Contributor

Heh 😆

@icecrime

icecrime commented Oct 13, 2015

Copy link
Copy Markdown

@crosbymichael

@ibuildthecloud

ibuildthecloud commented Oct 14, 2015

Copy link
Copy Markdown

We applied this to docker 1.8 and it worked. Docker 1.9 blows up for us for different reasons, but this patch seems good.

@kolyshkin kolyshkin mentioned this pull request Mar 20, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

status/0-triage

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@LK4D4 @tiborvass @mrunalp @crosbymichael @ibuildthecloud @icecrime @GordonTheTurtle