Set initial timestamp to the time the image was created #73

mac-chaffee · 2020-07-17T23:39:02Z

Previously, when docuum first starts, it would set the timestamp of every image to the time docuum started, meaning old images would be removed at random. Now, we use the time the image was created as the initial timestamp (which is of course updated when that image is used like normal).

Example running locally (with comments added to show the original CreatedAt dates):

$ cargo run -- --threshold '50 GB'
   Compiling docuum v0.9.5 (/Users/machaffe/personal/docuum)
    Finished dev [unoptimized + debuginfo] target(s) in 3.81s
     Running `target/debug/docuum --threshold '50 GB'`
[2020-07-17 18:20:11 -04:00 INFO] Waking up…
[2020-07-17 18:20:11 -04:00 INFO] Docker images are currently using 75.27 GB but the limit is 50.00 GB. Some images will be deleted.
[2020-07-17 18:20:12 -04:00 INFO] Deleting image sha256:3129a2ca29d75226dc5657a4629cdd5f38accda7f5b75bc5d5a76f5b4e0e5870…
Untagged: quay.io/coreos/configmap-reload:v0.0.1            <--- CreatedAt Feb 2017
Untagged: quay.io/coreos/configmap-reload@sha256:e2fd60ff0ae4500a75b80ebaa30e0e7deba9ad107833e8ca53f0047c42c5a057
Deleted: sha256:3129a2ca29d75226dc5657a4629cdd5f38accda7f5b75bc5d5a76f5b4e0e5870
Deleted: sha256:0065890d4ef6d9e21ba99dea6a990abe8b53db0b82562fe68dd3f37ef529b156
Deleted: sha256:91bd48b9e0b007ce035cccf10d1dc7e482695cf492c94808875fa84e71a85482
[2020-07-17 18:20:12 -04:00 INFO] Deleting image sha256:2faf6f7a322f5f8d565e05675d5ab6f4d1b4a4f65f88cb14df15e76b902b0536…
Untagged: quay.io/coreos/hyperkube:v1.7.6_coreos.0           <--- CreatedAt Sep 2017
Untagged: quay.io/coreos/hyperkube@sha256:699fc03fccb1c4662fee9d996cf30106aea55a6f594d16c1504cc7334dadcee4
Deleted: sha256:2faf6f7a322f5f8d565e05675d5ab6f4d1b4a4f65f88cb14df15e76b902b0536
Deleted: sha256:e5f68041508088cc8a24342143cb911297b424db7cfd8c2f4362124382a9eaf3
Deleted: sha256:3b91c2286ccf87e35ab72d4fbcc66f7b0d6834408c69726d250d778759c75359
Deleted: sha256:713bebfb34f9e43249fd9c44b3c935a0fcdc8575a02822f4fbfc65f6fd6a89d4
Deleted: sha256:c18c4811ea9f56e6561adc47460f2a42937e33b3e6782911ea4a3f36f01438c1
Deleted: sha256:fed386583ce4ea496bad7914fc503d437f8ba8d3590fb7c69ed8c526f879d6c0
Deleted: sha256:18f9b4e2e1bcd5abe381a557c44cba379884c88f6049564f58fd8c10ab5733df
[2020-07-17 18:20:14 -04:00 INFO] Deleting image sha256:da86e6ba6ca197bf6bc5e9d900febd906b133eaa4750e6bed647b0fbe50ed43e…
Error response from daemon: conflict: unable to delete da86e6ba6ca1 (cannot be forced) - image is being used by running container ac477f13429f
[2020-07-17 18:20:14 -04:00 ERROR] Unable to delete image sha256:da86e6ba6ca197bf6bc5e9d900febd906b133eaa4750e6bed647b0fbe50ed43e.
[2020-07-17 18:20:14 -04:00 INFO] Deleting image sha256:c81481184b1b001f225769d1482b360b4be18fe7dd53fa93cdbd45d568e905a7…
Untagged: justincormack/nsenter1:latest                      <--- CreatedAt Jan 2018
Untagged: justincormack/nsenter1@sha256:5af0be5e42ebd55eea2c593e4622f810065c3f45bb805eaacf43f08f3d06ffd8
Deleted: sha256:c81481184b1b001f225769d1482b360b4be18fe7dd53fa93cdbd45d568e905a7
Deleted: sha256:c43e635df3c82c2f4ce61495e8440e23ed6001c7c26c8353e6b743459a97d60e
<truncated>

Also this is my first foray into Rust, so please let me know if there are any best-practices I should be following, particularly around error handling 👍

Status: Ready

Fixes: #71

stepchowfun · 2020-07-17T23:46:57Z

Thanks so much for contributing! I'll take a look soon.

A small tip: I see you have a small lint error. If you have Toast installed, you can run toast lint to reproduce it locally. If you do not want to use Toast, you can run the various linters (e.g., Clippy, ShellCheck) manually or just rely on the CI system we have here.

Previously, when docuum first starts, it would set the timestamp of every image to the time docuum started, meaning old images would be removed at random. Now, we use the time the image was created as the initial timestamp (which is of course updated when that image is used like normal).

stepchowfun

This is looking really good. I left some minor comments. The most important feedback is to avoid unwrap as much as possible, unless we can be absolutely sure that it's safe (with justification provided in comments). If there is even the remotest possibility that an unwrap will panic, we should return an error instead so the caller can decide what to do.

src/run.rs

stepchowfun · 2020-07-19T21:35:43Z

toast.yml

@@ -94,7 +94,7 @@ tasks:
      . $HOME/.cargo/env
      cargo clippy --all-targets --all-features -- \
        --deny warnings --deny clippy::all --deny clippy::pedantic
-      cargo fmt --all -- --check
+      cargo fmt --all -- --check || echo "ERROR: Please correct the formatting errors above"


Is this because it was difficult to understand the output? I wonder if we should do this for all the lint checks (or none of them).

Either way, a few minor suggestions:

echo to standard error rather than standard out (e.g., echo '...' 1>&2)

Punctuate the sentence.

Use single quotes unless we need the interpolation capability of double quotes. I know this is nitpicky, but I find that this practice avoids mistakes in which Bash interprets some part of the string that you didn't intend it to (though here I think it's inconsequential).

Yes, when cargo fmt fails, it just prints adiff and I didn't know why or even what command was triggering that.
Whereas clippy has some explanation like error: unneeded return statement, so I knew how to address that.

mac-chaffee · 2020-07-19T22:34:18Z

This is probably the highest quality code review I've ever received, thanks @stepchowfun! I'll get started on addressing these comments

mac-chaffee · 2020-07-22T15:24:17Z

Still trying to find time to pick another fight with the Rust compiler to address these changes. If it turns out #75 is done and ready to be merged before this PR, just letting you know I'd be okay with fixing the resulting merge conflicts if need-be 👍

mac-chaffee · 2020-07-26T19:54:53Z

I resolved the comments I think I've addressed, feel free to re-comment since everything's shifted around now. Mainly did the following:

Replaced the unwraps with either ok_or_else or match. Handling errors seems to be a common noob trap, so more guidance is always appreciated
Replaced the map chain with a for-loop. It hurt me to abandon the clever iterator tricks, but I figured a for loop would help readability, especially since things seem to get messy when the map'd function is fallible. Open to other advice here too.

src/run.rs

stepchowfun

This is looking good to me. Would you mind rebasing now that the other PR has been merged? (Thanks for offering to do that.)

src/run.rs

mac-chaffee · 2020-07-27T03:38:25Z

I figured I'd have to throw away a lot of code when #75 was merged, but I learned a lot anyway!

Also for some reason I am now unable to run docuum locally after compiling. Filed a bug: #78

mac-chaffee · 2020-07-27T13:55:44Z

So @stepchowfun do you have a preference on what to do with this PR now that #75 was reverted?

If there's a quick path to getting Bollard back in, we could just leave this PR until that point.
Otherwise, I could try to recover those commits I squashed and go back to parsing CreatedAt from the CLI

stepchowfun

@mac-chaffee If I understand the situation correctly, I don't think we'll be able to bring back Bollard any time soon. If it's a bug in Docker, I guess we just need to wait for all the affected Docker versions (which includes the latest current version) to fall out of use?

I left some comments on the current version of the PR, but unfortunately I think we'll have to go back to the old way (parsing the output of the Docker CLI) so my comments might be pointless. I'm sorry you had to rebase your PR only to find out that we have to revert back to the old strategy after all. That's a big bummer!

Let me know if you are able to recover your old commits to restore the PR to how it was before the rebasing. I think it was essentially ready to ship, or at least close to it.

stepchowfun · 2020-07-27T23:56:31Z

src/run.rs

@@ -36,7 +36,7 @@ pub async fn image_id(docker: &Docker, image: &str) -> io::Result<String> {
 }

 // Ask Docker for the IDs of all the images.
-pub async fn image_ids(docker: &Docker) -> io::Result<HashSet<String>> {
+pub async fn image_ids(docker: &Docker) -> io::Result<HashMap<String, Duration>> {


What do you think about renaming this function to image_ids_and_creation_timestamps? It's kind of verbose, but the current name feels incomplete now.

(If this suggestion resonates with you, we can also rename the image_ids variable defined on line 169 accordingly.)

stepchowfun · 2020-07-28T00:02:13Z

src/run.rs

-    Ok(output.into_iter().map(|image| image.id).collect())
+    for image in output {
+        match image.created.try_into() {
+            Ok(unsigned) => images.insert(image.id, Duration::from_secs(unsigned)),


This is totally subjective, but I think the unsigned variable could have a better name. Maybe created to match the name of where it comes from? (I'm not super fond of created, but consistency is more important than my personal preferences.)

stepchowfun · 2020-07-28T00:02:59Z

src/run.rs

+    for image in output {
+        match image.created.try_into() {
+            Ok(unsigned) => images.insert(image.id, Duration::from_secs(unsigned)),
+            Err(e) => return Err(io::Error::new(io::ErrorKind::Other, e))


Elsewhere in this file we use the name error rather than e when pattern matching on an Err. Can we do that here too? (Alternatively we could change all the other ones. I don't feel strongly about it, as long as we're consistent.)

mdonoughe · 2020-07-28T00:45:32Z

We shouldn't need to wait that long. The docker team changed the documentation to match the current behavior so we just need Bollard to pick that up.

stepchowfun · 2020-07-28T00:54:11Z

@mdonoughe Oh so there is no behavior change on Docker's side? That's kind of unfortunate, because the behavior is weirdly inconsistent (integer vs. timestamp string) depending on whether BuildKit is enabled, right? Should Bollard have to deal with that?

mdonoughe · 2020-07-28T00:56:23Z

When you're using BuildKit there's an extra section specifically about BuildKit cache data and that's where the problem is.

stepchowfun · 2020-07-28T01:15:45Z

Ah, that makes sense. Thank you for clarifying! In that case, I think this can go either way: wait for the Bollard upstream fix (do they know about the issue?), bring back @mdonoughe's change, and then land this PR, or alternatively restore this PR to how it was before Bollard, land it, and then land the Bollard change separately when the time is right.

Either way is fine with me, but I imagine it could be faster to update this PR and land it rather than waiting on Bollard.

Fortunately, neither of these changes has any timeline requirement on my end, so it's up to y'all to decide how you'd like to proceed.

mdonoughe · 2020-07-28T01:23:21Z

I created fussybeaver/bollard#89 earlier today.

mac-chaffee · 2020-07-28T02:11:37Z

Did some (surprisingly easy) git reflog surgery and got the code back! The old code is at https://github.com/mac-chaffee/docuum/tree/BACKUP2 but the conversion of this change back into using bollard should be straight-forward.

stepchowfun · 2020-07-28T03:04:54Z

Nice! Looks good.

stepchowfun mentioned this pull request Jul 17, 2020

Smarter image deletion when initial state is empty #71

Closed

mac-chaffee added 2 commits July 17, 2020 20:22

Display error message when cargo fmt step fails

216331a

stepchowfun reviewed Jul 19, 2020

View reviewed changes

mac-chaffee added 3 commits July 26, 2020 14:36

Send lint error output to stderr

a30e970

Improve error handling for parse_docker_date

8d1315f

Move all date-parsing code into parse_docker_date

dac6070

mac-chaffee requested a review from stepchowfun July 26, 2020 19:01

stepchowfun reviewed Jul 27, 2020

View reviewed changes

src/run.rs Outdated Show resolved Hide resolved

stepchowfun reviewed Jul 27, 2020

View reviewed changes

src/run.rs Outdated Show resolved Hide resolved

stepchowfun reviewed Jul 27, 2020

View reviewed changes

src/run.rs Outdated Show resolved Hide resolved

stepchowfun reviewed Jul 27, 2020

View reviewed changes

src/run.rs Outdated Show resolved Hide resolved

Add dryness and punctuation

108f442

mac-chaffee requested a review from stepchowfun July 27, 2020 03:38

stepchowfun reviewed Jul 28, 2020

View reviewed changes

Rename image_ids -> image_ids_and_creation_timestamps

7a40fb3

mac-chaffee requested a review from stepchowfun July 28, 2020 02:07

Merge branch 'master' into master

49466e4

stepchowfun merged commit ba3c84d into stepchowfun:master Jul 28, 2020

This was referenced Jul 28, 2020

Release v0.11.0 #81

Merged

Use Docker API instead of Docker CLI #85

Merged

stepchowfun mentioned this pull request Aug 2, 2021

Question about vacuum behaviour #177

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set initial timestamp to the time the image was created #73

Set initial timestamp to the time the image was created #73

mac-chaffee commented Jul 17, 2020 •

edited

stepchowfun commented Jul 17, 2020

stepchowfun left a comment

stepchowfun Jul 19, 2020

mac-chaffee Jul 26, 2020

mac-chaffee commented Jul 19, 2020

mac-chaffee commented Jul 22, 2020

mac-chaffee commented Jul 26, 2020

stepchowfun left a comment

mac-chaffee commented Jul 27, 2020 •

edited

mac-chaffee commented Jul 27, 2020

stepchowfun left a comment

stepchowfun Jul 27, 2020

stepchowfun Jul 27, 2020

stepchowfun Jul 28, 2020

stepchowfun Jul 28, 2020

mdonoughe commented Jul 28, 2020

stepchowfun commented Jul 28, 2020

mdonoughe commented Jul 28, 2020

stepchowfun commented Jul 28, 2020

mdonoughe commented Jul 28, 2020

mac-chaffee commented Jul 28, 2020

stepchowfun commented Jul 28, 2020

Set initial timestamp to the time the image was created #73

Set initial timestamp to the time the image was created #73

Conversation

mac-chaffee commented Jul 17, 2020 • edited

stepchowfun commented Jul 17, 2020

stepchowfun left a comment

Choose a reason for hiding this comment

stepchowfun Jul 19, 2020

Choose a reason for hiding this comment

mac-chaffee Jul 26, 2020

Choose a reason for hiding this comment

mac-chaffee commented Jul 19, 2020

mac-chaffee commented Jul 22, 2020

mac-chaffee commented Jul 26, 2020

stepchowfun left a comment

Choose a reason for hiding this comment

mac-chaffee commented Jul 27, 2020 • edited

mac-chaffee commented Jul 27, 2020

stepchowfun left a comment

Choose a reason for hiding this comment

stepchowfun Jul 27, 2020

Choose a reason for hiding this comment

stepchowfun Jul 27, 2020

Choose a reason for hiding this comment

stepchowfun Jul 28, 2020

Choose a reason for hiding this comment

stepchowfun Jul 28, 2020

Choose a reason for hiding this comment

mdonoughe commented Jul 28, 2020

stepchowfun commented Jul 28, 2020

mdonoughe commented Jul 28, 2020

stepchowfun commented Jul 28, 2020

mdonoughe commented Jul 28, 2020

mac-chaffee commented Jul 28, 2020

stepchowfun commented Jul 28, 2020

mac-chaffee commented Jul 17, 2020 •

edited

mac-chaffee commented Jul 27, 2020 •

edited