Skip to content

fix delete running bundle dir when ctr t start a container again#2605

Merged
crosbymichael merged 1 commit intocontainerd:masterfrom
lifubang:runafterstart
Sep 21, 2018
Merged

fix delete running bundle dir when ctr t start a container again#2605
crosbymichael merged 1 commit intocontainerd:masterfrom
lifubang:runafterstart

Conversation

@lifubang
Copy link
Copy Markdown
Contributor

@lifubang lifubang commented Sep 1, 2018

Signed-off-by: Lifubang [email protected]

First, create a redis container:

root@dockerdemo:/opt/runctest/redis#
~/gocode/src/github.com/containerd/containerd/bin/ctr run -d --config ./config.json redis7
9:C 01 Sep 03:32:22.208 # oO0OoO0OoO0Oo Redis is starting oO0OoO0OoO0Oo
root@dockerdemo:/opt/runctest/redis#
~/gocode/src/github.com/containerd/containerd/bin/ctr t ls
TASK      PID      STATUS
redis7    28576    RUNNING
root@dockerdemo:/opt/runctest/redis# ls /run/containerd/io.containerd.runtime.v1.linux/default/
redis7

Then, I run ctr t start again:

root@dockerdemo:/opt/runctest/redis#
~/gocode/src/github.com/containerd/containerd/bin/ctr t start -d redis7
ctr: mkdir /run/containerd/io.containerd.runtime.v1.linux/default/redis7: file exists: unknown
root@dockerdemo:/opt/runctest/redis#
~/gocode/src/github.com/containerd/containerd/bin/ctr t ls
TASK    PID    STATUS    CONTAINER    
root@dockerdemo:/opt/runctest/redis# ls /run/containerd/io.containerd.runtime.v1.linux/default/
root@dockerdemo:/opt/runctest/redis#

redis7's workdir is deleted by mistake.

This is because os.RemoveAll in defer func in bundle.go

Please check it, thanks.

@lifubang lifubang changed the title fix delete running bundle dir when run t start cmd again fix delete running bundle dir when ctr t start cmd again Sep 1, 2018
@lifubang lifubang force-pushed the runafterstart branch 3 times, most recently from 666cd04 to 4403b42 Compare September 1, 2018 04:26
@lifubang lifubang changed the title fix delete running bundle dir when ctr t start cmd again fix delete running bundle dir when ctr t start a container again Sep 3, 2018
@lifubang
Copy link
Copy Markdown
Contributor Author

lifubang commented Sep 3, 2018 •
edited
Loading

@crosbymichael @AkihiroSuda I thiin this bug needs to be fixed ASAP. Because ctr is installed as docker-containerd-ctr. If I run docker-containerd-ctr t start to start a container which has beed started by docker. This container can't stop, pause, restore unless the host restart.

crosbymichael
crosbymichael reviewed Sep 4, 2018
View reviewed changes
Comment thread runtime/v1/linux/bundle.go Outdated
@crosbymichael
Copy link
Copy Markdown
Member

crosbymichael commented Sep 4, 2018

@lifubang see #2575

lifubang
lifubang commented Sep 5, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor Author

@lifubang lifubang left a comment

Choose a reason for hiding this comment

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

Thanks. I have finished.

@lifubang
Copy link
Copy Markdown
Contributor Author

lifubang commented Sep 20, 2018

@crosbymichael How about this one? I pull the latest version, there still have this error:

root@dockerdemo:/opt/runctest/redis# ctr t start -d redis
root@dockerdemo:/opt/runctest/redis# ctr t ls
TASK     PID      STATUS    
redis    24366    RUNNING
root@dockerdemo:/opt/runctest/redis# ctr t start -d redis
ctr: mkdir /run/containerd/io.containerd.runtime.v1.linux/default/redis: file exists: unknown
root@dockerdemo:/opt/runctest/redis# ctr t ls
TASK    PID    STATUS    
root@dockerdemo:/opt/runctest/redis#

@estesp
Copy link
Copy Markdown
Member

estesp commented Sep 20, 2018

@lifubang Can you squash these 2 commits? No reason to have the prior code version as a separate commit.

@crosbymichael this seems like a reasonable bug fix to not remove a container's workdir if inadvertently running a command a second time.

@lifubang
fix delete running bundle dir when run t start cmd again
557e8e0 
Signed-off-by: Lifubang <[email protected]>

code optimization after review

Signed-off-by: Lifubang <[email protected]>
@codecov-io
Copy link
Copy Markdown

codecov-io commented Sep 20, 2018

Codecov Report

Merging #2605 into master will increase coverage by 2.46%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2605      +/-   ##
=========================================
+ Coverage   45.04%   47.5%   +2.46%     
=========================================
  Files          92      87       -5     
  Lines       10106    8150    -1956     
=========================================
- Hits         4552    3872     -680     
+ Misses       4834    3556    -1278     
- Partials      720     722       +2
Flag Coverage Δ
#linux 47.5% <ø> (-1.28%) ⬇️
#windows ?
Impacted Files Coverage Δ
oci/spec.go 82.09% <0%> (-11.62%) ⬇️
remotes/docker/auth.go 63.82% <0%> (-3.97%) ⬇️
remotes/docker/status.go 21.42% <0%> (-3.58%) ⬇️
archive/compression/compression.go 50.9% <0%> (-2.43%) ⬇️
platforms/cpuinfo.go 4.65% <0%> (-1.01%) ⬇️
log/context.go 36.84% <0%> (-0.66%) ⬇️
namespaces/context.go 55% <0%> (-0.56%) ⬇️
errdefs/grpc.go 74.6% <0%> (-0.4%) ⬇️
remotes/docker/pusher.go 0% <0%> (ø) ⬆️
content/local/locks.go 100% <0%> (ø) ⬆️
... and 55 more

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 2623241...557e8e0. Read the comment docs.

@lifubang
Copy link
Copy Markdown
Contributor Author

lifubang commented Sep 20, 2018

@estesp @crosbymichael finished. Please check it. Thanks.

estesp
estesp approved these changes Sep 21, 2018
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

@crosbymichael
Copy link
Copy Markdown
Member

crosbymichael commented Sep 21, 2018

LGTM

@crosbymichael crosbymichael added this to the 1.2 milestone Sep 21, 2018
@crosbymichael crosbymichael merged commit 87d1118 into containerd:master Sep 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@crosbymichael crosbymichael crosbymichael left review comments

@estesp estesp estesp approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

1.2

Development

Successfully merging this pull request may close these issues.

4 participants

@lifubang @crosbymichael @estesp @codecov-io