-
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
Enable SubmitTaskStateChange for early digest reporting #4169
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…n to MANIFEST_PULLED state
Yiyuanzzz
previously approved these changes
May 10, 2024
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.
Thanks for the detailed implementation description!
prateekchaudhry
approved these changes
May 13, 2024
Yiyuanzzz
approved these changes
May 13, 2024
amogh09
added a commit
that referenced
this pull request
May 13, 2024
amogh09
added a commit
that referenced
this pull request
May 21, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR enables SubmitTaskStateChange (STSC) calls from Agent to ECS backend for early reporting of container image manifest digests. Containers that are in-scope of digest resolution (non-internal containers and containers whose image reference does not already have a digest) will have their image manifest digests reported to ECS backend with an additional STSC call that Agent will make after the task has reached
MANIFEST_PULLED
state. The container states in this additional STSC call will be "PENDING" which is a backend-recognized state for pending containers.This additional STSC call will be made only if there is at least one container for which digest was successfully resolved during transition to
MANIFEST_PULLED
state.An example of this new STSC call is shown below.
Implementation details
*Container.DigestResolutionRequired
method that checks if a container requires digest resolution. This method is now called in container state transition function forMANIFEST_PULLED
state to perform digest resolution only for containers that need it. The method returnstrue
for non-internal containers whoseImage
field does not already contain a digest.*Container.DigestResolved
method that checks if the container has had its image manifest digest resolved. The method is used to prevent emitting a container change event for containers ineligible for digest reporting (due to it being internal or having digest available in itsImage
field or due to a failure in digest resolution).*Task.HasAContainerWithResolvedDigest
method that checks if the task has at least one container with a successfully resolved digest. The method is used to prevent emitting a task change event for tasks that don't have any resolved digests to report.NewTaskStateChangeEvent
method is updated to generate a task state change event for transition toMANIFEST_PULLED
state if at least one of task's containers has a resolved digest.NewContainerStateChangeEvent
method is updated to generate a container state change event for transition toMANIFEST_PULLED
state if the container's digest was resolved. A new helper functioncontainerStatusChangeStatus
for mapping container's known status to a status eligible for STSC reporting is added which replaces*ContainerStatus.BackendStatus
method fromecs-agent
module. The reason to move the function toagent
module is that the function has implementation details of ECS Agent which are unrelated to the more general nature ofecs-agent
module. In addition to the existing logic of*ContainerStatus.BackendStatus
method, the new function also mapsContainerManifestPulled
state toContainerManifestPulled
state making the state eligible for STSC reporting.api.buildContainerStateChangePayload
method, that builds a container state change payload for STSC calls, is updated to allowContainerManifestPulled
state changes to go through and use the recently added (Add BackendStatusString method to ContainerStatus #4167)ContainerStatus.BackendStatusString
method to map the state to "PENDING" container state which is backend-recognized.*DockerTaskEngine.pullContainerManifest
method, which is the container state transition function forMANIFEST_PULLED
state, is updated to not resolve digest for containers that have digest populated in theirImage
field. This is because digest resolution is not needed for such containers and not populatingImageDigest
field during transition toMANIFEST_PULLED
state helps detect that the container's digest does not need to be reported early. It will be reported as usual after the container's image is pulled.*managedTask.emitTaskEvent
method, that is used to generate and submit task state change events, is updated to allow transitions toMANIFEST_PULLED
states to be reported. This gating is probably redundant given that it's performed inNewTaskStateChangeEvent
method also but this PR is keeping this additional gating as removing it is considered out-of-scope.Testing
Given that the change affects all tasks, a LOT of existing unit and integration tests have been updated to check that state change events for
MANIFEST_PULLED
container and task states are emitted or not emitted depending on the specific test cases.Thorough unit tests are added for all new functions introduced in this PR.
Manual testing was performed and it was observed that -
New tests cover the changes: yes
Description for the changelog
Feature: Early reporting of image manifest digests for eligible containers
Does this PR include breaking model changes? If so, Have you added transformation functions?
Licensing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.