Purge Obuilder Docker store when Docker is pruned #242
Conversation
|
@mtelvers is this something you think we could move forward and land? Or is there anything blocking it? |
| match Obuilder_build.backend obuilder with | ||
| | `Native _ -> Lwt.return 0 | ||
| | `Docker _ -> Obuilder_build.purge obuilder) >>= fun n -> | ||
| Log.info (fun f -> f "%i items prune from Obuilder" n); |
There was a problem hiding this comment.
| Log.info (fun f -> f "%i items prune from Obuilder" n); | |
| Log.info (fun f -> f "%i items pruned from Obuilder" n); |
| Builder.prune builder ~before Int.max_int >>= fun n -> | ||
| Lwt.return n |
There was a problem hiding this comment.
Builder.prune builder ~before Int.max_int : int Lwt.t and we don't need to operate in the int, here, so we no need to take it of and return it back to the Lwt.t context here:
| Builder.prune builder ~before Int.max_int >>= fun n -> | |
| Lwt.return n | |
| Builder.prune builder ~before Int.max_int |
| | `Native _ -> Lwt.return 0 | ||
| | `Docker _ -> Obuilder_build.purge obuilder) >>= fun n -> |
There was a problem hiding this comment.
Given that we are just ignoring the values wrapped in the Native | Docker tags here, why do we need to expose them in the Boulder_build API at all?
| let backend t = | ||
| t.sandbox_config |
There was a problem hiding this comment.
iiuc, the only use of this accessor is at https://github.com/ocurrent/ocluster/pull/242/files#diff-5b7638a06391f25c3c893df03fe8754d5c0702dfc2aa4e12c7881dcf077e106eR244 to classify the kind of configuration being used, which then let's us decide whether to actually call purge or to return 0 in a no-op. As such, it looks to me like there is no reason to leak this out of the Obuilder_build API. Instead, we can just handle that logic in the purge function itself. WDYT?
| @@ -20,3 +20,7 @@ val build : t -> | |||
| val healthcheck : t -> (unit, [> `Msg of string]) Lwt_result.t | |||
|
|
|||
| val cache_stats : t -> int * int | |||
|
|
|||
| val purge : t -> int Lwt.t | |||
There was a problem hiding this comment.
A dock comment explaining what this is meant to be used for would be helpful.
| (match obuilder with | ||
| | None -> Lwt.return 0 | ||
| | Some obuilder -> | ||
| match Obuilder_build.backend obuilder with | ||
| | `Native _ -> Lwt.return 0 | ||
| | `Docker _ -> Obuilder_build.purge obuilder) >>= fun n -> |
There was a problem hiding this comment.
If my suggestion about keeping the sandbox configuration type private makes sense, then this could be something like
| (match obuilder with | |
| | None -> Lwt.return 0 | |
| | Some obuilder -> | |
| match Obuilder_build.backend obuilder with | |
| | `Native _ -> Lwt.return 0 | |
| | `Docker _ -> Obuilder_build.purge obuilder) >>= fun n -> | |
| (match obuilder with | |
| | None -> Lwt.return 0 | |
| | Some obuilder -> Obuilder_build.purge obuilder) >>= fun n -> |
| match Obuilder_build.backend obuilder with | ||
| | `Native _ -> Lwt.return 0 | ||
| | `Docker _ -> Obuilder_build.purge obuilder) >>= fun n -> | ||
| Log.info (fun f -> f "%i items prune from Obuilder" n); | ||
| Lwt_process.exec ("", [| "docker"; "builder"; "prune"; "-af" |]) |
There was a problem hiding this comment.
I think this is out of scope for your changes, but seems worth clarifying any how:
According to https://docs.docker.com/reference/cli/docker/builder/prune/, this will
Remove all unused build cache, not just dangling ones
Whereas, according to https://docs.docker.com/reference/cli/docker/system/prune/, docker system prune (which we should have just called on line 237) will
Remove all unused containers, networks, images (both dangling and unused), and optionally, volumes.
Based on the warning in the docks, this includes all
unused build cache
So I'm wondering whether we actually need this additional step, or if docker system prune should not already cover it...
When docker is pruned, everything is deleted from Obuilder Docker store. However, obuilder does not know this has happened so the database is out of sync.