Skip to content
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

Fix error detection case when image that is being deleted does not exist - continued. #2008

Conversation

bendavies
Copy link
Contributor

Summary

Corrects the fix provided in #1897.

We were experiencing images not being removed and upgraded ecs-agent to fix the issue, after seeing the above PR.
It did not fix the issue.

Investigation revealed that the above PR does not fix the issue (at least for more recent docker versions because the actual error return from docker is Error: No such image - capital N.

Implementation details

Make the string contains check for no such image case insensitive.

Testing

  • update test TestDeleteImageNotFoundError to make the error the correct case.
  • left test TestDeleteImageNotFoundOldDockerMessageError alone, using existing lower case error message. I am not sure if that is the correct case for old docker versions, but now we have 2 tests covering both cases.

New tests cover the changes: yes, corrected existing.

Description for the changelog

Bug - Fix cleaning up images when some images are not found. Continuation of #1897

Licensing

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

@bendavies
Copy link
Contributor Author

bendavies commented Apr 29, 2019

By the way, can someone explain to me how the original report #1871 was fixed?
The original report was that processing of image removal stops abruptly when the first not found image was encountered, but I can't follow or see how that occurs at all. I'm not a Go person however, so that might explain it :)

@yhlee-aws
Copy link
Contributor

Could you fix the error reported by travis and update this PR please?
agent\engine\docker_image_manager.go:541:23: cannot refer to unexported name strings.toLower
agent\engine\docker_image_manager.go:541:23: undefined: strings.toLower

Also, PR #1897 is the PR that's fixing issue #1871.

@bendavies bendavies force-pushed the fix-docker-image-not-found-during-cleanup-case-sensitivity branch from d57c385 to 811fbcc Compare April 29, 2019 21:38
@bendavies
Copy link
Contributor Author

hi @yunhee-l thanks - fixed.
Yes, I mention #1897 in my OP, but I can't see how it fixes the symptom of image removal being halted after encountering a not found image, as described in #1871. Maybe i'm missing something obvious.

@bendavies
Copy link
Contributor Author

ping @shubham2892 as the original fixer

@shubham2892
Copy link
Contributor

By the way, can someone explain to me how the original report #1871 was fixed?

@bendavies Thanks for looking into this.

As far as I remember, it's related to the cleaning that happens after the If statement, feel free to explore this case more, it's possible I might have missed something.

Copy link
Contributor

@shubham2892 shubham2892 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bendavies Can you rebase the branch against dev please. Thanks.

@bendavies bendavies force-pushed the fix-docker-image-not-found-during-cleanup-case-sensitivity branch from 811fbcc to 23ab6ed Compare May 14, 2019 11:38
@bendavies bendavies changed the base branch from master to dev May 14, 2019 11:39
@bendavies
Copy link
Contributor Author

@shubham2892 done

@bendavies
Copy link
Contributor Author

will this make it into V1.28.0?

@yhlee-aws
Copy link
Contributor

It should be in V1.29.0.

@fenxiong fenxiong added this to the 1.28.1 milestone May 30, 2019
@fenxiong fenxiong mentioned this pull request May 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants