Add WCOW and vSMB functional tests

Update and un-skip WCOW uVM and container tests (and add WCOW uVM
benchmarks), as well as WCOW vSMB and LCOW boto files tests.

Add WCOW host process tests, including dedicated tests for setting
username, and verifying hostname and volume mounts.

Moved:
 - `lcow_bench_test.go` to `uvm_bench_test.go`
 - `lcow_container_test.go` to `container_test.go`
 - `lcow_test.go` to `lcow_uvm_test.go` and `uvm_test.go`

Fix bug where removing a direct-mapped vSMB share failes.

Run (non-virtualization/uVM) functional tests within CI.

Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>

.github/workflows/ci.yml

@@ -314,6 +314,30 @@ jobs:
       - name: Install gotestsum
         run: go install gotest.tools/gotestsum@${{ env.GOTESTSUM_VERSION }}
 
+      # Download PsExec so we can run (functional) tests as 'NT Authority\System'.
+      # Needed for hostprocess tests, as well ensuring backup and restore privileges for
+      # unpacking WCOW images.
+      - name: Install PsExec.exe
+        run: |
+          New-Item -ItemType Directory -Force '${{ github.workspace }}\bin' > $null
+          '${{ github.workspace }}\bin' | Out-File -FilePath $env:GITHUB_PATH -Encoding utf8 -Append
+
+          curl.exe -L --no-progress-meter --fail-with-body -o 'C:\PSTools.zip' `
+            'https://download.sysinternals.com/files/PSTools.zip' 2>&1
+          if ( $LASTEXITCODE ) {
+            Write-Output '::error::Could not download PSTools.zip'
+            exit $LASTEXITCODE
+          }
+
+          tar.exe xf 'C:\PSTools.zip' -C '${{ github.workspace }}\bin' 'PsExec*' 2>&1
+          if ( $LASTEXITCODE ) {
+            Write-Output '::error::Could not extract PsExec.exe'
+            exit $LASTEXITCODE
+          }
+
+          # accept the eula
+          & '${{ github.workspace }}/bin/psexec' -accepteula -nobanner cmd /c "exit 0" 2>$null
+
       # run tests
       - name: Test repo
         run: ${{ env.GOTESTSUM_CMD }} -gcflags=all=-d=checkptr -tags admin -timeout=20m ./...
@@ -343,13 +367,34 @@ jobs:
           ${{ env.GOTESTSUM_CMD_RAW }} ./containerd-shim-runhcs-v1.test.exe '-test.v'
         working-directory: test
 
+      - name: Build and run functional testing binary
+        run: |
+          ${{ env.GO_BUILD_TEST_CMD }} ./functional
+          if ( $LASTEXITCODE ) {
+            Write-Output '::error::Could not build functional.test.exe'
+            exit $LASTEXITCODE
+          }
+
+          # PsExec doesn't load GOBIN into path, so resolve gotestsum path
+          # don't run uVM (ie, nested virt) or LCOW integrity tests
+          $cmd = '${{ env.GOTESTSUM_CMD_RAW }} ./functional.test.exe -exclude="LCOW,LCOWIntegrity,uVM" -test.timeout=1h -test.v'
+          $cmd = $cmd -replace 'gotestsum', ((Get-Command gotestsum)[0].Source)
+          Write-Host "gotestsum command: $cmd"
+
+          # it appears, that in a GH runner, PsExec always runs noninteractively (even with `-i`) and
+          # doesn't capture or redirect std IO.
+          # Instead, write stdout/stderr to a file.
+          psexec -nobanner -w (Get-Location) -s cmd /c "$cmd > out.txt 2>&1"
+          $ec = $LASTEXITCODE
+          Get-Content out.txt
+
+          exit $ec
+        working-directory: test
+
       # build testing binaries
       - name: Build cri-containerd Testing Binary
         run: ${{ env.GO_BUILD_TEST_CMD }} ./cri-containerd
         working-directory: test
-      - name: Build functional Testing Binary
-        run: ${{ env.GO_BUILD_TEST_CMD }} ./functional
-        working-directory: test
       - name: Build runhcs Testing Binary
         run: ${{ env.GO_BUILD_TEST_CMD }} ./runhcs
         working-directory: test

internal/jobcontainers/jobcontainer.go

@@ -30,11 +30,6 @@ import (
 	"golang.org/x/sys/windows"
 )
 
-var (
-	fileBindingSupport   bool
-	checkBindSupportOnce sync.Once
-)
-
 const (
 	// jobContainerNameFmt is the naming format that job objects for job containers will follow.
 	jobContainerNameFmt = "JobContainer_%s"
@@ -181,15 +176,8 @@ func Create(ctx context.Context, id string, s *specs.Spec) (_ cow.Container, _ *
 	// show up at beforehand as you would need to know the containers ID before you launched it. Now that the
 	// rootfs location can be static, a user can easily supply C:\hpc\rest\of\path as their work dir and still
 	// supply anything outside of C:\hpc if they want another location on the host.
-	checkBindSupportOnce.Do(func() {
-		bindDLL := `C:\windows\system32\bindfltapi.dll`
-		if _, err := os.Stat(bindDLL); err == nil {
-			fileBindingSupport = true
-		}
-	})
-
 	var closer resources.ResourceCloser
-	if fileBindingSupport {
+	if FileBindingSupported() {
 		closer, err = container.bindSetup(ctx, s)
 	} else {
 		closer, err = container.fallbackSetup(ctx, s)
@@ -254,7 +242,7 @@ func (c *JobContainer) CreateProcess(ctx context.Context, config interface{}) (_
 		// If the working directory was changed, that means the user supplied %CONTAINER_SANDBOX_MOUNT_POINT%\\my\dir or something similar.
 		// In that case there's nothing left to do, as we don't want to join it with the mount point again.. If it *wasn't* changed, and there's
 		// no bindflt support then we need to join it with the mount point, as it's some normal path.
-		if !changed && !fileBindingSupport {
+		if !changed && !FileBindingSupported() {
 			workDir = filepath.Join(c.rootfsLocation, removeDriveLetter(workDir))
 		}
 	}
@@ -335,7 +323,7 @@ func (c *JobContainer) CreateProcess(ctx context.Context, config interface{}) (_
 	// (cmd in this case) after launch can now see C:\<rootfslocation> as it's in the silo. We could
 	// also add a new mode/flag for the shim where it's just a dummy process launcher, so we can invoke
 	// the shim instead of cmd and have more control over things.
-	if fileBindingSupport {
+	if FileBindingSupported() {
 		commandLine = "cmd /c " + commandLine
 	}
 

internal/jobcontainers/storage.go

@@ -8,22 +8,24 @@ import (
 	"os"
 	"path/filepath"
 
+	specs "github.com/opencontainers/runtime-spec/specs-go"
+	"github.com/pkg/errors"
+
 	"github.com/Microsoft/hcsshim/internal/layers"
 	"github.com/Microsoft/hcsshim/internal/log"
 	"github.com/Microsoft/hcsshim/internal/resources"
+	"github.com/Microsoft/hcsshim/internal/sync"
 	"github.com/Microsoft/hcsshim/internal/wclayer"
-	specs "github.com/opencontainers/runtime-spec/specs-go"
-	"github.com/pkg/errors"
 )
 
 // fallbackRootfsFormat is the fallback location for the rootfs if file binding support isn't available.
 // %s will be expanded with the container ID. Trailing backslash required for SetVolumeMountPoint and
-// DeleteVolumeMountPoint
+// DeleteVolumeMountPoint.
 const fallbackRootfsFormat = `C:\hpc\%s\`
 
 // defaultSiloRootfsLocation is the default location the rootfs for the container will show up
 // inside of a given silo. If bind filter support isn't available the rootfs will be
-// C:\hpc\<containerID>
+// C:\hpc\<containerID>.
 const defaultSiloRootfsLocation = `C:\hpc\`
 
 func (c *JobContainer) mountLayers(ctx context.Context, containerID string, s *specs.Spec, volumeMountPath string) (_ resources.ResourceCloser, err error) {
@@ -72,3 +74,22 @@ func (c *JobContainer) setupRootfsBinding(root, target string) error {
 	}
 	return nil
 }
+
+var fileBindingSupportedOnce = sync.OnceValue(func() (bool, error) {
+	// TODO: use windows.NewLazySystemDLL("bindfltapi.dll").Load() (or windows.LoadLibraryEx directly)
+
+	root := os.Getenv("SystemRoot")
+	if root == "" {
+		root = `C:\windows` // shouldn't really need this fall back, but ...
+	}
+	bindDLL := filepath.Join(root, `system32\bindfltapi.dll`)
+	if _, err := os.Stat(bindDLL); err != nil {
+		return false, err
+	}
+	return true, nil
+})
+
+func FileBindingSupported() bool {
+	b, _ := fileBindingSupportedOnce()
+	return b
+}

internal/uvm/vsmb.go

@@ -4,6 +4,7 @@ package uvm
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -62,7 +63,7 @@ func (uvm *UtilityVM) DefaultVSMBOptions(readOnly bool) *hcsschema.VirtualSmbSha
 }
 
 // findVSMBShare finds a share by `hostPath`. If not found returns `ErrNotAttached`.
-func (uvm *UtilityVM) findVSMBShare(ctx context.Context, m map[string]*VSMBShare, shareKey string) (*VSMBShare, error) {
+func (*UtilityVM) findVSMBShare(_ context.Context, m map[string]*VSMBShare, shareKey string) (*VSMBShare, error) {
 	share, ok := m[shareKey]
 	if !ok {
 		return nil, ErrNotAttached
@@ -129,7 +130,12 @@ func forceNoDirectMap(path string) (bool, error) {
 	var info winapi.FILE_ID_INFO
 	// We check for any error, rather than just ERROR_INVALID_PARAMETER. It seems better to also
 	// fall back if e.g. some other backing filesystem is used which returns a different error.
-	if err := windows.GetFileInformationByHandleEx(h, winapi.FileIdInfo, (*byte)(unsafe.Pointer(&info)), uint32(unsafe.Sizeof(info))); err != nil {
+	if err := windows.GetFileInformationByHandleEx(
+		h,
+		winapi.FileIdInfo,
+		(*byte)(unsafe.Pointer(&info)),
+		uint32(unsafe.Sizeof(info)),
+	); err != nil {
 		return true, nil
 	}
 	return false, nil
@@ -181,7 +187,7 @@ func (uvm *UtilityVM) AddVSMB(ctx context.Context, hostPath string, options *hcs
 	var requestType = guestrequest.RequestTypeUpdate
 	shareKey := getVSMBShareKey(hostPath, options.ReadOnly)
 	share, err := uvm.findVSMBShare(ctx, m, shareKey)
-	if err == ErrNotAttached {
+	if errors.Is(err, ErrNotAttached) {
 		requestType = guestrequest.RequestTypeAdd
 		uvm.vsmbCounter++
 		shareName := "s" + strconv.FormatUint(uvm.vsmbCounter, 16)
@@ -262,6 +268,24 @@ func (uvm *UtilityVM) RemoveVSMB(ctx context.Context, hostPath string, readOnly
 		return nil
 	}
 
+	// Cannot remove a directmapped vSMB share without first closing all open handles to the
+	// share files from inside the the uVM (otherwise, the removal would un-map the files from
+	// the uVM's memory and subsequent access's would fail).
+	// Rather than forgetting about the share on the host side, keep it (with refCount == 0)
+	// in case that directory is re-added back for some reason.
+	//
+	// Note: HCS (vmcompute.exe) issues a remove vSMB request to the guest GCS iff:
+	//  - vmwp.exe direct mapped the vSMB share; and
+	//  - the GCS (on its internal bridge) has the PurgeVSmbCachedHandlesSupported capability.
+	// We do not (currently) have the ability to check for either.
+	if !share.options.NoDirectmap {
+		log.G(ctx).WithFields(logrus.Fields{
+			"name": share.name,
+			"path": hostPath,
+		}).Debug("skipping remove of directmapped vSMB share")
+		return nil
+	}
+
 	modification := &hcsschema.ModifySettingRequest{
 		RequestType:  guestrequest.RequestTypeRemove,
 		Settings:     hcsschema.VirtualSmbShare{Name: share.name},