cri-integration: Add Windows default paths

This change adds the following:

  * Default paths to REPORT_DIR, CONTAINERD_STATE and
CONTAINERD_ROOT for Windows
  * Removes the need for nssm on Windows. The nssm service
has issues dealing with paths that contain spaces. Also, the
containerd binary is perfectly capable of registering itself
as a service in Windows, and Windows itself can take care of
any failure handling of the service. NSSM is useful for binaries
that do not have any kind of Windows service logic built into
them. That is not the case of containerd.
  * Use wrapper functions that run containerd, ctr and criclt
with properly quoted paths to pipes, sockets, state and root dirs.
Currently, if the state and root dirs contain spaces in them, the
command line flags on both Windows and Linux are not properly set.
The wrapper functions will allow us to use the readiness_check
and keepalive functions to retry the commands, while properly
quoting the paths and avoiding eval.

Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>

.github/workflows/windows-periodic.yml

@@ -162,7 +162,6 @@ jobs:
              ./script/setup/install-cni-windows
              mkdir /c/tmp
              export TEST_IMAGE_LIST=c:/repolist.toml
-             export CONTAINERD_ROOT=/c/ProgramData/containerd/root
              make cri-integration | tee c:/Logs/cri-integration.log
            EOF
            ssh -i $HOME/.ssh/id_rsa ${{ env.SSH_OPTS }} azureuser@${{ env.VM_PUB_IP }} "sh.exe -c 'cat /c/Logs/cri-integration.log | go-junit-report.exe > c:/Logs/junit_01.xml' "

script/test/cri-integration.sh

@@ -29,12 +29,15 @@ cd "${ROOT}"
 # FOCUS focuses the test to run.
 FOCUS=${FOCUS:-""}
 # REPORT_DIR is the the directory to store test logs.
+if [ $IS_WINDOWS -eq 0 ]; then
   REPORT_DIR=${REPORT_DIR:-"/tmp/test-integration"}
+else
+  REPORT_DIR=${REPORT_DIR:-"C:/Windows/Temp/test-integration"}
+fi
 # RUNTIME is the runtime handler to use in the test.
 RUNTIME=${RUNTIME:-""}
 
 CRI_ROOT="${CONTAINERD_ROOT}/io.containerd.grpc.v1.cri"
-
 mkdir -p "${REPORT_DIR}"
 test_setup "${REPORT_DIR}"
 

script/test/utils.sh

@@ -21,15 +21,19 @@ fi
 
 # RESTART_WAIT_PERIOD is the period to wait before restarting containerd.
 RESTART_WAIT_PERIOD=${RESTART_WAIT_PERIOD:-10}
-# CONTAINERD_FLAGS contains all containerd flags.
+
-CONTAINERD_FLAGS="--log-level=debug "
+if [ $IS_WINDOWS -eq 0 ]; then
+  CONTAINERD_CONFIG_DIR=${CONTAINERD_CONFIG_DIR:-"/tmp"}
+else
+  CONTAINERD_CONFIG_DIR=${CONTAINERD_CONFIG_DIR:-"c:/Windows/Temp"}
+fi
 
 # Use a configuration file for containerd.
 CONTAINERD_CONFIG_FILE=${CONTAINERD_CONFIG_FILE:-""}
 # The runtime to use (ignored when CONTAINERD_CONFIG_FILE is set)
 CONTAINERD_RUNTIME=${CONTAINERD_RUNTIME:-""}
 if [ -z "${CONTAINERD_CONFIG_FILE}" ]; then
-  config_file="/tmp/containerd-config-cri.toml"
+  config_file="${CONTAINERD_CONFIG_DIR}/containerd-config-cri.toml"
   truncate --size 0 "${config_file}"
   if command -v sestatus >/dev/null 2>&1; then
     cat >>${config_file} <<EOF
@@ -50,16 +54,31 @@ fi
 # CONTAINERD_TEST_SUFFIX is the suffix appended to the root/state directory used
 # by test containerd.
 CONTAINERD_TEST_SUFFIX=${CONTAINERD_TEST_SUFFIX:-"-test"}
+if [ $IS_WINDOWS -eq 0 ]; then
   # The containerd root directory.
   CONTAINERD_ROOT=${CONTAINERD_ROOT:-"/var/lib/containerd${CONTAINERD_TEST_SUFFIX}"}
   # The containerd state directory.
   CONTAINERD_STATE=${CONTAINERD_STATE:-"/run/containerd${CONTAINERD_TEST_SUFFIX}"}
   # The containerd socket address.
-if [ $IS_WINDOWS -eq 0 ]; then
   CONTAINERD_SOCK=${CONTAINERD_SOCK:-unix://${CONTAINERD_STATE}/containerd.sock}
   TRIMMED_CONTAINERD_SOCK="${CONTAINERD_SOCK#unix://}"
 else
-  CONTAINERD_SOCK=${CONTAINERD_SOCK:-npipe://./pipe/${CONTAINERD_STATE}/containerd}
+  # $ProgramData holds the Windows path to the ProgramData folder in standard Windows
+  # format. The backslash in the path may be interpreted by bash, so we convert the
+  # Windows path to POSIX path using cygpath.exe. The end result should be something
+  # similar to /c/ProgramData/.
+  POSIX_PROGRAM_DATA="$(cygpath.exe $ProgramData)"
+
+  CONTAINERD_ROOT=${CONTAINERD_ROOT:-"$POSIX_PROGRAM_DATA/containerd/root${CONTAINERD_TEST_SUFFIX}"}
+  CONTAINERD_STATE=${CONTAINERD_STATE:-"$POSIX_PROGRAM_DATA/containerd/state${CONTAINERD_TEST_SUFFIX}"}
+
+  # Remove drive letter
+  PIPE_STATE="${CONTAINERD_STATE#*:/}"
+  # Remove leading slash
+  PIPE_STATE="${PIPE_STATE#/}"
+  # Replace empty space with dash
+  PIPE_STATE="${PIPE_STATE// /"-"}"
+  CONTAINERD_SOCK=${CONTAINERD_SOCK:-npipe://./pipe/${PIPE_STATE}/containerd}
   TRIMMED_CONTAINERD_SOCK="${CONTAINERD_SOCK#npipe:}"
 fi
 
@@ -69,13 +88,6 @@ if [ $IS_WINDOWS -eq 1 ]; then
   EXE_SUFFIX=".exe"
 fi
 CONTAINERD_BIN=${CONTAINERD_BIN:-"containerd"}${EXE_SUFFIX}
-if [ -f "${CONTAINERD_CONFIG_FILE}" ]; then
-  CONTAINERD_FLAGS+="--config ${CONTAINERD_CONFIG_FILE} "
-fi
-
-CONTAINERD_FLAGS+="--address ${TRIMMED_CONTAINERD_SOCK} \
-  --state ${CONTAINERD_STATE} \
-  --root ${CONTAINERD_ROOT}"
 
 pid=
 
@@ -85,6 +97,47 @@ if [ "$(id -u)" -ne 0 ] && command -v sudo &> /dev/null; then
   sudo="sudo PATH=${PATH}"
 fi
 
+
+# The run_containerd function is a wrapper that will run the appropriate
+# containerd command based on the OS we're running the tests on. This wrapper
+# is needed if we plan to run the containerd command as part of a retry cycle
+# as is the case on Linux, where we use the keepalive function. Using a wrapper
+# allows us to avoid the need for eval, while allowing us to quote the paths
+# to the state and root folders. This allows us to use paths that have spaces
+# in them without erring out.
+run_containerd() {
+  # not used on linux
+  if [ $# -gt 0 ]; then
+    local report_dir=$1
+  fi
+  CMD=""
+  if [ ! -z "${sudo}" ]; then
+    CMD+="${sudo} "
+  fi
+  CMD+="${PWD}/bin/containerd"
+
+  if [ $IS_WINDOWS -eq 0 ]; then
+    $CMD --log-level=debug \
+      --config "${CONTAINERD_CONFIG_FILE}" \
+      --address "${TRIMMED_CONTAINERD_SOCK}" \
+      --state "${CONTAINERD_STATE}" \
+      --root "${CONTAINERD_ROOT}"
+  else
+    # Note(gsamfira): On Windows, we register a containerd-test service which will run under
+    # LocalSystem. This user is part of the local Administrators group and should have all
+    # required permissions to successfully start containers.
+    # The --register-service parameter will do this for us.
+    $CMD --log-level=debug \
+      --config "${CONTAINERD_CONFIG_FILE}" \
+      --address "${TRIMMED_CONTAINERD_SOCK}" \
+      --state "${CONTAINERD_STATE}" \
+      --root "${CONTAINERD_ROOT}" \
+      --log-file "${report_dir}/containerd.log" \
+      --service-name containerd-test \
+      --register-service
+  fi
+}
+
 # test_setup starts containerd.
 test_setup() {
   local report_dir=$1
@@ -97,18 +150,26 @@ test_setup() {
   # Create containerd in a different process group
   # so that we can easily clean them up.
   if [ $IS_WINDOWS -eq 0 ]; then
-    keepalive "${sudo} bin/containerd ${CONTAINERD_FLAGS}" \
+    keepalive run_containerd \
       "${RESTART_WAIT_PERIOD}" &> "${report_dir}/containerd.log" &
     pid=$!
   else
-    # NOTE(claudiub): For Windows HostProcess containers, containerd needs to be privileged enough to
+    if [ ! -d "${CONTAINERD_ROOT}" ]; then
-    # start them. For this, we can register containerd as a service, so the LocalSystem will run it
+      # Create the containerd ROOT dir and set full access to be inherited for "CREATOR OWNER"
-    # for us. Additionally, we don't need to worry about keeping it alive, Windows will do it for us.
+      # on all subfolders and files.
-    nssm install containerd-test "$(pwd)/bin/containerd.exe" ${CONTAINERD_FLAGS} \
+      mkdir -p "${CONTAINERD_ROOT}"
-      --log-file "${report_dir}/containerd.log"
+      cmd.exe /c 'icacls.exe "'$(cygpath -w "${CONTAINERD_ROOT}")'" /grant "CREATOR OWNER":(OI)(CI)(IO)F /T'
+    fi
+
+    run_containerd "$report_dir"
+
+    # Set failure flag on the test service. This will restart the service
+    # in case of failure.
+    sc.exe failure containerd-test reset=0 actions=restart/1000
+    sc.exe failureflag containerd-test 1
 
     # it might still result in SERVICE_START_PENDING, but we can ignore it.
-    nssm start containerd-test || true
+    sc.exe start containerd-test || true
     pid="1"  # for teardown
   fi
   set +m
@@ -120,16 +181,18 @@ test_setup() {
     echo "crictl is not in PATH"
     exit 1
   fi
-  readiness_check "${sudo} bin/ctr --address ${TRIMMED_CONTAINERD_SOCK} version"
+  readiness_check run_ctr
-  readiness_check "${sudo} ${crictl_path} --runtime-endpoint=${CONTAINERD_SOCK} info"
+  readiness_check run_crictl
 }
 
 # test_teardown kills containerd.
 test_teardown() {
   if [ -n "${pid}" ]; then
     if [ $IS_WINDOWS -eq 1 ]; then
-      nssm stop containerd-test
+      # Mark service for deletion. It will be deleted as soon as the service stops.
-      nssm remove containerd-test confirm
+      sc.exe delete containerd-test
+      # Stop the service
+      sc.exe stop containerd-test || true
     else
       pgid=$(ps -o pgid= -p "${pid}" || true)
       if [ ! -z "${pgid}" ]; then
@@ -141,6 +204,14 @@ test_teardown() {
   fi
 }
 
+run_ctr() {
+  ${sudo} ${PWD}/bin/ctr --address "${TRIMMED_CONTAINERD_SOCK}" version
+}
+
+run_crictl() {
+  ${sudo} ${crictl_path} --runtime-endpoint="${CONTAINERD_SOCK}" info
+}
+
 # keepalive runs a command and keeps it alive.
 # keepalive process is eventually killed in test_teardown.
 keepalive() {