Skip to content

Check if a process exists before returning it#4645

Merged
estesp merged 1 commit intocontainerd:masterfrom
masters-of-cats:master
Oct 22, 2020
Merged

Check if a process exists before returning it#4645
estesp merged 1 commit intocontainerd:masterfrom
masters-of-cats:master

Conversation

@gcapizzi
Copy link
Copy Markdown
Contributor

@gcapizzi gcapizzi commented Oct 21, 2020 •
edited
Loading

Fixes #4632. See also 8e598fc for an identical fix made on runc runtime v1.

This does not come with a test, as I don't have enough context to write one, but I'd be happy to add one with some help from the team. FWIW this makes this test on Garden pass, even after removing the workaround we have in place at the moment.

@k8s-ci-robot
Copy link
Copy Markdown

k8s-ci-robot commented Oct 21, 2020

Hi @gcapizzi. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@gcapizzi gcapizzi mentioned this pull request Oct 21, 2020
Closed
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Oct 21, 2020

Build succeeded.

estesp
estesp approved these changes Oct 21, 2020
View reviewed changes
Copy link
Copy Markdown
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

Zyqsempai
Zyqsempai approved these changes Oct 21, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@Zyqsempai Zyqsempai left a comment

Choose a reason for hiding this comment

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

LGTM

@AkihiroSuda
Copy link
Copy Markdown
Member

AkihiroSuda commented Oct 22, 2020

/ok-to-test

@gcapizzi @danail-branekov
Check if a process exists before returning it
8eda32e 
Fixes containerd#4632.

Signed-off-by: Giuseppe Capizzi <[email protected]>
Co-authored-by: Danail Branekov <[email protected]>
@danail-branekov
Copy link
Copy Markdown
Contributor

danail-branekov commented Oct 22, 2020

Updated the PR with a test for the fix

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Oct 22, 2020

Build succeeded.

@estesp estesp merged commit 656b487 into containerd:master Oct 22, 2020
@gcapizzi
Copy link
Copy Markdown
Contributor Author

gcapizzi commented Oct 22, 2020

@estesp @AkihiroSuda @Zyqsempai @crosbymichael thanks for the merge! Are you going to backport to 1.3 and 1.4 too, or do you need us to make separate PRs? Thanks!

This was referenced Oct 23, 2020
Merged
Merged
@estesp estesp added cherry-picked/1.3.x cherry-picked/1.4.x PR commits are cherry picked into the release/1.4 branch and removed cherry-pick/1.3.x labels Oct 23, 2020
@estesp
Copy link
Copy Markdown
Member

estesp commented Oct 23, 2020

Cherry picked for release/1.3 in #4649 and release/1.4 in #4650

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

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

+1 more reviewer

@Zyqsempai Zyqsempai Zyqsempai approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cherry-picked/1.4.x PR commits are cherry picked into the release/1.4 branch ok-to-test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

LoadProcess does not error when the process does not exist in V2 runtime

6 participants

@gcapizzi @k8s-ci-robot @AkihiroSuda @danail-branekov @estesp @Zyqsempai