Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Windows integration tests #4304

Merged
merged 20 commits into from
Sep 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
//go:build integration
// +build integration
//go:build unix && integration
// +build unix,integration

// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved.
//
Expand Down
31 changes: 31 additions & 0 deletions agent/engine/common_integ_testutil.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"context"
"fmt"
"os"
"runtime"
"testing"
"time"

Expand Down Expand Up @@ -143,13 +144,25 @@ func loggerConfigIntegrationTest(logfile string) string {
}

func VerifyContainerManifestPulledStateChange(t *testing.T, taskEngine TaskEngine) {
// Skip assertions on Windows because we don't run a local registry,
// so images are always cached and never pulled.
if runtime.GOOS == "windows" {
t.Log("Not expecting image manifest pulls on Windows")
return
}
stateChangeEvents := taskEngine.StateChangeEvents()
event := <-stateChangeEvents
assert.Equal(t, apicontainerstatus.ContainerManifestPulled, event.(api.ContainerStateChange).Status,
"Expected container to be at MANIFEST_PULLED state")
}

func VerifyTaskManifestPulledStateChange(t *testing.T, taskEngine TaskEngine) {
// Skip assertions on Windows because we don't run a local registry,
// so images are always cached and never pulled.
if runtime.GOOS == "windows" {
t.Log("Not expecting image manifest pulls on Windows")
return
}
stateChangeEvents := taskEngine.StateChangeEvents()
event := <-stateChangeEvents
assert.Equal(t, apitaskstatus.TaskManifestPulled, event.(api.TaskStateChange).Status,
Expand Down Expand Up @@ -222,6 +235,12 @@ func verifyContainerStoppedStateChangeWithRuntimeID(t *testing.T, taskEngine Tas
// has a specific status (identified by the containerStatus parameter)
func verifySpecificContainerStateChange(t *testing.T, taskEngine TaskEngine, containerName string,
containerStatus apicontainerstatus.ContainerStatus) {
// Skip assertions on Windows because we don't run a local registry,
// so images are always cached and never pulled.
if runtime.GOOS == "windows" && containerStatus == apicontainerstatus.ContainerManifestPulled {
t.Log("Not expecting image manifest pulls on Windows")
return
}
stateChangeEvents := taskEngine.StateChangeEvents()
event := <-stateChangeEvents
assert.Equal(t, event.(api.ContainerStateChange).ContainerName, containerName)
Expand Down Expand Up @@ -343,6 +362,12 @@ func InitTestEventCollection(taskEngine TaskEngine) *TestEvents {
// This method queries the TestEvents struct to check a Task Status.
// This method will block if there are no more stateChangeEvents from the DockerTaskEngine but is expected
func VerifyTaskStatus(status apitaskstatus.TaskStatus, taskARN string, testEvents *TestEvents, t *testing.T) {
// Skip assertions on Windows because we don't run a local registry,
// so images are always cached and never pulled.
if runtime.GOOS == "windows" && status == apitaskstatus.TaskManifestPulled {
t.Log("Not expecting image manifest pulls on Windows")
return
}
for {
if _, found := testEvents.RecordedEvents[statechange.TaskEvent][status.String()][taskARN]; found {
return
Expand All @@ -355,6 +380,12 @@ func VerifyTaskStatus(status apitaskstatus.TaskStatus, taskARN string, testEvent
// This method queries the TestEvents struct to check a Task Status.
// This method will block if there are no more stateChangeEvents from the DockerTaskEngine but is expected
func VerifyContainerStatus(status apicontainerstatus.ContainerStatus, ARNcontName string, testEvents *TestEvents, t *testing.T) {
// Skip assertions on Windows because we don't run a local registry,
// so images are always cached and never pulled.
if runtime.GOOS == "windows" && status == apicontainerstatus.ContainerManifestPulled {
t.Log("Not expecting image manifest pulls on Windows")
amogh09 marked this conversation as resolved.
Show resolved Hide resolved
return
}
for {
if _, found := testEvents.RecordedEvents[statechange.ContainerEvent][status.String()][ARNcontName]; found {
return
Expand Down
51 changes: 51 additions & 0 deletions agent/engine/common_windows_integ_testutil.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
//go:build windows && (sudo || integration)
// +build windows
// +build sudo integration

// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License"). You may
// not use this file except in compliance with the License. A copy of the
// License is located at
//
// http://aws.amazon.com/apache2.0/
//
// or in the "license" file accompanying this file. This file is distributed
// on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
// express or implied. See the License for the specific language governing
// permissions and limitations under the License.

package engine

import (
"os"

apicontainer "github.com/aws/amazon-ecs-agent/agent/api/container"
)

const (
testBaseImage = "amazon-ecs-ftest-windows-base:make"
dockerEndpoint = "npipe:////./pipe/docker_engine"
)

// REGISTRY_IMAGE_NAME is the Windows Server image from Microsoft Container Registry.
// https://github.com/aws/amazon-ecs-agent/blob/78a2bf0c7d3ebd3a13de3ac733af46dfb3816b18/scripts/run-integ-tests.ps1#L45
var (
testRegistryImage = os.Getenv("REGISTRY_IMAGE_NAME")
testRegistryImageWithDigest = os.Getenv("REGISTRY_IMAGE_NAME_WITH_DIGEST")
)

func CreateTestContainer() *apicontainer.Container {
return createTestContainerWithImageAndName(testBaseImage, "windows")
}

// GetLongRunningCommand returns the command that keeps the container running for the container
// that uses the default integ test image (amazon-ecs-ftest-windows-base:make for windows)
func GetLongRunningCommand() []string {
return []string{"ping", "-t", "localhost"}
}

func isDockerRunning() bool {
_, err := os.Stat("//./pipe/docker_engine")
Copy link
Member

@oldschool-engineer oldschool-engineer Aug 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the os.Stat implementation will work. In my testing it always returned the same result regardless of whether docker is running or not.
Here's an alternative implementation that does work.

package main

import (
    "flag"
    "golang.org/x/sys/windows/svc/mgr"
    "log"
)

func isDockerRunning() bool {
    var svcName = "docker"
    flag.StringVar(&svcName, "name", svcName, "name of the service")
    flag.Parse()
    svcMgmt, err := mgr.Connect()
    if err != nil {
        log.Fatalf("Error connecting to service manager: %v", err)
    }

    dockerSvc, err := svcMgmt.OpenService(svcName)
    if err != nil {
        log.Fatalf("Error opening service %q: %v", svcName, err)
    }

    dockerStatus, err := dockerSvc.Query()
    if err != nil {
        log.Fatalf("Error querying docker service: %v", err)
    }
    log.Printf("State:%v, ProcessId:%v, Win32ExitCode: %v", dockerStatus.State, dockerStatus.ProcessId, dockerStatus.Win32ExitCode)
    return dockerStatus.ProcessId != 0
}

func main() {
    dockerStatus := isDockerRunning()
    if dockerStatus != true {
        log.Fatalf("Docker is not running")
    }
    log.Printf("Docker is running")
}
2024/08/30 13:55:25 State:4, ProcessId:4500, Win32ExitCode: 0
2024/08/30 13:55:25 Docker is running
2024/08/30 13:55:04 State:1, ProcessId:0, Win32ExitCode: 1067
2024/08/30 13:55:04 Docker is not running

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have minimal knowledge/experience with Windows development, so bear with me while I explain my testing process...

To exercise the os.Stat implementation of isDockerRunning, I added a test

func TestIsDockerRunning(t *testing.T) {
	assert.True(t, isDockerRunning())
}

which I ran with

go test -tags integration  -count 1 -v ./agent/engine/... -run "TestIsDockerRunning"

I ran get-service docker to check if docker was already running. It was, and the test passed.
I ran stop-service docker to stop docker. The test failed when I reran it, as was expected.
I ran start-service docker to restart docker. The test passed.

Am I not starting/stopping docker correctly? I got the same results when I swapped out the os.Stat implementation with the service manager implementation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on your test results, it seems that Go is handling it properly if run as part of the integration test framework. Given your test results, I'm OK with your earlier implementation since it is very simple and it works. (Plus, the implementation I provided should be modified to include deferred closures to the service manager and service instance handle.)

My concern was whether Go's os.Stat method can differentiate between named pipes and regular on-disk files. Named pipes are accessed via file system drivers (NPFS), but they are are not part of the actual on-disk file system, which is (typically) NTFS.

Running the code snippet, as-is, on Windows in a stand-alone way doesn't seem to work right, however. That said, connecting through service manager requires the unit tests to be run as an administrator.


Am I not starting/stopping docker correctly?

There is more than one way to start/stop services on Windows. start-service docker and stop-service docker is the PowerShell way to start/stop docker and is perfectly acceptable.

return err == nil
}
Loading
Loading