Shorten the unix socket path for shim by linxiulei · Pull Request #3046 · containerd/containerd

linxiulei · 2019-02-26T06:32:53Z

Use sha256 hash to shorten the unix socket path to satisfy the
length limitation of abstract socket path

This commit also backports the feature storing address path to
a file from v2 to keep compatibility

Fixes #3032

Signed-off-by: Eric Lin linxiulei@gmail.com

AkihiroSuda · 2019-02-26T07:00:06Z

Thanks for working on this, but I think we should freeze v1 and prioritize v2 for this.

runtime/v1/linux/bundle.go

linxiulei · 2019-02-26T07:33:57Z

Thanks for working on this, but I think we should freeze v1 and prioritize v2 for this.

As long as it's compatible, I think there is no harm for fixing v1 : ). This commit applies to v2 as well.

runtime/v1/shim/client/client.go

AkihiroSuda · 2019-03-01T05:31:18Z

The socket file can't be found on the actual system.. Is it removed after establishing the connection? (where?)
👍 for forking instead of Chdir
doesn't seem to apply to v2

linxiulei · 2019-03-01T06:50:38Z

* The socket file can't be found on the actual system.. Is it removed after establishing the connection? (where?)

No, It was my fault. I didn't do well with the socket file in last commit that golang closed listener along with unlinking the socket file. I just re-push the new commit, please check it out.

* 👍 for forking instead of Chdir

As https://github.com/containerd/containerd/blob/master/sys/mount_linux.go#L93. Forking to implement socket(), bind(), listen() syscall is really tiresome.

* doesn't seem to apply to v2

May you provide the way to test it ? : )

codecov-io · 2019-03-06T04:07:40Z

Codecov Report

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

@@           Coverage Diff           @@
##           master    #3046   +/-   ##
=======================================
  Coverage   43.46%   43.46%           
=======================================
  Files         103      103           
  Lines       11033    11033           
=======================================
  Hits         4795     4795           
  Misses       5503     5503           
  Partials      735      735

Flag	Coverage Δ
#linux	`47.48% <ø> (ø)`	⬆️
#windows	`40.28% <ø> (ø)`	⬆️

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 e70a530...9bf769e. Read the comment docs.

codecov-io · 2019-03-06T04:07:40Z

Codecov Report

Merging #3046 into master will decrease coverage by <.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #3046      +/-   ##
==========================================
- Coverage    43.4%   43.39%   -0.01%     
==========================================
  Files         104      104              
  Lines       11096    11097       +1     
==========================================
  Hits         4816     4816              
- Misses       5544     5545       +1     
  Partials      736      736

Flag	Coverage Δ
#linux	`47.34% <0%> (-0.01%)`	⬇️
#windows	`40.24% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
runtime/v2/shim/util_unix.go	`0% <0%> (ø)`	⬆️

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 63d7a9c...a631796. Read the comment docs.

fuweid

no shim v2 change here runtime/v2/shim/util_unix.go
update your commit message because there is still abstract socket

linxiulei · 2019-03-06T08:39:28Z

1. no shim v2 change here `runtime/v2/shim/util_unix.go`

2. update your commit message because there is still abstract socket

Fixed, thanks

runtime/v2/shim/util_unix.go

runtime/v1/linux/bundle.go

runtime/v1/shim/client/client.go

fuweid · 2019-03-07T04:07:08Z

@linxiulei I think we can also add test case for regression :p

fuweid · 2019-03-07T12:32:49Z

runtime/v1/shim/client/client.go

could we create the same function for this? I think v1 should not import v2 package here. WDYT?

Yes, this sounds good, just copy the function

fuweid · 2019-03-07T12:34:20Z

runtime/v1/linux/bundle.go

I think we should return legacyone if the err is not nil(notfound), right?

This logic looks wrong here. We should handle an os.IsNotExist errror and then call legacyShimAddress

Just asking，if err == IsNotExist, the returning address would be empty, it seems we pass a potential err to Connect

I think it is fine to ignore the error here.

If we want to know what the error is, the log seems the way. But there is no ctx pass-through var, and we should use log.G(context.TODO()). I don't know it is good way to add it.

ping @AkihiroSuda @crosbymichael

this handling still looks wrong. if we get an error here then you should be returning the legacy address right?!

client_test.go

Use sha256 hash to shorten the unix socket path to satisfy the length limitation of abstract socket path This commit also backports the feature storing address path to a file from v2 to keep compatibility Fixes containerd#3032 Signed-off-by: Eric Lin <linxiulei@gmail.com>

fuweid · 2019-03-15T09:56:38Z

runtime/v1/linux/bundle.go

+
+func (b *bundle) decideShimAddress(namespace string) string {
+	address, err := b.loadAddress()
+	if err != nil {


let's handle the error in next pr :)

fuweid

LGTM

crosbymichael · 2019-03-15T14:10:44Z

LGTM

AkihiroSuda reviewed Feb 26, 2019

View reviewed changes

runtime/v1/linux/bundle.go Outdated Show resolved Hide resolved

fuweid reviewed Feb 26, 2019

View reviewed changes

runtime/v1/shim/client/client.go Outdated Show resolved Hide resolved

linxiulei force-pushed the fix_shim_socket branch from e1bec35 to e955e27 Compare March 1, 2019 06:45

linxiulei force-pushed the fix_shim_socket branch from e955e27 to 9bf769e Compare March 6, 2019 03:45

fuweid requested changes Mar 6, 2019

View reviewed changes

linxiulei force-pushed the fix_shim_socket branch from 9bf769e to da8fe5a Compare March 6, 2019 08:38

linxiulei changed the title ~~Use regular unix socket to connect shim~~ Shorten the unix socket path for shim Mar 6, 2019

fuweid reviewed Mar 6, 2019

View reviewed changes

runtime/v2/shim/util_unix.go Outdated Show resolved Hide resolved

runtime/v1/linux/bundle.go Outdated Show resolved Hide resolved

runtime/v1/shim/client/client.go Outdated Show resolved Hide resolved

linxiulei force-pushed the fix_shim_socket branch from da8fe5a to d1f8d4c Compare March 7, 2019 09:29

fuweid reviewed Mar 7, 2019

View reviewed changes

linxiulei force-pushed the fix_shim_socket branch 4 times, most recently from aa22c04 to dfd6aae Compare March 8, 2019 07:33

fuweid requested changes Mar 11, 2019

View reviewed changes

client_test.go Outdated Show resolved Hide resolved

linxiulei force-pushed the fix_shim_socket branch 4 times, most recently from aac643d to a950d4e Compare March 15, 2019 02:05

linxiulei force-pushed the fix_shim_socket branch from a950d4e to a631796 Compare March 15, 2019 03:58

fuweid reviewed Mar 15, 2019

View reviewed changes

crosbymichael merged commit ef45e4f into containerd:master Mar 15, 2019

haikuoliu mentioned this pull request Sep 21, 2019

Shorten the firecracker unix socket path firecracker-microvm/firecracker-containerd#264

Closed

thaJeztah mentioned this pull request Feb 14, 2020

[release/1.2 backport] Fix container pid race condition #4025

Merged

egernst mentioned this pull request Mar 2, 2021

metrics: unix socket path too long in some system kata-containers/kata-containers#497

Closed

saintube mentioned this pull request May 19, 2022

chore: fix test tempdir generation koordinator-sh/koordinator#156

Merged

Conversation

linxiulei commented Feb 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AkihiroSuda commented Feb 26, 2019

Uh oh!

Uh oh!

linxiulei commented Feb 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

AkihiroSuda commented Mar 1, 2019

Uh oh!

linxiulei commented Mar 1, 2019

Uh oh!

codecov-io commented Mar 6, 2019

Codecov Report

Uh oh!

codecov-io commented Mar 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

fuweid left a comment

Choose a reason for hiding this comment

Uh oh!

linxiulei commented Mar 6, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fuweid commented Mar 7, 2019

Uh oh!

fuweid Mar 7, 2019

Choose a reason for hiding this comment

Uh oh!

crosbymichael Mar 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

linxiulei Mar 8, 2019

Choose a reason for hiding this comment

Uh oh!

fuweid Mar 7, 2019

Choose a reason for hiding this comment

Uh oh!

crosbymichael Mar 7, 2019

Choose a reason for hiding this comment

Uh oh!

linxiulei Mar 8, 2019

Choose a reason for hiding this comment

Uh oh!

fuweid Mar 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crosbymichael Mar 14, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fuweid Mar 15, 2019

Choose a reason for hiding this comment

Uh oh!

fuweid left a comment

Choose a reason for hiding this comment

Uh oh!

crosbymichael commented Mar 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

linxiulei commented Feb 26, 2019 •

edited

Loading

linxiulei commented Feb 26, 2019 •

edited

Loading

codecov-io commented Mar 6, 2019 •

edited

Loading

crosbymichael Mar 7, 2019 •

edited

Loading

fuweid Mar 11, 2019 •

edited

Loading