AvailabilityZone show on taskmetadata endpoint #1674

cyastella · 2018-11-06T18:47:25Z

Summary

Expose availabilityZone to task metadata endpoint

Implementation details

Pass the availabilityZone from container instance level to agent metadata and expose availabilityZone from agent metadata to task response struct.

Testing

Builds on Linux (make release)
Builds on Windows (go build -out amazon-ecs-agent.exe ./agent)
Unit tests on Linux (make test) pass
Unit tests on Windows (go test -timeout=25s ./agent/...) pass
Integration tests on Linux (make run-integ-tests) pass
Integration tests on Windows (.\scripts\run-integ-tests.ps1) pass
Functional tests on Linux (make run-functional-tests) pass
Functional tests on Windows (.\scripts\run-functional-tests.ps1) pass

New tests cover the changes:

Description for the changelog

Licensing

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

linkar-ec2

Just for my understanding, why do we update both v2 and v3 metadata handlers?

linkar-ec2 · 2018-11-06T18:49:06Z

agent/api/ecsclient/client.go

 		if err == nil {
-			return containerInstanceArn, nil
+			return containerInstanceArn,availabilityzone, nil


nit: add space

linkar-ec2 · 2018-11-06T18:49:18Z

agent/api/ecsclient/client.go

 		}
 		// If trying to register fails, try to create the cluster before calling
 		// register again
 		clusterRef, err = client.CreateCluster(clusterRef)
 		if err != nil {
-			return "", err
+			return "","", err


nit: add space
Same for below

linkar-ec2 · 2018-11-06T18:50:26Z

agent/api/ecsclient/client.go

+	var availabilityzone_name = "ecs.availability-zone"
+	var availabilityzone = ""
+	attrs := resp.ContainerInstance.Attributes
+	if(resp != nil){


parentheses around if conditional not needed in Golang

linkar-ec2 · 2018-11-06T18:52:09Z

agent/api/ecsclient/client.go

+		return "","", err
+	}
+
+	var availabilityzone_name = "ecs.availability-zone"


Mixed caps can be used: availabilityZoneName
https://golang.org/doc/effective_go.html#mixed-caps
Same for other variables below.

Also, make this a const and put it at the top of the file

linkar-ec2 · 2018-11-06T18:54:31Z

agent/api/ecsclient/client_test.go

 	if err != nil {
 		t.Errorf("Should not be an error: %v", err)
 	}
 	if arn != "registerArn" {
 		t.Errorf("Wrong arn: %v", arn)
 	}
+	if availabilityzone != "us-west-2b"{


We can use assert.Equal instead of using an if statement to report the error like you do in TestRegisterContainerInstance +

+1 This test uses a different style of test assertions -- you can migrate it if you want :)

good point, will refactor the test style.

linkar-ec2 · 2018-11-06T18:57:44Z

agent/app/agent_test.go

@@ -628,7 +647,7 @@ func TestReregisterContainerInstanceInstanceTypeChanged(t *testing.T) {
 		mockDockerClient.EXPECT().ListPluginsWithFilters(gomock.Any(), gomock.Any(),
 			gomock.Any(), gomock.Any()).AnyTimes().Return([]string{}, nil),
 		client.EXPECT().RegisterContainerInstance(containerInstanceARN, gomock.Any(), gomock.Any()).Return(
-			"", awserr.New("", apierrors.InstanceTypeChangedErrorMessage, errors.New(""))),
+			"", "",awserr.New("", apierrors.InstanceTypeChangedErrorMessage, errors.New(""))),


nit: add space

linkar-ec2 · 2018-11-06T19:04:38Z

misc/v3-task-endpoint-validator/v3-task-endpoint-validator.go

@@ -184,7 +185,7 @@ func verifyTaskMetadataResponse(taskMetadataRawMsg json.RawMessage) error {
 		"KnownStatus":   "RUNNING",
 	}

-	taskExpectedFieldNotEmptyArray := []string{"TaskARN", "Family", "Revision", "PullStartedAt", "PullStoppedAt", "Containers"}
+	taskExpectedFieldNotEmptyArray := []string{"TaskARN", "Family", "Revision", "PullStartedAt", "PullStoppedAt", "Containers","AvailabilityZone"}


nit: add space

cyastella · 2018-11-06T22:09:42Z

@linkar-ec2 v3 handler shares the same task metadata response format with v2 handler, so to keep consistent, we add availabilityZone to both places.

yhlee-aws · 2018-11-07T18:18:41Z

misc/v3-task-endpoint-validator-windows/v3-task-endpoint-validator-windows.go

@@ -41,6 +41,7 @@ type TaskResponse struct {
 	PullStartedAt      *time.Time          `json:"PullStartedAt,omitempty"`
 	PullStoppedAt      *time.Time          `json:"PullStoppedAt,omitempty"`
 	ExecutionStoppedAt *time.Time          `json:"ExecutionStoppedAt,omitempty"`
+	//AvailabilityZone   string              `json:"AvailabilityZone"`


why is this commented out?

will update it, haven't add it to windows yet.

petderek · 2018-11-07T18:18:36Z

agent/api/ecsclient/client.go

+		return "","", err
+	}
+
+	var availabilityzone_name = "ecs.availability-zone"


Also, make this a const and put it at the top of the file

petderek · 2018-11-07T18:24:14Z

agent/app/agent_test.go

@@ -103,10 +104,11 @@ func TestDoStartMinimumSupportedDockerVersionTerminal(t *testing.T) {
 		stateManagerFactory:   stateManagerFactory,
 		saveableOptionFactory: saveableOptionFactory,
 	}
-
+	assert.Equal(t, "", agent.availabilityzone)


I don't think we need this here. The test isn't doing anything AZ-specific

petderek · 2018-11-07T18:25:02Z

agent/app/agent_test.go

 	exitCode := agent.doStart(eventstream.NewEventStream("events", ctx),
 		credentialsManager, state, imageManager, client)
 	assert.Equal(t, exitcodes.ExitTerminal, exitCode)
+	assert.Equal(t,"", agent.availabilityzone)


I don't think we need this here either.

will remove it.

petderek · 2018-11-07T18:27:02Z

misc/v3-task-endpoint-validator-windows/v3-task-endpoint-validator-windows.go

@@ -41,6 +41,7 @@ type TaskResponse struct {
 	PullStartedAt      *time.Time          `json:"PullStartedAt,omitempty"`
 	PullStoppedAt      *time.Time          `json:"PullStoppedAt,omitempty"`
 	ExecutionStoppedAt *time.Time          `json:"ExecutionStoppedAt,omitempty"`
+	//AvailabilityZone   string              `json:"AvailabilityZone"`


Why is this commented out?

will update it, haven't add it to windows yet.

petderek · 2018-11-07T18:29:45Z

agent/app/agent_test.go

@@ -763,10 +785,11 @@ func TestRegisterContainerInstanceWhenContainerInstanceARNIsNotSetHappyPath(t *t
 		credentialProvider: aws_credentials.NewCredentials(mockCredentialsProvider),
 		mobyPlugins:        mockMobyPlugins,
 	}
-
+	assert.Equal(t, "", agent.availabilityzone)


I don't think we need to check for emptiness before running the test.

haikuoliu · 2018-11-07T19:07:12Z

agent/api/ecsclient/client.go

 	seelog.Info("Registered container instance with cluster!")
 	err = validateRegisteredAttributes(registerRequest.Attributes, resp.ContainerInstance.Attributes)
-	return aws.StringValue(resp.ContainerInstance.ContainerInstanceArn), err
+	return aws.StringValue(resp.ContainerInstance.ContainerInstanceArn), availabilityzone,err


nit: space, this happens a lot, have you configured gofmt on your editor? It will automatically adjust spaces.

haikuoliu · 2018-11-07T19:08:27Z

agent/api/ecsclient/client.go

+	if(resp != nil){
+		for _, attr := range attrs {
+			if aws.StringValue(attr.Name) == availabilityzone_name {
+				availabilityzone = aws.StringValue(attr.Value)


You can add a break here

haikuoliu · 2018-11-07T19:17:48Z

agent/handlers/v2/response.go

@@ -88,6 +90,7 @@ func NewTaskResponse(taskARN string,
 		Revision:      task.Version,
 		DesiredStatus: task.GetDesiredStatus().String(),
 		KnownStatus:   task.GetKnownStatus().String(),
+		AvailabilityZone: az,


please gofmt this

haikuoliu · 2018-11-08T01:53:13Z

agent/app/agent.go

 	previousTaskEngine := engine.NewTaskEngine(agent.cfg, agent.dockerClient,
 		credentialsManager, containerChangeEventStream, imageManager, state,
 		agent.metadataManager, agent.resourceFields)

 	// previousStateManager is used to verify that our current runtime configuration is
 	// compatible with our past configuration as reflected by our state-file
 	previousStateManager, err := agent.newStateManager(previousTaskEngine, &previousCluster,
-		&previousContainerInstanceArn, &previousEC2InstanceID)
+		&previousContainerInstanceArn, &previousEC2InstanceID, &previousAZ)


Do you need to bump ECSDataVersion?

sharanyad · 2018-11-08T21:39:10Z

agent/api/ecsclient/client.go

+	var availabilityzone = ""
+	attrs := resp.ContainerInstance.Attributes
+	if resp != nil {
+		for _, attr := range attrs {


nit:
this can be for _, attr := range resp.ContainerInstance.Attributes {

sharanyad · 2018-11-08T21:42:50Z

agent/app/agent.go

@@ -427,6 +429,7 @@ func (agent *ecsAgent) newStateManager(
 		agent.saveableOptionFactory.AddSaveable("Cluster", cluster),
 		// This is for making testing easier as we can mock this
 		agent.saveableOptionFactory.AddSaveable("EC2InstanceID", savedInstanceID),
+		agent.saveableOptionFactory.AddSaveable("availabilityzone", availabilityzone),


nit: AvailabilityZone should be the key

sharanyad · 2018-11-08T21:43:16Z

agent/app/agent.go

@@ -107,6 +107,7 @@ type ecsAgent struct {
 	terminationHandler    sighandlers.TerminationHandler
 	mobyPlugins           mobypkgwrapper.Plugins
 	resourceFields        *taskresource.ResourceFields
+	availabilityzone      string


nit: availabilityZone

sharanyad · 2018-11-08T21:51:04Z

misc/v3-task-endpoint-validator-windows/v3-task-endpoint-validator-windows.go

@@ -36,6 +36,7 @@ type TaskResponse struct {
 	Revision           string              `json:"Revision"`
 	DesiredStatus      string              `json:"DesiredStatus,omitempty"`
 	KnownStatus        string              `json:"KnownStatus"`
+	AvailabilityZone   string              `json:"AvailabilityZone"`


I believe similar changes are needed in this file.

sharanyad · 2018-11-09T18:39:23Z

agent/api/ecsclient/client.go

@@ -41,6 +41,7 @@ const (
 	pollEndpointCacheSize = 1
 	pollEndpointCacheTTL  = 20 * time.Minute
 	roundtripTimeout      = 5 * time.Second
+	availabilityZoneName  = "ecs.availability-zone"


how about naming this azAttrName?

sharanyad · 2018-11-09T18:49:42Z

agent/api/task/task.go

@@ -94,6 +94,8 @@ type TaskOverrides struct{}
 type Task struct {
 	// Arn is the unique identifier for the task
 	Arn string
+	//AvailabilityZone is the zone for task locates in
+	AvailabilityZone string


why is this needed? as I understand, we use agent's availabilityZone field for v2/v3 responses, right?

haikuoliu · 2018-11-09T19:28:20Z

agent/statemanager/state_manager.go

@@ -70,7 +70,8 @@ const (
 	// 17)
 	//   a) Add 'secrets' field to 'apicontainer.Container'
 	//   b) Add 'ssmsecret' field to 'resources'
-	ECSDataVersion = 17
+	// 18) Add 'AvailabilityZone' field to the TaskResponse struct


The reason that we bump ECSDataVersion is we want to maintain compatibility when we serialize/deserizlize state manager, TaskResponse struct is not maintained by state manager. I suggested bumping the version because I saw you added az in state manager.

cyastella requested review from petderek and a team November 6, 2018 18:47

linkar-ec2 reviewed Nov 6, 2018

View reviewed changes

yhlee-aws reviewed Nov 7, 2018

View reviewed changes

petderek reviewed Nov 7, 2018

View reviewed changes

haikuoliu reviewed Nov 7, 2018

View reviewed changes

haikuoliu reviewed Nov 8, 2018

View reviewed changes

sharanyad reviewed Nov 8, 2018

View reviewed changes

yhlee-aws approved these changes Nov 9, 2018

View reviewed changes

sharanyad reviewed Nov 9, 2018

View reviewed changes

AvailabilityZone show on taskmetadata endpoint

f4cf236

haikuoliu approved these changes Nov 9, 2018

View reviewed changes

sharanyad approved these changes Nov 9, 2018

View reviewed changes

cyastella merged commit 44c6b76 into aws:dev Nov 9, 2018

yumex93 added this to the 1.23.0 milestone Dec 4, 2018

adnxn mentioned this pull request Jan 7, 2019

Expose AZ through task metadata endpoint #1559

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AvailabilityZone show on taskmetadata endpoint #1674

AvailabilityZone show on taskmetadata endpoint #1674

cyastella commented Nov 6, 2018 •

edited by yhlee-aws

Loading

linkar-ec2 left a comment

linkar-ec2 Nov 6, 2018

linkar-ec2 Nov 6, 2018

linkar-ec2 Nov 6, 2018

linkar-ec2 Nov 6, 2018

petderek Nov 7, 2018

linkar-ec2 Nov 6, 2018

petderek Nov 6, 2018

cyastella Nov 6, 2018

linkar-ec2 Nov 6, 2018

linkar-ec2 Nov 6, 2018

cyastella commented Nov 6, 2018

yhlee-aws Nov 7, 2018

cyastella Nov 7, 2018

petderek Nov 7, 2018

petderek Nov 7, 2018

petderek Nov 7, 2018

cyastella Nov 7, 2018

petderek Nov 7, 2018

cyastella Nov 7, 2018

petderek Nov 7, 2018

haikuoliu Nov 7, 2018

haikuoliu Nov 7, 2018

haikuoliu Nov 7, 2018

haikuoliu Nov 8, 2018

sharanyad Nov 8, 2018

sharanyad Nov 8, 2018

sharanyad Nov 8, 2018

sharanyad Nov 8, 2018

sharanyad Nov 9, 2018

sharanyad Nov 9, 2018

haikuoliu Nov 9, 2018 •

edited

Loading

AvailabilityZone show on taskmetadata endpoint #1674

AvailabilityZone show on taskmetadata endpoint #1674

Conversation

cyastella commented Nov 6, 2018 • edited by yhlee-aws Loading

Summary

Implementation details

Testing

Description for the changelog

Licensing

linkar-ec2 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyastella commented Nov 6, 2018

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

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

Choose a reason for hiding this comment

haikuoliu Nov 9, 2018 • edited Loading

Choose a reason for hiding this comment

cyastella commented Nov 6, 2018 •

edited by yhlee-aws

Loading

haikuoliu Nov 9, 2018 •

edited

Loading