Skip to content
This repository was archived by the owner on May 12, 2021. It is now read-only.

shimv2: update container status when process exited#1625

Closed
lifupan wants to merge 1 commit intokata-containers:masterfrom
lifupan:fixstop
Closed

shimv2: update container status when process exited#1625
lifupan wants to merge 1 commit intokata-containers:masterfrom
lifupan:fixstop

Conversation

@lifupan
Copy link
Member

@lifupan lifupan commented May 5, 2019

When a container process terminated, shimv2 should update
container's status internally, otherwise, ctr command would
get a wrong container status even the process had exited.

Fixes:#1602

Signed-off-by: lifupan lifupan@gmail.com

@lifupan
Copy link
Member Author

lifupan commented May 5, 2019

/test

When a container process terminated, shimv2 should update
container's status internally, otherwise, ctr command would
get a wrong container status even the process had exited.

Fixes:kata-containers#1602

Signed-off-by: lifupan <lifupan@gmail.com>
@lifupan
Copy link
Member Author

lifupan commented May 5, 2019

/test

}

// If the container has stopped, return directly when killed with
// SIGKILL or SIGTERM signal.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we still send other signal except SIGKILL and SIGTERM when container is stop

Copy link
Member Author

Choose a reason for hiding this comment

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

We cannot make sure when/what signals containerd would send, here only filter out the sigterm/sigkill signal.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the signal should be send to container, if the container init process are stopped, it won't deal the signal, I see kata agent will discard signal if container stop, so it also ok here.
https://github.com/kata-containers/agent/blob/master/grpc.go#L908

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually if the container has been stopped, the signal hadn't reached agent and would returned here

func (c *Container) signalProcess(processID string, signal syscall.Signal, all bool) error {
.

Here the filter would make sure even the container has been stopped, send SIGKILL/SIGTERM signal wouldn't return an err.

if err != nil {
return nil, errors.Wrap(err, "kill container")
}
if status.State.State == types.StateStopped {
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to fake success here? runc shim doesn't do it either. It feels like container state between containerd and kata shimv2 mismatched.

@lifupan
Copy link
Member Author

lifupan commented May 22, 2019

Close this PR since another one #1723 would fix the same issue.

@lifupan lifupan closed this May 22, 2019
@lifupan lifupan deleted the fixstop branch May 22, 2019 04:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

some question about kata shim v1/v2

3 participants