public_git
/
tart
mirror of https://github.com/cirruslabs/tart.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Address stacked OCI pull review feedback

This commit is contained in:
Yibo Zhuang 2026-08-11 19:09:30 -07:00
parent be44cc8b38
commit 53ceac8e22
No known key found for this signature in database
9 changed files with 189 additions and 108 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

14
Sources/tart/Commands/Clone.swift
View File

@ -32,7 +32,7 @@ struct Clone: AsyncParsableCommand {
var deduplicate: Bool = false
@Flag(help: "create a stacked disk that uses the source image as an immutable base")
var base: Bool = false
var stacked: Bool = false
@Option(help: ArgumentHelp("limit automatic pruning to n gigabytes", valueName: "n"))
var pruneLimit: UInt = 100
@ -52,13 +52,13 @@ struct Clone: AsyncParsableCommand {
let localStorage = try VMStorageLocal()
let remoteName = try? RemoteName(sourceName)
if base {
if stacked {
guard remoteName != nil else {
throw ValidationError("--base requires a remote image")
throw ValidationError("--stacked requires a remote image")
}
}
if let remoteName, try !ociStorage.hasUsableCachedImageForClone(remoteName, requireManifest: base) {
if let remoteName, try !ociStorage.hasUsableCachedImageForClone(remoteName, requireManifest: stacked) {
// Pull the VM in case it's OCI-based and doesn't exist locally yet
let registry = try Registry(host: remoteName.host, namespace: remoteName.namespace, insecure: insecure)
try await ociStorage.pull(remoteName, registry: registry, concurrency: concurrency, deduplicate: deduplicate)
@ -80,12 +80,12 @@ struct Clone: AsyncParsableCommand {
let generateMAC = try localStorage.hasVMsWithMACAddress(macAddress: sourceVM.macAddress())
&& sourceState != .Suspended
if base {
if stacked {
guard sourceVM.isStandalone else {
throw ValidationError("--base cannot use an image that already has a stacked disk")
throw ValidationError("--stacked cannot use an image that already has a stacked disk")
}
guard try VMConfig(fromURL: sourceVM.configURL).os == .darwin else {
throw ValidationError("--base currently supports only macOS images")
throw ValidationError("--stacked currently supports only macOS images")
}
try sourceVM.cloneAsStackedBase(to: tmpVMDir, generateMAC: generateMAC)
} else if sourceVM.isStackedCachedImage {

19
Sources/tart/Commands/Run.swift
View File

@ -964,7 +964,7 @@ struct AdditionalDisk {
init(parseFrom: String) throws {
let (diskPath, readOnly, syncModeRaw, cachingModeRaw) = Self.parseOptions(parseFrom)
(configuration, temporaryDiskLock) = try Self.craft(
self = try Self.craft(
diskPath,
readOnly: readOnly,
syncModeRaw: syncModeRaw,
@ -972,12 +972,17 @@ struct AdditionalDisk {
)
}
private init(configuration: VZStorageDeviceConfiguration, temporaryDiskLock: FileLock? = nil) {
self.configuration = configuration
self.temporaryDiskLock = temporaryDiskLock
}
private static func craft(
_ diskPath: String,
readOnly diskReadOnly: Bool,
syncModeRaw: String,
cachingModeRaw: String
) throws -> (VZStorageDeviceConfiguration, FileLock?) {
) throws -> AdditionalDisk {
let diskURL = URL(string: diskPath)
if (["nbd", "nbds", "nbd+unix", "nbds+unix"].contains(diskURL?.scheme)) {
@ -992,7 +997,7 @@ struct AdditionalDisk {
synchronizationMode: try VZDiskSynchronizationMode(syncModeRaw)
)
return (VZVirtioBlockDeviceConfiguration(attachment: nbdAttachment), nil)
return AdditionalDisk(configuration: VZVirtioBlockDeviceConfiguration(attachment: nbdAttachment))
}
// Expand the tilde (~) since at this point we're dealing with a local path,
@ -1023,7 +1028,7 @@ struct AdditionalDisk {
let blockAttachment = try VZDiskBlockDeviceStorageDeviceAttachment(fileHandle: FileHandle(fileDescriptor: fd, closeOnDealloc: true),
readOnly: diskReadOnly, synchronizationMode: try VZDiskSynchronizationMode(syncModeRaw))
return (VZVirtioBlockDeviceConfiguration(attachment: blockAttachment), nil)
return AdditionalDisk(configuration: VZVirtioBlockDeviceConfiguration(attachment: blockAttachment))
}
// Support remote VM names in --disk command-line argument
@ -1047,7 +1052,7 @@ struct AdditionalDisk {
synchronizationMode: try VZDiskImageSynchronizationMode(syncModeRaw)
)
return (VZVirtioBlockDeviceConfiguration(attachment: attachment), lock)
return AdditionalDisk(configuration: VZVirtioBlockDeviceConfiguration(attachment: attachment), temporaryDiskLock: lock)
}
// Unfortunately, VZDiskImageStorageDeviceAttachment does not support
@ -1062,7 +1067,7 @@ struct AdditionalDisk {
let diskImageAttachment = try VZDiskImageStorageDeviceAttachment(url: clonedDiskURL, readOnly: diskReadOnly)
return (VZVirtioBlockDeviceConfiguration(attachment: diskImageAttachment), lock)
return AdditionalDisk(configuration: VZVirtioBlockDeviceConfiguration(attachment: diskImageAttachment), temporaryDiskLock: lock)
}
// Error out if the disk is locked by the host (e.g. it was mounted in Finder),
@ -1078,7 +1083,7 @@ struct AdditionalDisk {
synchronizationMode: try VZDiskImageSynchronizationMode(syncModeRaw)
)
return (VZVirtioBlockDeviceConfiguration(attachment: diskImageAttachment), nil)
return AdditionalDisk(configuration: VZVirtioBlockDeviceConfiguration(attachment: diskImageAttachment))
}
static func parseOptions(_ parseFrom: String) -> (String, Bool, String, String) {

59
Sources/tart/ContentStore.swift
View File

@ -1,10 +1,8 @@
import Foundation
import System
enum ContentStoreError: Error, Equatable {
case invalidContentDigest(String)
case contentDigestMismatch(expected: String, actual: String)
case operationFailed(String)
}
/// Opaque content-addressed storage for immutable reconstructed files.
@ -45,7 +43,6 @@ struct ContentStore {
/// reconstructing this content entry on a later attempt.
func resumableContentURL(for contentDigest: String) throws -> URL {
let targetURL = try contentURL(for: contentDigest)
try FileManager.default.createDirectory(at: targetURL.deletingLastPathComponent(), withIntermediateDirectories: true)
return targetURL.deletingLastPathComponent().appendingPathComponent(".\(targetURL.lastPathComponent).partial")
}
@ -55,7 +52,6 @@ struct ContentStore {
/// descriptor and disappears when the owning process exits.
func lockURL(for contentDigest: String) throws -> URL {
let targetURL = try contentURL(for: contentDigest)
try FileManager.default.createDirectory(at: targetURL.deletingLastPathComponent(), withIntermediateDirectories: true)
let lockURL = targetURL.deletingLastPathComponent().appendingPathComponent(".\(targetURL.lastPathComponent).lock")
if !FileManager.default.fileExists(atPath: lockURL.path) {
@ -103,55 +99,22 @@ struct ContentStore {
}
let targetURL = try contentURL(for: contentDigest)
let lock = try FileLock(lockURL: baseURL)
try lock.lock()
defer { try? lock.unlock() }
while true {
if try moveItemWithoutReplacing(at: temporaryURL, to: targetURL) {
return targetURL
}
if let existingURL = try existingContentURL(for: contentDigest) {
try? FileManager.default.removeItem(at: temporaryURL)
return existingURL
}
// The destination exists but is corrupt. Swapping keeps the digest path
// continuously populated: if another repair wins first, both sides of
// this exchange are already digest-valid and the result remains valid.
if try exchangeItem(at: temporaryURL, with: targetURL) {
try? FileManager.default.removeItem(at: temporaryURL)
return targetURL
}
}
}
/// Atomically publishes a content entry without replacing an existing one.
/// Returns false when another installer already created the destination.
private func moveItemWithoutReplacing(at sourceURL: URL, to destinationURL: URL) throws -> Bool {
if renamex_np(sourceURL.path, destinationURL.path, UInt32(RENAME_EXCL)) == 0 {
return true
if let existingURL = try existingContentURL(for: contentDigest) {
try? FileManager.default.removeItem(at: temporaryURL)
return existingURL
}
if errno == EEXIST {
return false
if FileManager.default.fileExists(atPath: targetURL.path) {
_ = try FileManager.default.replaceItemAt(targetURL, withItemAt: temporaryURL)
} else {
try FileManager.default.moveItem(at: temporaryURL, to: targetURL)
}
let details = Errno(rawValue: CInt(errno))
throw ContentStoreError.operationFailed("failed to install content entry \(destinationURL.path): \(details)")
}
/// Atomically exchanges a verified temporary file with a corrupt content
/// entry. Returns false when the destination disappeared before the swap.
private func exchangeItem(at sourceURL: URL, with destinationURL: URL) throws -> Bool {
if renamex_np(sourceURL.path, destinationURL.path, UInt32(RENAME_SWAP)) == 0 {
return true
}
if errno == ENOENT {
return false
}
let details = Errno(rawValue: CInt(errno))
throw ContentStoreError.operationFailed("failed to repair content entry \(destinationURL.path): \(details)")
return targetURL
}
private func validatedDigestHex(_ contentDigest: String) throws -> String {

28
Sources/tart/DiskAttachmentSource.swift
View File

@ -1,28 +0,0 @@
import Foundation
import Virtualization
/// A disk-image-backed source that can become a Virtualization.Framework storage attachment.
protocol DiskAttachmentSource {
func makeAttachment(
readOnly: Bool,
cachingMode: VZDiskImageCachingMode,
synchronizationMode: VZDiskImageSynchronizationMode
) throws -> VZStorageDeviceAttachment
}
struct DiskImageAttachment: DiskAttachmentSource {
let url: URL
func makeAttachment(
readOnly: Bool,
cachingMode: VZDiskImageCachingMode,
synchronizationMode: VZDiskImageSynchronizationMode
) throws -> VZStorageDeviceAttachment {
try VZDiskImageStorageDeviceAttachment(
url: url,
readOnly: readOnly,
cachingMode: cachingMode,
synchronizationMode: synchronizationMode
)
}
}

2
Sources/tart/DiskImageStack.swift
View File

@ -34,7 +34,7 @@ enum DiskImageStackError: Error, Equatable, CustomStringConvertible {
}
}
struct DiskImageStack: DiskAttachmentSource {
struct DiskImageStack {
/// DiskImageKit-ready paths and block layout after Tart disk chunks have been
/// reconstructed into complete immutable files. The writable overlay stays
/// private to one VM.

32
Sources/tart/VM.swift
View File

@ -64,12 +64,7 @@ class VM: NSObject, VZVirtualMachineDelegate, ObservableObject {
// Initialize the virtual machine and its configuration
self.network = network
let disk: any DiskAttachmentSource = if vmDir.isStackedVM {
try vmDir.diskImageStack()
} else {
DiskImageAttachment(url: vmDir.diskURL)
}
configuration = try Self.craftConfiguration(disk: disk,
configuration = try Self.craftConfiguration(vmDir: vmDir,
nvramURL: vmDir.nvramURL, vmConfig: config,
network: network, additionalStorageDevices: additionalStorageDevices,
directorySharingDevices: directorySharingDevices,
@ -201,7 +196,7 @@ class VM: NSObject, VZVirtualMachineDelegate, ObservableObject {
// Initialize the virtual machine and its configuration
self.network = network
configuration = try Self.craftConfiguration(disk: DiskImageAttachment(url: vmDir.diskURL),
configuration = try Self.craftConfiguration(vmDir: vmDir,
nvramURL: vmDir.nvramURL,
vmConfig: config, network: network,
additionalStorageDevices: additionalStorageDevices,
@ -318,7 +313,7 @@ class VM: NSObject, VZVirtualMachineDelegate, ObservableObject {
}
static func craftConfiguration(
disk: any DiskAttachmentSource,
vmDir: VMDirectory,
nvramURL: URL,
vmConfig: VMConfig,
network: Network = NetworkShared(),
@ -413,11 +408,22 @@ class VM: NSObject, VZVirtualMachineDelegate, ObservableObject {
// When not specified, use "cached" caching mode for Linux VMs to prevent file-system corruption[1]
//
// [1]: https://github.com/cirruslabs/tart/pull/675
let attachment = try disk.makeAttachment(
readOnly: false,
cachingMode: caching ?? (vmConfig.os == .linux ? .cached : .automatic),
synchronizationMode: sync
)
let cachingMode = caching ?? (vmConfig.os == .linux ? .cached : .automatic)
let attachment: VZStorageDeviceAttachment
if vmDir.isStackedVM {
attachment = try vmDir.diskImageStack().makeAttachment(
readOnly: false,
cachingMode: cachingMode,
synchronizationMode: sync
)
} else {
attachment = try VZDiskImageStorageDeviceAttachment(
url: vmDir.diskURL,
readOnly: false,
cachingMode: cachingMode,
synchronizationMode: sync
)
}
var devices: [VZStorageDeviceConfiguration] = [VZVirtioBlockDeviceConfiguration(attachment: attachment)]
devices.append(contentsOf: additionalStorageDevices)

2
Sources/tart/VMDirectory+DiskImageStack.swift
View File

@ -80,7 +80,7 @@ extension VMDirectory {
var manifest = try OCIManifest(fromJSON: Data(contentsOf: manifestURL))
guard case .flat = try manifest.tartDiskRepresentation() else {
throw RuntimeError.VMConfigurationError("--base cannot use an image that already has a stacked disk")
throw RuntimeError.VMConfigurationError("--stacked cannot use an image that already has a stacked disk")
}
guard let firstDiskIndex = manifest.layers.firstIndex(where: { $0.mediaType == diskV2MediaType }) else {

73
Sources/tart/VMStorageOCI.swift
View File

@ -87,6 +87,17 @@ class VMStorageOCI: PrunableStorage {
return missingGroups.isEmpty
}
/// The lock-free pull fast path is only useful for a tag that already
/// points at this digest. New or retargeted tags validate once after taking
/// the host lock instead of hashing a large stack twice.
func hasCompleteLinkedImage(_ name: RemoteName, digestName: RemoteName, manifest: OCIManifest) throws -> Bool {
guard exists(name), linked(from: name, to: digestName) else {
return false
}
return try hasCompleteCachedImage(digestName, manifest: manifest)
}
/// Bytes that this pull may need to materialize locally. For stacked images
/// this is the sum of only the missing complete disk files, not the final
/// guest-visible disk block layout.
@ -294,8 +305,7 @@ class VMStorageOCI: PrunableStorage {
let digestName = RemoteName(host: name.host, namespace: name.namespace,
reference: Reference(digest: Digest.hash(manifestData)))
let hasCompleteDigestImage = try hasCompleteCachedImage(digestName, manifest: manifest)
if exists(name) && hasCompleteDigestImage && linked(from: name, to: digestName) {
if try hasCompleteLinkedImage(name, digestName: digestName, manifest: manifest) {
// optimistically check if we need to do anything at all before locking
defaultLogger.appendNewLine("\(digestName) image is already cached and linked!")
return
@ -335,6 +345,11 @@ class VMStorageOCI: PrunableStorage {
let tmpVMDirLock = try FileLock(lockURL: tmpVMDir.baseURL)
try tmpVMDirLock.lock()
// A previously pulled standalone image already has the complete base
// disk locally as disk.img. Promote that file into the content store
// before sizing or pulling so a stacked child only fetches overlays.
try reuseStandaloneDiskForStackedBaseIfPossible(manifest)
// Try to reclaim some cache space if we know the VM size in advance
if let requiredDiskStorageBytes = try requiredDiskStorageBytes(for: manifest) {
if let telemetryValue = Int(exactly: requiredDiskStorageBytes) {
@ -437,6 +452,60 @@ class VMStorageOCI: PrunableStorage {
return missingGroups
}
/// Seed a stacked image's immutable base from an already pulled standalone
/// OCI record when both manifests describe the same transport chunks. The
/// content store still verifies the whole-file digest before publishing it.
func reuseStandaloneDiskForStackedBaseIfPossible(_ manifest: OCIManifest) throws {
guard case .stacked(let base, _) = try manifest.tartDiskRepresentation(),
let contentDigest = base.contentDigest else {
return
}
let contentStore = try ContentStore()
// Content-store entries are verified when installed. Avoid hashing a
// potentially large prewarmed base again on every stacked pull.
guard try contentStore.contentURLIfPresent(for: contentDigest) == nil else {
return
}
for (_, vmDir, isSymlink) in try list() where !isSymlink && vmDir.isStandalone {
guard let manifestData = try? Data(contentsOf: vmDir.manifestURL),
let candidateManifest = try? OCIManifest(fromJSON: manifestData),
case .flat(let candidateBase) = try? candidateManifest.tartDiskRepresentation(),
diskChunksMatch(candidateBase.chunks, base.chunks) else {
continue
}
let temporaryURL = try contentStore.temporaryContentURL(for: contentDigest)
do {
try FileManager.default.copyItem(at: vmDir.diskURL, to: temporaryURL)
_ = try contentStore.install(temporaryURL, contentDigest: contentDigest)
return
} catch ContentStoreError.contentDigestMismatch {
try? FileManager.default.removeItem(at: temporaryURL)
} catch {
try? FileManager.default.removeItem(at: temporaryURL)
throw error
}
}
}
/// Compare the OCI transport identity while ignoring stacked-only
/// whole-file annotations added to the first base chunk.
private func diskChunksMatch(_ left: [OCIManifestLayer], _ right: [OCIManifestLayer]) -> Bool {
guard left.count == right.count else {
return false
}
return zip(left, right).allSatisfy { left, right in
left.mediaType == right.mediaType &&
left.size == right.size &&
left.digest == right.digest &&
left.uncompressedSize() == right.uncompressedSize() &&
left.uncompressedContentDigest() == right.uncompressedContentDigest()
}
}
func linked(from: RemoteName, to: RemoteName) -> Bool {
do {
let resolvedFrom = try FileManager.default.destinationOfSymbolicLink(atPath: vmURL(from).path)

68
Tests/TartTests/VMStorageOCITests.swift
View File

@ -23,7 +23,7 @@ final class VMStorageOCITests: XCTestCase {
}
}
func testBaseCloneRequiresManifestForLegacyStandaloneCachedImage() throws {
func testStackedCloneRequiresManifestForLegacyStandaloneCachedImage() throws {
try withTemporaryTartHome {
let manifest = try flatManifest()
let name = try digestName(for: manifest)
@ -123,6 +123,72 @@ final class VMStorageOCITests: XCTestCase {
}
}
func testStackedPullReusesPreviouslyPulledStandaloneDisk() throws {
try withTemporaryTartHome {
let diskData = Data([0])
let contentDigest = Digest.hash(diskData)
let flatManifest = try flatManifest()
let flatName = try digestName(for: flatManifest)
let storage = try VMStorageOCI()
let flatRecord = try storage.create(flatName)
try config().save(toURL: flatRecord.configURL)
XCTAssertTrue(FileManager.default.createFile(atPath: flatRecord.nvramURL.path, contents: Data()))
try diskData.write(to: flatRecord.diskURL)
try flatManifest.toJSON().write(to: flatRecord.manifestURL)
let stackedManifest = try stackedManifest(baseContentDigest: contentDigest)
XCTAssertNil(try ContentStore().existingContentURL(for: contentDigest))
try storage.reuseStandaloneDiskForStackedBaseIfPossible(stackedManifest)
let reusedURL = try XCTUnwrap(try ContentStore().existingContentURL(for: contentDigest))
XCTAssertEqual(try Data(contentsOf: reusedURL), diskData)
}
}
func testStackedPullDoesNotRehashInstalledBaseBeforeReuse() throws {
try withTemporaryTartHome {
let contentDigest = Digest.hash(Data("base".utf8))
let manifest = try stackedManifest(baseContentDigest: contentDigest)
let contentURL = try ContentStore().contentURL(for: contentDigest)
// Hashing this path would throw. Once an entry is published, this
// fast path must trust its presence and let normal pull validation
// repair unusable content later.
try FileManager.default.createDirectory(at: contentURL, withIntermediateDirectories: false)
XCTAssertNoThrow(try VMStorageOCI().reuseStandaloneDiskForStackedBaseIfPossible(manifest))
}
}
func testNewTagDoesNotValidateCachedStackBeforeLock() throws {
try withTemporaryTartHome {
let baseDigest = Digest.hash(Data("base".utf8))
let overlayDigest = Digest.hash(Data("overlay".utf8))
let manifest = try stackedManifest(
baseContentDigest: baseDigest,
overlayContentDigest: overlayDigest
)
let digestName = try digestName(for: manifest)
let tagName = RemoteName(
host: digestName.host,
namespace: digestName.namespace,
reference: Reference(tag: "latest")
)
let storage = try VMStorageOCI()
let record = try storage.create(digestName)
try config().save(toURL: record.configURL)
XCTAssertTrue(FileManager.default.createFile(atPath: record.nvramURL.path, contents: Data()))
try manifest.toJSON().write(to: record.manifestURL)
// Hashing this directory as a disk file throws. A new tag must skip
// validation until after it has taken the host lock.
let contentURL = try ContentStore().contentURL(for: baseDigest)
try FileManager.default.createDirectory(at: contentURL, withIntermediateDirectories: false)
XCTAssertFalse(try storage.hasCompleteLinkedImage(tagName, digestName: digestName, manifest: manifest))
}
}
func testStandaloneLayerCacheIgnoresStackedCachedImages() async throws {
try await withTemporaryTartHome {
var targetManifest = try flatManifest()