add envfile as task resource and retrieve from s3 #2383

jy19 · 2020-03-04T00:59:04Z

and write s3 contents to disk

Summary

Getting this pull request out to get it looked at. Using envfiles as a task resource isn't fully piped through yet.

Implementation details

envfile task resource -> retrieve each specified file from s3 and write contents to disk.

Testing

make test

New tests cover the changes: yes (unit tests)

Description for the changelog

Add envfile as task resource

Licensing

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

agent/taskresource/envFiles/envfile.go

yhlee-aws · 2020-03-04T19:07:56Z

agent/taskresource/envFiles/envfile.go

+	taskARN   string
+	createdAt time.Time
+	region    string
+	resourceDir string // path to store env var files


We are not capturing the "type" value (currently only s3) in this object, and implementation including error message assumes s3. We don't have to over engineer ant this point, but it'd be a good data to capture and at least make it available for future iteration.

good point, will add

on second thought, "type" wasn't included because we have a list of the environment files - each object in the list could be of a different type (if we ever support anything besides s3).
concerns/thoughts?

can't we store the type per file? we are accepting a list of env files, are you modeling the entire list as 1 task resource?

type IS stored per file - each EnvironmentFile object has a Value and Type. Unless you mean each file should be a separate resource? So each object in the list is its own resource? I've currently modeled the entire list as 1 task resource because that's how it's defined from the payload.

agent/taskresource/envFiles/envfile.go

agent/taskresource/envFiles/envfilestatus.go

agent/taskresource/envFiles/envfile.go

fenxiong · 2020-03-05T17:16:23Z

agent/taskresource/envFiles/envfile.go

+type EnvironmentFileResource struct {
+	cluster     string
+	taskARN     string
+	createdAt   time.Time


why this while there's a createdAtUnsafe below?

removed this and renamed places where it was accessed to createdAtUnsafe since it required a lock.

fenxiong · 2020-03-05T17:21:44Z

agent/taskresource/envFiles/envfile.go

+	lock                sync.RWMutex
+}
+
+func NewEnvironmentFileResource(cluster, taskARN, region, dataDirOnHost string, credentialsManager credentials.Manager,


i think where the code needs to write to is under ECS_DATADIR (by default /data), rather than under ECS_HOST_DATA_DIR (by default /var/lib/ecs),so dataDirOnHost should probably name as dataDir instead, and you don't really need to know the host dir here

/var/lib/ecs is what was decided in the design doc. lmk concerns around this

agent container has a bind mount /var/lib/ecs/data:/data. if you want to write to /var/lib/ecs/data on the host, you need to write to /data in the code.

sure i'll change the name here and make sure to pass in ECS_DATADIR when this method is called

agent/taskresource/envFiles/envfile.go

fenxiong · 2020-03-05T17:32:01Z

agent/taskresource/envFiles/envfile.go

+	return nil
+}
+
+func (envfile *EnvironmentFileResource) writeEnvFile(writeFunc func(file oswrapper.File) error, fullPathName string) error {


are we supporting windows? if so, writing file in windows is different. refer to agent/statemanager/state_manager_windows.go

nvm i see that credentialspec is using the same code, so it's probably fine

agent/taskresource/envFiles/envfile.go

yumex93 · 2020-03-06T23:34:03Z

agent/taskresource/envFiles/envfile.go

+
+	taskARNFields := strings.Split(taskARN, "/")
+	taskID := taskARNFields[len(taskARNFields)-1]
+	// we save envfiles to path: /var/lib/ecs/data/envfiles/cluster_name/task_id/${s3filename.env}


what does this s3filename.env mean?

the name of the s3 file but it is required to have extension .env

yumex93 · 2020-03-06T23:38:21Z

agent/taskresource/envFiles/envfile.go

+	// so this should be ok to be called by multiple go routines
+	tmpFile, err := envfile.ioutil.TempFile(envfile.resourceDir, envTempFilePrefix)
+	if err != nil {
+		seelog.Infof("something went wrong trying to create a temp file with prefix %s", envTempFilePrefix)


The log message should start with capital letter. Also, I think this should be Errorf, since the file downloading will fail here.

changed the logs in err != nil statements to be Errorf and capitalized letters

yumex93 · 2020-03-06T23:46:14Z

agent/taskresource/envFiles/envfile.go

+}
+
+// UnmarshalJSON deserializes the raw JSON to an EnvironmentFileResource struct
+func (envfile *EnvironmentFileResource) UnmarshalJSON(b []byte) error {


I think you miss the assignment of executionCredentialsID here.

oops you're right, also task arn. thanks for catching

fenxiong

couple small comments, mostly around lock..

fenxiong · 2020-03-06T23:36:29Z

agent/taskresource/envFiles/envfile.go

+
+// SetKnownStatus safely sets the currently known status of the resource
+func (envfile *EnvironmentFileResource) SetKnownStatus(status resourcestatus.ResourceStatus) {
+	envfile.lock.RLock()


should be Lock/Unlock

i didn't look at my RLock/RUnlock(s) vs Lock/Unlocks carefully based on the method, my bad. should all be fixed now.

fenxiong · 2020-03-06T23:36:44Z

agent/taskresource/envFiles/envfile.go

+		return
+	}
+
+	envfile.lock.RLock()


should be Lock/Unlock

agent/taskresource/envFiles/envfile.go

fenxiong · 2020-03-06T23:38:41Z

agent/taskresource/envFiles/envfile.go

+
+// GetName returns the name fo the resource
+func (envfile *EnvironmentFileResource) GetName() string {
+	envfile.lock.RLock()


nit: this doesn't need a lock

fenxiong · 2020-03-06T23:39:09Z

agent/taskresource/envFiles/envfile.go

+// SetAppliedStatus sets the applied status of the resource and returns whether
+// the resource is already in a transition
+func (envfile *EnvironmentFileResource) SetAppliedStatus(status resourcestatus.ResourceStatus) bool {
+	envfile.lock.RLock()


should be Lock/Unlock

agent/taskresource/envFiles/envfile.go

fenxiong · 2020-03-06T23:43:30Z

agent/taskresource/envFiles/envfile.go

+	resourceDir string // path to store env var files
+
+	// env file related attributes
+	environmentFilesSourceUnsafe []apicontainer.EnvironmentFile // list of env file objects


does this field ever change after initializing from NewEnvironmentFileResource? seems like it doesn't change so don't need lock or be marked as unsafe

right it doesn't change, was thinking of something else. unmarked as unsafe.

fenxiong · 2020-03-06T23:47:54Z

agent/taskresource/envFiles/envfile.go

+	return envfile.environmentFilesSourceUnsafe
+}
+
+func (envfile *EnvironmentFileResource) getExecutionCredentialsID() string {


nit: same as environmentFileSource, i don't quite see why you need a lock for executionCredentialsID, since no one writes to it. read can be concurrent without lock

oh i thought the credentials get rotated so it might change. i must've remembered wrong, credentials rotating during task resource creation seems unlikely

fenxiong · 2020-03-06T23:49:04Z

agent/taskresource/envFiles/envfile.go

+	CreatedAt              *time.Time                     `json:"createdAt,omitempty"`
+	DesiredStatus          *EnvironmentFileStatus         `json:"desiredStatus"`
+	KnownStatus            *EnvironmentFileStatus         `json:"knownStatus"`
+	EnvironmentFilesSource []apicontainer.EnvironmentFile `json:"envfileResources"`


nit: maybe envFileSources? should match the field name

made sure matches

fenxiong · 2020-03-06T23:52:19Z

agent/taskresource/envFiles/envfile.go

+	return nil
+}
+
+type EnvironmentFileResourceJSON struct {


i don't think this need to be exported?

un-exported

yumex93 · 2020-03-07T00:50:23Z

agent/taskresource/envFiles/envfile.go

+	envfile.credentialsManager = resourceFields.CredentialsManager
+	envfile.s3ClientCreator = factory.NewS3ClientCreator()
+	envfile.os = oswrapper.NewOS()
+	envfile.ioutil = ioutilwrapper.NewIOUtil()


There is one thing here you may want to think about is whether we want to redownload the S3 file when agent crash and restart during resource CREATED but task hasn't go to RUNNING. if we want to redownload, we need to reset the task resource status back to none here.

i think you're right. i guess if agent crashes we'd jsut re-download everything again, even if files were previously partially downloaded.

agent/taskresource/envFiles/envfilestatus.go

yhlee-aws · 2020-03-09T16:18:35Z

Travis check is failing for all 3 platforms with "The command "make static-check" exited with 2"

jy19 · 2020-03-09T18:28:48Z

go vet is failing on something related to the locks, am fixing.

petderek · 2020-03-09T21:27:09Z

agent/taskresource/envFiles/envfile_test.go

@@ -0,0 +1,260 @@
+// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved.


this needs a:

// +build unit

at the top

… and writing to disk

jy19 requested review from yhlee-aws and fenxiong March 4, 2020 00:59

yhlee-aws reviewed Mar 4, 2020

View reviewed changes

jy19 force-pushed the env_files branch from 7054493 to 6eec769 Compare March 4, 2020 19:23

yumex93 reviewed Mar 5, 2020

View reviewed changes

agent/taskresource/envFiles/envfile.go Show resolved Hide resolved

yumex93 reviewed Mar 5, 2020

View reviewed changes

agent/taskresource/envFiles/envfile.go Outdated Show resolved Hide resolved

yumex93 reviewed Mar 5, 2020

View reviewed changes

agent/taskresource/envFiles/envfile.go Outdated Show resolved Hide resolved

fenxiong reviewed Mar 5, 2020

View reviewed changes

jy19 force-pushed the env_files branch 2 times, most recently from c255ace to d48fc2c Compare March 6, 2020 22:41

yumex93 reviewed Mar 6, 2020

View reviewed changes

agent/taskresource/envFiles/envfile.go Show resolved Hide resolved

jy19 force-pushed the env_files branch 2 times, most recently from 5f7a0f9 to c95cc31 Compare March 6, 2020 23:22

yumex93 reviewed Mar 6, 2020

View reviewed changes

fenxiong reviewed Mar 6, 2020

View reviewed changes

yumex93 reviewed Mar 7, 2020

View reviewed changes

jy19 force-pushed the env_files branch 2 times, most recently from 1e4f610 to 91c14af Compare March 7, 2020 03:33

yumex93 approved these changes Mar 8, 2020

View reviewed changes

fenxiong approved these changes Mar 9, 2020

View reviewed changes

agent/taskresource/envFiles/envfilestatus.go Outdated Show resolved Hide resolved

jy19 force-pushed the env_files branch from 91c14af to 7d5dbb4 Compare March 9, 2020 18:36

petderek reviewed Mar 9, 2020

View reviewed changes

untested code of envfile as task resource + downloading stuff from s3…

aba9cd0

… and writing to disk

jy19 force-pushed the env_files branch from 7d5dbb4 to aba9cd0 Compare March 9, 2020 21:29

jy19 merged commit 40ddd41 into aws:env_files Mar 9, 2020

		@@ -0,0 +1,260 @@
		// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved.

add envfile as task resource and retrieve from s3 #2383

add envfile as task resource and retrieve from s3 #2383

Conversation

jy19 commented Mar 4, 2020

Summary

Implementation details

Testing

Description for the changelog

Licensing

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

fenxiong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jy19 Mar 7, 2020 • 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

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 commented Mar 9, 2020

jy19 commented Mar 9, 2020

Choose a reason for hiding this comment

jy19 Mar 7, 2020 •

edited

Loading