-
Notifications
You must be signed in to change notification settings - Fork 760
Fix system df to count content blobs and deduplicate shared storage #1555
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| //===----------------------------------------------------------------------===// | ||
| // Copyright © 2026 Apple Inc. and the container project authors. | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // https://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
| //===----------------------------------------------------------------------===// | ||
|
|
||
| import Foundation | ||
|
|
||
| extension FileManager { | ||
| /// Total bytes allocated on disk for all files in a directory (recursive). | ||
| public func allocatedSize(of directory: URL) -> UInt64 { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We shouldn't write any more code that uses URL for fs paths.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also noting that this would be a good candidate for extracting to a FilePathOps utility type in ContainerizationOS: apple/containerization#744. |
||
| guard | ||
| let enumerator = self.enumerator( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added |
||
| at: directory, | ||
| includingPropertiesForKeys: [.totalFileAllocatedSizeKey], | ||
| options: [.skipsHiddenFiles] | ||
| ) | ||
| else { | ||
| return 0 | ||
| } | ||
|
|
||
| var size: UInt64 = 0 | ||
| for case let fileURL as URL in enumerator { | ||
| guard let resourceValues = try? fileURL.resourceValues(forKeys: [.totalFileAllocatedSizeKey]), | ||
| let fileSize = resourceValues.totalFileAllocatedSize | ||
| else { | ||
| continue | ||
| } | ||
| size += UInt64(fileSize) | ||
| } | ||
| return size | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -92,10 +92,11 @@ extension ImagesHelper { | |
|
|
||
| private func initializeImagesService(root: URL, containerSystemConfig: ContainerSystemConfig, log: Logger, routes: inout [String: XPCServer.RouteHandler]) throws { | ||
| let contentStore = RemoteContentStoreClient() | ||
| let contentBlobsPath = root.appendingPathComponent("content/blobs/sha256") | ||
| let imageStore = try ImageStore(path: root, contentStore: contentStore) | ||
| let unpackStrategy = SnapshotStore.defaultUnpackStrategy(initImage: containerSystemConfig.vminit.image) | ||
| let snapshotStore = try SnapshotStore(path: root, unpackStrategy: unpackStrategy, log: log) | ||
| let service = try ImagesService(contentStore: contentStore, imageStore: imageStore, snapshotStore: snapshotStore, log: log) | ||
| let service = try ImagesService(contentStore: contentStore, contentBlobsPath: contentBlobsPath, imageStore: imageStore, snapshotStore: snapshotStore, log: log) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @adityaramani should the ImagesService know about the contentBlobsPath? That feels like an encapsulation break. It's only used for getting the total size of the content store - could we just add a content client call that returns that value?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes +1! Between container and containerization there are two types are implement the
They exist because there should be only one entity that is actually touching the files on disk - if we had multiple instances of The When you ask the With this change - it slightly breaks that abstraction and makes ImageService directly check a path on disk where the files the To solve the "get me the total size of the content store" problem - we can probably do two things.
|
||
| let harness = ImagesServiceHarness(service: service, log: log) | ||
|
|
||
| routes[ImagesServiceXPCRoute.imagePull.rawValue] = XPCServer.route(harness.pull) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comments should include more details on what is and isn't counted. As far as I can tell:
directorywill include those files' sizes in the total.directorywill include those files' sizes in the total, resulting in inaccuracy due to multiple counting.Do we need to address the double counting issues, or will the directories we enumerate never have these sorts of things?