engine: update container metadata when container status stays the same #1972
Conversation
| @@ -422,11 +428,6 @@ func (mtask *managedTask) handleContainerChange(containerChange dockerContainerC | |||
| } | |||
| } | |||
|
|
|||
| // Update the container health status | |||
| if container.HealthStatusShouldBeReported() { | |||
There was a problem hiding this comment.
Could you explain a bit why this block of code is deleted?
There was a problem hiding this comment.
updateContainerMetadata already has this part of logic.
agent/engine/task_manager.go
Outdated
|
|
||
| // Only update container metadata when status keeps the same, ignore updating metadata if there | ||
| // is backward transition | ||
| if event.Status == containerKnownStatus { |
There was a problem hiding this comment.
should we explicitly check for RUNNING status if the use case covers only that?
There was a problem hiding this comment.
I will update it to start from only covering RUNNING since so far I haven't found out other use cases.
yumex93
force-pushed
the
fix-health-status
branch
from
5eccffc
to
1887d82
Compare
| @@ -407,6 +407,11 @@ func (mtask *managedTask) handleContainerChange(containerChange dockerContainerC | |||
| if event.Status <= containerKnownStatus { | |||
| seelog.Infof("Managed task [%s]: redundant container state change. %s to %s, but already %s", | |||
| mtask.Arn, container.Name, event.Status.String(), containerKnownStatus.String()) | |||
|
|
|||
| // Only update container metadata when status stays RUNNING | |||
| if event.Status == containerKnownStatus && event.Status == apicontainerstatus.ContainerRunning { | |||
There was a problem hiding this comment.
I wonder if we are updating the container metadata without the container state change, should we do this checking "if event.Status == containerKnownStatus" independently outside this "if" block and update the log with this change? Also if we only update container metadata when status stays RUNNING, maybe checking "event.Status == apicontainerstatus.ContainerRunning" before "event.Status == containerKnownStatus" makes more sense?
There was a problem hiding this comment.
I don't really wanna have repetitive code for logging, that's why the if is inside the block. And what's the reason behind checking "event.Status == apicontainerstatus.ContainerRunning" first? I think it does not really make a big difference.
Agent discards container metadata update for redundant container status changing, however, for some cases, even the status is not changed, the metadata may change. Using container health status as an example, there is situation that when container status just transits to RUNNING, health status is still UNKNOWN and after some time, it changes to HEALTHY.
Summary
Implementation details
Testing
New tests cover the changes:
yes
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.