Skip to content

Fix missing close#5051

Merged
dmcgowan merged 1 commit intocontainerd:masterfrom
wzshiming:fix/missing-close
Feb 26, 2021
Merged

Fix missing close#5051
dmcgowan merged 1 commit intocontainerd:masterfrom
wzshiming:fix/missing-close

Conversation

@wzshiming
Copy link
Copy Markdown
Contributor

@wzshiming wzshiming commented Feb 18, 2021

Signed-off-by: Shiming Zhang [email protected]

@wzshiming
Fix missing close
05ef2fe 
Signed-off-by: Shiming Zhang <[email protected]>
@k8s-ci-robot
Copy link
Copy Markdown

k8s-ci-robot commented Feb 18, 2021

Hi @wzshiming. 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.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Feb 18, 2021

Build succeeded.

qiutongs
qiutongs reviewed Feb 18, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@qiutongs qiutongs left a comment

Choose a reason for hiding this comment

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

Not familier with this code. Happy to discuss.

Comment thread runtime/v2/runc/v1/service.go
Comment thread runtime/v2/runc/v1/service.go
Comment thread runtime/v2/runc/v1/service.go
@wzshiming
Copy link
Copy Markdown
Contributor Author

wzshiming commented Feb 18, 2021

@qiutongs I follow the logic of v2

containerd/runtime/v2/runc/v2/service.go

Lines 174 to 238 in 746cef0

func (s *service) StartShim(ctx context.Context, id, containerdBinary, containerdAddress, containerdTTRPCAddress string) (_ string, retErr error) {
cmd, err := newCommand(ctx, id, containerdBinary, containerdAddress, containerdTTRPCAddress)
if err != nil {
return "", err
}
grouping := id
spec, err := readSpec()
if err != nil {
return "", err
}
for _, group := range groupLabels {
if groupID, ok := spec.Annotations[group]; ok {
grouping = groupID
break
}
}
address, err := shim.SocketAddress(ctx, containerdAddress, grouping)
if err != nil {
return "", err
}
socket, err := shim.NewSocket(address)
if err != nil {
// the only time where this would happen is if there is a bug and the socket
// was not cleaned up in the cleanup method of the shim or we are using the
// grouping functionality where the new process should be run with the same
// shim as an existing container
if !shim.SocketEaddrinuse(err) {
return "", errors.Wrap(err, "create new shim socket")
}
if shim.CanConnect(address) {
if err := shim.WriteAddress("address", address); err != nil {
return "", errors.Wrap(err, "write existing socket for shim")
}
return address, nil
}
if err := shim.RemoveSocket(address); err != nil {
return "", errors.Wrap(err, "remove pre-existing socket")
}
if socket, err = shim.NewSocket(address); err != nil {
return "", errors.Wrap(err, "try create new shim socket 2x")
}
}
defer func() {
if retErr != nil {
socket.Close()
_ = shim.RemoveSocket(address)
}
}()
f, err := socket.File()
if err != nil {
return "", err
}
cmd.ExtraFiles = append(cmd.ExtraFiles, f)
if err := cmd.Start(); err != nil {
f.Close()
return "", err
}
defer func() {
if retErr != nil {
cmd.Process.Kill()
}
}()

@qiutongs
Copy link
Copy Markdown
Contributor

qiutongs commented Feb 18, 2021 •
edited
Loading

LGTM

Thank you for pointing it out. In that case, your code looks good. My previous questions are for curiosity then.

@wzshiming
Copy link
Copy Markdown
Contributor Author

wzshiming commented Feb 23, 2021

Hi @thaJeztah
Could you have time to take a look.

@fuweid fuweid self-requested a review February 24, 2021 05:42
@wzshiming wzshiming requested a review from thaJeztah February 24, 2021 09:45
crosbymichael
crosbymichael approved these changes Feb 25, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@crosbymichael crosbymichael left a comment

Choose a reason for hiding this comment

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

LGTM

dmcgowan
dmcgowan approved these changes Feb 26, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@dmcgowan dmcgowan left a comment

Choose a reason for hiding this comment

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

LGTM

@dmcgowan dmcgowan merged commit 10bbd1a into containerd:master Feb 26, 2021
@wzshiming wzshiming deleted the fix/missing-close branch February 27, 2021 06:10
@fuweid fuweid mentioned this pull request Mar 18, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dmcgowan dmcgowan dmcgowan approved these changes

@crosbymichael crosbymichael crosbymichael approved these changes

@mxpv mxpv mxpv approved these changes

@fuweid fuweid Awaiting requested review from fuweid

@thaJeztah thaJeztah Awaiting requested review from thaJeztah

+1 more reviewer

@qiutongs qiutongs qiutongs left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

needs-ok-to-test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@wzshiming @k8s-ci-robot @qiutongs @dmcgowan @crosbymichael @mxpv @thaJeztah @fuweid