Branch remove token #1711

tommyhahn · 2018-11-29T20:08:38Z

Summary

Merge Sharanya's RCI idempotency changes and remove the token from the state file in app/agent.go

Implementation details

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.

sharanyad · 2018-11-29T20:10:50Z

you might want to fix your author email in your commits :)

yumex93 · 2018-11-30T17:38:38Z

CHANGELOG.md

@@ -1,5 +1,9 @@
 # Changelog

+## 1.22.1


This needs to be changed to ##1.23.0-dev, since we are going to bump the major version this time.

yumex93 · 2018-11-30T17:57:16Z

agent/app/agent.go

@@ -323,7 +325,7 @@ func (agent *ecsAgent) newTaskEngine(containerChangeEventStream *eventstream.Eve
 	}

 	// We try to set these values by loading the existing state file first
-	var previousCluster, previousEC2InstanceID, previousContainerInstanceArn, previousAZ string
+	var previousCluster, previousEC2InstanceID, previousContainerInstanceArn, previousAZ, previousRegistrationToken string


I do not think we still need previousRegistrationToken here, since we remove registrationToken from state file, it will never be initialized here. You may want to double check with @sharanyad

yumex93 · 2018-11-30T17:58:54Z

agent/app/agent.go

@@ -370,6 +372,7 @@ func (agent *ecsAgent) newTaskEngine(containerChangeEventStream *eventstream.Eve

 	// Use the values we loaded if there's no issue
 	agent.containerInstanceARN = previousContainerInstanceArn
+	agent.registrationToken = previousRegistrationToken


previousRegistrationToken will never be initialized here. So the agent.registrationToken will always equal to "". It can be removed

yumex93 · 2018-11-30T18:02:59Z

agent/app/agent.go

@@ -479,13 +482,18 @@ func (agent *ecsAgent) registerContainerInstance(
 		tags = mergeTags(tags, ec2Tags)
 	}

+	if agent.registrationToken == "" {
+		agent.registrationToken = uuid.New()
+		stateManager.Save()


Since we no more need to save registrationToken in the state file, we do not need to call stateManager.Save() here, considering at the end of the this 'registerContainerInstance' function, we already call stateManager.Save() to save all saveable options.

sharanyad · 2018-11-30T18:05:09Z

agent/app/agent.go

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


since we do not store this on disk, we may not need this field anymore.

sharanyad · 2018-11-30T18:06:09Z

agent/app/agent.go

@@ -479,13 +482,18 @@ func (agent *ecsAgent) registerContainerInstance(
 		tags = mergeTags(tags, ec2Tags)
 	}

+	if agent.registrationToken == "" {


this check is not needed -- for each register call, we'll create a new token.

sharanyad · 2018-11-30T18:10:01Z

agent/app/agent_test.go

-		stateManagerFactory.EXPECT().NewStateManager(gomock.Any(),
-			gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(),
-		).Return(
+		stateManagerFactory.EXPECT().NewStateManager(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(


nit: this can be broken into two lines

sharanyad · 2018-11-30T18:13:15Z

agent/app/agent.go

@@ -479,13 +482,18 @@ func (agent *ecsAgent) registerContainerInstance(
 		tags = mergeTags(tags, ec2Tags)
 	}

+	if agent.registrationToken == "" {
+		agent.registrationToken = uuid.New()
+		stateManager.Save()


this should not be needed.

yumex93 · 2018-11-30T23:02:11Z

agent/app/agent_test.go

@@ -676,7 +676,7 @@ func TestReregisterContainerInstanceHappyPath(t *testing.T) {
 		mockMobyPlugins.EXPECT().Scan().AnyTimes().Return([]string{""}, nil),
 		mockDockerClient.EXPECT().ListPluginsWithFilters(gomock.Any(), gomock.Any(),
 			gomock.Any(), gomock.Any()).AnyTimes().Return([]string{}, nil),
-		client.EXPECT().RegisterContainerInstance(containerInstanceARN, gomock.Any(), gomock.Any()).Return(containerInstanceARN, availabilityZone, nil),
+		client.EXPECT().RegisterContainerInstance(containerInstanceARN, gomock.Any(), gomock.Any(), gomock.Any()).Return(containerInstanceARN, availabilityZone, nil),


nit: this can also break into two lines

sharanyad

code lgtm. Hold off on merging until tests pass. Thanks!

sharanyad · 2018-12-01T00:49:28Z

CHANGELOG.md

@@ -1,5 +1,9 @@
 # Changelog

+## 1.23.0-dev
+* Bug - Fixed a bug where agent can register container instance back to back and gets
+  assigned two container instance ARNs [#1579](https://github.com/aws/amazon-ecs-agent/pull/1579)


this should be changed to this PR link

fenxiong added the bot/test label Nov 29, 2018

tommyhahn force-pushed the branch_remove_token branch from 9372674 to d90140a Compare November 29, 2018 22:39

tommyhahn added bot/test and removed bot/test labels Nov 29, 2018

haikuoliu requested a review from a team November 29, 2018 23:38

tommyhahn force-pushed the branch_remove_token branch from d90140a to 8375d2a Compare November 29, 2018 23:38

yumex93 requested a review from sharanyad November 29, 2018 23:43

yumex93 added bot/test and removed bot/test labels Nov 29, 2018

tommyhahn force-pushed the branch_remove_token branch from 8375d2a to d457705 Compare November 30, 2018 01:11

yumex93 added bot/test and removed bot/test labels Nov 30, 2018

yumex93 reviewed Nov 30, 2018

View reviewed changes

sharanyad reviewed Nov 30, 2018

View reviewed changes

tommyhahn force-pushed the branch_remove_token branch from 2e64f34 to b8d52d5 Compare November 30, 2018 22:30

yumex93 reviewed Nov 30, 2018

View reviewed changes

tommyhahn force-pushed the branch_remove_token branch from b8d52d5 to 3418d83 Compare November 30, 2018 23:12

sharanyad approved these changes Dec 1, 2018

View reviewed changes

RCI idempotency changes

200de25

tommyhahn force-pushed the branch_remove_token branch from 3418d83 to f8239b1 Compare December 3, 2018 04:15

app/agent.go: remove token from state file

0fe43ba

tommyhahn force-pushed the branch_remove_token branch from f8239b1 to 0fe43ba Compare December 3, 2018 04:23

yumex93 approved these changes Dec 3, 2018

View reviewed changes

tommyhahn added bot/test and removed bot/test labels Dec 3, 2018

yumex93 removed the bot/test label Dec 4, 2018

yumex93 added bot/test and removed bot/test labels Dec 4, 2018

tommyhahn merged commit b53bf01 into aws:dev Dec 4, 2018

yumex93 added this to the 1.23.0 milestone Dec 4, 2018

yumex93 mentioned this pull request Dec 4, 2018

make RCI calls idempotent #1579

Closed

8 tasks

tommyhahn deleted the branch_remove_token branch January 2, 2019 19:40

adnxn mentioned this pull request Jan 7, 2019

[ECS] [request] Support idempotency for ECS RegisterContainerInstance API aws/containers-roadmap#1901

Open

danehlim mentioned this pull request Oct 26, 2023

Update ecs-agent/ ECS model with missing fields + consume the same #3999

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Branch remove token #1711

Branch remove token #1711

tommyhahn commented Nov 29, 2018 •

edited by yumex93

Loading

sharanyad commented Nov 29, 2018

yumex93 Nov 30, 2018

yumex93 Nov 30, 2018 •

edited

Loading

yumex93 Nov 30, 2018

yumex93 Nov 30, 2018

sharanyad Nov 30, 2018

sharanyad Nov 30, 2018

sharanyad Nov 30, 2018

sharanyad Nov 30, 2018

yumex93 Nov 30, 2018

sharanyad left a comment

sharanyad Dec 1, 2018

Branch remove token #1711

Branch remove token #1711

Conversation

tommyhahn commented Nov 29, 2018 • edited by yumex93 Loading

Summary

Implementation details

Testing

Description for the changelog

Licensing

sharanyad commented Nov 29, 2018

Choose a reason for hiding this comment

yumex93 Nov 30, 2018 • edited Loading

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

sharanyad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tommyhahn commented Nov 29, 2018 •

edited by yumex93

Loading

yumex93 Nov 30, 2018 •

edited

Loading