From 78ad7a2d3ab3964254f4f7bc3da76251d226dd3a Mon Sep 17 00:00:00 2001 From: Gabriel Adrian Samfira Date: Tue, 7 Dec 2021 08:49:41 +0000 Subject: [PATCH] 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 --- .github/workflows/windows-periodic.yml | 1 - script/test/cri-integration.sh | 7 +- script/test/utils.sh | 127 +++++++++++++++++++------ 3 files changed, 104 insertions(+), 31 deletions(-) diff --git a/.github/workflows/windows-periodic.yml b/.github/workflows/windows-periodic.yml index 745bfde86..b2e2f426e 100644 --- a/.github/workflows/windows-periodic.yml +++ b/.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' " diff --git a/script/test/cri-integration.sh b/script/test/cri-integration.sh index cf96b1798..916d8713b 100755 --- a/script/test/cri-integration.sh +++ b/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. -REPORT_DIR=${REPORT_DIR:-"/tmp/test-integration"} +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}" diff --git a/script/test/utils.sh b/script/test/utils.sh index ce6c3bd00..fa5e5b49d 100755 --- a/script/test/utils.sh +++ b/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} < /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 - # start them. For this, we can register containerd as a service, so the LocalSystem will run it - # for us. Additionally, we don't need to worry about keeping it alive, Windows will do it for us. - nssm install containerd-test "$(pwd)/bin/containerd.exe" ${CONTAINERD_FLAGS} \ - --log-file "${report_dir}/containerd.log" + if [ ! -d "${CONTAINERD_ROOT}" ]; then + # Create the containerd ROOT dir and set full access to be inherited for "CREATOR OWNER" + # on all subfolders and files. + mkdir -p "${CONTAINERD_ROOT}" + 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 "${sudo} ${crictl_path} --runtime-endpoint=${CONTAINERD_SOCK} info" + readiness_check run_ctr + readiness_check run_crictl } # test_teardown kills containerd. test_teardown() { if [ -n "${pid}" ]; then if [ $IS_WINDOWS -eq 1 ]; then - nssm stop containerd-test - nssm remove containerd-test confirm + # Mark service for deletion. It will be deleted as soon as the service stops. + 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() { @@ -158,7 +229,7 @@ readiness_check() { local command=$1 local MAX_ATTEMPTS=5 local attempt_num=1 - until ${command} &> /dev/null || (( attempt_num == MAX_ATTEMPTS )) + until ${command} &>/dev/null || (( attempt_num == MAX_ATTEMPTS )) do echo "$attempt_num attempt \"$command\"! Trying again in $attempt_num seconds..." sleep $(( attempt_num++ ))