Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 1 commit into from
Sep 21, 2018
Merged

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

merged 1 commit into from
Sep 21, 2018

Conversation

lifubang
Copy link
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
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
Copy link
Member

@lifubang see #2575

lifubang
lifubang commented Sep 5, 2018
View reviewed changes
Copy link
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
Contributor Author

@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
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

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
Contributor Author

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

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

@crosbymichael
Copy link
Member

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