Adding adapter to have volumes/links use DependsOn field #1830

ubhattacharjya · 2019-01-31T22:08:13Z

Summary

This implements how VolumesFrom and Links field in Container can start to use the DependsOn field in container

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=30s ./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.

petderek · 2019-01-31T22:23:57Z

agent/api/task/task.go

+				_, ok := task.ContainerByName(volume.SourceContainer)
+				if !ok {
+					return fmt.Errorf("could not find container with name %s", volume.SourceContainer)
+				} else {


nit: you can one-line this:

if _, ok := task.Foo(); !ok { }

petderek · 2019-01-31T22:34:11Z

agent/api/task/task.go

@@ -250,6 +252,18 @@ func (task *Task) PostUnmarshalTask(cfg *config.Config,
 		}
 	}

+	err := task.initializeContainerOrderingForVolumes()


Have we already validated the links / volumes by now?

No I think. The links/volumes are validated just before before creating the container, while adding the hostconfig.

sharanyad

Are we deprecating the existing ContainerDependency model and moving to DependsOn ?

sharanyad · 2019-02-01T18:57:22Z

agent/api/task/task.go

@@ -250,6 +252,18 @@ func (task *Task) PostUnmarshalTask(cfg *config.Config,
 		}
 	}

+	err := task.initializeContainerOrderingForVolumes()
+	if err != nil {
+		seelog.Errorf("Task [%s]: could not initialize volumes for DependsOn field of container: %v", task.Arn, err)


Calling this - could not initialize volumes dependency for container.. may be more readable.

sharanyad · 2019-02-01T18:59:50Z

agent/api/task/task_test.go

+		Containers:         []*apicontainer.Container{container, container1},
+	}
+
+	task.initializeContainerOrderingForVolumes()


can you assert error returned from this method as well?

sharanyad · 2019-02-01T19:02:35Z

agent/api/task/task_test.go

@@ -2816,3 +2816,129 @@ func TestAssociationByTypeAndName(t *testing.T) {
 	association, ok = task.AssociationByTypeAndName("other-type", "dev1")
 	assert.False(t, ok)
 }
+


You could think about using table driven tests for some of these

I have reduced the number of tests by combining the cases in one...It brought down the number of tests to 1 and also checking all scenarios.

consider making the container names mirror the use case of the test.

ubhattacharjya · 2019-02-01T20:12:22Z

Are we deprecating the existing ContainerDependency model and moving to DependsOn ?

I have not yet made that change. I have just implemented the initial adapter to populate the DependsOn field of containers with volumes and link containers.

sharanyad · 2019-02-01T23:18:30Z

agent/api/task/task_test.go

+
+	err := task.initializeContainerOrderingForVolumes()
+	assert.Error(t, err)
+}


Please add negative test cases for links as well.

adnxn · 2019-02-04T21:58:24Z

agent/api/task/task.go

+			for _, volume := range container.VolumesFrom {
+				if _, ok := task.ContainerByName(volume.SourceContainer); !ok {
+					return fmt.Errorf("could not find container with name %s", volume.SourceContainer)
+				} else {


nit: do we need the else here? if !ok then the we return an error anyways.

adnxn · 2019-02-04T22:00:10Z

agent/api/task/task.go

+				linkName := linkParts[0]
+				if _, ok := task.ContainerByName(linkName); !ok {
+					return fmt.Errorf("could not find container with name %s", linkName)
+				} else {


nit: same as above, doesn't seem like we need the else branch here.

Thanks for pointing out. Removed the else.

adnxn · 2019-02-04T22:14:55Z

also - are you expecting all tests to be passing right now?

ubhattacharjya · 2019-02-04T22:35:17Z

also - are you expecting all tests to be passing right now?

The tests should pass. As I have not touched any existing logic

adnxn · 2019-02-06T17:38:15Z

is this the same windows integ test that @sharanyad references here? #1833 (comment)

ubhattacharjya · 2019-02-06T18:50:05Z

is this the same windows integ test that @sharanyad references here? #1833 (comment)

I think so...TestVolumesFromRO this is the one which is timing out here. I have reinitialted the tests a few times. TestVolumesFromRO passed once and some other test timed out at that time. So I guess it should be unrelated to my change.

adnxn

ubhattacharjya requested a review from petderek January 31, 2019 22:08

petderek reviewed Jan 31, 2019

View reviewed changes

petderek requested a review from sharanyad January 31, 2019 22:34

ubhattacharjya force-pushed the ContainerOrderingVolumesLinksAgain branch from 5277886 to c5879c6 Compare February 1, 2019 19:07

sharanyad reviewed Feb 1, 2019

View reviewed changes

ubhattacharjya requested review from sharanyad and a team February 1, 2019 19:09

ubhattacharjya force-pushed the ContainerOrderingVolumesLinksAgain branch from c5879c6 to 7da495c Compare February 1, 2019 20:07

sharanyad reviewed Feb 1, 2019

View reviewed changes

ubhattacharjya force-pushed the ContainerOrderingVolumesLinksAgain branch 4 times, most recently from 6f5be32 to 57de8d1 Compare February 4, 2019 21:35

petderek approved these changes Feb 4, 2019

View reviewed changes

ubhattacharjya force-pushed the ContainerOrderingVolumesLinksAgain branch 2 times, most recently from 93bba69 to fe31655 Compare February 4, 2019 21:51

sharanyad approved these changes Feb 4, 2019

View reviewed changes

adnxn reviewed Feb 4, 2019

View reviewed changes

ubhattacharjya force-pushed the ContainerOrderingVolumesLinksAgain branch from fe31655 to 246ab82 Compare February 4, 2019 22:33

Adding adapter to have volumes/links use DependsOn field

08567bb

ubhattacharjya force-pushed the ContainerOrderingVolumesLinksAgain branch from 246ab82 to 08567bb Compare February 5, 2019 01:04

ubhattacharjya added bot/test and removed bot/test labels Feb 5, 2019

ubhattacharjya added the bot/test label Feb 5, 2019

sharanyad mentioned this pull request Feb 6, 2019

testing: Windows Flakey integ tests #1835

Closed

adnxn approved these changes Feb 7, 2019

View reviewed changes

ubhattacharjya merged commit 5fbfab0 into aws:container-ordering-feature Feb 7, 2019

ubhattacharjya deleted the ContainerOrderingVolumesLinksAgain branch April 8, 2019 21:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding adapter to have volumes/links use DependsOn field #1830

Adding adapter to have volumes/links use DependsOn field #1830

ubhattacharjya commented Jan 31, 2019

petderek Jan 31, 2019 •

edited

Loading

petderek Jan 31, 2019

ubhattacharjya Feb 1, 2019

sharanyad left a comment

sharanyad Feb 1, 2019

sharanyad Feb 1, 2019

sharanyad Feb 1, 2019

ubhattacharjya Feb 1, 2019

petderek Feb 4, 2019

ubhattacharjya commented Feb 1, 2019 •

edited by adnxn

Loading

sharanyad Feb 1, 2019

adnxn Feb 4, 2019

adnxn Feb 4, 2019

ubhattacharjya Feb 4, 2019

adnxn commented Feb 4, 2019

ubhattacharjya commented Feb 4, 2019

adnxn commented Feb 6, 2019 •

edited

Loading

ubhattacharjya commented Feb 6, 2019

adnxn left a comment

Adding adapter to have volumes/links use DependsOn field #1830

Adding adapter to have volumes/links use DependsOn field #1830

Conversation

ubhattacharjya commented Jan 31, 2019

Summary

Implementation details

Testing

Description for the changelog

Licensing

petderek Jan 31, 2019 • edited Loading

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

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

ubhattacharjya commented Feb 1, 2019 • edited by adnxn 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

adnxn commented Feb 4, 2019

ubhattacharjya commented Feb 4, 2019

adnxn commented Feb 6, 2019 • edited Loading

ubhattacharjya commented Feb 6, 2019

adnxn left a comment

Choose a reason for hiding this comment

petderek Jan 31, 2019 •

edited

Loading

ubhattacharjya commented Feb 1, 2019 •

edited by adnxn

Loading

adnxn commented Feb 6, 2019 •

edited

Loading