Prevent invalid errors from terminate

[stevenh]

Both Process.Kill() and Process.Wait() can return errors that don't impact the correct behaviour of terminate.

Instead of letting these get returned and logged, which causes confusion, silently ignore them.

Currently the test needs to be a string test as the errors are private to the runtime packages, so its our only option.

This can be seen if init fails during the setns.

Signed-off-by: Steven Hartland steven.hartland@multiplay.co.uk

[cyphar]

NACK. That's just not a good idea. Can you explain why you need the errors to be filtered? "Just silently ignore them inside a library" smells bad to me.

[stevenh]

We've requested the process be terminated, reporting an error due to the fact that the process had already gone is something the caller simply isn't interested in, as the desired effect has been achieved.

Its analogous to a method to remove a file reporting "file doesn't exist", the caller simply doesn't care.

To bring in some context:

1. When the init process fails to exec execSetns fails.
2. When execSetns fails it calls p.cmd.Wait() so the process has gone before return.
3. newParentProcess returns the error.
4. The cleanup code in container start() ensures everything is cleaned up by calling terminate.
5. terminate returns the bogus error os: process already finished which is then logged.

[cyphar]

We've requested the process be terminated, reporting an error due to the fact that the process had already gone is something the caller simply isn't interested in, as the desired effect has been achieved.

That depends on your perspective, and is not logic that should exist in a library. If you've asked us to destroy a container, under the assumption the container already exists, then pretending as though it already exists can cause logic bugs and other such issues.

Its analogous to a method to remove a file reporting "file doesn't exist", the caller simply doesn't care.

Some callers might care, and that's why unlink(2) (for example) does return -ENOENT in such cases. It's up to the user of the API to make a decision about what they care about -- not the library.

4. The cleanup code in container start() ensures everything is cleaned up by calling terminate.
5. terminate returns the bogus error os: process already finished which is then logged.

Right, so it looks like our code is incorrectly handling this case. The solution is to fix the calling code -- not to filter code inside this code.

[stevenh]

If this was an public method I would totally agree, but as this is private and used solely for error recovery this seemed good.

However you are indeed correct that even though its private additional consumers of terminate could still be added where this behaviour is not desired, so I've moved the check to the consumer instead.

[stevenh]

@cyphar does the move of the test satisfy your requirement?

[stevenh]

@cyphar any update on this?

[cyphar]

I think the point I was trying to make was that the solution was to fix when we call .terminate() internally, such that we won't call .terminate() when unecessary. But looking at it again, I want to see what the other maintainers think. I'll be honest, my main concern is how heavily this depends on the string representation of errors.

/cc @crosbymichael @mrunalp