-
Notifications
You must be signed in to change notification settings - Fork 618
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.
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
metadata service continued #981
Changes from 16 commits
6e9aadd
ed6aaef
203ec87
d04bb67
14274b3
9d5d867
011b1d4
b2ee17c
a152504
cdf61a5
b06c3f3
9e0d80f
4640038
508d9e5
de5fa33
d5146c5
13fb758
68fa765
b4f68c6
e9fd7cf
6817a23
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -123,6 +123,10 @@ type Container struct { | |
// handled properly so that the state storage continues to work. | ||
SentStatusUnsafe ContainerStatus `json:"SentStatus"` | ||
|
||
// MetadataFileUpdated is set to true when we have completed updating the | ||
// metadata file | ||
MetadataFileUpdated bool `json:"metadataFileUpdated"` | ||
|
||
knownExitCode *int | ||
KnownPortBindings []PortBinding | ||
|
||
|
@@ -317,3 +321,20 @@ func (c *Container) IsInternal() bool { | |
func (c *Container) IsRunning() bool { | ||
return c.GetKnownStatus().IsRunning() | ||
} | ||
|
||
// IsMetadataFileUpdated() returns true if the metadata file has been once the | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you get rid of the parenthesis in the comment? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yep gone. |
||
// metadata file is ready and will no longer change | ||
func (c *Container) IsMetadataFileUpdated() bool { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lint: Please add a comment here for public methods, same for below. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed |
||
c.lock.RLock() | ||
defer c.lock.RUnlock() | ||
|
||
return c.MetadataFileUpdated | ||
} | ||
|
||
// SetMetadataFileUpdated sets the container's MetadataFileUpdated status to true | ||
func (c *Container) SetMetadataFileUpdated() { | ||
c.lock.Lock() | ||
defer c.lock.Unlock() | ||
|
||
c.MetadataFileUpdated = true | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -353,6 +353,8 @@ func environmentConfig() (Config, error) { | |
errs = append(errs, err) | ||
} | ||
} | ||
containerMetadataEnabled := utils.ParseBool(os.Getenv("ECS_ENABLE_CONTAINER_METADATA"), false) | ||
dataDirOnHost := os.Getenv("ECS_HOST_DATA_DIR") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens when ECS Init/whatever's starting the Agent mounts some other directory as Agent's data directory ( It doesn't seem like a strong enough abstraction to be dependent on Agent configuration options to expect There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can get rid of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @aaithal hm you're right, there's a failure case here. i agree it makes sense to validate ECS_HOST_DATA_DIR is mounted by inspecting the Agent container. however, i'm not sure if it makes more sense to fail during agent creation with a configuration error since the user will be expecting the metadata feature to be enabled, or if it would make more sense for us to surface a warning log and just disable the metadata feature. i think failing explicitly during agent creation, or do you have a more sensible way to communicate this error to the user? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I agree There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @aaithal so as per our discussion earlier, we will call this out in the README under usage instructions for the metadata feature. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are a few assumptions here that I think are problematic:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adding to samuelkarp's second point, I believe the metadata feature does not work when the agent is run outside a container on Linux environments. Initially I was told this is a minor concern, but do we have use cases for the agent being run outside a container on Linux? For Windows, this situation is explicitly handled (As noted in the current documentation). |
||
|
||
if len(errs) > 0 { | ||
err = utils.NewMultiError(errs...) | ||
|
@@ -393,6 +395,8 @@ func environmentConfig() (Config, error) { | |
CNIPluginsPath: cniPluginsPath, | ||
AWSVPCBlockInstanceMetdata: awsVPCBlockInstanceMetadata, | ||
AWSVPCAdditionalLocalRoutes: additionalLocalRoutes, | ||
ContainerMetadataEnabled: containerMetadataEnabled, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you also update |
||
DataDirOnHost: dataDirOnHost, | ||
}, err | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -120,6 +120,10 @@ func TestEnvironmentConfig(t *testing.T) { | |
additionalLocalRoutesJSON := `["1.2.3.4/22","5.6.7.8/32"]` | ||
os.Setenv("ECS_AWSVPC_ADDITIONAL_LOCAL_ROUTES", additionalLocalRoutesJSON) | ||
defer os.Unsetenv("ECS_AWSVPC_ADDITIONAL_LOCAL_ROUTES") | ||
os.Setenv("ECS_ENABLE_CONTAINER_METADATA", "true") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not checked below? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed. added check below. |
||
os.Setenv("ECS_HOST_DATA_DIR", "/etc/ecs/") | ||
defer os.Unsetenv("ECS_ENABLE_CONTAINER_METADATA") | ||
defer os.Unsetenv("ECS_HOST_DATA_DIR") | ||
|
||
conf, err := environmentConfig() | ||
assert.NoError(t, err) | ||
|
@@ -146,6 +150,9 @@ func TestEnvironmentConfig(t *testing.T) { | |
serializedAdditionalLocalRoutesJSON, err := json.Marshal(conf.AWSVPCAdditionalLocalRoutes) | ||
assert.NoError(t, err, "should marshal additional local routes") | ||
assert.Equal(t, additionalLocalRoutesJSON, string(serializedAdditionalLocalRoutesJSON)) | ||
assert.Equal(t, (90 * time.Second), conf.TaskCleanupWaitDuration) | ||
assert.Equal(t, "/etc/ecs/", conf.DataDirOnHost, "Wrong value for DataDirOnHost") | ||
assert.True(t, conf.ContainerMetadataEnabled, "Wrong value for ContainerMetadataEnabled") | ||
} | ||
|
||
func TestTrimWhitespace(t *testing.T) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
// Copyright 2017 Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
// | ||
// Licensed under the Apache License, Version 2.0 (the "License"). You may | ||
// not use this file except in compliance with the License. A copy of the | ||
// License is located at | ||
// | ||
// http://aws.amazon.com/apache2.0/ | ||
// | ||
// or in the "license" file accompanying this file. This file is distributed | ||
// on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either | ||
// express or implied. See the License for the specific language governing | ||
// permissions and limitations under the License. | ||
|
||
package containermetadata | ||
|
||
//go:generate go run ../../scripts/generate/mockgen.go github.com/aws/amazon-ecs-agent/agent/containermetadata Manager,DockerMetadataClient mocks/containermetadata_mocks.go |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
in the case the ECS Agent is running as a container xxxxx because the ECS Agent is not running as a container
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.