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

make image sort stable, and correct #1247

Merged
merged 1 commit into from Jul 24, 2018

Conversation

@rade
Copy link
Contributor

commented Jul 24, 2018

The ByCreatedDesc sort was broken in two ways:

  1. If there were two images with a zero CreatedAt time, the sort order would depend on the order in which they appeared in the orginal (unsorted) list, thus making the order unstable.

  2. Images with a zero CreatedAt time were considered more recent than any images with a non-zero CreatedAt time, which is the exact opposite of what we want.

These bugs may be the cause of the 'flipping' between different images we are seeing in #1127.

@rade rade requested a review from squaremo Jul 24, 2018

make image sort stable, and correct
The ByCreatedDesc sort was broken in two ways:

1. If there were two images with a zero CreatedAt time, the sort order
would depend on the order in which they appeared in the
orginal (unsorted) list, thus making the order unstable.

2. Images with a zero CreatedAt time were considered more recent than
any images with a non-zero CreatedAt time, which is the exact opposite
of what we want.

These bugs _may_ be the cause of the 'flipping' between different
images we are seeing in #1127.

@rade rade force-pushed the stable-image-sort branch from d3ceea6 to aca71d8 Jul 24, 2018

@squaremo
Copy link
Member

left a comment

LGTM, thanks Matthias 🎇

checkSorted(t, imgs)
}

func checkSorted(t *testing.T, imgs []Info) {

This comment has been minimized.

Copy link
@squaremo

squaremo Jul 24, 2018

Member

Instead of this we could check the property of being sorted, that is, a_n < a_n+1.

This comment has been minimized.

Copy link
@rade

rade Jul 24, 2018

Author Contributor

Yes, but that would involve writing a < predicate, for which it would be rather tempting to copy the one we have in the code, which rather defeats the purpose of the test - we are not testing Sort here but the predicate.

@rade rade merged commit 5bd3929 into master Jul 24, 2018

1 check passed

ci/circleci: build Your tests passed on CircleCI!
Details
rade added a commit that referenced this pull request Jul 24, 2018
prevent bogus manifest fetching success
An error while fetching an image manifest would return a nil
error (hence indicating success) with a unit value image.Info{}
struct.

That is bad news for the caller in Warmer.warm(), which will map an
image tag to that empty image.Info{}, polluting the cache entry for
the image+tag and image in memcached.

When we subsequently use this info to determine the latest suitable
tag, we encounter zero CreatedAt timestamps, which, prior to the
changes in #1247, #1249 and #1250 would cause the wrong images to be
released.

Fixes #1127.

@hiddeco hiddeco deleted the stable-image-sort branch Apr 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.