Repository navigation
[Runtime] Clean container rootfs and rw volumes periodically - #2380
amanmaurya92 wants to merge 1 commit into
Conversation
jglogan
left a comment
There was a problem hiding this comment.
@amanmaurya92 Thanks! Let me know what you think about doing fail-fast on user requests that arrive while clean is in progress (see comment).
| } | ||
|
|
||
| do { | ||
| try await self.cleanLock.withLock { _ in |
There was a problem hiding this comment.
I was thinking about the user experience for having a lock here. If a clean operation takes a significant amount of time, I wonder if it's better to just fail a user-initiated clean immediately with .invalidState(message: "clean already in progress") if another clean (user or periodic) is in progress.
What do you think? The drawback is that scripts and code using the API client would have to deal with it, and they would need to parse the message to differentiate between that and "container not running".
The user wouldn't be puzzled by a container clean just sitting there though; they'd get immediate feedback.
If we did that we wouldn't need an async lock; I think just an actor-isolated isCleaning variable that gets cleared by a defer would be all that's necessary.
| } | ||
| self.log.info("running periodic container clean") | ||
| try await self.performClean() | ||
| self.log.info("periodic container clean completed") |
There was a problem hiding this comment.
It'd be convenient to compute a duration value for time elapsed for the clean and include it in the data for this log message.
jglogan
left a comment
There was a problem hiding this comment.
You'll also need to actually build the project to see errors such as these:
/Users/runner/actions-runner/_work/container/container/Sources/Services/RuntimeLinux/Server/RuntimeService.swift:350:22: error: actor-isolated instance method 'startPeriodicClean()' cannot be called from outside of the actor
348 | try await self.startInitProcess(lock: lock)
349 | await self.setState(.running)
350 | self.startPeriodicClean()
| `- error: actor-isolated instance method 'startPeriodicClean()' cannot be called from outside of the actor
351 | } else {
352 | try await self.startExecProcess(processId: id, lock: lock)
/Users/runner/actions-runner/_work/container/container/Sources/Services/RuntimeLinux/Server/RuntimeService.swift:879:34: error: result of call to 'withLock(logMetadata:_:)' is unused [#no-usage]
Also, your commits need to be signed and verified; see https://github.com/apple/containerization/blob/main/CONTRIBUTING.md#pull-requests.
a7c7bc7 to
facb733
Compare
|
@jglogan I have addressed everything, built locally, and signed off. and I completely agree with the fail-fast approach. |
|
@amanmaurya92 Cool, I'll have a look. Don't forget to set up commit signing; we can't merge without signed and verified commits. |
| do { | ||
| try await self.performClean() | ||
| let duration = clock.now - start | ||
| self.log.info("periodic container clean completed", metadata: ["duration": "\(duration)"]) |
There was a problem hiding this comment.
nit: instead of using duration directly which I think encodes a unit into the result, let's use a metadata key that indicates the unit: elapsedSeconds.
| self.log.info("periodic container clean completed", metadata: ["duration": "\(duration)"]) | |
| self.log.info("periodic container clean completed", metadata: ["elapsedSeconds": "\(duration.secondsWithMilliseconds)"]) |
With a private extension on Duration we can limit the precision so that the output is easier to read:
extension Duration {
/// Seconds as a string with millisecond resolution, e.g. "12.345".
fileprivate var secondsWithMilliseconds: String {
let (seconds, attoseconds) = self.components
let ms = seconds * 1000 + attoseconds / 1_000_000_000_000_000
return String(format: "%d.%03d", ms / 1000, ms % 1000)
}
}
| var failed: [String] = [] | ||
| for path in targets { | ||
| do { | ||
| try await ctr.container.filesystemOperation(operation: .trim, path: path) |
There was a problem hiding this comment.
Let's move the clean timing from performPeriodicCleaning to here. That way we can see elapsed time per path, and it will be visible for both scheduled and ad hoc clean operations.
Periodically clean container root filesystems and rw-mounted block volumes to free deallocated Linux filesystem blocks on host disk images. - Fail-fast manual clean requests with .invalidState when already in progress - Use actor-isolated isCleaning flag instead of AsyncLock - Calculate and log elapsed clean duration using ContinuousClock - Start periodic clean task from actor isolation context Resolves apple#2206 Signed-off-by: amanmaurya92 <amanmaurya9209@gmail.com>
facb733 to
5abc2e7
Compare
Motivation
For long-running containers that frequently create and delete files (such as builder containers), allocated disk image files on the host continuously grow in size because deallocated filesystem blocks in the Linux guest are never automatically trimmed.
Following #1949 (which introduced the manual
container cleancommand), this change enables periodic background cleanup so that filesystem blocks are reclaimed automatically on the host.Modifications
clean(_:)into an internal@discardableResult private func performClean() async throws -> [String]method that trims/(if read-write) and all non-read-only block-backed mounts.cleanLock: AsyncLockto serialize manualcontainer cleanXPC requests with periodic background cleaning, preventing concurrent trim operations on the guest filesystem.cleanInterval: Duration? = .seconds(3600)toRuntimeService.init(defaulting to 1 hour).TaskinstartProcesswhen the container init process begins and enters.running.stopPeriodicClean()to cancel the task cleanly during container teardown incleanUpContainer.performPeriodicClean()without throwing to prevent cleanup failures from disrupting the container workload.Result
Running containers now periodically trim their root filesystem and read-write mounted volumes every hour, releasing host storage without user intervention. Manual
container cleancommands and scheduled cleans are safely serialized.Resolves #2206
Verification
swiftc -parse Sources/Services/RuntimeLinux/Server/RuntimeService.swiftinapple-container-devDocker container.maxConcurrentCleans == 1and clean task cancellation on teardown.swift-format.