Ignore 'wait: no child processes' error when calling mount/umount

[shyamjvs]

Fixes #103753

I've only fixed the exec commands that are part of Mount() and Unmount() functions and that too in the linux mount helper. Not touching others, since I'm not sure about the implications. Let me know if I should.

/kind bug
/sig storage
/sig scalability

/assign @liggitt

NONE

[jingxu97]

How we make sure this unmount must be successful at this point?

[shyamjvs]

Good catch.

So exec:CombinedOutput() essentially runs exec:Run() which does this:

func (c *Cmd) Run() error {
	if err := c.Start(); err != nil {
		return err
	}
	return c.Wait()
}

This error can only come from c.Wait() which mean c.Start() went through without any errors. Within c.Wait(), we run the following code:

func (c *Cmd) Wait() error {
	if c.Process == nil {
		return errors.New("exec: not started")
	}
	if c.finished {
		return errors.New("exec: Wait was already called")
	}
	c.finished = true

	state, err := c.Process.Wait()
	if c.waitDone != nil {
		close(c.waitDone)
	}
	c.ProcessState = state

	var copyError error
	for range c.goroutine {
		if err := <-c.errch; err != nil && copyError == nil {
			copyError = err
		}
	}

	c.closeDescriptors(c.closeAfterWait)

	if err != nil {
		return err
	} else if !state.Success() {
		return &ExitError{ProcessState: state}
	}

	return copyError
}

And this errNoChildProcesses (i.e ECHILD error) comes specifically from this line state, err := c.Process.Wait() which internally makes an os syscall. But we seem to be returning that error before returning the error for the process exit state:

	if err != nil {
		return err
	} else if !state.Success() {
		return &ExitError{ProcessState: state}
	}

So additionally if we check that c.ProcessState is a success, then we should be good. Like this:

if err.Error() == errNoChildProcesses && command.ProcessState.Success() {
      // We don't consider this an error (see - k/k issue #103753)
      return nil
}

Does the above sgty?

[shyamjvs]

I've updated it to a slightly better version actually to help expose the actual process error when we get errNoChildProcesses:

		if err.Error() == errNoChildProcesses {
			if command.ProcessState.Success() {
				// We don't consider errNoChildProcesses an error if the process itself succeeded (see - k/k issue #103753).
				return nil
			}
			// Rewrite err with the actual exit error of the process.
			err = &exec.ExitError{ProcessState: command.ProcessState}
		}

[liggitt]

/unassign
/assign @jsafrane @msau42
for determination whether this is fixing a release-blocking issue

[msau42]

From the issue description, the impact appears to be that mount/unmount could take up to 1s longer on heavily loaded nodes. I believe this exec race has also existed for many releases so is not considered a regression. @shyamjvs please correct me if I am mistaken.

For now, we can target 1.23 for the fix.

/triage accepted
/milestone v1.23

[shyamjvs]

@msau42 Yes, that's right.

For now, we can target 1.23 for the fix.

Sounds good

[jingxu97]

are you able to test it with this scenario?

[shyamjvs]

I observed this on a customer prod cluster and couldn't reproduce it on my end so far. Seems to depend on the volume usage pattern and pod churn on their nodes.

[gnufied]

btw - rather than comparing strings I wonder if we could have unwrapped the error and looked for syscall.ECHILD error specifically?

[jingxu97]

/lgtm

[jsafrane]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane, shyamjvs

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• staging/src/k8s.io/mount-utils/OWNERS [jsafrane]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment