Skip to content
This repository has been archived by the owner on Mar 11, 2020. It is now read-only.

[17.03.3] containerd cherry-picks #2

Merged
merged 2 commits into from
Jun 5, 2017
Merged

[17.03.3] containerd cherry-picks #2

merged 2 commits into from
Jun 5, 2017

Conversation

mlaventure
Copy link

  • Fixes a panic #872
  • Fixes an issue preventing containerd to start if it was killed/crashed while cleaning up a container #828

Deng Guangxing and others added 2 commits May 25, 2017 13:22
@mlaventure
remove container stat dir when no process found in restore
b66ba7a 
Under some abnormal cases, container state dir exist while pid file not.
containerd restore would print this error to log without handling this,
it will set container state to stopped without removing its state dir.

trying to start the container again will cause error:
`mkdir /var/run/docker/libcontainerd/containerd/xxx: file exists`

Signed-off-by: Deng Guangxing <dengguangxing@huawei.com>
(cherry picked from commit 6feebe9)
Signed-off-by: Kenfe-Mickael Laventure <mickael.laventure@gmail.com>
@mlaventure
Fix close a nil chan err
6e5b935 
If docker exec to a container, and then kill the runtime's shim process
it will close a nil chan in Supervisor' execExit interface.

Supervisor.exit
            |---> s.delete
            |         |
            |         V
            |     execMap := s.getDeleteExecSyncMap(t.ID) make getExecSyncChannel return nil
            |
            |--->s.execExit
            |         |
            |         V
            |     synCh := s.getExecSyncChannel(t.ID, t.PID)
            |         |
            |         V
            |     close(synCh) close a nil chan make container panic

Signed-off-by: yangshukui <yangshukui@huawei.com>
(cherry picked from commit de0e943)
Signed-off-by: Kenfe-Mickael Laventure <mickael.laventure@gmail.com>
@mlaventure mlaventure merged commit 6c46389 into docker-archive:docker-1.13.x Jun 5, 2017
@mlaventure mlaventure deleted the fix-panic branch June 5, 2017 20:19
seemethere pushed a commit to seemethere/containerd that referenced this pull request Feb 16, 2018
@kolyshkin
MkdirAll: fix usage
acc6f4e 
(below is a quote from my runc commit 6f82d4b)

TL;DR: check for IsExist(err) after a failed MkdirAll() is both
redundant and wrong -- so two reasons to remove it.

Quoting MkdirAll documentation:

> MkdirAll creates a directory named path, along with any necessary
> parents, and returns nil, or else returns an error. If path
> is already a directory, MkdirAll does nothing and returns nil.

This means two things:

1. If a directory to be created already exists, no error is
returned.

2. If the error returned is IsExist (EEXIST), it means there exists
a non-directory with the same name as MkdirAll need to use for
directory. Example: we want to MkdirAll("a/b"), but file "a"
(or "a/b") already exists, so MkdirAll fails.

The above is a theory, based on quoted documentation and my UNIX
knowledge.

3. In practice, though, current MkdirAll implementation [1] returns
ENOTDIR in most of cases described in docker-archive#2, with the exception when
there is a race between MkdirAll and someone else creating the
last component of MkdirAll argument as a file. In this very case
MkdirAll() will indeed return EEXIST.

Because of docker-archive#1, IsExist check after MkdirAll is not needed.

Because of docker-archive#2 and docker-archive#3, ignoring IsExist error is just plain wrong,
as directory we require is not created. It's cleaner to report
the error now.

Note this error is all over the tree, I guess due to copy-paste,
or trying to follow the same usage pattern as for Mkdir(),
or some not quite correct examples on the Internet.

[1] https://github.com/golang/go/blob/f9ed2f75/src/os/path.go

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@mlaventure