[test] Update WCOW uVM and vSMB, and HostProcess functional tests (#1965

) * Initial file reorg & rename WCOW tests can be integrated directly into existing LCOW tests as subtests, after generalizing the original (LCOW-only) tests to run both types of uVMs and containers. Break the change into two: (1) move (and rename) the original LCOW-only tests; and (2) generalize the tests and add the WCOW components. To simplify the diffs, this commit only includes the first process. Specifically, move: - `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` Within `lcow_uvm_test.go`, combine and generalize kernel arg tests (i.e., `TestLCOW_UVMNoSCSINoVPMemInitrd` and `TestLCOW_UVMNoSCSISingleVPMemVHD`) to `TestLCOW_UVM_KernelArgs`. Combine and generalize boot/time tests (e.g., `TestLCOW_TimeUVMStartVHD`, `TestLCOW_UVMStart_KernelDirect_VHD`) to `TestLCOW_UVM_Boot`. Also, since go1.21, `"github.com/Microsoft/hcsshim/internal/sync"` is no longer necessary, so replace it with `"sync".OnceValue[s]`. Signed-off-by: Hamza El-Saawy <[email protected]> * Export `FileBindingSupported` function Expose `FileBindingSupported()` function from `"internal\jobcontainers"` so it can be used in functional testing code. Switch from `sync.Once` to checking for `bindfltapi.dll` during package init, since the check is (relatively) cheap. Signed-off-by: Hamza El-Saawy <[email protected]> * Add WCOW and vSMB functional tests Un-skip and fix WCOW uVM and container tests. Add WCOW: - uVM benchmarks - vSMB tests - Host Process tests For WCOW host process tests, add dedicated tests for setting username, and verifying hostname and volume mounts. Fix bug where removing a direct-mapped vSMB share fails. Run (non-virtualization/uVM) functional tests within CI. Starting Host Process containers requires SYSTEM to create a process with a specified token, so use PsExec.exe (from sysutils) to run tests. Make sure container specs are created with the default working directory (`C:\`), similar to how `internal\cmd` works). Signed-off-by: Hamza El-Saawy <[email protected]> --------- Signed-off-by: Hamza El-Saawy <[email protected]>
microsoft · May 28, 2024 · 32498a7 · 32498a7
1 parent 43d1ab5
commit 32498a7
Show file tree

Hide file tree

Showing 28 changed files with 2,397 additions and 784 deletions.
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -6,7 +6,7 @@ on:
 env:
   GO_VERSION: "oldstable"
 
-  GO_BUILD_CMD: "go build \"-ldflags=-s -w\" -trimpath"
+  GO_BUILD_CMD: 'go build "-ldflags=-s -w" -trimpath'
   GO_BUILD_TEST_CMD: "go test -mod=mod -gcflags=all=-d=checkptr -c -tags functional"
 
   GOTESTSUM_VERSION: "latest"
@@ -325,6 +325,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 ./...
@@ -354,13 +378,42 @@ 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
+          $gotestsum = Get-Command -Name 'gotestsum' -CommandType Application -ErrorAction 'Stop' |
+            Select-Object -First 1 -ExpandProperty Source
+          if ( [string]::IsNullOrEmpty($gotestsum) ) {
+            Write-Output '::error::could not find 'gotestsum.exe' path'
+            exit $LASTEXITCODE
+          }
+
+          # 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 -log-level=info'
+          $cmd = $cmd -replace 'gotestsum', $gotestsum
+          Write-Host "gotestsum command: $cmd"
+
+          # Apparently, in a GH runner, PsExec always runs noninteractively (even with `-i`)
+          # and doesn't capture or redirect std IO.
+          # So redirect 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

diff --git a/internal/jobcontainers/jobcontainer.go b/internal/jobcontainers/jobcontainer.go
@@ -31,11 +31,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"
@@ -186,15 +181,8 @@ func Create(ctx context.Context, id string, s *specs.Spec, createOpts CreateOpti
 	// 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, createOpts)
 	} else {
 		closer, err = container.fallbackSetup(ctx, s, createOpts)
@@ -259,7 +247,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))
 		}
 	}
@@ -340,7 +328,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
 	}
 

diff --git a/internal/jobcontainers/storage.go b/internal/jobcontainers/storage.go
@@ -5,24 +5,33 @@ package jobcontainers
 import (
 	"context"
 	"fmt"
+	"os"
+	"path/filepath"
+
+	specs "github.com/opencontainers/runtime-spec/specs-go"
 
 	"github.com/Microsoft/hcsshim/internal/layers"
 	"github.com/Microsoft/hcsshim/internal/log"
 	"github.com/Microsoft/hcsshim/internal/resources"
-	specs "github.com/opencontainers/runtime-spec/specs-go"
 )
 
 // 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, wl layers.WCOWLayers, volumeMountPath string) (_ resources.ResourceCloser, err error) {
+func (*JobContainer) mountLayers(
+	ctx context.Context,
+	containerID string,
+	s *specs.Spec,
+	wl layers.WCOWLayers,
+	volumeMountPath string,
+) (_ resources.ResourceCloser, err error) {
 	if s.Root == nil {
 		s.Root = &specs.Root{}
 	}
@@ -75,3 +84,18 @@ func (c *JobContainer) setupRootfsBinding(root, target string) error {
 	}
 	return nil
 }
+
+var fileBindingSupported = func() bool {
+	// 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`)
+	st, err := os.Stat(bindDLL)
+	return err == nil && st.Mode().IsRegular()
+}()
+
+// FileBindingSupported returns whether the bind filter is available.
+func FileBindingSupported() bool { return fileBindingSupported }
diff --git a/internal/sync/once.go b/internal/sync/once.go
diff --git a/internal/uvm/vsmb.go b/internal/uvm/vsmb.go
@@ -133,7 +133,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
@@ -282,6 +287,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},