Awsvpc windows : Check if the pause image is loaded on the container instance #2498

rawahars · 2020-06-26T18:13:12Z

Summary

This change is the first step for enabling awsvpc network mode for ECS Windows.
We will be caching pause image in the ECS-Optimized Windows AMI. When the agent starts, we will check if the pause image has been loaded. Without a pause container image loaded on the container instance, awsvpc mode cannot be supported by the instance.
The changes are specific for Windows instances.

Implementation details

Majority of the changes are refactoring of existing code which is used in Linux. We have made following changes-

Moved common functions from pause_linux.go to load.go
Moved the common code from IsLoaded function to a function in load.go
Unit tests for the same were moved from Linux specific test files to common test files

Testing

The unit tests and integration tests were executed on Linux and Windows platform.

New tests cover the changes:
No. We will be reusing the existing code and relevant unit tests which have been moved to the common files.
New tests cover the changes:

Description for the changelog

On agent startup for Windows instances, check if the pause image has been loaded.

Licensing

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Awsvpc windows

… We will cache pause image on ECS-Optimized Windows AMI. On agent startup, we check if the pause image is already loaded. The following changes are made as part of this commit: 1. Moved common functions from pause_linux.go to load.go 2. Moved the common code from IsLoaded function to a function in load.go 3. Unit tests for the same were moved from Linux specific files to common files This was a refactoring change with minimal addition of new code

…mazon-ecs-agent into awsvpc-windows

agent/eni/pause/load.go

agent/eni/pause/pause_windows.go

agent/app/agent_windows.go

yhlee-aws · 2020-06-30T22:06:31Z

agent/config/config_windows.go

+	DefaultPauseContainerImageName = "amazonaws.com/ecs/pause-windows"
+	DefaultPauseContainerTag = "latest"


why not move these default values into the const block?

As discussed with @sparrc , we have added the Windows pause image name and tag in the Makefile and corresponding changes have been made in build file to invoke Windows agent build using the loader flags.

Additionally, a Powershell script has been created which can build standalone agent on Windows.

vsiddharth

Overall changes look good, left minor comments and questions.

agent/app/agent_windows.go

vsiddharth · 2020-06-30T21:50:36Z

agent/app/agent_windows.go

@@ -279,5 +279,9 @@ func (agent *ecsAgent) getPlatformDevices() []*ecs.PlatformDevice {
 }

 func (agent *ecsAgent) loadPauseContainer() error {
-	return nil
+	//The pause image would be cached in th ECS-Optimized Windows AMI's and will be available. We will throw an error if the image is not loaded.
+	//If the agent is run on non-supported instances then pause image has to be loaded manually by the client.


What do you mean by non-supported instance types and manual loading of pause container? Could you please elaborate.

non-supported instances are those which are launched using Non-ECS-Optimized Windows AMI. Manual loading of the pause container refers to creating/loading (using docker load) a pause image.

vsiddharth · 2020-06-30T21:54:04Z

agent/config/config_windows.go

@@ -57,6 +57,8 @@ func DefaultConfig() Config {
 	programData := utils.DefaultIfBlank(os.Getenv("ProgramData"), `C:\ProgramData`)
 	ecsRoot := filepath.Join(programData, "Amazon", "ECS")
 	dataDir := filepath.Join(ecsRoot, "data")
+	DefaultPauseContainerImageName = "amazonaws.com/ecs/pause-windows"
+	DefaultPauseContainerTag = "latest"


Does using latest make sense given that we might use different ECS Optimized Windows AMIs?
Can the pause containers tag be something specific like ltsc2019 ?

Considering that ECS would support future releases of Windows, it would be advisable to use a generic tag. We can probably consider checking the version of windows on the pause container to ensure that it is using the supported version.

agent/eni/pause/load.go

vsiddharth · 2020-06-30T22:00:36Z

agent/eni/pause/load.go

+		config.DefaultPauseContainerImageName, config.DefaultPauseContainerTag, dockerClient)
+
+	if err != nil {
+		return false, err


Why are some errors wrapped and others not? Can you make it consistent across the module?

The error message returned from 'getPauseContainerImage' is sufficient to understand the error.
Therefore, we have returned the same error from this function.

agent/eni/pause/pause_unsupported.go

agent/eni/pause/pause_windows.go

vsiddharth · 2020-06-30T22:10:53Z

agent/config/config_windows.go

@@ -57,6 +57,8 @@ func DefaultConfig() Config {
 	programData := utils.DefaultIfBlank(os.Getenv("ProgramData"), `C:\ProgramData`)
 	ecsRoot := filepath.Join(programData, "Amazon", "ECS")
 	dataDir := filepath.Join(ecsRoot, "data")
+	DefaultPauseContainerImageName = "amazonaws.com/ecs/pause-windows"


Shouldn't this be a const or a var?
Also should there be a mechanism for customers to override the defaultPauseContainer?

As discussed with the agent team, we have added the Windows pause image name and tag in the Makefile and corresponding changes have been made in build file to invoke Windows agent build using the loader flags. The default values for both name and tag will be injected using the loader flag.

Additionally, a Powershell script has been created which can build standalone agent on Windows.

yhlee-aws

Can you squash some of the commits into one? merges you'll have to leave as is, but at least the last 3 commits could be squashed into one commit.

sparrc

I'm curious why did you decide to add env vars for the windows pause container?

Currently we set the linux pause container name using build flags in the Makefile target (see https://github.com/aws/amazon-ecs-agent/blob/master/Makefile#L148)

Why was this pattern not followed? If there is some reason that windows cannot follow this pattern, perhaps we should make it consistent and set the linux pause container name and tag using env vars as well.

sparrc · 2020-07-01T15:59:15Z

agent/config/config_windows.go

+
+	// We check if the pause container image name and tag are overridden by setting an environment variable
+	// If not then we assign the default name and tag
+	pauseContainerImageNameFromEnv := os.Getenv("PAUSE_CONTAINER_IMAGE_NAME")


For consistency with our other env vars, these should be prepended with "ECS_"

also, these should be parsed in the environmentConfig function, where we parse the rest of our env vars: https://github.com/aws/amazon-ecs-agent/blob/master/agent/config/config.go#L484

Following our offline discussion, we have decided against introducing any environment variables.
As discussed, we have added the Windows pause image name and tag in the Makefile and corresponding changes have been made in build file to invoke Windows agent build using the loader flags.

Additionally, a Powershell script has been created which can build standalone agent on Windows.

scripts/build

1. The pause container image name and tag have been moved to Makefile 2. The corresponding config is populated using loader flags at build time. This is similar to how Linux agent populates these values 3. Formatting changes 4. Modified the Readme

sparrc

vsiddharth

LGTM

…instance (aws#2498) * Changes to check if the pause image has been loaded on agent startup. We will cache pause image on ECS-Optimized Windows AMI. On agent startup, we check if the pause image is already loaded. The following changes are made as part of this commit: 1. Moved common functions from pause_linux.go to load.go 2. Moved the common code from IsLoaded function to a function in load.go 3. Unit tests for the same were moved from Linux specific files to common files This was a refactoring change with minimal addition of new code

…instance (aws#2498) * Changes to check if the pause image has been loaded on agent startup. We will cache pause image on ECS-Optimized Windows AMI. On agent startup, we check if the pause image is already loaded. The following changes are made as part of this commit: 1. Moved common functions from pause_linux.go to load.go 2. Moved the common code from IsLoaded function to a function in load.go 3. Unit tests for the same were moved from Linux specific files to common files

rawahars and others added 3 commits June 25, 2020 06:18

Merge pull request #2 from aws/awsvpc-windows

3ce8a79

Awsvpc windows

Merge branch 'awsvpc-windows' of https://github.com/harsh-rawat-a2z/a…

138df14

…mazon-ecs-agent into awsvpc-windows

rawahars requested review from vsiddharth, fenxiong and singholt June 26, 2020 18:14

fenxiong added the bot/test label Jun 29, 2020

amazon-ecs-bot removed the bot/test label Jun 29, 2020

rawahars requested a review from a team June 29, 2020 17:41

fenxiong added the bot/test label Jun 29, 2020

amazon-ecs-bot removed the bot/test label Jun 29, 2020

fenxiong reviewed Jun 30, 2020

View reviewed changes

agent/eni/pause/load.go Outdated Show resolved Hide resolved

agent/eni/pause/pause_windows.go Outdated Show resolved Hide resolved

agent/app/agent_windows.go Show resolved Hide resolved

fenxiong approved these changes Jun 30, 2020

View reviewed changes

fenxiong added the bot/test label Jun 30, 2020

amazon-ecs-bot removed the bot/test label Jun 30, 2020

yhlee-aws reviewed Jun 30, 2020

View reviewed changes

vsiddharth suggested changes Jun 30, 2020

View reviewed changes

yhlee-aws reviewed Jun 30, 2020

View reviewed changes

rawahars force-pushed the awsvpc-windows branch 2 times, most recently from d6d2264 to 3616559 Compare July 1, 2020 02:32

rawahars requested review from vsiddharth and yhlee-aws July 1, 2020 02:34

rawahars force-pushed the awsvpc-windows branch from 3616559 to dd34e09 Compare July 1, 2020 02:44

sparrc requested changes Jul 1, 2020

View reviewed changes

rawahars force-pushed the awsvpc-windows branch 2 times, most recently from 96e69fa to fe2291e Compare July 1, 2020 23:05

rawahars requested a review from sparrc July 1, 2020 23:17

fenxiong added the bot/test label Jul 1, 2020

amazon-ecs-bot removed the bot/test label Jul 1, 2020

sparrc reviewed Jul 1, 2020

View reviewed changes

scripts/build Outdated Show resolved Hide resolved

rawahars force-pushed the awsvpc-windows branch from fe2291e to 50b7cdc Compare July 2, 2020 00:11

rawahars force-pushed the awsvpc-windows branch from 50b7cdc to b89935a Compare July 2, 2020 00:19

sparrc approved these changes Jul 2, 2020

View reviewed changes

sparrc added the bot/test label Jul 2, 2020

amazon-ecs-bot removed the bot/test label Jul 2, 2020

vsiddharth approved these changes Jul 6, 2020

View reviewed changes

fenxiong merged commit d12ec98 into aws:awsvpc-windows Jul 6, 2020

rawahars deleted the awsvpc-windows branch April 28, 2021 04:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Awsvpc windows : Check if the pause image is loaded on the container instance #2498

Awsvpc windows : Check if the pause image is loaded on the container instance #2498

rawahars commented Jun 26, 2020

yhlee-aws Jun 30, 2020

rawahars Jul 1, 2020

vsiddharth left a comment

vsiddharth Jun 30, 2020

rawahars Jul 1, 2020

vsiddharth Jun 30, 2020

rawahars Jul 1, 2020

vsiddharth Jun 30, 2020

rawahars Jul 1, 2020

vsiddharth Jun 30, 2020

rawahars Jul 1, 2020

yhlee-aws left a comment

sparrc left a comment •

edited

Loading

sparrc Jul 1, 2020

sparrc Jul 1, 2020

rawahars Jul 1, 2020 •

edited

Loading

sparrc left a comment

vsiddharth left a comment

		DefaultPauseContainerImageName = "amazonaws.com/ecs/pause-windows"
		DefaultPauseContainerTag = "latest"

Awsvpc windows : Check if the pause image is loaded on the container instance #2498

Awsvpc windows : Check if the pause image is loaded on the container instance #2498

Conversation

rawahars commented Jun 26, 2020

Summary

Implementation details

Testing

Description for the changelog

Licensing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vsiddharth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yhlee-aws left a comment

Choose a reason for hiding this comment

sparrc left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rawahars Jul 1, 2020 • edited Loading

Choose a reason for hiding this comment

sparrc left a comment

Choose a reason for hiding this comment

vsiddharth left a comment

Choose a reason for hiding this comment

sparrc left a comment •

edited

Loading

rawahars Jul 1, 2020 •

edited

Loading