Skip to content

windows: ensure a init process exist after "Create()"#1603

Merged
crosbymichael merged 2 commits intocontainerd:masterfrom
mlaventure:fix-windows-flaky-tests
Oct 9, 2017
Merged

windows: ensure a init process exist after "Create()"#1603
crosbymichael merged 2 commits intocontainerd:masterfrom
mlaventure:fix-windows-flaky-tests

Conversation

@mlaventure
Copy link
Contributor

@mlaventure mlaventure commented Oct 5, 2017

Signed-off-by: Kenfe-Mickael Laventure mickael.laventure@gmail.com

--
Closes #1601

@estesp
Copy link
Member

estesp commented Oct 5, 2017

Seems reasonable, but struggling with some general code understanding; I don't ever see p.status referenced anywhere, so unsure why setting it is useful/if it is even used anywhere? If p.status is unused, then how is setting hcs before the goroutine helping the situation as it appears to only cause the Status() to try and distinguish between created and running? Sorry for the extra banter--just trying to make sure I understand this code.

@mlaventure
Copy link
Contributor Author

mlaventure commented Oct 5, 2017

hum, fair point. The p.hcs would affect it though. I guess I should have another look 😅

@mlaventure mlaventure force-pushed the fix-windows-flaky-tests branch from c19c899 to 14eaabd Compare October 6, 2017 00:10
@mlaventure mlaventure changed the title windows: Prevent a quick dying process to be reported as running windows: ensure a init process exist after "Create()" Oct 6, 2017
@mlaventure mlaventure force-pushed the fix-windows-flaky-tests branch 4 times, most recently from 3d2de6d to 3f614db Compare October 9, 2017 15:40
@mlaventure
windows: Fix a few races
ad52664 
Signed-off-by: Kenfe-Mickael Laventure <mickael.laventure@gmail.com>
@mlaventure
windows: Create init process with task
cfa8756 
This prevents `task.Wait()` to return an error if it is called before the task
is started.

Signed-off-by: Kenfe-Mickael Laventure <mickael.laventure@gmail.com>
@mlaventure mlaventure force-pushed the fix-windows-flaky-tests branch from 3f614db to cfa8756 Compare October 9, 2017 15:40
@mlaventure
Copy link
Contributor Author

mlaventure commented Oct 9, 2017

Rebased, and this should be good to go now

@codecov-io
Copy link

codecov-io commented Oct 9, 2017

Codecov Report

Merging #1603 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1603   +/-   ##
=======================================
  Coverage   46.44%   46.44%           
=======================================
  Files          26       26           
  Lines        3542     3542           
=======================================
  Hits         1645     1645           
  Misses       1519     1519           
  Partials      378      378

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 b2e3482...cfa8756. Read the comment docs.

estesp
estesp approved these changes Oct 9, 2017
View reviewed changes
Copy link
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM

I'm unable to test, but looks like Windows CI is happy with the changes :)

@crosbymichael
Copy link
Member

crosbymichael commented Oct 9, 2017

LGTM

@crosbymichael crosbymichael merged commit 72bb45a into containerd:master Oct 9, 2017
@mlaventure mlaventure deleted the fix-windows-flaky-tests branch October 9, 2017 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@estesp estesp estesp approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mlaventure @estesp @codecov-io @crosbymichael