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.

Sign up for GitHub

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

AvailabilityZone show on taskmetadata endpoint #1674

Merged
merged 1 commit into from
Nov 9, 2018
+450 −137
Merged

AvailabilityZone show on taskmetadata endpoint #1674

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
28 changes: 20 additions & 8 deletions agent/api/ecsclient/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ const (
pollEndpointCacheSize = 1
pollEndpointCacheTTL = 20 * time.Minute
roundtripTimeout = 5 * time.Second
azAttrName = "ecs.availability-zone"
)

// APIECSClient implements ECSClient
Expand Down Expand Up @@ -108,7 +109,7 @@ func (client *APIECSClient) CreateCluster(clusterName string) (string, error) {
// instance ARN allows a container instance to update its registered
// resources.
func (client *APIECSClient) RegisterContainerInstance(containerInstanceArn string,
attributes []*ecs.Attribute, tags []*ecs.Tag) (string, error) {
attributes []*ecs.Attribute, tags []*ecs.Tag) (string, string, error) {
clusterRef := client.config.Cluster
// If our clusterRef is empty, we should try to create the default
if clusterRef == "" {
Expand All @@ -119,22 +120,22 @@ func (client *APIECSClient) RegisterContainerInstance(containerInstanceArn strin
}()
// Attempt to register without checking existence of the cluster so we don't require
// excess permissions in the case where the cluster already exists and is active
containerInstanceArn, err := client.registerContainerInstance(clusterRef, containerInstanceArn, attributes, tags)
containerInstanceArn, availabilityzone, err := client.registerContainerInstance(clusterRef, containerInstanceArn, attributes, tags)
if err == nil {
return containerInstanceArn, nil
return containerInstanceArn, availabilityzone, nil
}
// 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
}
}
return client.registerContainerInstance(clusterRef, containerInstanceArn, attributes, tags)
}

func (client *APIECSClient) registerContainerInstance(clusterRef string, containerInstanceArn string,
attributes []*ecs.Attribute, tags []*ecs.Tag) (string, error) {
attributes []*ecs.Attribute, tags []*ecs.Tag) (string, string, error) {
registerRequest := ecs.RegisterContainerInstanceInput{Cluster: &clusterRef}
var registrationAttributes []*ecs.Attribute
if containerInstanceArn != "" {
Expand Down Expand Up @@ -164,18 +165,29 @@ func (client *APIECSClient) registerContainerInstance(clusterRef string, contain

resources, err := client.getResources()
if err != nil {
return "", err
return "", "", err
}

registerRequest.TotalResources = resources
resp, err := client.standardClient.RegisterContainerInstance(&registerRequest)
if err != nil {
seelog.Errorf("Unable to register as a container instance with ECS: %v", err)
return "", err
return "", "", err
}

var availabilityzone = ""
if resp != nil {
for _, attr := range resp.ContainerInstance.Attributes {
if aws.StringValue(attr.Name) == azAttrName {
availabilityzone = aws.StringValue(attr.Value)
Copy link
Contributor

Choose a reason for hiding this comment

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

You can add a break here

break
}
}
}

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
}

func (client *APIECSClient) setInstanceIdentity(registerRequest ecs.RegisterContainerInstanceInput) ecs.RegisterContainerInstanceInput {
Expand Down
23 changes: 16 additions & 7 deletions agent/api/ecsclient/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,7 +329,8 @@ func TestReRegisterContainerInstance(t *testing.T) {

fakeCapabilities := []string{"capability1", "capability2"}
expectedAttributes := map[string]string{
"ecs.os-type": config.OSType,
"ecs.os-type": config.OSType,
"ecs.availability-zone": "us-west-2b",
}
for i := range fakeCapabilities {
expectedAttributes[fakeCapabilities[i]] = ""
Expand Down Expand Up @@ -375,13 +376,14 @@ func TestReRegisterContainerInstance(t *testing.T) {
nil),
)

arn, err := client.RegisterContainerInstance("arn:test", capabilities, containerInstanceTags)
arn, availabilityzone, err := client.RegisterContainerInstance("arn:test", capabilities, containerInstanceTags)
if err != nil {
t.Errorf("Should not be an error: %v", err)
}
if arn != "registerArn" {
t.Errorf("Wrong arn: %v", arn)
}
assert.Equal(t, "us-west-2b", availabilityzone, "availabilityZone is incorrect")
}

func TestRegisterContainerInstance(t *testing.T) {
Expand All @@ -398,6 +400,7 @@ func TestRegisterContainerInstance(t *testing.T) {
"ecs.os-type": config.OSType,
"my_custom_attribute": "Custom_Value1",
"my_other_custom_attribute": "Custom_Value2",
"ecs.availability-zone": "us-west-2b",
}
capabilities := buildAttributeList(fakeCapabilities, nil)

Expand Down Expand Up @@ -435,9 +438,10 @@ func TestRegisterContainerInstance(t *testing.T) {
nil),
)

arn, err := client.RegisterContainerInstance("", capabilities, containerInstanceTags)
arn, availabilityzone, err := client.RegisterContainerInstance("", capabilities, containerInstanceTags)
assert.NoError(t, err)
assert.Equal(t, "registerArn", arn)
assert.Equal(t, "us-west-2b", availabilityzone)
}

func TestRegisterContainerInstanceNoIID(t *testing.T) {
Expand All @@ -460,6 +464,7 @@ func TestRegisterContainerInstanceNoIID(t *testing.T) {
"ecs.os-type": config.OSType,
"my_custom_attribute": "Custom_Value1",
"my_other_custom_attribute": "Custom_Value2",
"ecs.availability-zone": "us-west-2b",
}
capabilities := buildAttributeList(fakeCapabilities, nil)

Expand Down Expand Up @@ -495,9 +500,10 @@ func TestRegisterContainerInstanceNoIID(t *testing.T) {
nil),
)

arn, err := client.RegisterContainerInstance("", capabilities, containerInstanceTags)
arn, availabilityzone, err := client.RegisterContainerInstance("", capabilities, containerInstanceTags)
assert.NoError(t, err)
assert.Equal(t, "registerArn", arn)
assert.Equal(t, "us-west-2b", availabilityzone)
}

// TestRegisterContainerInstanceWithNegativeResource tests the registeration should fail with negative resource
Expand All @@ -521,7 +527,7 @@ func TestRegisterContainerInstanceWithNegativeResource(t *testing.T) {
mockEC2Metadata.EXPECT().GetDynamicData(ec2.InstanceIdentityDocumentResource).Return("instanceIdentityDocument", nil),
mockEC2Metadata.EXPECT().GetDynamicData(ec2.InstanceIdentityDocumentSignatureResource).Return("signature", nil),
)
_, err := client.RegisterContainerInstance("", nil, nil)
_, _, err := client.RegisterContainerInstance("", nil, nil)
assert.Error(t, err, "Register resource with negative value should cause registration fail")
}

Expand Down Expand Up @@ -551,7 +557,7 @@ func TestRegisterContainerInstanceWithEmptyTags(t *testing.T) {
nil),
)

_, err := client.RegisterContainerInstance("", nil, make([]*ecs.Tag, 0))
_, _, err := client.RegisterContainerInstance("", nil, make([]*ecs.Tag, 0))
assert.NoError(t, err)
}

Expand Down Expand Up @@ -625,13 +631,16 @@ func TestRegisterBlankCluster(t *testing.T) {
nil),
)

arn, err := client.RegisterContainerInstance("", nil, nil)
arn, availabilityzone, err := client.RegisterContainerInstance("", nil, nil)
if err != nil {
t.Errorf("Should not be an error: %v", err)
}
if arn != "registerArn" {
t.Errorf("Wrong arn: %v", arn)
}
if availabilityzone != "" {
t.Errorf("wrong availability zone: %v", availabilityzone)
}
}

func TestDiscoverTelemetryEndpoint(t *testing.T) {
Expand Down
2 changes: 1 addition & 1 deletion agent/api/interface.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ type ECSClient interface {
// instance ARN allows a container instance to update its registered
// resources.
RegisterContainerInstance(existingContainerInstanceArn string,
attributes []*ecs.Attribute, tags []*ecs.Tag) (string, error)
attributes []*ecs.Attribute, tags []*ecs.Tag) (string, string, error)
// SubmitTaskStateChange sends a state change and returns an error
// indicating if it was submitted
SubmitTaskStateChange(change TaskStateChange) error
Expand Down
7 changes: 4 additions & 3 deletions agent/api/mocks/api_mocks.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

18 changes: 11 additions & 7 deletions agent/app/agent.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ type ecsAgent struct {
terminationHandler sighandlers.TerminationHandler
mobyPlugins mobypkgwrapper.Plugins
resourceFields *taskresource.ResourceFields
availabilityZone string
}

// newAgent returns a new ecsAgent object, but does not start anything
Expand Down Expand Up @@ -240,7 +241,7 @@ func (agent *ecsAgent) doStart(containerChangeEventStream *eventstream.EventStre

// Initialize the state manager
stateManager, err := agent.newStateManager(taskEngine,
&agent.cfg.Cluster, &agent.containerInstanceARN, &currentEC2InstanceID)
&agent.cfg.Cluster, &agent.containerInstanceARN, &currentEC2InstanceID, &agent.availabilityZone)
if err != nil {
seelog.Criticalf("Error creating state manager: %v", err)
return exitcodes.ExitTerminal
Expand Down Expand Up @@ -321,15 +322,15 @@ func (agent *ecsAgent) newTaskEngine(containerChangeEventStream *eventstream.Eve
}

// We try to set these values by loading the existing state file first
var previousCluster, previousEC2InstanceID, previousContainerInstanceArn string
var previousCluster, previousEC2InstanceID, previousContainerInstanceArn, previousAZ string
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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need to bump ECSDataVersion?

if err != nil {
seelog.Criticalf("Error creating state manager: %v", err)
return nil, "", err
Expand Down Expand Up @@ -413,7 +414,8 @@ func (agent *ecsAgent) newStateManager(
taskEngine engine.TaskEngine,
cluster *string,
containerInstanceArn *string,
savedInstanceID *string) (statemanager.StateManager, error) {
savedInstanceID *string,
availabilityZone *string) (statemanager.StateManager, error) {

if !agent.cfg.Checkpoint {
return statemanager.NewNoopStateManager(), nil
Expand All @@ -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),
)
}

Expand Down Expand Up @@ -481,7 +484,7 @@ func (agent *ecsAgent) registerContainerInstance(
}

seelog.Info("Registering Instance with ECS")
containerInstanceArn, err := client.RegisterContainerInstance("", capabilities, tags)
containerInstanceArn, availabilityZone, err := client.RegisterContainerInstance("", capabilities, tags)
if err != nil {
seelog.Errorf("Error registering: %v", err)
if retriable, ok := err.(apierrors.Retriable); ok && !retriable.Retry() {
Expand All @@ -499,6 +502,7 @@ func (agent *ecsAgent) registerContainerInstance(
}
seelog.Infof("Registration completed successfully. I am running as '%s' in cluster '%s'", containerInstanceArn, agent.cfg.Cluster)
agent.containerInstanceARN = containerInstanceArn
agent.availabilityZone = availabilityZone
// Save our shiny new containerInstanceArn
stateManager.Save()
return nil
Expand All @@ -509,7 +513,7 @@ func (agent *ecsAgent) registerContainerInstance(
// from a check point.
func (agent *ecsAgent) reregisterContainerInstance(client api.ECSClient,
capabilities []*ecs.Attribute, tags []*ecs.Tag) error {
_, err := client.RegisterContainerInstance(agent.containerInstanceARN, capabilities, tags)
_, _, err := client.RegisterContainerInstance(agent.containerInstanceARN, capabilities, tags)

if err == nil {
return nil
Expand Down Expand Up @@ -551,7 +555,7 @@ func (agent *ecsAgent) startAsyncRoutines(
statsEngine := stats.NewDockerStatsEngine(agent.cfg, agent.dockerClient, containerChangeEventStream)

// Start serving the endpoint to fetch IAM Role credentials and other task metadata
go handlers.ServeTaskHTTPEndpoint(credentialsManager, state, agent.containerInstanceARN, agent.cfg, statsEngine)
go handlers.ServeTaskHTTPEndpoint(credentialsManager, state, agent.containerInstanceARN, agent.cfg, statsEngine, agent.availabilityZone)

// Start sending events to the backend
go eventhandler.HandleEngineEvents(taskEngine, client, taskHandler)
Expand Down
6 changes: 3 additions & 3 deletions agent/app/agent_compatibility_linux_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ func TestCompatibilityEnabledSuccess(t *testing.T) {

gomock.InOrder(
saveableOptionFactory.EXPECT().AddSaveable(gomock.Any(), gomock.Any()).AnyTimes(),
stateManagerFactory.EXPECT().NewStateManager(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes().Return(stateManager, nil),
stateManagerFactory.EXPECT().NewStateManager(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes().Return(stateManager, nil),
stateManager.EXPECT().Load().AnyTimes(),
state.EXPECT().AllTasks().Return([]*apitask.Task{}),
)
Expand Down Expand Up @@ -79,7 +79,7 @@ func TestCompatibilityDefaultEnabledFail(t *testing.T) {
}
gomock.InOrder(
saveableOptionFactory.EXPECT().AddSaveable(gomock.Any(), gomock.Any()).AnyTimes(),
stateManagerFactory.EXPECT().NewStateManager(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes().Return(stateManager, nil),
stateManagerFactory.EXPECT().NewStateManager(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes().Return(stateManager, nil),
stateManager.EXPECT().Load().AnyTimes(),
state.EXPECT().AllTasks().Return(getTaskListWithOneBadTask()),
)
Expand Down Expand Up @@ -110,7 +110,7 @@ func TestCompatibilityExplicitlyEnabledFail(t *testing.T) {
}
gomock.InOrder(
saveableOptionFactory.EXPECT().AddSaveable(gomock.Any(), gomock.Any()).AnyTimes(),
stateManagerFactory.EXPECT().NewStateManager(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes().Return(stateManager, nil),
stateManagerFactory.EXPECT().NewStateManager(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes().Return(stateManager, nil),
stateManager.EXPECT().Load().AnyTimes(),
state.EXPECT().AllTasks().Return(getTaskListWithOneBadTask()),
)
Expand Down
Loading