Fix mem leak in newHcsTask()

[kiashok]

newHcsTask() starts a goroutine for hyperv containers to enable hcsTask and hcsExec
resource cleanup as expected. But when containers exit before the uvm in the normal
case, this goroutine is leaked as it is waiting on a block wait on the UVM.
This commit ensures that we wait for container exit as well so that we do not leak the routine.

Testing/Validation:

• Validated via debugger that we are terminating the goroutine and also freeing
  up resources.
• Tested with multiple container recreates on the same UVM.

Note: This is an interim fix. The cleaner approach needs more code changes in well known code paths and more validation to ensure we are not breaking any existing flow. Since timeline to get this fix in is sensitive, proposing this fix. We will be revisiting this again.

[helsaawy]

if feels weird to couple the two together, since this duplicates a lot of logic thats in WaitCtx, and this is a very specialized case of waiting on two different channels with a context timeout.

maybe you can add Done() and WaitError functions to the uVM (similar to what we do for containers) that abstracts away the wait logic.

That way waitForHostOrContainerExit can select on them, and we can avoid the current differentiation in the logic if the timeout happens during the wait on the compute system or the output processing

//
// internal\uvm\types.go
//

type UtilityVM struct {
    // ...

	waitErr error
	waitCh  chan struct{}

	exitErr error
	exitCh  chan struct{}

    // ...
}

//
// internal\uvm\start.go
//

func (uvm *UtilityVM) Start(ctx context.Context) (err error) {
    // ...

	// Start waiting on the utility VM.
	go func() {
		// the original context may have timeout or propagate a cancellation
		// copy the original to prevent it affecting the background wait go routine
		cCtx := context.WithoutCancel(pCtx)
		uvm.waitErr := uvm.hcsSystem.WaitCtx(cCtx)

		if uvm.waitErr == nil {
			uvm.exitErr = uvm.hcsSystem.ExitError()
		} else {
			uvm.exitErr = uvm.waitErr
		}
		close(uvm.exitCh)

		if uvm.outputProcessingDone != nil {
			logrus.WithField(logfields.UVMID, uvm.id).Debug("uvm exited, waiting for output processing to complete")
			<-uvm.outputProcessingDone:
		}
		close(uvm.waitCh)
	}()

    // ...
}

//
// internal\uvm\wait.go
//

func (uvm *UtilityVM) Done() <-chan struct{} { return uvm.waitCh }

func (uvm *UtilityVM) WaitError() { return uvm.waitErr }

func (uvm *UtilityVM) WaitCtx(ctx context.Context) (err error) {
	select {
		case <-uvm.waitCh:
			err = uvm.waitErr
		case <-ctx.Done():
			err = fmt.Errorf("failed to wait on uvm: %w", ctx.Err())
		}
	}

	return err
}

//
// cmd/containerd-shim-runhcs-v1/task_hcs.go
//

func (ht *hcsTask) waitForHostOrContainerExit() {
	// ...

	var err error
	select {
	case <- ht.host.Done():
		err = ht.host.WaitError()
	case <- ht.c.WaitChannel()
		err = ht.c.WaitError()
	}

	// ...
}

[kiashok]

This is the long term fix in mind. There are callers to uvm.Wait() and uvm.WaitCtx() that might be expecting outputProcessDone (that is IO to complete along with hcsSystem termination too) and we need to account for them as well before removing from uvm.WaitCtx(). Additionally, hcsshim lifecycle management could be looked into and optimized to make things easier as well. This needs additional validation and careful consideration. Since there is an urgency to fix this leak, wanted to lean more towards a quicker least risky fix to ensure no additional regressions and revisit on a cleaner long term fix.

[ambarve]

Can we also add (or update) at least two functional tests that start a container in the UVM and exits cleanly and the other crashes the UVM? I think we could use something like https://github.com/uber-go/goleak to verify if we are leaking any goroutines.

[kiashok]

We will be revisiting this leak with the longer time fix as this affects only hyperv wcow/lcow and it is not being used in upstream currently.