Maybe usec.signal here as we made sure that we have private pidns.

Maybe don't ignore error, because otherwise you might be waiting for 10 seconds for something that will never happen.

Or, you can be explicit and usePidfdSendSignal here to be 100% sure that it succeeded.

Can you add a wrapper in libcontainer/internal/linux for unix.EpollWait? Using the retryOnEINTR helper, so we don't have to do it here.

I'll rebase to add this based on#4697.

we need a linter to suggest automatically to use the single-line if here:if err := ...; err == nil {...

It can't use a single line to check the err, because we have to log the err and go to next.

But you can check if no errors, IOWerr==nil. It's exactly the same

What am I missing?

We need to print this error in the next line, but anyway, I have declared the err as a block scope var now.

That is okay. I think using an else is even simpler. Or something like:

iferr:=c.Kill...();err!=nil {logrus.Debugf("pidfd....")}else {returnnil}

I think doing epoll with only one fd is kind of overkill, I wonder if there are simpler solutions. But if, as I mentioned in another comment, we can kill all the processes in the cgroup, then the epoll might be worth it?

Is there a case where n < 0? I mean, do we need this if here?

I'll refactor to cover this condition.

I know the old code was killing only the init process. But would it make sense to kill all the processes in the cgroup instead?

We can do it as another PR, if that makes sense.

As you mentioned in the above, we can consider this order:

1. Use cgroup.kill if the kernel supports it
2. Use pidfd if the kernel supports it
3. Just send a signal otherwise

But I think we still need to consider whether the container has a private pid ns or not.
What I think it's that, it's reasonable to kill only the init process for a private pid ns container.

Taking into account this:#4517 (comment).

It seems simpler to kill pid1 if it has it's own pidns. Otherwise, cgroup.kill or, if not supported, send a signal.

Does it make sense?

I don't think it's no need to usecgroup.kill to kill all process in the cgroup to kill the pid1 if it has a private own pidns. We just only need to kill the exact pid1 process.

Exactly what I said in the last comment :)

s/doesn't/does/?

no

Opening again, sorry, but the if checks that it does have a private PID namespace. The comment says it doesn't. Something seems odd. Am I missing something?

Taking into account this:#4517 (comment).

It seems simpler to kill pid1 if it has it's own pidns. Otherwise, cgroup.kill or, if not supported, send a signal.

Does it make sense?

I can't wrap my head around this. Before thisPR on force, we were usingcontainer.Signal(). Now, if a container is stopped and doesn't have a private pidns, we don't do nothing here, and at the end we docontainer.Destroy().

The commit msg doesn't mention anything about this. Am I missing something?

Yes, it's a mistake, removed now. Thanks.

epoll_wait returns -1 on error.Let's return -1 here too.

Opening again, sorry, but the if checks that it does have a private PID namespace. The comment says it doesn't. Something seems odd. Am I missing something?

I can't wrap my head around this either. Why don't we use a pidfd inc.Signal() (if that is available, if it's not we fallback to sending a signal to the pid number) and create a new function to wait on the process to die? Again, if that if pidfd is possible, we wait on that, if it's not, we fallback to wait as we do now.

Having a (public/exported)Kill() andKillViaPidfd() seems like something that should be abstracted.

What we are doing now seems kind of complex:

• If it has a pidns, then try to kill with a pidfd the pid1
• If it doesn't have a pidns, then try to kill the process sending a signal to the pid number (even if pidfd is supported, why?). The function we call here also handles the case of having a private pidns, which makes this more tricky.

If what I propose doesn't seem okay, I'm open to other ways to simplify this :)