diff --git a/Sources/tart/Commands/Clone.swift b/Sources/tart/Commands/Clone.swift index 3c8110d..62cc75c 100644 --- a/Sources/tart/Commands/Clone.swift +++ b/Sources/tart/Commands/Clone.swift @@ -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 { diff --git a/Sources/tart/Commands/Run.swift b/Sources/tart/Commands/Run.swift index 30ffb59..e39a716 100644 --- a/Sources/tart/Commands/Run.swift +++ b/Sources/tart/Commands/Run.swift @@ -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) { diff --git a/Sources/tart/ContentStore.swift b/Sources/tart/ContentStore.swift index 6863e9c..7608e61 100644 --- a/Sources/tart/ContentStore.swift +++ b/Sources/tart/ContentStore.swift @@ -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 { diff --git a/Sources/tart/DiskAttachmentSource.swift b/Sources/tart/DiskAttachmentSource.swift deleted file mode 100644 index b3fc951..0000000 --- a/Sources/tart/DiskAttachmentSource.swift +++ /dev/null @@ -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 - ) - } -} diff --git a/Sources/tart/DiskImageStack.swift b/Sources/tart/DiskImageStack.swift index 30b2026..2a60e76 100644 --- a/Sources/tart/DiskImageStack.swift +++ b/Sources/tart/DiskImageStack.swift @@ -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. diff --git a/Sources/tart/VM.swift b/Sources/tart/VM.swift index 9bf2323..a2fb948 100644 --- a/Sources/tart/VM.swift +++ b/Sources/tart/VM.swift @@ -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) diff --git a/Sources/tart/VMDirectory+DiskImageStack.swift b/Sources/tart/VMDirectory+DiskImageStack.swift index dc3edd7..027672f 100644 --- a/Sources/tart/VMDirectory+DiskImageStack.swift +++ b/Sources/tart/VMDirectory+DiskImageStack.swift @@ -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 { diff --git a/Sources/tart/VMStorageOCI.swift b/Sources/tart/VMStorageOCI.swift index 19150ab..4848f77 100644 --- a/Sources/tart/VMStorageOCI.swift +++ b/Sources/tart/VMStorageOCI.swift @@ -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) diff --git a/Tests/TartTests/VMStorageOCITests.swift b/Tests/TartTests/VMStorageOCITests.swift index 33fcf32..5a4a8fc 100644 --- a/Tests/TartTests/VMStorageOCITests.swift +++ b/Tests/TartTests/VMStorageOCITests.swift @@ -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()