Merge pull request #6347 from gabriel-samfira/fix-windows-test-setup

cri-integration: Add Windows defaults and fix spaces issue

.github/workflows/windows-periodic.yml

@@ -158,7 +158,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() {