From b5d600ff19aa9be5033aa322de444d82f5997332 Mon Sep 17 00:00:00 2001 From: chruffins <23645059+chruffins@users.noreply.github.com> Date: Mon, 31 Aug 2026 18:37:18 +0000 Subject: [PATCH 01/19] Add shared-layer lifecycle management --- .github/workflows/deploy-staging.yml | 3 +- .github/workflows/test.yml | 11 - cmd/api/api/images.go | 44 ++- cmd/api/api/images_test.go | 114 ++++-- lib/devices/mdev_linux.go | 6 +- lib/diskutilization/diskutilization.go | 28 +- lib/diskutilization/diskutilization_test.go | 16 + lib/hypervisor/cloudhypervisor/config.go | 11 +- lib/hypervisor/cloudhypervisor/config_test.go | 10 - lib/hypervisor/config.go | 3 +- lib/hypervisor/qemu/config.go | 11 +- lib/hypervisor/qemu/config_test.go | 43 --- lib/hypervisor/qemu/machine_test.go | 13 - lib/hypervisor/qemu/profile.go | 2 +- lib/hypervisor/socket_pid.go | 5 - lib/hypervisor/socket_pid_linux.go | 142 +++----- lib/hypervisor/socket_pid_linux_test.go | 339 ------------------ lib/hypervisor/socket_pid_other.go | 5 - lib/images/credentials_test.go | 23 +- lib/images/layer_gc.go | 212 +++++++++++ lib/images/lifecycle_test.go | 207 +++++++++++ lib/images/manager_test.go | 18 +- lib/images/manifest_model.go | 27 +- lib/images/manifest_model_test.go | 6 +- lib/images/metrics.go | 30 +- lib/images/metrics_test.go | 6 +- lib/images/oci_public.go | 5 +- lib/images/recovery_regression_test.go | 9 +- lib/images/testlayers_test.go | 52 --- lib/images/testutil/testutil.go | 107 ------ lib/instances/admission_allocations.go | 2 +- lib/instances/admission_allocations_test.go | 22 +- lib/instances/create_mdev_test.go | 6 +- lib/instances/delete_test.go | 10 - lib/instances/fork.go | 7 +- lib/instances/guestmemory_linux_test.go | 2 +- lib/instances/manager.go | 1 - lib/instances/manager_test.go | 24 +- lib/instances/network_test.go | 3 +- lib/instances/process_identity_test.go | 41 --- lib/instances/qemu_lifecycle_test.go | 3 +- lib/instances/query.go | 55 +++ lib/instances/restore.go | 8 +- lib/instances/snapshot_test.go | 113 ------ lib/instances/version_upgrade_test.go | 4 +- lib/instances/vm_config_validation.go | 25 +- lib/instances/vm_config_validation_test.go | 15 +- lib/middleware/resolve.go | 8 - lib/middleware/resolve_test.go | 52 --- lib/paths/paths.go | 5 + lib/resources/gpu.go | 2 +- openapi.yaml | 6 - 52 files changed, 814 insertions(+), 1108 deletions(-) delete mode 100644 lib/hypervisor/socket_pid.go create mode 100644 lib/images/layer_gc.go create mode 100644 lib/images/lifecycle_test.go delete mode 100644 lib/images/testlayers_test.go delete mode 100644 lib/images/testutil/testutil.go delete mode 100644 lib/instances/process_identity_test.go diff --git a/.github/workflows/deploy-staging.yml b/.github/workflows/deploy-staging.yml index 909ca4db3..0c257b308 100644 --- a/.github/workflows/deploy-staging.yml +++ b/.github/workflows/deploy-staging.yml @@ -31,5 +31,4 @@ jobs: --ref main \ --field env=staging \ --field ref="$HYPEMAN_REF" \ - --field cli-version=latest \ - --field triggered_by="${{ github.actor }}" + --field cli-version=latest diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 7ccb6f999..6bc5ee363 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -13,17 +13,6 @@ on: required: false type: string -# Every branch push triggers a full run and superseded runs used to keep -# running, holding self-hosted slots that every repo on the shared pool -# queues behind. Group by ref so a newer push cancels the older run for -# that branch. main and workflow_dispatch runs get run_id, a unique group -# each, because sharing a group is unsafe even with cancellation off: -# GitHub cancels an existing pending run when a newer one enters the -# group, which would drop a commit's only test signal. -concurrency: - group: test-${{ (github.event_name == 'push' && github.ref != 'refs/heads/main') && github.ref || github.run_id }} - cancel-in-progress: ${{ github.event_name == 'push' && github.ref != 'refs/heads/main' }} - # A slash-command dispatch supplies a fork repository and immutable commit SHA. # Normal push runs use the upstream repository and pushed ref. env: diff --git a/cmd/api/api/images.go b/cmd/api/api/images.go index 7ae20ebdc..e9f6defc8 100644 --- a/cmd/api/api/images.go +++ b/cmd/api/api/images.go @@ -94,10 +94,6 @@ func (s *ApiService) CreateImage(ctx context.Context, request oapi.CreateImageRe return oapi.CreateImage202JSONResponse(imageToOAPI(*img)), nil } -// TagImage handles POST /images/{name}/tag. -// Note: ResolveResource skips POST /images/{name}/tag, so the source is -// resolved by ImageManager.TagImage and a missing source gets the specific -// 404 body below. func (s *ApiService) TagImage(ctx context.Context, request oapi.TagImageRequestObject) (oapi.TagImageResponseObject, error) { if request.Body == nil { return oapi.TagImage400JSONResponse{ @@ -108,23 +104,33 @@ func (s *ApiService) TagImage(ctx context.Context, request oapi.TagImageRequestO img, err := s.ImageManager.TagImage(ctx, request.Name, request.Body.Target) if err != nil { - return tagImageErrorResponse(ctx, err, request.Name, request.Body.Target), nil + log := logger.FromContext(ctx) + switch { + case errors.Is(err, images.ErrInvalidName): + return oapi.TagImage400JSONResponse{ + Code: "invalid_name", + Message: err.Error(), + }, nil + case errors.Is(err, images.ErrNotFound): + return oapi.TagImage404JSONResponse{ + Code: "not_found", + Message: "source image not found", + }, nil + case errors.Is(err, images.ErrImageNotReady): + return oapi.TagImage409JSONResponse{ + Code: "image_not_ready", + Message: err.Error(), + }, nil + default: + log.ErrorContext(ctx, "failed to tag image", "error", err, "source", request.Name, "target", request.Body.Target) + return oapi.TagImage500JSONResponse{ + Code: "internal_error", + Message: "failed to tag image", + }, nil + } } - return oapi.TagImage200JSONResponse(imageToOAPI(*img)), nil -} -func tagImageErrorResponse(ctx context.Context, err error, source, target string) oapi.TagImageResponseObject { - switch { - case errors.Is(err, images.ErrInvalidName): - return oapi.TagImage400JSONResponse{Code: "invalid_name", Message: err.Error()} - case errors.Is(err, images.ErrNotFound): - return oapi.TagImage404JSONResponse{Code: "not_found", Message: "source image not found"} - case errors.Is(err, images.ErrImageNotReady): - return oapi.TagImage409JSONResponse{Code: "image_not_ready", Message: err.Error()} - default: - logger.FromContext(ctx).ErrorContext(ctx, "failed to tag image", "error", err, "source", source, "target", target) - return oapi.TagImage500JSONResponse{Code: "internal_error", Message: "failed to tag image"} - } + return oapi.TagImage200JSONResponse(imageToOAPI(*img)), nil } // GetImage gets image details by name diff --git a/cmd/api/api/images_test.go b/cmd/api/api/images_test.go index b6a1857b5..2a5d02367 100644 --- a/cmd/api/api/images_test.go +++ b/cmd/api/api/images_test.go @@ -2,12 +2,14 @@ package api import ( "context" + "encoding/json" "fmt" + "os" + "path/filepath" "testing" "time" "github.com/kernel/hypeman/lib/images" - "github.com/kernel/hypeman/lib/images/testutil" "github.com/kernel/hypeman/lib/oapi" "github.com/kernel/hypeman/lib/paths" "github.com/stretchr/testify/assert" @@ -513,36 +515,58 @@ func seedReadyDigestOnlyImage(t *testing.T, svc *ApiService, imageRef string, im require.NoError(t, err) require.True(t, ref.IsDigest(), "test helper expects a digest reference") - testutil.SeedReadyImage(t, paths.New(svc.Config.DataDir), testutil.Seed{ - Repository: ref.Repository(), - DigestHex: ref.DigestHex(), - Name: imageRef, - Tags: imageTags, - }) + p := paths.New(svc.Config.DataDir) + digestDir := p.ImageDigestDir(ref.Repository(), ref.DigestHex()) + require.NoError(t, os.MkdirAll(digestDir, 0o755)) + require.NoError(t, os.WriteFile(p.ImageDigestPath(ref.Repository(), ref.DigestHex()), []byte("rootfs"), 0o644)) + + meta := struct { + Name string `json:"name"` + Digest string `json:"digest"` + Status string `json:"status"` + SizeBytes int64 `json:"size_bytes"` + Tags map[string]string `json:"tags,omitempty"` + CreatedAt time.Time `json:"created_at"` + }{ + Name: imageRef, + Digest: "sha256:" + ref.DigestHex(), + Status: "ready", + SizeBytes: int64(len("rootfs")), + Tags: imageTags, + CreatedAt: time.Now().UTC(), + } + + data, err := json.Marshal(meta) + require.NoError(t, err) + require.NoError(t, os.WriteFile(p.ImageMetadata(ref.Repository(), ref.DigestHex()), data, 0o644)) } func TestTagImage_ErrorStatusMapping(t *testing.T) { t.Parallel() cases := []struct { - name string - err error - want oapi.TagImageResponseObject + name string + err error + wantType any + wantCode string }{ { - name: "invalid name -> 400", - err: fmt.Errorf("tag: %w", images.ErrInvalidName), - want: oapi.TagImage400JSONResponse{Code: "invalid_name", Message: "tag: invalid image name"}, + name: "invalid name -> 400", + err: fmt.Errorf("tag: %w", images.ErrInvalidName), + wantType: oapi.TagImage400JSONResponse{}, + wantCode: "invalid_name", }, { - name: "not found -> 404", - err: fmt.Errorf("tag: %w", images.ErrNotFound), - want: oapi.TagImage404JSONResponse{Code: "not_found", Message: "source image not found"}, + name: "not found -> 404", + err: fmt.Errorf("tag: %w", images.ErrNotFound), + wantType: oapi.TagImage404JSONResponse{}, + wantCode: "not_found", }, { - name: "not ready -> 409", - err: fmt.Errorf("tag: %w", images.ErrImageNotReady), - want: oapi.TagImage409JSONResponse{Code: "image_not_ready", Message: "tag: image is not ready"}, + name: "not ready -> 409", + err: fmt.Errorf("tag: %w", images.ErrImageNotReady), + wantType: oapi.TagImage409JSONResponse{}, + wantCode: "image_not_ready", }, } @@ -554,7 +578,8 @@ func TestTagImage_ErrorStatusMapping(t *testing.T) { Body: &oapi.TagImageRequest{Target: "docker.io/library/alpine:stable"}, }) require.NoError(t, err) - require.Equal(t, tc.want, resp) + require.IsType(t, tc.wantType, resp) + require.Equal(t, tc.wantCode, tagImageErrorCode(resp)) }) } } @@ -570,16 +595,53 @@ func TestTagImage_MissingBody(t *testing.T) { require.IsType(t, oapi.TagImage400JSONResponse{}, resp) } +func tagImageErrorCode(resp oapi.TagImageResponseObject) string { + switch r := resp.(type) { + case oapi.TagImage400JSONResponse: + return r.Code + case oapi.TagImage404JSONResponse: + return r.Code + case oapi.TagImage409JSONResponse: + return r.Code + case oapi.TagImage500JSONResponse: + return r.Code + default: + return "" + } +} + // seedReadyContentImage writes a ready image into the shared content layout // plus a repository tag reference, without pulling from a registry. func seedReadyContentImage(t *testing.T, svc *ApiService, repository, tag, digestHex string) { t.Helper() - testutil.SeedReadyImage(t, paths.New(svc.Config.DataDir), testutil.Seed{ - Repository: repository, - Tag: tag, - DigestHex: digestHex, - Content: true, - }) + + p := paths.New(svc.Config.DataDir) + contentDir := p.ImageContentDir(digestHex) + require.NoError(t, os.MkdirAll(contentDir, 0o755)) + require.NoError(t, os.WriteFile(p.ImageContentPath(digestHex), []byte("rootfs"), 0o644)) + + meta := struct { + Name string `json:"name"` + Digest string `json:"digest"` + Status string `json:"status"` + SizeBytes int64 `json:"size_bytes"` + CreatedAt time.Time `json:"created_at"` + }{ + Name: repository + ":" + tag, + Digest: "sha256:" + digestHex, + Status: "ready", + SizeBytes: int64(len("rootfs")), + CreatedAt: time.Now().UTC(), + } + data, err := json.Marshal(meta) + require.NoError(t, err) + require.NoError(t, os.WriteFile(p.ImageContentMetadata(digestHex), data, 0o644)) + + linkPath := p.ImageRepositoryTagSymlink(repository, tag) + target, err := filepath.Rel(filepath.Dir(linkPath), contentDir) + require.NoError(t, err) + require.NoError(t, os.MkdirAll(filepath.Dir(linkPath), 0o755)) + require.NoError(t, os.Symlink(target, linkPath)) } func TestTagImage_Success(t *testing.T) { diff --git a/lib/devices/mdev_linux.go b/lib/devices/mdev_linux.go index e8495fda8..fe4ad50cb 100644 --- a/lib/devices/mdev_linux.go +++ b/lib/devices/mdev_linux.go @@ -150,7 +150,7 @@ func discoverMdevVFsWith(busPath, pciPath string, listMdevs func() ([]MdevDevice vfs = append(vfs, VirtualFunction{ PCIAddress: vfAddr, ParentGPU: parentGPU, - Allocated: hasMdev, + HasMdev: hasMdev, }) } if len(vfErrs) > 0 { @@ -276,7 +276,7 @@ func countAvailableVFsForProfilesParallel(vfs []VirtualFunction, profiles []prof // Group free VFs by parent GPU (done once, shared by all goroutines) freeVFsByParent := make(map[string][]VirtualFunction) for _, vf := range vfs { - if vf.Allocated { + if vf.HasMdev { continue } freeVFsByParent[vf.ParentGPU] = append(freeVFsByParent[vf.ParentGPU], vf) @@ -476,7 +476,7 @@ func selectLeastLoadedVF(ctx context.Context, vfs []VirtualFunction, profileType allGPUs := make(map[string]bool) for _, vf := range vfs { allGPUs[vf.ParentGPU] = true - if !vf.Allocated { + if !vf.HasMdev { freeVFsByGPU[vf.ParentGPU] = append(freeVFsByGPU[vf.ParentGPU], vf) } } diff --git a/lib/diskutilization/diskutilization.go b/lib/diskutilization/diskutilization.go index 27fef3c81..a5f55d677 100644 --- a/lib/diskutilization/diskutilization.go +++ b/lib/diskutilization/diskutilization.go @@ -4,6 +4,7 @@ import ( "io/fs" "os" "path/filepath" + "strings" "syscall" "github.com/kernel/hypeman/lib/paths" @@ -56,7 +57,7 @@ func Collect(p *paths.Paths) (Breakdown, error) { return false } name := entry.Name() - return name == "rootfs.erofs" || name == "rootfs.ext4" + return name == "rootfs.erofs" || name == "rootfs.ext4" || strings.HasPrefix(name, "layer.") }) if err != nil { return Breakdown{}, err @@ -178,6 +179,7 @@ func sumDirectChildFileAllocatedBytes(root string, childFile string) (int64, err func sumMatchingFilesAllocatedBytes(root string, match func(path string, entry fs.DirEntry) bool) (int64, error) { var total int64 + seen := make(map[fileIdentity]struct{}) err := filepath.WalkDir(root, func(path string, entry fs.DirEntry, err error) error { if err != nil { if os.IsNotExist(err) { @@ -185,9 +187,24 @@ func sumMatchingFilesAllocatedBytes(root string, match func(path string, entry f } return err } - if match(path, entry) { - total += allocatedBytesForPath(path) + if !match(path, entry) { + return nil } + info, statErr := os.Lstat(path) + if statErr != nil { + if os.IsNotExist(statErr) { + return nil + } + return statErr + } + if stat, ok := info.Sys().(*syscall.Stat_t); ok { + identity := fileIdentity{dev: uint64(stat.Dev), ino: uint64(stat.Ino)} + if _, exists := seen[identity]; exists { + return nil + } + seen[identity] = struct{}{} + } + total += allocatedBytesForPath(path) return nil }) if err != nil { @@ -244,6 +261,11 @@ func sumSnapshotTreeAllocatedBytes(root string, sharedExtents *sharedExtentTrack return privateTotal, sharedTotal, nil } +type fileIdentity struct { + dev uint64 + ino uint64 +} + func allocatedBytesForPath(path string) int64 { info, err := os.Lstat(path) if err != nil { diff --git a/lib/diskutilization/diskutilization_test.go b/lib/diskutilization/diskutilization_test.go index 50a787017..ff854438d 100644 --- a/lib/diskutilization/diskutilization_test.go +++ b/lib/diskutilization/diskutilization_test.go @@ -98,6 +98,22 @@ func TestCollect_UsesAllocatedBytesAndClassifiesSnapshots(t *testing.T) { require.Equal(t, otherTotal, utilization.SnapshotOther) } +func TestCollect_DeduplicatesHardLinkedImagesAndCountsLayers(t *testing.T) { + p := paths.New(t.TempDir()) + imagePath := filepath.Join(p.ImagesDir(), "repo", "digest", "rootfs.erofs") + require.NoError(t, createSparseTestFile(imagePath, 8192, []sparseWrite{{offset: 0, data: []byte("image")}})) + aliasPath := filepath.Join(p.ImagesDir(), "content", "digest", "rootfs.erofs") + require.NoError(t, os.MkdirAll(filepath.Dir(aliasPath), 0755)) + require.NoError(t, os.Link(imagePath, aliasPath)) + + layerPath := filepath.Join(p.ImageLayersDir(), "layer-digest", "layer.erofs") + require.NoError(t, createSparseTestFile(layerPath, 8192, []sparseWrite{{offset: 0, data: []byte("layer")}})) + + utilization, err := Collect(p) + require.NoError(t, err) + require.Equal(t, allocatedBytesForPath(imagePath)+allocatedBytesForPath(layerPath), utilization.Images) +} + func createSparseTestFile(path string, size int64, writes []sparseWrite) error { if err := os.MkdirAll(filepath.Dir(path), 0755); err != nil { return err diff --git a/lib/hypervisor/cloudhypervisor/config.go b/lib/hypervisor/cloudhypervisor/config.go index ca3d98a55..e9f91fe4a 100644 --- a/lib/hypervisor/cloudhypervisor/config.go +++ b/lib/hypervisor/cloudhypervisor/config.go @@ -126,20 +126,13 @@ func ToVMConfig(cfg hypervisor.VMConfig) vmm.VmConfig { // Device passthrough configuration var devices *[]vmm.DeviceConfig - deviceCount := len(cfg.PCIDevices) - if cfg.VGPUDevicePath != "" { - deviceCount++ - } - if deviceCount > 0 { - deviceConfigs := make([]vmm.DeviceConfig, 0, deviceCount) + if len(cfg.PCIDevices) > 0 { + deviceConfigs := make([]vmm.DeviceConfig, 0, len(cfg.PCIDevices)) for _, path := range cfg.PCIDevices { deviceConfigs = append(deviceConfigs, vmm.DeviceConfig{ Path: path, }) } - if cfg.VGPUDevicePath != "" { - deviceConfigs = append(deviceConfigs, vmm.DeviceConfig{Path: cfg.VGPUDevicePath}) - } devices = &deviceConfigs } diff --git a/lib/hypervisor/cloudhypervisor/config_test.go b/lib/hypervisor/cloudhypervisor/config_test.go index be39d13af..b5cdb96e9 100644 --- a/lib/hypervisor/cloudhypervisor/config_test.go +++ b/lib/hypervisor/cloudhypervisor/config_test.go @@ -8,16 +8,6 @@ import ( "github.com/stretchr/testify/require" ) -func TestToVMConfigIncludesVGPUDevice(t *testing.T) { - path := "/sys/bus/mdev/devices/aa618089-8b16-4d01-a136-25a0f3c73123" - - vmCfg := ToVMConfig(hypervisor.VMConfig{VGPUDevicePath: path}) - - require.NotNil(t, vmCfg.Devices) - require.Len(t, *vmCfg.Devices, 1) - assert.Equal(t, path, (*vmCfg.Devices)[0].Path) -} - func TestToVMConfig_GuestMemoryBalloon(t *testing.T) { cfg := hypervisor.VMConfig{ VCPUs: 1, diff --git a/lib/hypervisor/config.go b/lib/hypervisor/config.go index e46aa3446..07f392e8d 100644 --- a/lib/hypervisor/config.go +++ b/lib/hypervisor/config.go @@ -24,8 +24,7 @@ type VMConfig struct { VsockSocket string // PCI device passthrough (GPU, etc.) - PCIDevices []string - VGPUDevicePath string + PCIDevices []string // Boot configuration. Empty BootMode preserves the existing direct-kernel // behavior for Linux callers. diff --git a/lib/hypervisor/qemu/config.go b/lib/hypervisor/qemu/config.go index f00b1a5af..b3959a29a 100644 --- a/lib/hypervisor/qemu/config.go +++ b/lib/hypervisor/qemu/config.go @@ -119,10 +119,13 @@ func buildArgs(cfg hypervisor.VMConfig, machine MachineType) []string { args = append(args, "-device", fmt.Sprintf("%s,guest-cid=%d", virtioDevice(microvm, "vhost-vsock"), cfg.VsockCID)) } - // Whole-device PCI passthrough (vGPU attaches via VGPUDevicePath below) + // PCI device passthrough (GPU, mdev vGPU, etc.) for _, devicePath := range cfg.PCIDevices { var deviceArg string - if strings.HasPrefix(devicePath, "/sys/bus/pci/devices/") { + if strings.HasPrefix(devicePath, "/sys/bus/mdev/devices/") { + // mdev device (vGPU) - use sysfsdev parameter + deviceArg = fmt.Sprintf("vfio-pci,sysfsdev=%s", devicePath) + } else if strings.HasPrefix(devicePath, "/sys/bus/pci/devices/") { // Full sysfs path for regular PCI device - extract the PCI address // Using filepath.Base is more robust than manual string splitting pciAddr := filepath.Base(strings.TrimSuffix(devicePath, "/")) @@ -134,10 +137,6 @@ func buildArgs(cfg hypervisor.VMConfig, machine MachineType) []string { args = append(args, "-device", deviceArg) } - if cfg.VGPUDevicePath != "" { - args = append(args, "-device", fmt.Sprintf("vfio-pci,sysfsdev=%s", cfg.VGPUDevicePath)) - } - // Serial console output to file. Use a chardev with append=on so QEMU // opens the file with O_APPEND. Without it, QEMU writes at its internal // fd offset; if the file is externally truncated (e.g. log rotation via diff --git a/lib/hypervisor/qemu/config_test.go b/lib/hypervisor/qemu/config_test.go index 71e8080af..4d4ae0595 100644 --- a/lib/hypervisor/qemu/config_test.go +++ b/lib/hypervisor/qemu/config_test.go @@ -152,49 +152,6 @@ func TestBuildArgs_Vsock(t *testing.T) { assert.Contains(t, args, "vhost-vsock-pci,guest-cid=123") } -func TestBuildArgs_VGPU(t *testing.T) { - t.Parallel() - - for _, path := range []string{ - "/sys/bus/mdev/devices/aa618089-8b16-4d01-a136-25a0f3c73123", - "/sys/bus/pci/devices/0000:82:00.4", - } { - path := path - t.Run(path, func(t *testing.T) { - t.Parallel() - args := BuildArgs(hypervisor.VMConfig{ - VCPUs: 1, - MemoryBytes: 512 * 1024 * 1024, - VGPUDevicePath: path, - }) - assert.Contains(t, args, "vfio-pci,sysfsdev="+path) - }) - } -} - -func TestBuildArgs_VGPUAfterPCIDevices(t *testing.T) { - args := BuildArgs(hypervisor.VMConfig{ - VCPUs: 1, - MemoryBytes: 512 * 1024 * 1024, - PCIDevices: []string{"0000:01:00.0"}, - VGPUDevicePath: "/sys/bus/mdev/devices/aa618089-8b16-4d01-a136-25a0f3c73123", - }) - - pciDeviceIndex := -1 - vgpuDeviceIndex := -1 - for i, arg := range args { - switch arg { - case "vfio-pci,host=0000:01:00.0": - pciDeviceIndex = i - case "vfio-pci,sysfsdev=/sys/bus/mdev/devices/aa618089-8b16-4d01-a136-25a0f3c73123": - vgpuDeviceIndex = i - } - } - - assert.Greater(t, pciDeviceIndex, -1) - assert.Greater(t, vgpuDeviceIndex, pciDeviceIndex) -} - func TestBuildArgs_PCIPassthrough(t *testing.T) { cfg := hypervisor.VMConfig{ VCPUs: 1, diff --git a/lib/hypervisor/qemu/machine_test.go b/lib/hypervisor/qemu/machine_test.go index 9484cdcde..9998d9280 100644 --- a/lib/hypervisor/qemu/machine_test.go +++ b/lib/hypervisor/qemu/machine_test.go @@ -81,19 +81,6 @@ func TestQEMUCapabilitiesAdvertiseFork(t *testing.T) { assert.True(t, (MicroVMProfile{}).capabilities().SupportsFork) } -func TestMicroVMValidateConfigRejectsVFIODevices(t *testing.T) { - t.Parallel() - err := (MicroVMProfile{}).validateConfig(hypervisor.VMConfig{ - PCIDevices: []string{"0000:82:00.4"}, - }) - require.ErrorContains(t, err, "microvm does not support PCI devices") - - err = (MicroVMProfile{}).validateConfig(hypervisor.VMConfig{ - VGPUDevicePath: "/sys/bus/mdev/devices/aa618089-8b16-4d01-a136-25a0f3c73123", - }) - require.ErrorContains(t, err, "microvm does not support PCI devices") -} - func TestValidateConfigMicroVM(t *testing.T) { t.Parallel() if _, err := microVMMachineType(); err != nil { diff --git a/lib/hypervisor/qemu/profile.go b/lib/hypervisor/qemu/profile.go index 0652551f9..07485b849 100644 --- a/lib/hypervisor/qemu/profile.go +++ b/lib/hypervisor/qemu/profile.go @@ -55,7 +55,7 @@ func (MicroVMProfile) validateConfig(cfg hypervisor.VMConfig) error { if cfg.HotplugBytes > 0 { return fmt.Errorf("microvm does not support hotplug memory") } - if len(cfg.PCIDevices) > 0 || cfg.VGPUDevicePath != "" { + if len(cfg.PCIDevices) > 0 { return fmt.Errorf("microvm does not support PCI devices") } diff --git a/lib/hypervisor/socket_pid.go b/lib/hypervisor/socket_pid.go deleted file mode 100644 index 0d009f9ee..000000000 --- a/lib/hypervisor/socket_pid.go +++ /dev/null @@ -1,5 +0,0 @@ -package hypervisor - -import "errors" - -var ErrNoOwningProcess = errors.New("no owning process found") diff --git a/lib/hypervisor/socket_pid_linux.go b/lib/hypervisor/socket_pid_linux.go index 45b280edc..7f46ebfa3 100644 --- a/lib/hypervisor/socket_pid_linux.go +++ b/lib/hypervisor/socket_pid_linux.go @@ -4,77 +4,36 @@ package hypervisor import ( "bufio" - "errors" "fmt" - "io/fs" "os" "path/filepath" - "slices" "strconv" "strings" - "syscall" ) -var procDir = "/proc" - -// soAcceptcon marks a listening socket in /proc/net/unix (__SO_ACCEPTCON). -const soAcceptcon = 0x10000 - // ResolveProcessPID finds the process currently holding the listening Unix -// socket for the given hypervisor control path, via the socket inode in -// /proc/net/unix and each process's fd table. The fd scan requires the -// caller to hold CAP_SYS_PTRACE (or run as root) so no live owner is missed; -// an ErrNoOwningProcess result is proof the listener is gone. -func ResolveProcessPID(socketPath string) (pid int, err error) { - return resolveProcessPID(socketPath, 0) -} - -// ResolveProcessPIDForOwner resolves a socket while preferring an expected -// owner when the socket descriptor is temporarily shared with a child process. -func ResolveProcessPIDForOwner(socketPath string, ownerPID int) (pid int, err error) { - return resolveProcessPID(socketPath, ownerPID) -} - -func resolveProcessPID(socketPath string, ownerPID int) (pid int, err error) { +// socket for the given hypervisor control path. +func ResolveProcessPID(socketPath string) (int, error) { socketRef, err := socketRefForPath(socketPath) - if err != nil { - return 0, err - } - // Confirm the expected owner first so a live stored PID does not - // require scanning every process fd. - if ownerPID > 0 && processHoldsSocketRef(ownerPID, socketRef) { - return ownerPID, nil + if err == nil { + if pid, refErr := pidBySocketRef(socketRef); refErr == nil { + return pid, nil + } } - return pidBySocketRef(socketRef, ownerPID) -} -func processHoldsSocketRef(pid int, socketRef string) bool { - fdEntries, err := os.ReadDir(filepath.Join(procDir, strconv.Itoa(pid), "fd")) - if err != nil { - return false + if pid, cmdErr := pidByCmdline(socketPath); cmdErr == nil { + return pid, nil } - for _, fdEntry := range fdEntries { - target, err := os.Readlink(filepath.Join(procDir, strconv.Itoa(pid), "fd", fdEntry.Name())) - if err != nil { - // Skip fds that cannot be read, like the full scan does: an fd - // vanishing mid-scan must not hide a listener held by a later fd. - continue - } - if strings.TrimSpace(target) == socketRef { - return true - } - } - return false + + return 0, fmt.Errorf("resolve process pid for socket %s: no owning process found", socketPath) } -func pidBySocketRef(socketRef string, ownerPID int) (int, error) { - procEntries, err := os.ReadDir(procDir) +func pidBySocketRef(socketRef string) (int, error) { + procEntries, err := os.ReadDir("/proc") if err != nil { return 0, fmt.Errorf("read /proc: %w", err) } - var owners []int - var scanErr error for _, entry := range procEntries { if !entry.IsDir() { continue @@ -85,57 +44,62 @@ func pidBySocketRef(socketRef string, ownerPID int) (int, error) { continue } - fdEntries, err := os.ReadDir(filepath.Join(procDir, entry.Name(), "fd")) + fdEntries, err := os.ReadDir(filepath.Join("/proc", entry.Name(), "fd")) if err != nil { - if errors.Is(err, fs.ErrNotExist) || errors.Is(err, syscall.ESRCH) { - continue - } - scanErr = err continue } for _, fdEntry := range fdEntries { - target, err := os.Readlink(filepath.Join(procDir, entry.Name(), "fd", fdEntry.Name())) + target, err := os.Readlink(filepath.Join("/proc", entry.Name(), "fd", fdEntry.Name())) if err != nil { - if errors.Is(err, fs.ErrNotExist) || errors.Is(err, syscall.ESRCH) { - continue - } - scanErr = err continue } if strings.TrimSpace(target) == socketRef { - owners = append(owners, pid) - break + return pid, nil } } } - // The scan observed ownerPID holding the listener fd — the same evidence - // the fast path uses — so a child transiently sharing the inherited fd - // must not turn a proven owner into an error. - if ownerPID > 0 && slices.Contains(owners, ownerPID) { - return ownerPID, nil - } - if len(owners) == 1 { - return owners[0], nil - } - if len(owners) > 1 { - return 0, fmt.Errorf("resolve process pid for %s: multiple owning processes found: %v", socketRef, owners) + return 0, fmt.Errorf("resolve process pid for %s: no owning process found", socketRef) +} + +func pidByCmdline(socketPath string) (int, error) { + procEntries, err := os.ReadDir("/proc") + if err != nil { + return 0, fmt.Errorf("read /proc: %w", err) } - if scanErr != nil { - return 0, fmt.Errorf("resolve process pid for %s: inspect process fds: %w", socketRef, scanErr) + + for _, entry := range procEntries { + if !entry.IsDir() { + continue + } + + pid, err := strconv.Atoi(entry.Name()) + if err != nil { + continue + } + + cmdline, err := os.ReadFile(filepath.Join("/proc", entry.Name(), "cmdline")) + if err != nil || len(cmdline) == 0 { + continue + } + for _, arg := range strings.Split(string(cmdline), "\x00") { + if arg == socketPath { + return pid, nil + } + } } - return 0, fmt.Errorf("resolve process pid for %s: %w", socketRef, ErrNoOwningProcess) + + return 0, fmt.Errorf("resolve process pid for socket %s: no matching command line found", socketPath) } func socketRefForPath(socketPath string) (string, error) { - file, err := os.Open(filepath.Join(procDir, "net", "unix")) + file, err := os.Open("/proc/net/unix") if err != nil { return "", fmt.Errorf("open /proc/net/unix: %w", err) } defer file.Close() scanner := bufio.NewScanner(file) - var socketRef string for scanner.Scan() { fields := strings.Fields(scanner.Text()) if len(fields) < 7 { @@ -148,26 +112,14 @@ func socketRefForPath(socketPath string) (string, error) { if path != socketPath { continue } - // Accepted server-side sockets list the bound path too; only the - // listener identifies the owning process. - flags, parseErr := strconv.ParseUint(fields[3], 16, 32) - if parseErr != nil || flags&soAcceptcon == 0 { - continue - } inode := fields[6] if inode == "" { break } - if socketRef != "" { - return "", fmt.Errorf("resolve process pid for socket %s: multiple socket inodes found", socketPath) - } - socketRef = fmt.Sprintf("socket:[%s]", inode) + return fmt.Sprintf("socket:[%s]", inode), nil } if err := scanner.Err(); err != nil { return "", fmt.Errorf("scan /proc/net/unix: %w", err) } - if socketRef != "" { - return socketRef, nil - } - return "", fmt.Errorf("resolve process pid for socket %s: socket inode not found: %w", socketPath, ErrNoOwningProcess) + return "", fmt.Errorf("resolve process pid for socket %s: socket inode not found", socketPath) } diff --git a/lib/hypervisor/socket_pid_linux_test.go b/lib/hypervisor/socket_pid_linux_test.go index d47044e9c..270524532 100644 --- a/lib/hypervisor/socket_pid_linux_test.go +++ b/lib/hypervisor/socket_pid_linux_test.go @@ -3,14 +3,10 @@ package hypervisor import ( - "context" - "errors" "net" "os" - "os/exec" "path/filepath" "testing" - "time" "github.com/stretchr/testify/require" ) @@ -27,338 +23,3 @@ func TestResolveProcessPID(t *testing.T) { require.NoError(t, err) require.Equal(t, os.Getpid(), pid) } - -func TestResolveProcessPIDIgnoresConnectedSocketEntries(t *testing.T) { - tmpDir := t.TempDir() - socketPath := filepath.Join(tmpDir, "test.sock") - - listener, err := net.Listen("unix", socketPath) - require.NoError(t, err) - defer listener.Close() - - // Accepted server-side sockets share the listener's path in - // /proc/net/unix; they must not make the listener's inode ambiguous. - conn, err := net.Dial("unix", socketPath) - require.NoError(t, err) - defer conn.Close() - accepted, err := listener.Accept() - require.NoError(t, err) - defer accepted.Close() - - pid, err := ResolveProcessPID(socketPath) - require.NoError(t, err) - require.Equal(t, os.Getpid(), pid) -} - -func TestResolveProcessPIDFailsForDuplicateSocketPaths(t *testing.T) { - oldProcDir := procDir - procDir = t.TempDir() - t.Cleanup(func() { procDir = oldProcDir }) - - socketPath := "/tmp/test.sock" - require.NoError(t, os.MkdirAll(filepath.Join(procDir, "net"), 0o755)) - require.NoError(t, os.WriteFile(filepath.Join(procDir, "net", "unix"), []byte( - "00000000: 00000002 00000000 00010000 0001 01 12345 "+socketPath+"\n"+ - "00000000: 00000002 00000000 00010000 0001 01 67890 "+socketPath+"\n"), 0o644)) - - _, err := ResolveProcessPID(socketPath) - require.ErrorContains(t, err, "multiple socket inodes found") -} - -func TestResolveProcessPIDToleratesExitedProcess(t *testing.T) { - oldProcDir := procDir - procDir = t.TempDir() - t.Cleanup(func() { procDir = oldProcDir }) - - socketPath := "/tmp/test.sock" - require.NoError(t, os.MkdirAll(filepath.Join(procDir, "net"), 0o755)) - require.NoError(t, os.WriteFile(filepath.Join(procDir, "net", "unix"), []byte("00000000: 00000002 00000000 00010000 0001 01 12345 "+socketPath+"\n"), 0o644)) - require.NoError(t, os.MkdirAll(filepath.Join(procDir, "100"), 0o755)) - fdDir := filepath.Join(procDir, "200", "fd") - require.NoError(t, os.MkdirAll(fdDir, 0o755)) - require.NoError(t, os.Symlink("socket:[12345]", filepath.Join(fdDir, "3"))) - - pid, err := ResolveProcessPID(socketPath) - require.NoError(t, err) - require.Equal(t, 200, pid) -} - -func TestResolveProcessPIDForOwnerPrefersExpectedProcess(t *testing.T) { - oldProcDir := procDir - procDir = t.TempDir() - t.Cleanup(func() { procDir = oldProcDir }) - - socketPath := "/tmp/test.sock" - require.NoError(t, os.MkdirAll(filepath.Join(procDir, "net"), 0o755)) - require.NoError(t, os.WriteFile(filepath.Join(procDir, "net", "unix"), []byte("00000000: 00000002 00000000 00010000 0001 01 12345 "+socketPath+"\n"), 0o644)) - for _, pid := range []string{"100", "101"} { - fdDir := filepath.Join(procDir, pid, "fd") - require.NoError(t, os.MkdirAll(fdDir, 0o755)) - require.NoError(t, os.Symlink("socket:[12345]", filepath.Join(fdDir, "3"))) - } - - _, err := ResolveProcessPID(socketPath) - require.ErrorContains(t, err, "multiple owning processes found") - - pid, err := ResolveProcessPIDForOwner(socketPath, 101) - require.NoError(t, err) - require.Equal(t, 101, pid) -} - -func TestPidBySocketRefPrefersExpectedOwnerAmongMultiple(t *testing.T) { - oldProcDir := procDir - procDir = t.TempDir() - t.Cleanup(func() { procDir = oldProcDir }) - - // The full scan itself must prefer the expected owner when a child - // transiently shares the inherited listener fd: the fast path can miss on - // a transient fd-dir read failure, and the scan's observation of the - // owner holding the fd is the same evidence the fast path would have used. - for _, pid := range []string{"100", "101"} { - fdDir := filepath.Join(procDir, pid, "fd") - require.NoError(t, os.MkdirAll(fdDir, 0o755)) - require.NoError(t, os.Symlink("socket:[12345]", filepath.Join(fdDir, "3"))) - } - - pid, err := pidBySocketRef("socket:[12345]", 101) - require.NoError(t, err) - require.Equal(t, 101, pid) - - _, err = pidBySocketRef("socket:[12345]", 0) - require.ErrorContains(t, err, "multiple owning processes found") - - _, err = pidBySocketRef("socket:[12345]", 999) - require.ErrorContains(t, err, "multiple owning processes found") -} - -func TestResolveProcessPIDReportsNoOwnerAfterExitedProcesses(t *testing.T) { - oldProcDir := procDir - procDir = t.TempDir() - t.Cleanup(func() { procDir = oldProcDir }) - - socketPath := "/tmp/test.sock" - require.NoError(t, os.MkdirAll(filepath.Join(procDir, "net"), 0o755)) - require.NoError(t, os.WriteFile(filepath.Join(procDir, "net", "unix"), []byte("00000000: 00000002 00000000 00010000 0001 01 12345 "+socketPath+"\n"), 0o644)) - require.NoError(t, os.MkdirAll(filepath.Join(procDir, "100"), 0o755)) - - _, err := ResolveProcessPID(socketPath) - require.ErrorIs(t, err, ErrNoOwningProcess) - require.NotContains(t, err.Error(), "inspect process fds") -} - -func TestResolveProcessPIDReportsMissingSocket(t *testing.T) { - oldProcDir := procDir - procDir = t.TempDir() - t.Cleanup(func() { procDir = oldProcDir }) - - require.NoError(t, os.MkdirAll(filepath.Join(procDir, "net"), 0o755)) - require.NoError(t, os.WriteFile(filepath.Join(procDir, "net", "unix"), nil, 0o644)) - - _, err := ResolveProcessPID("/tmp/missing.sock") - require.ErrorIs(t, err, ErrNoOwningProcess) -} - -func TestResolveProcessPIDFailsWhenFDIsUnreadable(t *testing.T) { - oldProcDir := procDir - procDir = t.TempDir() - t.Cleanup(func() { procDir = oldProcDir }) - - socketPath := "/tmp/test.sock" - require.NoError(t, os.MkdirAll(filepath.Join(procDir, "net"), 0o755)) - require.NoError(t, os.WriteFile(filepath.Join(procDir, "net", "unix"), []byte("00000000: 00000002 00000000 00010000 0001 01 12345 "+socketPath+"\n"), 0o644)) - fdDir := filepath.Join(procDir, "123", "fd") - require.NoError(t, os.MkdirAll(fdDir, 0o755)) - require.NoError(t, os.WriteFile(filepath.Join(fdDir, "3"), nil, 0o644)) - - _, err := ResolveProcessPID(socketPath) - require.Error(t, err) - require.ErrorContains(t, err, "inspect process fds") - require.False(t, errors.Is(err, ErrNoOwningProcess)) -} - -func TestResolveProcessPIDForOwnerConfirmsCandidateWithoutFullScan(t *testing.T) { - oldProcDir := procDir - procDir = t.TempDir() - t.Cleanup(func() { procDir = oldProcDir }) - - socketPath := "/tmp/test.sock" - require.NoError(t, os.MkdirAll(filepath.Join(procDir, "net"), 0o755)) - require.NoError(t, os.WriteFile(filepath.Join(procDir, "net", "unix"), []byte("00000000: 00000002 00000000 00010000 0001 01 12345 "+socketPath+"\n"), 0o644)) - - fdDir := filepath.Join(procDir, "100", "fd") - require.NoError(t, os.MkdirAll(fdDir, 0o755)) - require.NoError(t, os.Symlink("socket:[12345]", filepath.Join(fdDir, "3"))) - - // An unreadable sibling fd must not block confirming the candidate. - siblingFDDir := filepath.Join(procDir, "123", "fd") - require.NoError(t, os.MkdirAll(siblingFDDir, 0o755)) - require.NoError(t, os.WriteFile(filepath.Join(siblingFDDir, "3"), nil, 0o644)) - - pid, err := ResolveProcessPIDForOwner(socketPath, 100) - require.NoError(t, err) - require.Equal(t, 100, pid) -} - -func TestResolveProcessPIDForOwnerSkipsUnreadableCandidateFD(t *testing.T) { - oldProcDir := procDir - procDir = t.TempDir() - t.Cleanup(func() { procDir = oldProcDir }) - - socketPath := "/tmp/test.sock" - require.NoError(t, os.MkdirAll(filepath.Join(procDir, "net"), 0o755)) - require.NoError(t, os.WriteFile(filepath.Join(procDir, "net", "unix"), []byte("00000000: 00000002 00000000 00010000 0001 01 12345 "+socketPath+"\n"), 0o644)) - - // An unreadable fd before the listener fd must not abort the candidate - // check; the scan skips it and still finds the match. - fdDir := filepath.Join(procDir, "100", "fd") - require.NoError(t, os.MkdirAll(fdDir, 0o755)) - require.NoError(t, os.WriteFile(filepath.Join(fdDir, "1"), nil, 0o644)) - require.NoError(t, os.Symlink("socket:[12345]", filepath.Join(fdDir, "3"))) - - pid, err := ResolveProcessPIDForOwner(socketPath, 100) - require.NoError(t, err) - require.Equal(t, 100, pid) -} - -func TestResolveProcessPIDForOwnerFallsThroughWhenCandidateLacksSocket(t *testing.T) { - oldProcDir := procDir - procDir = t.TempDir() - t.Cleanup(func() { procDir = oldProcDir }) - - socketPath := "/tmp/test.sock" - require.NoError(t, os.MkdirAll(filepath.Join(procDir, "net"), 0o755)) - require.NoError(t, os.WriteFile(filepath.Join(procDir, "net", "unix"), []byte("00000000: 00000002 00000000 00010000 0001 01 12345 "+socketPath+"\n"), 0o644)) - - candidateFDDir := filepath.Join(procDir, "999", "fd") - require.NoError(t, os.MkdirAll(candidateFDDir, 0o755)) - require.NoError(t, os.Symlink("socket:[99999]", filepath.Join(candidateFDDir, "3"))) - - ownerFDDir := filepath.Join(procDir, "200", "fd") - require.NoError(t, os.MkdirAll(ownerFDDir, 0o755)) - require.NoError(t, os.Symlink("socket:[12345]", filepath.Join(ownerFDDir, "3"))) - - pid, err := ResolveProcessPIDForOwner(socketPath, 999) - require.NoError(t, err) - require.Equal(t, 200, pid) -} - -func TestResolveProcessPIDForOwnerFallsThroughWhenCandidateIsGone(t *testing.T) { - oldProcDir := procDir - procDir = t.TempDir() - t.Cleanup(func() { procDir = oldProcDir }) - - socketPath := "/tmp/test.sock" - require.NoError(t, os.MkdirAll(filepath.Join(procDir, "net"), 0o755)) - require.NoError(t, os.WriteFile(filepath.Join(procDir, "net", "unix"), []byte("00000000: 00000002 00000000 00010000 0001 01 12345 "+socketPath+"\n"), 0o644)) - - ownerFDDir := filepath.Join(procDir, "200", "fd") - require.NoError(t, os.MkdirAll(ownerFDDir, 0o755)) - require.NoError(t, os.Symlink("socket:[12345]", filepath.Join(ownerFDDir, "3"))) - - pid, err := ResolveProcessPIDForOwner(socketPath, 999) - require.NoError(t, err) - require.Equal(t, 200, pid) -} - -func TestResolveProcessPIDForOwnerReportsMissingSocket(t *testing.T) { - oldProcDir := procDir - procDir = t.TempDir() - t.Cleanup(func() { procDir = oldProcDir }) - - require.NoError(t, os.MkdirAll(filepath.Join(procDir, "net"), 0o755)) - require.NoError(t, os.WriteFile(filepath.Join(procDir, "net", "unix"), []byte("00000000: 00000002 00000000 00010000 0001 01 12345 /tmp/other.sock\n"), 0o644)) - - _, err := ResolveProcessPIDForOwner("/tmp/missing.sock", 100) - require.ErrorIs(t, err, ErrNoOwningProcess) -} - -func TestResolveProcessPIDForOwnerReportsMissingSocketWithHeaderOnlyUnixTable(t *testing.T) { - oldProcDir := procDir - procDir = t.TempDir() - t.Cleanup(func() { procDir = oldProcDir }) - - require.NoError(t, os.MkdirAll(filepath.Join(procDir, "net"), 0o755)) - require.NoError(t, os.WriteFile(filepath.Join(procDir, "net", "unix"), []byte("Num RefCount Protocol Flags Type St Inode Path\n"), 0o644)) - - _, err := ResolveProcessPIDForOwner("/tmp/missing.sock", 100) - require.ErrorIs(t, err, ErrNoOwningProcess) -} - -func TestResolveProcessPIDForOwnerReportsDuplicateSocketInodes(t *testing.T) { - oldProcDir := procDir - procDir = t.TempDir() - t.Cleanup(func() { procDir = oldProcDir }) - - socketPath := "/tmp/test.sock" - require.NoError(t, os.MkdirAll(filepath.Join(procDir, "net"), 0o755)) - require.NoError(t, os.WriteFile(filepath.Join(procDir, "net", "unix"), []byte( - "00000000: 00000002 00000000 00010000 0001 01 12345 "+socketPath+"\n"+ - "00000000: 00000002 00000000 00010000 0001 01 67890 "+socketPath+"\n"), 0o644)) - - fdDir := filepath.Join(procDir, "100", "fd") - require.NoError(t, os.MkdirAll(fdDir, 0o755)) - require.NoError(t, os.Symlink("socket:[12345]", filepath.Join(fdDir, "3"))) - - _, err := ResolveProcessPIDForOwner(socketPath, 100) - require.ErrorContains(t, err, "multiple socket inodes found") -} - -func TestResolveProcessPIDForOwnerConfirmsLiveListener(t *testing.T) { - tmpDir := t.TempDir() - socketPath := filepath.Join(tmpDir, "test.sock") - - listener, err := net.Listen("unix", socketPath) - require.NoError(t, err) - defer listener.Close() - - pid, err := ResolveProcessPIDForOwner(socketPath, os.Getpid()) - require.NoError(t, err) - require.Equal(t, os.Getpid(), pid) -} - -func TestResolveProcessPIDIgnoresCommandLineBystander(t *testing.T) { - socketPath := filepath.Join(t.TempDir(), "test.sock") - require.NoError(t, os.WriteFile(socketPath, nil, 0o600)) - - // A process carrying the socket path in its command line (e.g. a debug - // client like ch-remote) without holding the listener must not resolve - // as the owner; a missing listener is proof the hypervisor is gone. - bystander := exec.Command("sh", "-c", "sleep 30", "sh", socketPath) - require.NoError(t, bystander.Start()) - t.Cleanup(func() { - _ = bystander.Process.Kill() - _ = bystander.Wait() - }) - - _, err := ResolveProcessPID(socketPath) - require.ErrorIs(t, err, ErrNoOwningProcess) -} - -func TestResolveProcessPIDDuringProcessChurn(t *testing.T) { - socketPath := filepath.Join(t.TempDir(), "test.sock") - listener, err := net.Listen("unix", socketPath) - require.NoError(t, err) - defer listener.Close() - - ctx, cancel := context.WithCancel(context.Background()) - done := make(chan struct{}) - go func() { - defer close(done) - for ctx.Err() == nil { - _ = exec.CommandContext(ctx, "/bin/true").Run() - } - }() - defer func() { - cancel() - <-done - }() - - // Resolve without an owner hint so every iteration runs the full /proc - // scan; the owner fast path never exercises the churn tolerance. - deadline := time.Now().Add(2 * time.Second) - for time.Now().Before(deadline) { - pid, err := ResolveProcessPID(socketPath) - require.NoError(t, err) - require.Equal(t, os.Getpid(), pid) - } -} diff --git a/lib/hypervisor/socket_pid_other.go b/lib/hypervisor/socket_pid_other.go index 1fb594653..75db657e6 100644 --- a/lib/hypervisor/socket_pid_other.go +++ b/lib/hypervisor/socket_pid_other.go @@ -9,8 +9,3 @@ import "fmt" func ResolveProcessPID(socketPath string) (int, error) { return 0, fmt.Errorf("resolve process pid for socket %s: not supported on this platform", socketPath) } - -// ResolveProcessPIDForOwner is only implemented on Linux. -func ResolveProcessPIDForOwner(socketPath string, _ int) (int, error) { - return ResolveProcessPID(socketPath) -} diff --git a/lib/images/credentials_test.go b/lib/images/credentials_test.go index 7f680b980..87a72c39b 100644 --- a/lib/images/credentials_test.go +++ b/lib/images/credentials_test.go @@ -83,10 +83,8 @@ func TestCreateImageRequestCredentialsAreNotPersisted(t *testing.T) { } func TestInflightPullRejectsDifferentCredentials(t *testing.T) { - m := &manager{ - inflightPulls: make(map[string]*inflightImagePull), - borrowedCredentialsTimeout: time.Minute, - } + m := newTestManager(nil) + m.borrowedCredentialsTimeout = time.Minute const digest = "sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" credentials := &authn.AuthConfig{Username: "AWS", Password: "token-a"} inflight := m.registerInflightPull(digest, credentials) @@ -98,10 +96,8 @@ func TestInflightPullRejectsDifferentCredentials(t *testing.T) { } func TestBorrowedCredentialsExpireWhileQueued(t *testing.T) { - m := &manager{ - inflightPulls: make(map[string]*inflightImagePull), - borrowedCredentialsTimeout: time.Millisecond, - } + m := newTestManager(nil) + m.borrowedCredentialsTimeout = time.Millisecond const digest = "sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" inflight := m.registerInflightPull(digest, &authn.AuthConfig{Username: "AWS", Password: "secret"}) defer m.releaseInflightPull(digest, inflight)() @@ -119,7 +115,7 @@ func TestBorrowedCredentialsExpireWhileQueued(t *testing.T) { } func TestBorrowedAuthRejectsReplacedInflightPull(t *testing.T) { - m := &manager{inflightPulls: make(map[string]*inflightImagePull)} + m := newTestManager(nil) const digest = "sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" first := m.registerInflightPull(digest, &authn.AuthConfig{Username: "first"}) second := m.registerInflightPull(digest, &authn.AuthConfig{Username: "second"}) @@ -174,12 +170,9 @@ func TestRecoverInterruptedCredentialedPullFailsForFreshRetry(t *testing.T) { p := paths.New(t.TempDir()) client, err := newOCIClient(p.SystemOCICache()) require.NoError(t, err) - m := &manager{ - paths: p, - ociClient: client, - queue: queue.New(1), - readySubscribers: make(map[string][]chan StatusEvent), - } + m := newTestManager(p) + m.ociClient = client + m.queue = queue.New(1) const repository = "registry.example/private/image" const digest = "sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" diff --git a/lib/images/layer_gc.go b/lib/images/layer_gc.go new file mode 100644 index 000000000..aefdadfc0 --- /dev/null +++ b/lib/images/layer_gc.go @@ -0,0 +1,212 @@ +package images + +import ( + "context" + "fmt" + "io/fs" + "log/slog" + "os" + "path/filepath" + "strings" + "time" +) + +// layerEvictionGracePeriod keeps freshly written layer artifacts and temp +// directories out of cleanup so recovery and eviction never race builds that +// are still writing them. +const layerEvictionGracePeriod = 10 * time.Minute + +// referencedLayerDigests returns the set of layer blob digests referenced by +// the manifest models of every image in the content layout, plus the digests +// currently referenced by in-flight builds. Layer artifacts in this set are +// protected from eviction. Unreadable manifest models are skipped with a +// warning so one corrupt record cannot disable eviction entirely. +func (m *manager) referencedLayerDigests() map[string]struct{} { + refs := m.inflightLayerRefSnapshot() + contentRoot := filepath.Join(m.paths.ImagesDir(), "content") + err := filepath.WalkDir(contentRoot, func(path string, entry fs.DirEntry, err error) error { + if err != nil { + if os.IsNotExist(err) { + return nil + } + return err + } + if entry.IsDir() || entry.Name() != "manifest.json" { + return nil + } + digestHex := filepath.Base(filepath.Dir(path)) + model, readErr := readManifestModel(m.paths, digestHex) + if readErr != nil { + slog.Warn("skipping unreadable manifest model for layer eviction", "digest", digestHex, "error", readErr) + return nil + } + if model == nil { + return nil + } + for _, layer := range model.Layers { + refs[strings.TrimPrefix(layer.Digest, "sha256:")] = struct{}{} + } + return nil + }) + if err != nil && !os.IsNotExist(err) { + slog.Warn("failed to walk content manifests for layer eviction", "error", err) + } + return refs +} + +// inflightLayerRefSnapshot returns the layer digests currently retained by +// in-flight builds. +func (m *manager) inflightLayerRefSnapshot() map[string]struct{} { + m.layerRefMu.Lock() + defer m.layerRefMu.Unlock() + refs := make(map[string]struct{}, len(m.inflightLayerRefs)) + for digestHex := range m.inflightLayerRefs { + refs[digestHex] = struct{}{} + } + return refs +} + +// reconcileLayerStore evicts unreferenced layer artifacts and refreshes the +// cached disk usage totals so accounting reflects the removals. +func (m *manager) reconcileLayerStore() { + m.evictUnreferencedLayerArtifacts() + m.refreshDiskUsageTotals() +} + +// evictUnreferencedLayerArtifacts removes layer artifacts that no image +// manifest model references, deleting the digest directory entirely. Artifacts +// newer than the grace period are kept so in-flight builds never lose work. +func (m *manager) evictUnreferencedLayerArtifacts() { + refs := m.referencedLayerDigests() + + layersDir := m.paths.ImageLayersDir() + entries, err := os.ReadDir(layersDir) + if err != nil { + if !os.IsNotExist(err) { + slog.Warn("layer eviction failed to list layer store", "error", err) + } + return + } + + cutoff := time.Now().Add(-m.layerEvictionGrace) + evicted := 0 + var evictedBytes int64 + for _, entry := range entries { + if !entry.IsDir() { + continue + } + digestHex := entry.Name() + if _, referenced := refs[digestHex]; referenced { + continue + } + size, removed := m.tryEvictLayerArtifact(digestHex, filepath.Join(layersDir, digestHex), cutoff) + if !removed { + continue + } + evicted++ + evictedBytes += size + } + if evicted > 0 { + slog.Info("evicted unreferenced layer artifacts", "count", evicted, "bytes", evictedBytes) + if m.metrics != nil { + m.metrics.layerArtifactsEvicted.Add(context.Background(), int64(evicted)) + } + } +} + +// tryEvictLayerArtifact removes one unreferenced layer artifact if it is still +// stale and no build is materializing it. The per-digest lock is taken with +// TryLock so eviction never blocks behind an in-flight conversion. +func (m *manager) tryEvictLayerArtifact(digestHex, dirPath string, cutoff time.Time) (int64, bool) { + lock := m.layerDigestLock(digestHex) + if !lock.TryLock() { + return 0, false + } + defer lock.Unlock() + + // The candidate was selected outside the lock; re-check that a build has + // not retained the digest and the artifact has not been rewritten since. + if _, referenced := m.inflightLayerRefSnapshot()[digestHex]; referenced { + return 0, false + } + info, statErr := os.Stat(dirPath) + if statErr != nil || info.ModTime().After(cutoff) { + return 0, false + } + size, err := dirSize(dirPath) + if err != nil { + slog.Warn("failed to measure layer artifact size", "digest", digestHex, "error", err) + } + if err := os.RemoveAll(dirPath); err != nil { + slog.Warn("failed to evict unreferenced layer artifact", "digest", digestHex, "error", err) + return 0, false + } + return size, true +} + +// cleanStaleImageTempDirs removes temp directories left behind by builds that +// were interrupted mid-install, mid-materialization, or mid-tag promotion. +// Only directories older than the grace period are removed so live builds are +// never disturbed. +func (m *manager) cleanStaleImageTempDirs() { + roots := []string{ + m.paths.ImageLayersDir(), + filepath.Join(m.paths.ImagesDir(), "content"), + } + cutoff := time.Now().Add(-m.layerEvictionGrace) + for _, root := range roots { + err := filepath.WalkDir(root, func(path string, entry fs.DirEntry, err error) error { + if err != nil { + if os.IsNotExist(err) { + return nil + } + return err + } + if !entry.IsDir() { + return nil + } + name := entry.Name() + if !strings.HasPrefix(name, ".unpack-") && !strings.HasPrefix(name, ".install-") && !strings.HasPrefix(name, ".tag-stage-") { + return nil + } + info, statErr := os.Stat(path) + if statErr == nil && info.ModTime().Before(cutoff) { + _ = os.RemoveAll(path) + } + return fs.SkipDir + }) + if err != nil && !os.IsNotExist(err) { + slog.Warn("failed to clean stale image temp dirs", "root", root, "error", err) + } + } +} + +// totalLayerArtifactBytes sums the bytes held by materialized layer +// artifacts, matching what diskutilization.Collect counts for the same store. +func totalLayerArtifactBytes(layersDir string) (int64, error) { + var total int64 + err := filepath.WalkDir(layersDir, func(path string, entry fs.DirEntry, err error) error { + if err != nil { + if os.IsNotExist(err) { + return nil + } + return err + } + if entry.IsDir() { + return nil + } + if !strings.HasPrefix(entry.Name(), "layer.") { + return nil + } + info, statErr := entry.Info() + if statErr != nil { + return nil + } + total += info.Size() + return nil + }) + if err != nil && !os.IsNotExist(err) { + return 0, fmt.Errorf("walk layer artifacts: %w", err) + } + return total, nil +} diff --git a/lib/images/lifecycle_test.go b/lib/images/lifecycle_test.go new file mode 100644 index 000000000..1554f1501 --- /dev/null +++ b/lib/images/lifecycle_test.go @@ -0,0 +1,207 @@ +package images + +import ( + "context" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + "time" + + gcr "github.com/google/go-containerregistry/pkg/v1" + "github.com/google/go-containerregistry/pkg/v1/empty" + "github.com/google/go-containerregistry/pkg/v1/layout" + "github.com/google/go-containerregistry/pkg/v1/mutate" + "github.com/kernel/hypeman/lib/paths" + "github.com/stretchr/testify/require" +) + +// writeSharedLayout writes several images into one OCI layout cache, each +// annotated with its own digest tag. +func writeSharedLayout(t *testing.T, p *paths.Paths, imgs ...gcr.Image) []string { + t.Helper() + + layoutPath, err := layout.Write(p.SystemOCICache(), empty.Index) + require.NoError(t, err) + + digests := make([]string, 0, len(imgs)) + for _, img := range imgs { + digest, err := img.Digest() + require.NoError(t, err) + require.NoError(t, layoutPath.AppendImage(img, layout.WithAnnotations(map[string]string{ + "org.opencontainers.image.ref.name": digestToLayoutTag(digest.String()), + }))) + digests = append(digests, digest.String()) + } + return digests +} + +func layerHexes(t *testing.T, p *paths.Paths) map[string]struct{} { + t.Helper() + entries, err := os.ReadDir(p.ImageLayersDir()) + require.NoError(t, err) + hexes := make(map[string]struct{}) + for _, entry := range entries { + if entry.IsDir() { + hexes[entry.Name()] = struct{}{} + } + } + return hexes +} + +// TestSharedLayersMaterializeOnceAndEvictWithReferences is the end-to-end +// lifecycle: two images share a base layer, the shared artifact is created +// once, survives the deletion of one image, and is evicted only when its last +// reference is gone. +func TestSharedLayersMaterializeOnceAndEvictWithReferences(t *testing.T) { + if _, err := exec.LookPath("mkfs.erofs"); err != nil { + t.Skip("mkfs.erofs not available") + } + dataDir := t.TempDir() + p := paths.New(dataDir) + mgr, err := NewManager(p, 1, nil) + require.NoError(t, err) + m := mgr.(*manager) + m.layerEvictionGrace = 0 + + base := syntheticLayer(t, "base.txt", "shared base content") + topA := syntheticLayer(t, "a.txt", "app A payload") + topB := syntheticLayer(t, "b.txt", "app B payload") + + imgA, err := mutate.AppendLayers(empty.Image, base, topA) + require.NoError(t, err) + imgB, err := mutate.AppendLayers(empty.Image, base, topB) + require.NoError(t, err) + + digests := writeSharedLayout(t, p, imgA, imgB) + digestA, digestB := digests[0], digests[1] + + baseManifest, err := imgA.Manifest() + require.NoError(t, err) + baseHex := baseManifest.Layers[0].Digest.Hex + topAHex := baseManifest.Layers[1].Digest.Hex + topBManifest, err := imgB.Manifest() + require.NoError(t, err) + topBHex := topBManifest.Layers[1].Digest.Hex + + ctx := context.Background() + const repoA = "kernel.local/apps/app-a" + const repoB = "kernel.local/apps/app-b" + + eventsA := make(chan StatusEvent, 2) + m.subscribeToReady(digestToLayoutTag(digestA), eventsA) + defer m.unsubscribeFromReady(digestToLayoutTag(digestA), eventsA) + _, err = m.ImportLocalImage(ctx, repoA, "v1", digestA) + require.NoError(t, err) + select { + case event := <-eventsA: + require.Equal(t, StatusReady, event.Status) + case <-time.After(30 * time.Second): + t.Fatal("image A did not become ready") + } + + eventsB := make(chan StatusEvent, 2) + m.subscribeToReady(digestToLayoutTag(digestB), eventsB) + defer m.unsubscribeFromReady(digestToLayoutTag(digestB), eventsB) + _, err = m.ImportLocalImage(ctx, repoB, "v1", digestB) + require.NoError(t, err) + select { + case event := <-eventsB: + require.Equal(t, StatusReady, event.Status) + case <-time.After(30 * time.Second): + t.Fatal("image B did not become ready") + } + + // The shared base layer materialized exactly once, alongside the two tops. + hexes := layerHexes(t, p) + require.Len(t, hexes, 3) + require.Contains(t, hexes, baseHex) + require.Contains(t, hexes, topAHex) + require.Contains(t, hexes, topBHex) + + // Deleting image A evicts only its unique layer; the shared base survives. + require.NoError(t, m.DeleteImage(ctx, repoA+"@"+digestA)) + hexes = layerHexes(t, p) + require.Len(t, hexes, 2) + require.Contains(t, hexes, baseHex, "shared base must survive while referenced") + require.Contains(t, hexes, topBHex) + require.NotContains(t, hexes, topAHex) + + // Deleting image B removes the last references: everything is evicted. + require.NoError(t, m.DeleteImage(ctx, repoB+"@"+digestB)) + hexes = layerHexes(t, p) + require.Empty(t, hexes, "unreferenced layer artifacts must be evicted") +} + +func TestTotalImageBytesIncludesLayerArtifacts(t *testing.T) { + p := paths.New(t.TempDir()) + m := newTestManager(p) + + digestHex := "cd01cd01cd01cd01cd01cd01cd01cd01cd01cd01cd01cd01cd01cd01cd01cd01" + require.NoError(t, os.MkdirAll(p.ImageLayerDir(digestHex), 0o755)) + payload := make([]byte, 4096) + require.NoError(t, os.WriteFile(p.ImageLayerArtifact(digestHex), payload, 0o644)) + + totals, err := m.getDiskUsageTotals() + require.NoError(t, err) + require.GreaterOrEqual(t, totals.layerBytes, int64(len(payload))) + + totalBytes, err := m.TotalImageBytes(context.Background()) + require.NoError(t, err) + require.Equal(t, totals.readyImageBytes+totals.layerBytes, totalBytes) +} + +func TestCleanStaleImageTempDirsRemovesOnlyOldDirectories(t *testing.T) { + p := paths.New(t.TempDir()) + m := newTestManager(p) + m.layerEvictionGrace = time.Hour + + layersDir := p.ImageLayersDir() + staleDir := filepath.Join(layersDir, "ab12", ".unpack-stale") + freshDir := filepath.Join(layersDir, "cd34", ".unpack-fresh") + require.NoError(t, os.MkdirAll(staleDir, 0o755)) + require.NoError(t, os.MkdirAll(freshDir, 0o755)) + old := time.Now().Add(-2 * time.Hour) + require.NoError(t, os.Chtimes(staleDir, old, old)) + + m.cleanStaleImageTempDirs() + + _, err := os.Stat(staleDir) + require.True(t, os.IsNotExist(err), "stale temp dir must be removed") + _, err = os.Stat(freshDir) + require.NoError(t, err, "fresh temp dir must survive cleanup") +} + +func TestEvictionKeepsReferencedAndFreshArtifacts(t *testing.T) { + p := paths.New(t.TempDir()) + m := newTestManager(p) + m.layerEvictionGrace = time.Hour + + referencedHex := "ef01ef01ef01ef01ef01ef01ef01ef01ef01ef01ef01ef01ef01ef01ef01ef01" + orphanFreshHex := "ab23ab23ab23ab23ab23ab23ab23ab23ab23ab23ab23ab23ab23ab23ab23ab23" + + // A manifest model referencing one layer protects it regardless of age. + model := &imageManifestModel{ + SchemaVersion: manifestModelSchemaVersion, + Digest: "sha256:" + referencedHex, + Config: manifestConfigRef{ + Digest: "sha256:" + strings.Repeat("c", 64), + DiffIDs: []string{"sha256:" + referencedHex}, + }, + Layers: []layerDescriptor{{Digest: "sha256:" + referencedHex, DiffID: "sha256:" + referencedHex}}, + } + require.NoError(t, writeManifestModel(p, referencedHex, model)) + require.NoError(t, os.MkdirAll(p.ImageLayerDir(referencedHex), 0o755)) + require.NoError(t, os.WriteFile(p.ImageLayerArtifact(referencedHex), []byte("kept"), 0o644)) + + // An unreferenced but fresh artifact is protected by the grace period. + require.NoError(t, os.MkdirAll(p.ImageLayerDir(orphanFreshHex), 0o755)) + require.NoError(t, os.WriteFile(p.ImageLayerArtifact(orphanFreshHex), []byte("fresh"), 0o644)) + + m.reconcileLayerStore() + + hexes := layerHexes(t, p) + require.Contains(t, hexes, referencedHex) + require.Contains(t, hexes, orphanFreshHex) +} diff --git a/lib/images/manager_test.go b/lib/images/manager_test.go index 13a6835e2..36015dd5b 100644 --- a/lib/images/manager_test.go +++ b/lib/images/manager_test.go @@ -7,6 +7,7 @@ import ( "os" "path/filepath" "strings" + "sync" "testing" "time" @@ -17,6 +18,19 @@ import ( "github.com/stretchr/testify/require" ) +// newTestManager returns a manager with the maps NewManager initializes, so +// tests can construct one directly without nil-map guards in the manager. +func newTestManager(p *paths.Paths) *manager { + return &manager{ + paths: p, + tagGenerations: make(map[string]uint64), + layerDigestLocks: make(map[string]*sync.Mutex), + inflightLayerRefs: make(map[string]int), + inflightPulls: make(map[string]*inflightImagePull), + readySubscribers: make(map[string][]chan StatusEvent), + } +} + func TestConversionFailedErr(t *testing.T) { t.Run("without detail", func(t *testing.T) { assert.EqualError(t, conversionFailedErr(nil, nil), "image conversion failed") @@ -734,9 +748,9 @@ func TestDeleteAndRecreateDuringBuildTail(t *testing.T) { require.NoError(t, err) staleRef := NewResolvedRef(normalized, digestStr) m.updateStatusByDigest(staleRef, StatusFailed, errors.New("stale build"), firstMeta.BuildID) - staleBundle, err := m.ociClient.extractOCIImageBundle(digestHex) + staleResult, _, _, err := m.ociClient.extractOCIImageDetails(digestHex) require.NoError(t, err) - require.ErrorIs(t, m.finalizeImage(staleRef, &pullResult{Metadata: staleBundle.Meta}, 1, firstMeta.BuildID, ""), errStaleBuild) + require.ErrorIs(t, m.finalizeImage(staleRef, &pullResult{Metadata: staleResult}, 1, firstMeta.BuildID, ""), errStaleBuild) currentMeta, err = readMetadata(p, repo, digestHex) require.NoError(t, err) require.Equal(t, StatusPending, currentMeta.Status) diff --git a/lib/images/manifest_model.go b/lib/images/manifest_model.go index 16a913ce4..a640f3e08 100644 --- a/lib/images/manifest_model.go +++ b/lib/images/manifest_model.go @@ -66,6 +66,7 @@ func (m *imageManifestModel) blobReferences() []string { return refs } +// writeManifestModel persists the manifest model for a digest atomically. func validateManifestModel(digestHex string, model *imageManifestModel) error { if model == nil { return fmt.Errorf("manifest model is nil") @@ -169,10 +170,28 @@ func readManifestModel(p *paths.Paths, digestHex string) (*imageManifestModel, e // writeJSONAtomic writes data to path via a temp file in the same directory // followed by a rename, so readers never observe a partial document. func writeJSONAtomic(path string, data []byte) error { - if err := installAtomically(path, func(tempPath string) error { - return os.WriteFile(tempPath, data, 0o644) - }); err != nil { - return fmt.Errorf("write %s: %w", filepath.Base(path), err) + if err := os.MkdirAll(filepath.Dir(path), 0755); err != nil { + return fmt.Errorf("create directory: %w", err) + } + tempFile, err := os.CreateTemp(filepath.Dir(path), "."+filepath.Base(path)+".tmp-*") + if err != nil { + return fmt.Errorf("create temp file: %w", err) + } + tempPath := tempFile.Name() + defer os.Remove(tempPath) + if err := tempFile.Chmod(0644); err != nil { + _ = tempFile.Close() + return fmt.Errorf("chmod temp file: %w", err) + } + if _, err := tempFile.Write(data); err != nil { + _ = tempFile.Close() + return fmt.Errorf("write temp file: %w", err) + } + if err := tempFile.Close(); err != nil { + return fmt.Errorf("close temp file: %w", err) + } + if err := os.Rename(tempPath, path); err != nil { + return fmt.Errorf("rename into place: %w", err) } return nil } diff --git a/lib/images/manifest_model_test.go b/lib/images/manifest_model_test.go index bb0ebf722..d73125445 100644 --- a/lib/images/manifest_model_test.go +++ b/lib/images/manifest_model_test.go @@ -46,9 +46,8 @@ func TestExtractManifestModel(t *testing.T) { client, layoutTag := writeSyntheticLayout(t, img) - bundle, err := client.extractOCIImageBundle(layoutTag) + model, err := client.extractManifestModel(layoutTag) require.NoError(t, err) - model := bundle.Model manifest, err := img.Manifest() require.NoError(t, err) @@ -89,9 +88,8 @@ func TestExtractManifestModelPlatform(t *testing.T) { client, layoutTag := writeSyntheticLayout(t, img) - bundle, err := client.extractOCIImageBundle(layoutTag) + model, err := client.extractManifestModel(layoutTag) require.NoError(t, err) - model := bundle.Model require.Equal(t, "linux/amd64", model.Platform) } diff --git a/lib/images/metrics.go b/lib/images/metrics.go index d860885b2..75164a6cd 100644 --- a/lib/images/metrics.go +++ b/lib/images/metrics.go @@ -11,11 +11,12 @@ import ( // Metrics holds the metrics instruments for image operations. type Metrics struct { - buildDuration metric.Float64Histogram - buildPhaseDuration metric.Float64Histogram - ociLayerCount metric.Int64Histogram - ociCompressedBytes metric.Int64Histogram - pullsTotal metric.Int64Counter + buildDuration metric.Float64Histogram + buildPhaseDuration metric.Float64Histogram + ociLayerCount metric.Int64Histogram + ociCompressedBytes metric.Int64Histogram + pullsTotal metric.Int64Counter + layerArtifactsEvicted metric.Int64Counter } // newMetrics creates and registers all image metrics. @@ -78,6 +79,14 @@ func newMetrics(meter metric.Meter, m *manager) (*Metrics, error) { return nil, err } + layerArtifactsEvicted, err := meter.Int64Counter( + "hypeman_images_layer_artifacts_evicted_total", + metric.WithDescription("Total number of shared layer artifacts evicted after their last reference was removed"), + ) + if err != nil { + return nil, err + } + // Register observable gauges for queue length and total images buildQueueLength, err := meter.Int64ObservableGauge( "hypeman_images_build_queue_length", @@ -123,11 +132,12 @@ func newMetrics(meter metric.Meter, m *manager) (*Metrics, error) { } return &Metrics{ - buildDuration: buildDuration, - buildPhaseDuration: buildPhaseDuration, - ociLayerCount: ociLayerCount, - ociCompressedBytes: ociCompressedBytes, - pullsTotal: pullsTotal, + buildDuration: buildDuration, + buildPhaseDuration: buildPhaseDuration, + ociLayerCount: ociLayerCount, + ociCompressedBytes: ociCompressedBytes, + pullsTotal: pullsTotal, + layerArtifactsEvicted: layerArtifactsEvicted, }, nil } diff --git a/lib/images/metrics_test.go b/lib/images/metrics_test.go index 93007b300..1f58ab48c 100644 --- a/lib/images/metrics_test.go +++ b/lib/images/metrics_test.go @@ -16,10 +16,8 @@ import ( func TestImageBuildPhaseMetrics(t *testing.T) { reader := otelmetric.NewManualReader() provider := otelmetric.NewMeterProvider(otelmetric.WithReader(reader)) - m := &manager{ - paths: paths.New(t.TempDir()), - queue: queue.New(1), - } + m := newTestManager(paths.New(t.TempDir())) + m.queue = queue.New(1) metrics, err := newMetrics(provider.Meter("test"), m) require.NoError(t, err) diff --git a/lib/images/oci_public.go b/lib/images/oci_public.go index 7d336745b..554e1967f 100644 --- a/lib/images/oci_public.go +++ b/lib/images/oci_public.go @@ -47,7 +47,10 @@ func (c *OCIClient) InspectManifestForLinux(ctx context.Context, imageRef string // PullAndUnpack pulls an OCI image and unpacks it to a directory (public for system manager). // Always targets Linux platform since hypeman VMs are Linux guests. func (c *OCIClient) PullAndUnpack(ctx context.Context, imageRef, digest, exportDir string) error { - _, err := c.client.pullAndExport(ctx, imageRef, digest, exportDir) + result, err := c.client.pullAndExport(ctx, imageRef, digest, exportDir) + if result != nil { + defer result.cleanup() + } if err != nil { return fmt.Errorf("pull and unpack: %w", err) } diff --git a/lib/images/recovery_regression_test.go b/lib/images/recovery_regression_test.go index cd7d2e7ed..398bad182 100644 --- a/lib/images/recovery_regression_test.go +++ b/lib/images/recovery_regression_test.go @@ -46,12 +46,9 @@ func TestRecoverInterruptedBuildsCapturedFixtureMarksBuildFailed(t *testing.T) { client, err := newOCIClient(p.SystemOCICache()) require.NoError(t, err) - m := &manager{ - paths: p, - ociClient: client, - queue: queue.New(1), - readySubscribers: make(map[string][]chan StatusEvent), - } + m := newTestManager(p) + m.ociClient = client + m.queue = queue.New(1) m.RecoverInterruptedBuilds() diff --git a/lib/images/testlayers_test.go b/lib/images/testlayers_test.go deleted file mode 100644 index d56cf1015..000000000 --- a/lib/images/testlayers_test.go +++ /dev/null @@ -1,52 +0,0 @@ -package images - -import ( - "archive/tar" - "bytes" - "compress/gzip" - "io" - "testing" - - gcr "github.com/google/go-containerregistry/pkg/v1" - "github.com/google/go-containerregistry/pkg/v1/tarball" - "github.com/stretchr/testify/require" -) - -type tarEntrySpec struct { - name string - content string - isDir bool - mode int64 -} - -// specLayer builds a gzipped tar layer from entry specs in order. -func specLayer(t *testing.T, entries []tarEntrySpec) gcr.Layer { - t.Helper() - - var buf bytes.Buffer - gzw := gzip.NewWriter(&buf) - tw := tar.NewWriter(gzw) - for _, entry := range entries { - if entry.isDir { - require.NoError(t, tw.WriteHeader(&tar.Header{Name: entry.name, Typeflag: tar.TypeDir, Mode: entry.mode})) - continue - } - require.NoError(t, tw.WriteHeader(&tar.Header{ - Name: entry.name, - Typeflag: tar.TypeReg, - Mode: entry.mode, - Size: int64(len(entry.content)), - })) - _, err := tw.Write([]byte(entry.content)) - require.NoError(t, err) - } - require.NoError(t, tw.Close()) - require.NoError(t, gzw.Close()) - - data := buf.Bytes() - layer, err := tarball.LayerFromOpener(func() (io.ReadCloser, error) { - return io.NopCloser(bytes.NewReader(data)), nil - }) - require.NoError(t, err) - return layer -} diff --git a/lib/images/testutil/testutil.go b/lib/images/testutil/testutil.go deleted file mode 100644 index aa28cd1e4..000000000 --- a/lib/images/testutil/testutil.go +++ /dev/null @@ -1,107 +0,0 @@ -// Package testutil provides helpers for seeding on-disk image state in tests. -package testutil - -import ( - "encoding/json" - "fmt" - "os" - "path/filepath" - "testing" - "time" - - "github.com/kernel/hypeman/lib/images" - "github.com/kernel/hypeman/lib/paths" - "github.com/stretchr/testify/require" -) - -const seedImageContent = "rootfs!" - -// Seed describes a ready image to seed directly to disk, bypassing pulls, -// builds, and conversion. -type Seed struct { - Repository string - DigestHex string - // Tag optionally creates a tag symlink pointing at the image. - Tag string - // Name overrides the metadata "name" field; it defaults to - // repository:tag, or repository@sha256:digest when Tag is empty. - Name string - // Tags records resource tags in the metadata. - Tags map[string]string - // Content writes the image into the shared content layout instead of - // the legacy per-repository digest layout. - Content bool -} - -type imageMetadata struct { - Name string `json:"name"` - Digest string `json:"digest"` - Status string `json:"status"` - SizeBytes int64 `json:"size_bytes"` - Tags map[string]string `json:"tags,omitempty"` - CreatedAt time.Time `json:"created_at"` -} - -// SeedReadyImage writes a ready image to disk per s. -func SeedReadyImage(t testing.TB, p *paths.Paths, s Seed) { - t.Helper() - require.NoError(t, seedImage(p, s)) -} - -func seedImage(p *paths.Paths, s Seed) error { - dir := p.ImageDigestDir(s.Repository, s.DigestHex) - disk := p.ImageDigestPath(s.Repository, s.DigestHex) - metadata := p.ImageMetadata(s.Repository, s.DigestHex) - linkPath := p.ImageTagSymlink(s.Repository, s.Tag) - target := s.DigestHex - if s.Content { - dir = p.ImageContentDir(s.DigestHex) - disk = p.ImageContentPath(s.DigestHex) - metadata = p.ImageContentMetadata(s.DigestHex) - linkPath = p.ImageRepositoryTagSymlink(s.Repository, s.Tag) - rel, err := filepath.Rel(filepath.Dir(linkPath), p.ImageContentDir(s.DigestHex)) - if err != nil { - return fmt.Errorf("rel content symlink target: %w", err) - } - target = rel - } - - if err := os.MkdirAll(dir, 0o755); err != nil { - return fmt.Errorf("create image dir: %w", err) - } - if err := os.WriteFile(disk, []byte(seedImageContent), 0o644); err != nil { - return fmt.Errorf("write disk image: %w", err) - } - - name := s.Name - if name == "" && s.Tag != "" { - name = s.Repository + ":" + s.Tag - } else if name == "" { - name = s.Repository + "@sha256:" + s.DigestHex - } - data, err := json.Marshal(imageMetadata{ - Name: name, - Digest: "sha256:" + s.DigestHex, - Status: images.StatusReady, - SizeBytes: int64(len(seedImageContent)), - Tags: s.Tags, - CreatedAt: time.Now().UTC(), - }) - if err != nil { - return fmt.Errorf("marshal metadata: %w", err) - } - if err := os.WriteFile(metadata, data, 0o644); err != nil { - return fmt.Errorf("write metadata: %w", err) - } - - if s.Tag == "" { - return nil - } - if err := os.MkdirAll(filepath.Dir(linkPath), 0o755); err != nil { - return fmt.Errorf("create tag dir: %w", err) - } - if err := os.Symlink(target, linkPath); err != nil { - return fmt.Errorf("create tag symlink: %w", err) - } - return nil -} diff --git a/lib/instances/admission_allocations.go b/lib/instances/admission_allocations.go index 17d0f5e94..e07cc71bd 100644 --- a/lib/instances/admission_allocations.go +++ b/lib/instances/admission_allocations.go @@ -107,7 +107,7 @@ func (m *manager) rollbackAdmissionAllocationActive(stored *StoredMetadata) { // Failed post-boot/restore steps should not leave the cached visible // allocation marked active. Clear the in-memory PID first so any later sync // from this metadata view also treats the instance as inactive. - stored.HypervisorProcessIdentity.Clear() + stored.HypervisorPID = nil m.setAdmissionAllocationActive(stored, false) } diff --git a/lib/instances/admission_allocations_test.go b/lib/instances/admission_allocations_test.go index 17674e046..26c4252c4 100644 --- a/lib/instances/admission_allocations_test.go +++ b/lib/instances/admission_allocations_test.go @@ -20,11 +20,11 @@ func TestRollbackAdmissionAllocationActiveClearsVisibleAllocation(t *testing.T) } pid := 1234 stored := &StoredMetadata{ - Id: "inst-1", - Name: "test-instance", - Vcpus: 2, - Size: 1024, - HypervisorProcessIdentity: HypervisorProcessIdentity{HypervisorPID: &pid}, + Id: "inst-1", + Name: "test-instance", + Vcpus: 2, + Size: 1024, + HypervisorPID: &pid, } m.setAdmissionAllocationActive(stored, true) @@ -52,12 +52,12 @@ func TestReconcileAdmissionAllocationsMarksMissingSocketInactive(t *testing.T) { pid := 4321 stored := StoredMetadata{ - Id: "inst-2", - Name: "test-instance", - Vcpus: 2, - Size: 1024, - HypervisorProcessIdentity: HypervisorProcessIdentity{HypervisorPID: &pid}, - SocketPath: socketPath, + Id: "inst-2", + Name: "test-instance", + Vcpus: 2, + Size: 1024, + HypervisorPID: &pid, + SocketPath: socketPath, } m := &manager{ diff --git a/lib/instances/create_mdev_test.go b/lib/instances/create_mdev_test.go index 05b3e9d8c..4943303ab 100644 --- a/lib/instances/create_mdev_test.go +++ b/lib/instances/create_mdev_test.go @@ -45,7 +45,7 @@ func TestCreateInstanceRejectsUnsupportedVGPUBeforeResourceReservation(t *testin assert.Zero(t, validator.reserveCalls) } -func TestWrapCreateVGPUErr(t *testing.T) { +func TestWrapCreateMdevErr(t *testing.T) { t.Parallel() for _, tc := range []struct { @@ -61,7 +61,7 @@ func TestWrapCreateVGPUErr(t *testing.T) { wantInvalidRequest: true, }, { - name: "other vGPU error", + name: "other mdev error", err: errors.New("boom"), wantMessage: "create vGPU for profile profile: boom", }, @@ -69,7 +69,7 @@ func TestWrapCreateVGPUErr(t *testing.T) { t.Run(tc.name, func(t *testing.T) { t.Parallel() - err := wrapCreateVGPUErr("profile", tc.err) + err := wrapCreateMdevErr("profile", tc.err) assert.ErrorIs(t, err, tc.err) if tc.wantInvalidRequest { diff --git a/lib/instances/delete_test.go b/lib/instances/delete_test.go index 20a14d8c3..d13beeb5d 100644 --- a/lib/instances/delete_test.go +++ b/lib/instances/delete_test.go @@ -2,7 +2,6 @@ package instances import ( "os/exec" - "syscall" "testing" "time" @@ -19,15 +18,6 @@ func TestWaitForProcessExit_ReapsZombieChild(t *testing.T) { require.True(t, exited, "zombie child should be detected/reaped as exited") } -func TestWaitForProcessExit_EPERMProcessIsAlive(t *testing.T) { - t.Parallel() - if syscall.Kill(1, 0) == nil { - t.Skip("running as root") - } - - assert.False(t, WaitForProcessExit(1, 100*time.Millisecond)) -} - func TestWaitForProcessExit_TimesOutForRunningProcess(t *testing.T) { t.Parallel() cmd := exec.Command("sleep", "2") diff --git a/lib/instances/fork.go b/lib/instances/fork.go index 4c0046c7e..c77138578 100644 --- a/lib/instances/fork.go +++ b/lib/instances/fork.go @@ -280,7 +280,7 @@ func (m *manager) forkInstanceFromStoppedOrStandby(ctx context.Context, id strin forkMeta.ExpiresAt = nil forkMeta.StartedAt = nil forkMeta.StoppedAt = nil - forkMeta.HypervisorProcessIdentity.Clear() + forkMeta.HypervisorPID = nil forkMeta.SocketPath = m.paths.InstanceSocket(forkID, starter.SocketName()) forkMeta.DataDir = dstDir forkMeta.VsockSocket = m.paths.InstanceSocket(forkID, hypervisor.VsockSocketNameForType(forkMeta.HypervisorType)) @@ -298,11 +298,6 @@ func (m *manager) forkInstanceFromStoppedOrStandby(ctx context.Context, id strin // phase (Standby for snapshot forks, Stopped for stopped forks) will be // recorded by the appropriate operation when the fork is acted on. forkMeta.Phases.Reset() - // A vGPU assignment is never shared with a fork: normally stop already - // released it, and an assignment retained by a failed release must stay - // with the source so only one instance retries it. The fork acquires its - // own vGPU on start from GPUProfile. - clearStoredVGPUDevice(&forkMeta) switch source.State { case StateStandby: forkMeta.Phases.Record(phasetracking.PhaseStandby, now) diff --git a/lib/instances/guestmemory_linux_test.go b/lib/instances/guestmemory_linux_test.go index 87a40992e..224a74cbd 100644 --- a/lib/instances/guestmemory_linux_test.go +++ b/lib/instances/guestmemory_linux_test.go @@ -211,7 +211,7 @@ func requireHypervisorPID(t *testing.T, ctx context.Context, mgr *manager, insta t.Helper() inst, err := mgr.GetInstance(ctx, instanceID) require.NoError(t, err) - if inst.HypervisorPID != nil && ProcessExists(*inst.HypervisorPID) { + if inst.HypervisorPID != nil && processExists(*inst.HypervisorPID) { return *inst.HypervisorPID } if pid, err := hypervisor.ResolveProcessPID(inst.SocketPath); err == nil { diff --git a/lib/instances/manager.go b/lib/instances/manager.go index de5cbf8ed..c815a0eb7 100644 --- a/lib/instances/manager.go +++ b/lib/instances/manager.go @@ -206,7 +206,6 @@ type manager struct { nativeCodecPaths map[string]string imageUsageRecorder ImageUsageRecorder guestAgentReadyProbe func(context.Context, *StoredMetadata) bool - shutdownGuestFn func(context.Context, hypervisor.VsockDialer, int32) error // Shared lifecycle event subscriptions for internal consumers. lifecycleEvents *lifecycleSubscribers diff --git a/lib/instances/manager_test.go b/lib/instances/manager_test.go index 6593edca4..df2f2fcb4 100644 --- a/lib/instances/manager_test.go +++ b/lib/instances/manager_test.go @@ -4,7 +4,6 @@ import ( "bytes" "context" "crypto/tls" - "errors" "fmt" "io" "net" @@ -165,26 +164,6 @@ func waitForInstanceState(ctx context.Context, mgr Manager, instanceID string, e return nil, fmt.Errorf("instance %s did not reach %s within %v (last state: %s)", instanceID, expected, timeout, lastState) } -// deleteInstanceEventually deletes an instance, retrying while the hypervisor -// finishes dying. Delete fails closed when the VMM has not exited within its -// short post-SIGKILL wait; on loaded CI hosts kernel-side teardown can outlast -// that wait, and the contract is that a retried delete converges. -func deleteInstanceEventually(t *testing.T, ctx context.Context, mgr Manager, instanceID string) { - t.Helper() - deadline := time.Now().Add(integrationTestTimeout(30 * time.Second)) - for { - err := mgr.DeleteInstance(ctx, instanceID) - if err == nil || errors.Is(err, ErrNotFound) { - return - } - if time.Now().After(deadline) { - t.Fatalf("delete instance %s did not converge: %v", instanceID, err) - } - t.Logf("delete instance %s not yet converged, retrying: %v", instanceID, err) - time.Sleep(time.Second) - } -} - func integrationTestTimeout(timeout time.Duration) time.Duration { if os.Getenv("CI") == "true" && timeout < 45*time.Second { return 45 * time.Second @@ -1666,7 +1645,8 @@ func TestStandbyAndRestore(t *testing.T) { // Cleanup (no sleep needed - DeleteInstance handles process cleanup) t.Log("Cleaning up...") - deleteInstanceEventually(t, ctx, manager, inst.Id) + err = manager.DeleteInstance(ctx, inst.Id) + require.NoError(t, err) t.Log("Standby/restore test complete!") } diff --git a/lib/instances/network_test.go b/lib/instances/network_test.go index 67cf7b0f9..0c25455f0 100644 --- a/lib/instances/network_test.go +++ b/lib/instances/network_test.go @@ -296,7 +296,8 @@ func TestCreateInstanceWithNetwork(t *testing.T) { // Cleanup t.Log("Cleaning up instance...") - deleteInstanceEventually(t, ctx, manager, inst.Id) + err = manager.DeleteInstance(ctx, inst.Id) + require.NoError(t, err) // Verify TAP deleted after instance cleanup t.Log("Verifying TAP deleted after cleanup...") diff --git a/lib/instances/process_identity_test.go b/lib/instances/process_identity_test.go deleted file mode 100644 index e6fdf1f46..000000000 --- a/lib/instances/process_identity_test.go +++ /dev/null @@ -1,41 +0,0 @@ -package instances - -import ( - "encoding/json" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -// TestHypervisorProcessIdentityJSONKeysStayFlat guards the on-disk metadata -// format: the identity struct is embedded anonymously so its fields keep the -// flat JSON keys metadata files were written with before the struct existed. -func TestHypervisorProcessIdentityJSONKeysStayFlat(t *testing.T) { - pid := 1234 - stored := StoredMetadata{ - Id: "inst-json", - HypervisorProcessIdentity: HypervisorProcessIdentity{ - HypervisorPID: &pid, - HypervisorStartTime: 42, - HypervisorBootID: "boot-id", - }, - } - - data, err := json.Marshal(stored) - require.NoError(t, err) - - var keys map[string]json.RawMessage - require.NoError(t, json.Unmarshal(data, &keys)) - assert.Contains(t, keys, "HypervisorPID") - assert.Contains(t, keys, "HypervisorStartTime") - assert.Contains(t, keys, "HypervisorBootID") - assert.NotContains(t, keys, "HypervisorProcessIdentity") - - var decoded StoredMetadata - require.NoError(t, json.Unmarshal(data, &decoded)) - require.NotNil(t, decoded.HypervisorPID) - assert.Equal(t, pid, *decoded.HypervisorPID) - assert.Equal(t, uint64(42), decoded.HypervisorStartTime) - assert.Equal(t, "boot-id", decoded.HypervisorBootID) -} diff --git a/lib/instances/qemu_lifecycle_test.go b/lib/instances/qemu_lifecycle_test.go index ec7da113a..114c76390 100644 --- a/lib/instances/qemu_lifecycle_test.go +++ b/lib/instances/qemu_lifecycle_test.go @@ -154,7 +154,8 @@ func runQEMUStandbyAndRestore(t *testing.T, hypervisorType hypervisor.Type, inst // Cleanup t.Log("Cleaning up...") - deleteInstanceEventually(t, ctx, manager, inst.Id) + err = manager.DeleteInstance(ctx, inst.Id) + require.NoError(t, err) // Verify cleanup assert.NoDirExists(t, p.InstanceDir(inst.Id)) diff --git a/lib/instances/query.go b/lib/instances/query.go index 0621460da..42728304d 100644 --- a/lib/instances/query.go +++ b/lib/instances/query.go @@ -7,9 +7,11 @@ import ( "io" "os" "path/filepath" + "runtime" "slices" "strconv" "strings" + "syscall" "time" "github.com/kernel/hypeman/lib/guest" @@ -569,6 +571,59 @@ func (m *manager) toInstanceWithStateDerivation(ctx context.Context, meta *metad return inst } +func refreshHypervisorPID(stored *StoredMetadata, state State) { + if !state.RequiresVMM() && state != StateUnknown { + return + } + if stored.HypervisorPID != nil && processExists(*stored.HypervisorPID) { + return + } + if stored.SocketPath == "" { + return + } + if pid, err := hypervisor.ResolveProcessPID(stored.SocketPath); err == nil { + stored.HypervisorPID = &pid + return + } +} + +func processExists(pid int) bool { + if pid <= 0 { + return false + } + err := syscall.Kill(pid, 0) + if err != nil && err != syscall.EPERM { + return false + } + if runtime.GOOS != "linux" { + return true + } + state, err := readLinuxProcessState(pid) + if err != nil { + return true + } + return state != "Z" +} + +func readLinuxProcessState(pid int) (string, error) { + statusPath := filepath.Join("/proc", strconv.Itoa(pid), "status") + data, err := os.ReadFile(statusPath) + if err != nil { + return "", err + } + for _, line := range strings.Split(string(data), "\n") { + if !strings.HasPrefix(line, "State:") { + continue + } + fields := strings.Fields(line) + if len(fields) < 2 { + return "", fmt.Errorf("malformed process state in %s", statusPath) + } + return fields[1], nil + } + return "", fmt.Errorf("process state missing from %s", statusPath) +} + // parseExitSentinel reads the last lines of the serial console log to find the // HYPEMAN-EXIT sentinel written by init before shutdown. // Returns the exit code, message, and whether a sentinel was found. diff --git a/lib/instances/restore.go b/lib/instances/restore.go index ab27903ba..c209274e2 100644 --- a/lib/instances/restore.go +++ b/lib/instances/restore.go @@ -298,8 +298,7 @@ func (m *manager) restoreInstance( attribute.String("operation", "restore_from_snapshot"), ) log.InfoContext(ctx, "restoring from snapshot", "instance_id", id, "snapshot_dir", snapshotDir, "hypervisor", stored.HypervisorType) - // restoreFromSnapshot records the hypervisor process identity on stored. - _, hv, err := m.restoreFromSnapshot(restoreCtx, stored, snapshotDir, restoreOptions) + pid, hv, err := m.restoreFromSnapshot(restoreCtx, stored, snapshotDir, restoreOptions) restoreSpanEnd(err) if err != nil { log.ErrorContext(ctx, "failed to restore from snapshot", "instance_id", id, "error", err) @@ -309,6 +308,9 @@ func (m *manager) restoreInstance( return nil, err } + // Store the PID for later cleanup + stored.HypervisorPID = &pid + // 6. Transition: Paused → Running (resume) resumeCtx, resumeSpanEnd := m.startLifecycleStep(ctx, "resume_vm", attribute.String("instance_id", id), @@ -446,7 +448,7 @@ func (m *manager) restoreFromSnapshot( if err != nil { return 0, nil, fmt.Errorf("restore vm: %w", err) } - pid = resolveRuntimeHypervisorPID(log, stored, pid) + pid = resolveRuntimeHypervisorPID(log, stored.SocketPath, pid) log.DebugContext(ctx, "VM restored from snapshot successfully", "instance_id", stored.Id, "pid", pid) return pid, hv, nil diff --git a/lib/instances/snapshot_test.go b/lib/instances/snapshot_test.go index b23e364ee..917f88d46 100644 --- a/lib/instances/snapshot_test.go +++ b/lib/instances/snapshot_test.go @@ -8,7 +8,6 @@ import ( "testing" "time" - "github.com/kernel/hypeman/lib/devices" "github.com/kernel/hypeman/lib/hypervisor" "github.com/kernel/hypeman/lib/images" snapshotstore "github.com/kernel/hypeman/lib/snapshot" @@ -16,118 +15,6 @@ import ( "github.com/stretchr/testify/require" ) -func TestForkSnapshotClearsVGPUAssignment(t *testing.T) { - mgr, _ := setupTestManager(t) - ctx := context.Background() - - sourceID := "snapshot-vgpu-source" - createStoppedSnapshotSourceFixture(t, mgr, sourceID, sourceID, mgr.defaultHypervisor) - - meta, err := mgr.loadMetadata(sourceID) - require.NoError(t, err) - meta.GPUProfile = "NVIDIA L40S-2Q" - meta.GPUFramework = devices.VGPUFramework("future-framework") - meta.GPUDevicePath = "/sys/bus/pci/devices/0000:82:00.4" - meta.GPUMdevUUID = "retained-uuid" - require.NoError(t, mgr.saveMetadata(meta)) - - snapshot, err := mgr.CreateSnapshot(ctx, sourceID, CreateSnapshotRequest{ - Kind: SnapshotKindStopped, - Name: "snapshot-vgpu", - }) - require.NoError(t, err) - - forked, err := mgr.ForkSnapshot(ctx, snapshot.Id, ForkSnapshotRequest{ - Name: "snapshot-vgpu-fork", - TargetState: StateStopped, - }) - require.NoError(t, err) - assert.Equal(t, "NVIDIA L40S-2Q", forked.GPUProfile) - assert.Equal(t, devices.VGPUFrameworkNone, forked.GPUFramework) - assert.Empty(t, forked.GPUDevicePath) - assert.Empty(t, forked.GPUMdevUUID) - - source, err := mgr.loadMetadata(sourceID) - require.NoError(t, err) - assert.Equal(t, "/sys/bus/pci/devices/0000:82:00.4", source.GPUDevicePath) -} - -func TestRestoreSnapshotDoesNotResurrectStaleVGPUAssignment(t *testing.T) { - mgr, _ := setupTestManager(t) - ctx := context.Background() - - sourceID := "snapshot-vgpu-restore-stale" - createStoppedSnapshotSourceFixture(t, mgr, sourceID, sourceID, mgr.defaultHypervisor) - - meta, err := mgr.loadMetadata(sourceID) - require.NoError(t, err) - meta.GPUProfile = "NVIDIA L40S-2Q" - meta.GPUFramework = devices.VGPUFramework("future-framework") - meta.GPUDevicePath = "/sys/bus/pci/devices/0000:82:00.4" - meta.GPUMdevUUID = "retained-uuid" - require.NoError(t, mgr.saveMetadata(meta)) - - snapshot, err := mgr.CreateSnapshot(ctx, sourceID, CreateSnapshotRequest{ - Kind: SnapshotKindStopped, - Name: "snapshot-vgpu-restore-stale", - }) - require.NoError(t, err) - - // The retained assignment is released successfully after the snapshot - // was taken; a restore must not resurrect the snapshot's embedded copy. - meta, err = mgr.loadMetadata(sourceID) - require.NoError(t, err) - clearStoredVGPUDevice(&meta.StoredMetadata) - require.NoError(t, mgr.saveMetadata(meta)) - - _, err = mgr.RestoreSnapshot(ctx, sourceID, snapshot.Id, RestoreSnapshotRequest{ - TargetState: StateStopped, - TargetHypervisor: mgr.defaultHypervisor, - }) - require.NoError(t, err) - - restored, err := mgr.loadMetadata(sourceID) - require.NoError(t, err) - assert.Equal(t, devices.VGPUFrameworkNone, restored.GPUFramework) - assert.Empty(t, restored.GPUDevicePath) - assert.Empty(t, restored.GPUMdevUUID) -} - -func TestRestoreSnapshotKeepsCurrentVGPUAssignment(t *testing.T) { - mgr, _ := setupTestManager(t) - ctx := context.Background() - - sourceID := "snapshot-vgpu-restore-retained" - createStoppedSnapshotSourceFixture(t, mgr, sourceID, sourceID, mgr.defaultHypervisor) - - snapshot, err := mgr.CreateSnapshot(ctx, sourceID, CreateSnapshotRequest{ - Kind: SnapshotKindStopped, - Name: "snapshot-vgpu-restore-retained", - }) - require.NoError(t, err) - - // An assignment retained after the snapshot was taken (e.g. from a - // failed release on stop) must survive the restore for the next retry. - meta, err := mgr.loadMetadata(sourceID) - require.NoError(t, err) - meta.GPUFramework = devices.VGPUFramework("future-framework") - meta.GPUDevicePath = "/sys/bus/pci/devices/0000:82:00.4" - meta.GPUMdevUUID = "retained-uuid" - require.NoError(t, mgr.saveMetadata(meta)) - - _, err = mgr.RestoreSnapshot(ctx, sourceID, snapshot.Id, RestoreSnapshotRequest{ - TargetState: StateStopped, - TargetHypervisor: mgr.defaultHypervisor, - }) - require.NoError(t, err) - - restored, err := mgr.loadMetadata(sourceID) - require.NoError(t, err) - assert.Equal(t, devices.VGPUFramework("future-framework"), restored.GPUFramework) - assert.Equal(t, "/sys/bus/pci/devices/0000:82:00.4", restored.GPUDevicePath) - assert.Equal(t, "retained-uuid", restored.GPUMdevUUID) -} - func TestStoppedSnapshotLifecycleAndForkAfterSourceDeletion(t *testing.T) { t.Parallel() mgr, _ := setupTestManager(t) diff --git a/lib/instances/version_upgrade_test.go b/lib/instances/version_upgrade_test.go index 3b93eb54f..9e078d533 100644 --- a/lib/instances/version_upgrade_test.go +++ b/lib/instances/version_upgrade_test.go @@ -134,8 +134,8 @@ func TestCloudHypervisorVersionUpgradeRestore(t *testing.T) { // Cleanup t.Log("Cleaning up...") - deleteInstanceEventually(t, ctx, mgr, inst.Id) - deleteInstanceEventually(t, ctx, mgr, inst2.Id) + require.NoError(t, mgr.DeleteInstance(ctx, inst.Id)) + require.NoError(t, mgr.DeleteInstance(ctx, inst2.Id)) t.Log("Version upgrade restore test complete!") } diff --git a/lib/instances/vm_config_validation.go b/lib/instances/vm_config_validation.go index 61639e9b4..fe1530b54 100644 --- a/lib/instances/vm_config_validation.go +++ b/lib/instances/vm_config_validation.go @@ -6,10 +6,7 @@ import ( "github.com/kernel/hypeman/lib/hypervisor" ) -const ( - baseInstanceDiskCount = 3 // rootfs, writable overlay, and config disk - plannedVGPUDevicePath = "planned-vgpu-device" -) +const baseInstanceDiskCount = 3 // rootfs, writable overlay, and config disk func instanceDiskCount(volumes []VolumeAttachment) int { count := baseInstanceDiskCount @@ -25,13 +22,15 @@ func instanceDiskCount(volumes []VolumeAttachment) int { // validateCreateVMConfig performs side-effect-free backend validation against // the complete device plan before image, PCI, network, or filesystem work. func (m *manager) validateCreateVMConfig(starter hypervisor.VMStarter, req CreateInstanceRequest, hvType hypervisor.Type) error { - hasVGPU := req.GPU != nil && req.GPU.Profile != "" + pciDeviceCount := len(req.Devices) + if req.GPU != nil && req.GPU.Profile != "" { + pciDeviceCount++ + } return validatePlannedVMConfig(starter, hvType, m.plannedVMConfig( req.HotplugSize, req.Volumes, req.NetworkEnabled, - len(req.Devices), - hasVGPU, + pciDeviceCount, )) } @@ -42,13 +41,15 @@ func (m *manager) validateStoredVMConfig(starter hypervisor.VMStarter, snapshotK } func (m *manager) plannedStoredVMConfig(snapshotKind SnapshotKind, meta StoredMetadata) hypervisor.VMConfig { - hasVGPU := storedVGPUDevicePath(&meta) != "" || meta.GPUProfile != "" + pciDeviceCount := len(meta.Devices) + if meta.GPUMdevUUID != "" || meta.GPUProfile != "" { + pciDeviceCount++ + } config := m.plannedVMConfig( meta.HotplugSize, meta.Volumes, meta.NetworkEnabled, - len(meta.Devices), - hasVGPU, + pciDeviceCount, ) if snapshotKind == SnapshotKindStandby { // Standby restore/fork reuses the frozen snapshot device model, so live @@ -63,7 +64,6 @@ func (m *manager) plannedVMConfig( volumes []VolumeAttachment, networkEnabled bool, pciDeviceCount int, - hasVGPU bool, ) hypervisor.VMConfig { diskCount := instanceDiskCount(volumes) @@ -77,9 +77,6 @@ func (m *manager) plannedVMConfig( if networkEnabled { config.Networks = []hypervisor.NetworkConfig{{}} } - if hasVGPU { - config.VGPUDevicePath = plannedVGPUDevicePath - } return config } diff --git a/lib/instances/vm_config_validation_test.go b/lib/instances/vm_config_validation_test.go index d25456722..c7037e6ac 100644 --- a/lib/instances/vm_config_validation_test.go +++ b/lib/instances/vm_config_validation_test.go @@ -17,34 +17,21 @@ func TestPlannedVMConfig(t *testing.T) { []VolumeAttachment{{Overlay: true}, {Overlay: false}}, true, 2, - true, ) assert.Equal(t, int64(1024), config.HotplugBytes) require.Len(t, config.Disks, 6, "three instance disks plus two overlay-volume disks plus one plain volume") require.Len(t, config.Networks, 1) require.Len(t, config.PCIDevices, 2) - assert.Equal(t, plannedVGPUDevicePath, config.VGPUDevicePath) assert.Equal(t, int64(3), config.VsockCID) } func TestPlannedVMConfigWithoutOptionalDevices(t *testing.T) { t.Parallel() - config := (&manager{}).plannedVMConfig(0, nil, false, 0, false) + config := (&manager{}).plannedVMConfig(0, nil, false, 0) require.Len(t, config.Disks, baseInstanceDiskCount) assert.Empty(t, config.Networks) assert.Empty(t, config.PCIDevices) - assert.Empty(t, config.VGPUDevicePath) -} - -func TestPlannedStoredVMConfigSeparatesVGPUFromPCIDevices(t *testing.T) { - t.Parallel() - config := (&manager{}).plannedStoredVMConfig(SnapshotKindStopped, StoredMetadata{ - Devices: []string{"pci-device"}, - GPUProfile: "gpu-profile", - }) - require.Len(t, config.PCIDevices, 1) - assert.Equal(t, plannedVGPUDevicePath, config.VGPUDevicePath) } func TestPlannedStoredVMConfigStandbyIgnoresLiveBalloonPolicy(t *testing.T) { diff --git a/lib/middleware/resolve.go b/lib/middleware/resolve.go index 2658d3ddd..ab8507c62 100644 --- a/lib/middleware/resolve.go +++ b/lib/middleware/resolve.go @@ -83,14 +83,6 @@ func ResolveResource(resolvers Resolvers, errResponder ErrorResponder) func(http resourceType = "ingress" paramName = "id" case strings.HasPrefix(path, "/images/"): - // The tag route resolves the source in the handler, which returns a - // specific 404 body for a missing source; don't intercept it with - // the generic one. - rctx := chi.RouteContext(ctx) - if r.Method == http.MethodPost && rctx != nil && rctx.RoutePattern() == "/images/{name}/tag" { - next.ServeHTTP(w, r) - return - } resolver = resolvers.Image resourceType = "image" paramName = "name" diff --git a/lib/middleware/resolve_test.go b/lib/middleware/resolve_test.go index 2f5a5924e..5536c540f 100644 --- a/lib/middleware/resolve_test.go +++ b/lib/middleware/resolve_test.go @@ -127,55 +127,3 @@ func TestResolveResource_ResolvesBuilderByID(t *testing.T) { assert.Equal(t, "bld_123", resolver.receivedName, "Builder resolver was not invoked with the path ID") } - -func TestResolveResource_SkipsOnlyImageTagPosts(t *testing.T) { - resolver := &mockResolver{} - - middleware := ResolveResource(Resolvers{Image: resolver}, func(w http.ResponseWriter, err error, lookup string) { - w.WriteHeader(http.StatusNotFound) - }) - - r := chi.NewRouter() - r.With(middleware).Post("/images/{name}/tag", func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusOK) - }) - r.With(middleware).Post("/images/{name}", func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusOK) - }) - r.With(middleware).Post("/images/{name}/metadata/tag", func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusOK) - }) - - t.Run("tag route bypasses resolver", func(t *testing.T) { - resolver.receivedName = "" - req := httptest.NewRequest(http.MethodPost, "/images/docker.io%2Flibrary%2Falpine:latest/tag", nil) - w := httptest.NewRecorder() - r.ServeHTTP(w, req) - - require.Equal(t, http.StatusOK, w.Code, "body: %s", w.Body.String()) - assert.Empty(t, resolver.receivedName, - "the tag route must not be intercepted by the resolver") - }) - - t.Run("other image post resolves", func(t *testing.T) { - resolver.receivedName = "" - req := httptest.NewRequest(http.MethodPost, "/images/alpine:latest", nil) - w := httptest.NewRecorder() - r.ServeHTTP(w, req) - - require.Equal(t, http.StatusOK, w.Code, "body: %s", w.Body.String()) - assert.Equal(t, "alpine:latest", resolver.receivedName, - "only the tag route should bypass image resolution") - }) - - t.Run("other tag-suffixed route resolves", func(t *testing.T) { - resolver.receivedName = "" - req := httptest.NewRequest(http.MethodPost, "/images/alpine:latest/metadata/tag", nil) - w := httptest.NewRecorder() - r.ServeHTTP(w, req) - - require.Equal(t, http.StatusOK, w.Code, "body: %s", w.Body.String()) - assert.Equal(t, "alpine:latest", resolver.receivedName, - "only the exact image tag route should bypass resolution") - }) -} diff --git a/lib/paths/paths.go b/lib/paths/paths.go index 9086242e6..81844c01f 100644 --- a/lib/paths/paths.go +++ b/lib/paths/paths.go @@ -189,6 +189,11 @@ func (p *Paths) ImageLayerDir(layerHex string) string { return filepath.Join(p.ImageLayersDir(), layerHex) } +// ImageLayerArtifact returns the path to the default materialized layer artifact. +func (p *Paths) ImageLayerArtifact(layerHex string) string { + return p.ImageLayerArtifactForFormat(layerHex, "erofs") +} + // ImageLayerArtifactForFormat returns the path to a materialized layer artifact. func (p *Paths) ImageLayerArtifactForFormat(layerHex, format string) string { return filepath.Join(p.ImageLayerDir(layerHex), "layer."+format) diff --git a/lib/resources/gpu.go b/lib/resources/gpu.go index 6dfd537ae..fba449dca 100644 --- a/lib/resources/gpu.go +++ b/lib/resources/gpu.go @@ -49,7 +49,7 @@ func getVGPUStatus(ctx context.Context, framework devices.VGPUFramework, vfs []d usedSlots := 0 // Count used VFs (those with a vGPU assigned) for _, vf := range vfs { - if vf.Allocated { + if vf.HasMdev { usedSlots++ } } diff --git a/openapi.yaml b/openapi.yaml index 3621c3e8c..c896bbbc1 100644 --- a/openapi.yaml +++ b/openapi.yaml @@ -2709,12 +2709,6 @@ paths: application/json: schema: $ref: "#/components/schemas/Error" - 401: - description: Unauthorized - content: - application/json: - schema: - $ref: "#/components/schemas/Error" 500: description: Internal server error content: From 8470579cc7c7c66b727a8635b279c794deb51350 Mon Sep 17 00:00:00 2001 From: chruffins <23645059+chruffins@users.noreply.github.com> Date: Mon, 31 Aug 2026 21:35:25 +0000 Subject: [PATCH 02/19] Clean up stale image claims --- lib/images/layer_gc.go | 6 +++--- lib/images/manager_test.go | 2 -- lib/middleware/resolve.go | 7 +++++++ lib/middleware/resolve_test.go | 38 ++++++++++++++++++++++++++++++++++ 4 files changed, 48 insertions(+), 5 deletions(-) diff --git a/lib/images/layer_gc.go b/lib/images/layer_gc.go index aefdadfc0..b5fc322cc 100644 --- a/lib/images/layer_gc.go +++ b/lib/images/layer_gc.go @@ -118,11 +118,11 @@ func (m *manager) evictUnreferencedLayerArtifacts() { // stale and no build is materializing it. The per-digest lock is taken with // TryLock so eviction never blocks behind an in-flight conversion. func (m *manager) tryEvictLayerArtifact(digestHex, dirPath string, cutoff time.Time) (int64, bool) { - lock := m.layerDigestLock(digestHex) - if !lock.TryLock() { + unlock, ok := m.layerLocks.tryLock(digestHex) + if !ok { return 0, false } - defer lock.Unlock() + defer unlock() // The candidate was selected outside the lock; re-check that a build has // not retained the digest and the artifact has not been rewritten since. diff --git a/lib/images/manager_test.go b/lib/images/manager_test.go index 36015dd5b..01b91b808 100644 --- a/lib/images/manager_test.go +++ b/lib/images/manager_test.go @@ -7,7 +7,6 @@ import ( "os" "path/filepath" "strings" - "sync" "testing" "time" @@ -24,7 +23,6 @@ func newTestManager(p *paths.Paths) *manager { return &manager{ paths: p, tagGenerations: make(map[string]uint64), - layerDigestLocks: make(map[string]*sync.Mutex), inflightLayerRefs: make(map[string]int), inflightPulls: make(map[string]*inflightImagePull), readySubscribers: make(map[string][]chan StatusEvent), diff --git a/lib/middleware/resolve.go b/lib/middleware/resolve.go index ab8507c62..5eaa75d54 100644 --- a/lib/middleware/resolve.go +++ b/lib/middleware/resolve.go @@ -83,6 +83,13 @@ func ResolveResource(resolvers Resolvers, errResponder ErrorResponder) func(http resourceType = "ingress" paramName = "id" case strings.HasPrefix(path, "/images/"): + // The tag route resolves the source in the handler, which returns a + // specific 404 body for a missing source; don't intercept it with + // the generic one. + if r.Method == http.MethodPost && strings.HasSuffix(path, "/tag") { + next.ServeHTTP(w, r) + return + } resolver = resolvers.Image resourceType = "image" paramName = "name" diff --git a/lib/middleware/resolve_test.go b/lib/middleware/resolve_test.go index 5536c540f..e9cfa477f 100644 --- a/lib/middleware/resolve_test.go +++ b/lib/middleware/resolve_test.go @@ -93,6 +93,44 @@ func TestResolveResource_URLDecodesImageName(t *testing.T) { } } +func TestResolveResource_SkipsOnlyImageTagPosts(t *testing.T) { + resolver := &mockResolver{} + + middleware := ResolveResource(Resolvers{Image: resolver}, func(w http.ResponseWriter, err error, lookup string) { + w.WriteHeader(http.StatusNotFound) + }) + + r := chi.NewRouter() + r.With(middleware).Post("/images/{name}/tag", func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + }) + r.With(middleware).Post("/images/{name}", func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + }) + + t.Run("tag route bypasses resolver", func(t *testing.T) { + resolver.receivedName = "" + req := httptest.NewRequest(http.MethodPost, "/images/docker.io%2Flibrary%2Falpine:latest/tag", nil) + w := httptest.NewRecorder() + r.ServeHTTP(w, req) + + require.Equal(t, http.StatusOK, w.Code, "body: %s", w.Body.String()) + assert.Empty(t, resolver.receivedName, + "the tag route must not be intercepted by the resolver") + }) + + t.Run("other image post resolves", func(t *testing.T) { + resolver.receivedName = "" + req := httptest.NewRequest(http.MethodPost, "/images/alpine:latest", nil) + w := httptest.NewRecorder() + r.ServeHTTP(w, req) + + require.Equal(t, http.StatusOK, w.Code, "body: %s", w.Body.String()) + assert.Equal(t, "alpine:latest", resolver.receivedName, + "only the tag route should bypass image resolution") + }) +} + func TestResolveResource_ResolvesBuilderByID(t *testing.T) { // Regression test: the path-dispatch switch must include a /builders/ // case, otherwise the Builder resolver is never invoked and resolved From 5938bb61da82828cfae004f3d2fbab597ec812d1 Mon Sep 17 00:00:00 2001 From: chruffins <23645059+chruffins@users.noreply.github.com> Date: Tue, 1 Sep 2026 18:56:15 +0000 Subject: [PATCH 03/19] Harden hypervisor and image cleanup --- .github/workflows/deploy-staging.yml | 3 +- .github/workflows/test.yml | 5 + lib/hypervisor/socket_pid.go | 5 + lib/hypervisor/socket_pid_linux.go | 142 +++++--- lib/hypervisor/socket_pid_linux_test.go | 339 ++++++++++++++++++++ lib/hypervisor/socket_pid_other.go | 5 + lib/instances/admission_allocations.go | 2 +- lib/instances/admission_allocations_test.go | 22 +- lib/instances/fork.go | 4 +- lib/instances/manager.go | 14 + lib/instances/manager_test.go | 18 ++ lib/instances/process_identity_test.go | 41 +++ lib/instances/query.go | 53 +-- lib/instances/restore.go | 6 +- lib/instances/start.go | 12 + 15 files changed, 554 insertions(+), 117 deletions(-) create mode 100644 lib/hypervisor/socket_pid.go create mode 100644 lib/instances/process_identity_test.go diff --git a/.github/workflows/deploy-staging.yml b/.github/workflows/deploy-staging.yml index 0c257b308..909ca4db3 100644 --- a/.github/workflows/deploy-staging.yml +++ b/.github/workflows/deploy-staging.yml @@ -31,4 +31,5 @@ jobs: --ref main \ --field env=staging \ --field ref="$HYPEMAN_REF" \ - --field cli-version=latest + --field cli-version=latest \ + --field triggered_by="${{ github.actor }}" diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 6bc5ee363..870b87467 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -13,6 +13,11 @@ on: required: false type: string +# Superseded branch pushes should release self-hosted test capacity. +concurrency: + group: test-${{ (github.event_name == 'push' && github.ref != 'refs/heads/main') && github.ref || github.run_id }} + cancel-in-progress: ${{ github.event_name == 'push' && github.ref != 'refs/heads/main' }} + # A slash-command dispatch supplies a fork repository and immutable commit SHA. # Normal push runs use the upstream repository and pushed ref. env: diff --git a/lib/hypervisor/socket_pid.go b/lib/hypervisor/socket_pid.go new file mode 100644 index 000000000..0d009f9ee --- /dev/null +++ b/lib/hypervisor/socket_pid.go @@ -0,0 +1,5 @@ +package hypervisor + +import "errors" + +var ErrNoOwningProcess = errors.New("no owning process found") diff --git a/lib/hypervisor/socket_pid_linux.go b/lib/hypervisor/socket_pid_linux.go index 7f46ebfa3..45b280edc 100644 --- a/lib/hypervisor/socket_pid_linux.go +++ b/lib/hypervisor/socket_pid_linux.go @@ -4,70 +4,77 @@ package hypervisor import ( "bufio" + "errors" "fmt" + "io/fs" "os" "path/filepath" + "slices" "strconv" "strings" + "syscall" ) +var procDir = "/proc" + +// soAcceptcon marks a listening socket in /proc/net/unix (__SO_ACCEPTCON). +const soAcceptcon = 0x10000 + // ResolveProcessPID finds the process currently holding the listening Unix -// socket for the given hypervisor control path. -func ResolveProcessPID(socketPath string) (int, error) { +// socket for the given hypervisor control path, via the socket inode in +// /proc/net/unix and each process's fd table. The fd scan requires the +// caller to hold CAP_SYS_PTRACE (or run as root) so no live owner is missed; +// an ErrNoOwningProcess result is proof the listener is gone. +func ResolveProcessPID(socketPath string) (pid int, err error) { + return resolveProcessPID(socketPath, 0) +} + +// ResolveProcessPIDForOwner resolves a socket while preferring an expected +// owner when the socket descriptor is temporarily shared with a child process. +func ResolveProcessPIDForOwner(socketPath string, ownerPID int) (pid int, err error) { + return resolveProcessPID(socketPath, ownerPID) +} + +func resolveProcessPID(socketPath string, ownerPID int) (pid int, err error) { socketRef, err := socketRefForPath(socketPath) - if err == nil { - if pid, refErr := pidBySocketRef(socketRef); refErr == nil { - return pid, nil - } + if err != nil { + return 0, err } - - if pid, cmdErr := pidByCmdline(socketPath); cmdErr == nil { - return pid, nil + // Confirm the expected owner first so a live stored PID does not + // require scanning every process fd. + if ownerPID > 0 && processHoldsSocketRef(ownerPID, socketRef) { + return ownerPID, nil } - - return 0, fmt.Errorf("resolve process pid for socket %s: no owning process found", socketPath) + return pidBySocketRef(socketRef, ownerPID) } -func pidBySocketRef(socketRef string) (int, error) { - procEntries, err := os.ReadDir("/proc") +func processHoldsSocketRef(pid int, socketRef string) bool { + fdEntries, err := os.ReadDir(filepath.Join(procDir, strconv.Itoa(pid), "fd")) if err != nil { - return 0, fmt.Errorf("read /proc: %w", err) + return false } - - for _, entry := range procEntries { - if !entry.IsDir() { - continue - } - - pid, err := strconv.Atoi(entry.Name()) + for _, fdEntry := range fdEntries { + target, err := os.Readlink(filepath.Join(procDir, strconv.Itoa(pid), "fd", fdEntry.Name())) if err != nil { + // Skip fds that cannot be read, like the full scan does: an fd + // vanishing mid-scan must not hide a listener held by a later fd. continue } - - fdEntries, err := os.ReadDir(filepath.Join("/proc", entry.Name(), "fd")) - if err != nil { - continue - } - for _, fdEntry := range fdEntries { - target, err := os.Readlink(filepath.Join("/proc", entry.Name(), "fd", fdEntry.Name())) - if err != nil { - continue - } - if strings.TrimSpace(target) == socketRef { - return pid, nil - } + if strings.TrimSpace(target) == socketRef { + return true } } - - return 0, fmt.Errorf("resolve process pid for %s: no owning process found", socketRef) + return false } -func pidByCmdline(socketPath string) (int, error) { - procEntries, err := os.ReadDir("/proc") +func pidBySocketRef(socketRef string, ownerPID int) (int, error) { + procEntries, err := os.ReadDir(procDir) if err != nil { return 0, fmt.Errorf("read /proc: %w", err) } + var owners []int + var scanErr error for _, entry := range procEntries { if !entry.IsDir() { continue @@ -78,28 +85,57 @@ func pidByCmdline(socketPath string) (int, error) { continue } - cmdline, err := os.ReadFile(filepath.Join("/proc", entry.Name(), "cmdline")) - if err != nil || len(cmdline) == 0 { + fdEntries, err := os.ReadDir(filepath.Join(procDir, entry.Name(), "fd")) + if err != nil { + if errors.Is(err, fs.ErrNotExist) || errors.Is(err, syscall.ESRCH) { + continue + } + scanErr = err continue } - for _, arg := range strings.Split(string(cmdline), "\x00") { - if arg == socketPath { - return pid, nil + for _, fdEntry := range fdEntries { + target, err := os.Readlink(filepath.Join(procDir, entry.Name(), "fd", fdEntry.Name())) + if err != nil { + if errors.Is(err, fs.ErrNotExist) || errors.Is(err, syscall.ESRCH) { + continue + } + scanErr = err + continue + } + if strings.TrimSpace(target) == socketRef { + owners = append(owners, pid) + break } } } - return 0, fmt.Errorf("resolve process pid for socket %s: no matching command line found", socketPath) + // The scan observed ownerPID holding the listener fd — the same evidence + // the fast path uses — so a child transiently sharing the inherited fd + // must not turn a proven owner into an error. + if ownerPID > 0 && slices.Contains(owners, ownerPID) { + return ownerPID, nil + } + if len(owners) == 1 { + return owners[0], nil + } + if len(owners) > 1 { + return 0, fmt.Errorf("resolve process pid for %s: multiple owning processes found: %v", socketRef, owners) + } + if scanErr != nil { + return 0, fmt.Errorf("resolve process pid for %s: inspect process fds: %w", socketRef, scanErr) + } + return 0, fmt.Errorf("resolve process pid for %s: %w", socketRef, ErrNoOwningProcess) } func socketRefForPath(socketPath string) (string, error) { - file, err := os.Open("/proc/net/unix") + file, err := os.Open(filepath.Join(procDir, "net", "unix")) if err != nil { return "", fmt.Errorf("open /proc/net/unix: %w", err) } defer file.Close() scanner := bufio.NewScanner(file) + var socketRef string for scanner.Scan() { fields := strings.Fields(scanner.Text()) if len(fields) < 7 { @@ -112,14 +148,26 @@ func socketRefForPath(socketPath string) (string, error) { if path != socketPath { continue } + // Accepted server-side sockets list the bound path too; only the + // listener identifies the owning process. + flags, parseErr := strconv.ParseUint(fields[3], 16, 32) + if parseErr != nil || flags&soAcceptcon == 0 { + continue + } inode := fields[6] if inode == "" { break } - return fmt.Sprintf("socket:[%s]", inode), nil + if socketRef != "" { + return "", fmt.Errorf("resolve process pid for socket %s: multiple socket inodes found", socketPath) + } + socketRef = fmt.Sprintf("socket:[%s]", inode) } if err := scanner.Err(); err != nil { return "", fmt.Errorf("scan /proc/net/unix: %w", err) } - return "", fmt.Errorf("resolve process pid for socket %s: socket inode not found", socketPath) + if socketRef != "" { + return socketRef, nil + } + return "", fmt.Errorf("resolve process pid for socket %s: socket inode not found: %w", socketPath, ErrNoOwningProcess) } diff --git a/lib/hypervisor/socket_pid_linux_test.go b/lib/hypervisor/socket_pid_linux_test.go index 270524532..d47044e9c 100644 --- a/lib/hypervisor/socket_pid_linux_test.go +++ b/lib/hypervisor/socket_pid_linux_test.go @@ -3,10 +3,14 @@ package hypervisor import ( + "context" + "errors" "net" "os" + "os/exec" "path/filepath" "testing" + "time" "github.com/stretchr/testify/require" ) @@ -23,3 +27,338 @@ func TestResolveProcessPID(t *testing.T) { require.NoError(t, err) require.Equal(t, os.Getpid(), pid) } + +func TestResolveProcessPIDIgnoresConnectedSocketEntries(t *testing.T) { + tmpDir := t.TempDir() + socketPath := filepath.Join(tmpDir, "test.sock") + + listener, err := net.Listen("unix", socketPath) + require.NoError(t, err) + defer listener.Close() + + // Accepted server-side sockets share the listener's path in + // /proc/net/unix; they must not make the listener's inode ambiguous. + conn, err := net.Dial("unix", socketPath) + require.NoError(t, err) + defer conn.Close() + accepted, err := listener.Accept() + require.NoError(t, err) + defer accepted.Close() + + pid, err := ResolveProcessPID(socketPath) + require.NoError(t, err) + require.Equal(t, os.Getpid(), pid) +} + +func TestResolveProcessPIDFailsForDuplicateSocketPaths(t *testing.T) { + oldProcDir := procDir + procDir = t.TempDir() + t.Cleanup(func() { procDir = oldProcDir }) + + socketPath := "/tmp/test.sock" + require.NoError(t, os.MkdirAll(filepath.Join(procDir, "net"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(procDir, "net", "unix"), []byte( + "00000000: 00000002 00000000 00010000 0001 01 12345 "+socketPath+"\n"+ + "00000000: 00000002 00000000 00010000 0001 01 67890 "+socketPath+"\n"), 0o644)) + + _, err := ResolveProcessPID(socketPath) + require.ErrorContains(t, err, "multiple socket inodes found") +} + +func TestResolveProcessPIDToleratesExitedProcess(t *testing.T) { + oldProcDir := procDir + procDir = t.TempDir() + t.Cleanup(func() { procDir = oldProcDir }) + + socketPath := "/tmp/test.sock" + require.NoError(t, os.MkdirAll(filepath.Join(procDir, "net"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(procDir, "net", "unix"), []byte("00000000: 00000002 00000000 00010000 0001 01 12345 "+socketPath+"\n"), 0o644)) + require.NoError(t, os.MkdirAll(filepath.Join(procDir, "100"), 0o755)) + fdDir := filepath.Join(procDir, "200", "fd") + require.NoError(t, os.MkdirAll(fdDir, 0o755)) + require.NoError(t, os.Symlink("socket:[12345]", filepath.Join(fdDir, "3"))) + + pid, err := ResolveProcessPID(socketPath) + require.NoError(t, err) + require.Equal(t, 200, pid) +} + +func TestResolveProcessPIDForOwnerPrefersExpectedProcess(t *testing.T) { + oldProcDir := procDir + procDir = t.TempDir() + t.Cleanup(func() { procDir = oldProcDir }) + + socketPath := "/tmp/test.sock" + require.NoError(t, os.MkdirAll(filepath.Join(procDir, "net"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(procDir, "net", "unix"), []byte("00000000: 00000002 00000000 00010000 0001 01 12345 "+socketPath+"\n"), 0o644)) + for _, pid := range []string{"100", "101"} { + fdDir := filepath.Join(procDir, pid, "fd") + require.NoError(t, os.MkdirAll(fdDir, 0o755)) + require.NoError(t, os.Symlink("socket:[12345]", filepath.Join(fdDir, "3"))) + } + + _, err := ResolveProcessPID(socketPath) + require.ErrorContains(t, err, "multiple owning processes found") + + pid, err := ResolveProcessPIDForOwner(socketPath, 101) + require.NoError(t, err) + require.Equal(t, 101, pid) +} + +func TestPidBySocketRefPrefersExpectedOwnerAmongMultiple(t *testing.T) { + oldProcDir := procDir + procDir = t.TempDir() + t.Cleanup(func() { procDir = oldProcDir }) + + // The full scan itself must prefer the expected owner when a child + // transiently shares the inherited listener fd: the fast path can miss on + // a transient fd-dir read failure, and the scan's observation of the + // owner holding the fd is the same evidence the fast path would have used. + for _, pid := range []string{"100", "101"} { + fdDir := filepath.Join(procDir, pid, "fd") + require.NoError(t, os.MkdirAll(fdDir, 0o755)) + require.NoError(t, os.Symlink("socket:[12345]", filepath.Join(fdDir, "3"))) + } + + pid, err := pidBySocketRef("socket:[12345]", 101) + require.NoError(t, err) + require.Equal(t, 101, pid) + + _, err = pidBySocketRef("socket:[12345]", 0) + require.ErrorContains(t, err, "multiple owning processes found") + + _, err = pidBySocketRef("socket:[12345]", 999) + require.ErrorContains(t, err, "multiple owning processes found") +} + +func TestResolveProcessPIDReportsNoOwnerAfterExitedProcesses(t *testing.T) { + oldProcDir := procDir + procDir = t.TempDir() + t.Cleanup(func() { procDir = oldProcDir }) + + socketPath := "/tmp/test.sock" + require.NoError(t, os.MkdirAll(filepath.Join(procDir, "net"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(procDir, "net", "unix"), []byte("00000000: 00000002 00000000 00010000 0001 01 12345 "+socketPath+"\n"), 0o644)) + require.NoError(t, os.MkdirAll(filepath.Join(procDir, "100"), 0o755)) + + _, err := ResolveProcessPID(socketPath) + require.ErrorIs(t, err, ErrNoOwningProcess) + require.NotContains(t, err.Error(), "inspect process fds") +} + +func TestResolveProcessPIDReportsMissingSocket(t *testing.T) { + oldProcDir := procDir + procDir = t.TempDir() + t.Cleanup(func() { procDir = oldProcDir }) + + require.NoError(t, os.MkdirAll(filepath.Join(procDir, "net"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(procDir, "net", "unix"), nil, 0o644)) + + _, err := ResolveProcessPID("/tmp/missing.sock") + require.ErrorIs(t, err, ErrNoOwningProcess) +} + +func TestResolveProcessPIDFailsWhenFDIsUnreadable(t *testing.T) { + oldProcDir := procDir + procDir = t.TempDir() + t.Cleanup(func() { procDir = oldProcDir }) + + socketPath := "/tmp/test.sock" + require.NoError(t, os.MkdirAll(filepath.Join(procDir, "net"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(procDir, "net", "unix"), []byte("00000000: 00000002 00000000 00010000 0001 01 12345 "+socketPath+"\n"), 0o644)) + fdDir := filepath.Join(procDir, "123", "fd") + require.NoError(t, os.MkdirAll(fdDir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(fdDir, "3"), nil, 0o644)) + + _, err := ResolveProcessPID(socketPath) + require.Error(t, err) + require.ErrorContains(t, err, "inspect process fds") + require.False(t, errors.Is(err, ErrNoOwningProcess)) +} + +func TestResolveProcessPIDForOwnerConfirmsCandidateWithoutFullScan(t *testing.T) { + oldProcDir := procDir + procDir = t.TempDir() + t.Cleanup(func() { procDir = oldProcDir }) + + socketPath := "/tmp/test.sock" + require.NoError(t, os.MkdirAll(filepath.Join(procDir, "net"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(procDir, "net", "unix"), []byte("00000000: 00000002 00000000 00010000 0001 01 12345 "+socketPath+"\n"), 0o644)) + + fdDir := filepath.Join(procDir, "100", "fd") + require.NoError(t, os.MkdirAll(fdDir, 0o755)) + require.NoError(t, os.Symlink("socket:[12345]", filepath.Join(fdDir, "3"))) + + // An unreadable sibling fd must not block confirming the candidate. + siblingFDDir := filepath.Join(procDir, "123", "fd") + require.NoError(t, os.MkdirAll(siblingFDDir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(siblingFDDir, "3"), nil, 0o644)) + + pid, err := ResolveProcessPIDForOwner(socketPath, 100) + require.NoError(t, err) + require.Equal(t, 100, pid) +} + +func TestResolveProcessPIDForOwnerSkipsUnreadableCandidateFD(t *testing.T) { + oldProcDir := procDir + procDir = t.TempDir() + t.Cleanup(func() { procDir = oldProcDir }) + + socketPath := "/tmp/test.sock" + require.NoError(t, os.MkdirAll(filepath.Join(procDir, "net"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(procDir, "net", "unix"), []byte("00000000: 00000002 00000000 00010000 0001 01 12345 "+socketPath+"\n"), 0o644)) + + // An unreadable fd before the listener fd must not abort the candidate + // check; the scan skips it and still finds the match. + fdDir := filepath.Join(procDir, "100", "fd") + require.NoError(t, os.MkdirAll(fdDir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(fdDir, "1"), nil, 0o644)) + require.NoError(t, os.Symlink("socket:[12345]", filepath.Join(fdDir, "3"))) + + pid, err := ResolveProcessPIDForOwner(socketPath, 100) + require.NoError(t, err) + require.Equal(t, 100, pid) +} + +func TestResolveProcessPIDForOwnerFallsThroughWhenCandidateLacksSocket(t *testing.T) { + oldProcDir := procDir + procDir = t.TempDir() + t.Cleanup(func() { procDir = oldProcDir }) + + socketPath := "/tmp/test.sock" + require.NoError(t, os.MkdirAll(filepath.Join(procDir, "net"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(procDir, "net", "unix"), []byte("00000000: 00000002 00000000 00010000 0001 01 12345 "+socketPath+"\n"), 0o644)) + + candidateFDDir := filepath.Join(procDir, "999", "fd") + require.NoError(t, os.MkdirAll(candidateFDDir, 0o755)) + require.NoError(t, os.Symlink("socket:[99999]", filepath.Join(candidateFDDir, "3"))) + + ownerFDDir := filepath.Join(procDir, "200", "fd") + require.NoError(t, os.MkdirAll(ownerFDDir, 0o755)) + require.NoError(t, os.Symlink("socket:[12345]", filepath.Join(ownerFDDir, "3"))) + + pid, err := ResolveProcessPIDForOwner(socketPath, 999) + require.NoError(t, err) + require.Equal(t, 200, pid) +} + +func TestResolveProcessPIDForOwnerFallsThroughWhenCandidateIsGone(t *testing.T) { + oldProcDir := procDir + procDir = t.TempDir() + t.Cleanup(func() { procDir = oldProcDir }) + + socketPath := "/tmp/test.sock" + require.NoError(t, os.MkdirAll(filepath.Join(procDir, "net"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(procDir, "net", "unix"), []byte("00000000: 00000002 00000000 00010000 0001 01 12345 "+socketPath+"\n"), 0o644)) + + ownerFDDir := filepath.Join(procDir, "200", "fd") + require.NoError(t, os.MkdirAll(ownerFDDir, 0o755)) + require.NoError(t, os.Symlink("socket:[12345]", filepath.Join(ownerFDDir, "3"))) + + pid, err := ResolveProcessPIDForOwner(socketPath, 999) + require.NoError(t, err) + require.Equal(t, 200, pid) +} + +func TestResolveProcessPIDForOwnerReportsMissingSocket(t *testing.T) { + oldProcDir := procDir + procDir = t.TempDir() + t.Cleanup(func() { procDir = oldProcDir }) + + require.NoError(t, os.MkdirAll(filepath.Join(procDir, "net"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(procDir, "net", "unix"), []byte("00000000: 00000002 00000000 00010000 0001 01 12345 /tmp/other.sock\n"), 0o644)) + + _, err := ResolveProcessPIDForOwner("/tmp/missing.sock", 100) + require.ErrorIs(t, err, ErrNoOwningProcess) +} + +func TestResolveProcessPIDForOwnerReportsMissingSocketWithHeaderOnlyUnixTable(t *testing.T) { + oldProcDir := procDir + procDir = t.TempDir() + t.Cleanup(func() { procDir = oldProcDir }) + + require.NoError(t, os.MkdirAll(filepath.Join(procDir, "net"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(procDir, "net", "unix"), []byte("Num RefCount Protocol Flags Type St Inode Path\n"), 0o644)) + + _, err := ResolveProcessPIDForOwner("/tmp/missing.sock", 100) + require.ErrorIs(t, err, ErrNoOwningProcess) +} + +func TestResolveProcessPIDForOwnerReportsDuplicateSocketInodes(t *testing.T) { + oldProcDir := procDir + procDir = t.TempDir() + t.Cleanup(func() { procDir = oldProcDir }) + + socketPath := "/tmp/test.sock" + require.NoError(t, os.MkdirAll(filepath.Join(procDir, "net"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(procDir, "net", "unix"), []byte( + "00000000: 00000002 00000000 00010000 0001 01 12345 "+socketPath+"\n"+ + "00000000: 00000002 00000000 00010000 0001 01 67890 "+socketPath+"\n"), 0o644)) + + fdDir := filepath.Join(procDir, "100", "fd") + require.NoError(t, os.MkdirAll(fdDir, 0o755)) + require.NoError(t, os.Symlink("socket:[12345]", filepath.Join(fdDir, "3"))) + + _, err := ResolveProcessPIDForOwner(socketPath, 100) + require.ErrorContains(t, err, "multiple socket inodes found") +} + +func TestResolveProcessPIDForOwnerConfirmsLiveListener(t *testing.T) { + tmpDir := t.TempDir() + socketPath := filepath.Join(tmpDir, "test.sock") + + listener, err := net.Listen("unix", socketPath) + require.NoError(t, err) + defer listener.Close() + + pid, err := ResolveProcessPIDForOwner(socketPath, os.Getpid()) + require.NoError(t, err) + require.Equal(t, os.Getpid(), pid) +} + +func TestResolveProcessPIDIgnoresCommandLineBystander(t *testing.T) { + socketPath := filepath.Join(t.TempDir(), "test.sock") + require.NoError(t, os.WriteFile(socketPath, nil, 0o600)) + + // A process carrying the socket path in its command line (e.g. a debug + // client like ch-remote) without holding the listener must not resolve + // as the owner; a missing listener is proof the hypervisor is gone. + bystander := exec.Command("sh", "-c", "sleep 30", "sh", socketPath) + require.NoError(t, bystander.Start()) + t.Cleanup(func() { + _ = bystander.Process.Kill() + _ = bystander.Wait() + }) + + _, err := ResolveProcessPID(socketPath) + require.ErrorIs(t, err, ErrNoOwningProcess) +} + +func TestResolveProcessPIDDuringProcessChurn(t *testing.T) { + socketPath := filepath.Join(t.TempDir(), "test.sock") + listener, err := net.Listen("unix", socketPath) + require.NoError(t, err) + defer listener.Close() + + ctx, cancel := context.WithCancel(context.Background()) + done := make(chan struct{}) + go func() { + defer close(done) + for ctx.Err() == nil { + _ = exec.CommandContext(ctx, "/bin/true").Run() + } + }() + defer func() { + cancel() + <-done + }() + + // Resolve without an owner hint so every iteration runs the full /proc + // scan; the owner fast path never exercises the churn tolerance. + deadline := time.Now().Add(2 * time.Second) + for time.Now().Before(deadline) { + pid, err := ResolveProcessPID(socketPath) + require.NoError(t, err) + require.Equal(t, os.Getpid(), pid) + } +} diff --git a/lib/hypervisor/socket_pid_other.go b/lib/hypervisor/socket_pid_other.go index 75db657e6..1fb594653 100644 --- a/lib/hypervisor/socket_pid_other.go +++ b/lib/hypervisor/socket_pid_other.go @@ -9,3 +9,8 @@ import "fmt" func ResolveProcessPID(socketPath string) (int, error) { return 0, fmt.Errorf("resolve process pid for socket %s: not supported on this platform", socketPath) } + +// ResolveProcessPIDForOwner is only implemented on Linux. +func ResolveProcessPIDForOwner(socketPath string, _ int) (int, error) { + return ResolveProcessPID(socketPath) +} diff --git a/lib/instances/admission_allocations.go b/lib/instances/admission_allocations.go index e07cc71bd..17d0f5e94 100644 --- a/lib/instances/admission_allocations.go +++ b/lib/instances/admission_allocations.go @@ -107,7 +107,7 @@ func (m *manager) rollbackAdmissionAllocationActive(stored *StoredMetadata) { // Failed post-boot/restore steps should not leave the cached visible // allocation marked active. Clear the in-memory PID first so any later sync // from this metadata view also treats the instance as inactive. - stored.HypervisorPID = nil + stored.HypervisorProcessIdentity.Clear() m.setAdmissionAllocationActive(stored, false) } diff --git a/lib/instances/admission_allocations_test.go b/lib/instances/admission_allocations_test.go index 26c4252c4..17674e046 100644 --- a/lib/instances/admission_allocations_test.go +++ b/lib/instances/admission_allocations_test.go @@ -20,11 +20,11 @@ func TestRollbackAdmissionAllocationActiveClearsVisibleAllocation(t *testing.T) } pid := 1234 stored := &StoredMetadata{ - Id: "inst-1", - Name: "test-instance", - Vcpus: 2, - Size: 1024, - HypervisorPID: &pid, + Id: "inst-1", + Name: "test-instance", + Vcpus: 2, + Size: 1024, + HypervisorProcessIdentity: HypervisorProcessIdentity{HypervisorPID: &pid}, } m.setAdmissionAllocationActive(stored, true) @@ -52,12 +52,12 @@ func TestReconcileAdmissionAllocationsMarksMissingSocketInactive(t *testing.T) { pid := 4321 stored := StoredMetadata{ - Id: "inst-2", - Name: "test-instance", - Vcpus: 2, - Size: 1024, - HypervisorPID: &pid, - SocketPath: socketPath, + Id: "inst-2", + Name: "test-instance", + Vcpus: 2, + Size: 1024, + HypervisorProcessIdentity: HypervisorProcessIdentity{HypervisorPID: &pid}, + SocketPath: socketPath, } m := &manager{ diff --git a/lib/instances/fork.go b/lib/instances/fork.go index c77138578..b3da65ca7 100644 --- a/lib/instances/fork.go +++ b/lib/instances/fork.go @@ -280,7 +280,7 @@ func (m *manager) forkInstanceFromStoppedOrStandby(ctx context.Context, id strin forkMeta.ExpiresAt = nil forkMeta.StartedAt = nil forkMeta.StoppedAt = nil - forkMeta.HypervisorPID = nil + forkMeta.HypervisorProcessIdentity.Clear() forkMeta.SocketPath = m.paths.InstanceSocket(forkID, starter.SocketName()) forkMeta.DataDir = dstDir forkMeta.VsockSocket = m.paths.InstanceSocket(forkID, hypervisor.VsockSocketNameForType(forkMeta.HypervisorType)) @@ -611,7 +611,7 @@ func cloneStoredMetadata(src StoredMetadata) StoredMetadata { } if src.HypervisorPID != nil { pid := *src.HypervisorPID - dst.HypervisorPID = &pid + dst.HypervisorProcessIdentity.Set(pid) } if src.ExpiresAt != nil { expiresAt := *src.ExpiresAt diff --git a/lib/instances/manager.go b/lib/instances/manager.go index c815a0eb7..f972041c6 100644 --- a/lib/instances/manager.go +++ b/lib/instances/manager.go @@ -206,6 +206,7 @@ type manager struct { nativeCodecPaths map[string]string imageUsageRecorder ImageUsageRecorder guestAgentReadyProbe func(context.Context, *StoredMetadata) bool + shutdownGuestFn func(context.Context, hypervisor.VsockDialer, int32) error // Shared lifecycle event subscriptions for internal consumers. lifecycleEvents *lifecycleSubscribers @@ -661,6 +662,19 @@ func (m *manager) StopInstance(ctx context.Context, id string) (*Instance, error return nil, err } if current.State == StateStopped { + if current.GPUMdevUUID != "" { + if err := devices.DestroyMdev(ctx, current.GPUMdevUUID); err != nil { + return nil, fmt.Errorf("destroy retained mdev %s: %w", current.GPUMdevUUID, err) + } + meta, err := m.loadMetadata(id) + if err != nil { + return nil, err + } + meta.GPUMdevUUID = "" + if err := m.saveMetadata(meta); err != nil { + return nil, fmt.Errorf("save metadata after mdev cleanup: %w", err) + } + } if err := m.markRestartManualStopLocked(ctx, id); err != nil { return nil, err } diff --git a/lib/instances/manager_test.go b/lib/instances/manager_test.go index df2f2fcb4..4964363ea 100644 --- a/lib/instances/manager_test.go +++ b/lib/instances/manager_test.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "crypto/tls" + "errors" "fmt" "io" "net" @@ -164,6 +165,23 @@ func waitForInstanceState(ctx context.Context, mgr Manager, instanceID string, e return nil, fmt.Errorf("instance %s did not reach %s within %v (last state: %s)", instanceID, expected, timeout, lastState) } +// deleteInstanceEventually retries deletion while a hypervisor finishes dying. +func deleteInstanceEventually(t *testing.T, ctx context.Context, mgr Manager, instanceID string) { + t.Helper() + deadline := time.Now().Add(integrationTestTimeout(30 * time.Second)) + for { + err := mgr.DeleteInstance(ctx, instanceID) + if err == nil || errors.Is(err, ErrNotFound) { + return + } + if time.Now().After(deadline) { + t.Fatalf("delete instance %s did not converge: %v", instanceID, err) + } + t.Logf("delete instance %s not yet converged, retrying: %v", instanceID, err) + time.Sleep(time.Second) + } +} + func integrationTestTimeout(timeout time.Duration) time.Duration { if os.Getenv("CI") == "true" && timeout < 45*time.Second { return 45 * time.Second diff --git a/lib/instances/process_identity_test.go b/lib/instances/process_identity_test.go new file mode 100644 index 000000000..e6fdf1f46 --- /dev/null +++ b/lib/instances/process_identity_test.go @@ -0,0 +1,41 @@ +package instances + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestHypervisorProcessIdentityJSONKeysStayFlat guards the on-disk metadata +// format: the identity struct is embedded anonymously so its fields keep the +// flat JSON keys metadata files were written with before the struct existed. +func TestHypervisorProcessIdentityJSONKeysStayFlat(t *testing.T) { + pid := 1234 + stored := StoredMetadata{ + Id: "inst-json", + HypervisorProcessIdentity: HypervisorProcessIdentity{ + HypervisorPID: &pid, + HypervisorStartTime: 42, + HypervisorBootID: "boot-id", + }, + } + + data, err := json.Marshal(stored) + require.NoError(t, err) + + var keys map[string]json.RawMessage + require.NoError(t, json.Unmarshal(data, &keys)) + assert.Contains(t, keys, "HypervisorPID") + assert.Contains(t, keys, "HypervisorStartTime") + assert.Contains(t, keys, "HypervisorBootID") + assert.NotContains(t, keys, "HypervisorProcessIdentity") + + var decoded StoredMetadata + require.NoError(t, json.Unmarshal(data, &decoded)) + require.NotNil(t, decoded.HypervisorPID) + assert.Equal(t, pid, *decoded.HypervisorPID) + assert.Equal(t, uint64(42), decoded.HypervisorStartTime) + assert.Equal(t, "boot-id", decoded.HypervisorBootID) +} diff --git a/lib/instances/query.go b/lib/instances/query.go index 42728304d..f54b1600c 100644 --- a/lib/instances/query.go +++ b/lib/instances/query.go @@ -7,11 +7,9 @@ import ( "io" "os" "path/filepath" - "runtime" "slices" "strconv" "strings" - "syscall" "time" "github.com/kernel/hypeman/lib/guest" @@ -571,57 +569,8 @@ func (m *manager) toInstanceWithStateDerivation(ctx context.Context, meta *metad return inst } -func refreshHypervisorPID(stored *StoredMetadata, state State) { - if !state.RequiresVMM() && state != StateUnknown { - return - } - if stored.HypervisorPID != nil && processExists(*stored.HypervisorPID) { - return - } - if stored.SocketPath == "" { - return - } - if pid, err := hypervisor.ResolveProcessPID(stored.SocketPath); err == nil { - stored.HypervisorPID = &pid - return - } -} - func processExists(pid int) bool { - if pid <= 0 { - return false - } - err := syscall.Kill(pid, 0) - if err != nil && err != syscall.EPERM { - return false - } - if runtime.GOOS != "linux" { - return true - } - state, err := readLinuxProcessState(pid) - if err != nil { - return true - } - return state != "Z" -} - -func readLinuxProcessState(pid int) (string, error) { - statusPath := filepath.Join("/proc", strconv.Itoa(pid), "status") - data, err := os.ReadFile(statusPath) - if err != nil { - return "", err - } - for _, line := range strings.Split(string(data), "\n") { - if !strings.HasPrefix(line, "State:") { - continue - } - fields := strings.Fields(line) - if len(fields) < 2 { - return "", fmt.Errorf("malformed process state in %s", statusPath) - } - return fields[1], nil - } - return "", fmt.Errorf("process state missing from %s", statusPath) + return ProcessExists(pid) } // parseExitSentinel reads the last lines of the serial console log to find the diff --git a/lib/instances/restore.go b/lib/instances/restore.go index c209274e2..8f302251b 100644 --- a/lib/instances/restore.go +++ b/lib/instances/restore.go @@ -308,8 +308,8 @@ func (m *manager) restoreInstance( return nil, err } - // Store the PID for later cleanup - stored.HypervisorPID = &pid + // Store the process identity for later cleanup. + stored.HypervisorProcessIdentity.Set(pid) // 6. Transition: Paused → Running (resume) resumeCtx, resumeSpanEnd := m.startLifecycleStep(ctx, "resume_vm", @@ -448,7 +448,7 @@ func (m *manager) restoreFromSnapshot( if err != nil { return 0, nil, fmt.Errorf("restore vm: %w", err) } - pid = resolveRuntimeHypervisorPID(log, stored.SocketPath, pid) + pid = resolveRuntimeHypervisorPID(log, stored, pid) log.DebugContext(ctx, "VM restored from snapshot successfully", "instance_id", stored.Id, "pid", pid) return pid, hv, nil diff --git a/lib/instances/start.go b/lib/instances/start.go index b162ad90f..8ad5785cc 100644 --- a/lib/instances/start.go +++ b/lib/instances/start.go @@ -62,6 +62,18 @@ func (m *manager) startInstance( rollbackMeta := *meta rollbackMeta.Phases = meta.Phases.Clone() + // A failed stop can retain an mdev UUID. Release it before creating the + // replacement and persist the cleared metadata before start can fail again. + if stored.GPUMdevUUID != "" { + if err := devices.DestroyMdev(ctx, stored.GPUMdevUUID); err != nil { + return nil, fmt.Errorf("destroy retained mdev before start: %w", err) + } + stored.GPUMdevUUID = "" + if err := m.saveMetadata(meta); err != nil { + return nil, fmt.Errorf("save metadata after mdev cleanup: %w", err) + } + } + // 2a. Clear stale exit info from previous run and apply command overrides stored.ExitCode = nil stored.ExitMessage = "" From a731cb027e1231d7edb0bcb988896d70a0e636c6 Mon Sep 17 00:00:00 2001 From: chruffins <23645059+chruffins@users.noreply.github.com> Date: Tue, 1 Sep 2026 19:05:03 +0000 Subject: [PATCH 04/19] Fix lifecycle regression tests --- lib/instances/fork.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/instances/fork.go b/lib/instances/fork.go index b3da65ca7..fc4ed782e 100644 --- a/lib/instances/fork.go +++ b/lib/instances/fork.go @@ -611,7 +611,7 @@ func cloneStoredMetadata(src StoredMetadata) StoredMetadata { } if src.HypervisorPID != nil { pid := *src.HypervisorPID - dst.HypervisorProcessIdentity.Set(pid) + dst.HypervisorPID = &pid } if src.ExpiresAt != nil { expiresAt := *src.ExpiresAt From 5e1768b1dee4f5d3e01656d964de190531ec1fd6 Mon Sep 17 00:00:00 2001 From: chruffins <23645059+chruffins@users.noreply.github.com> Date: Tue, 1 Sep 2026 20:28:23 +0000 Subject: [PATCH 05/19] Serialize layer reconciliation with image finalization --- lib/images/layer_gc.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/lib/images/layer_gc.go b/lib/images/layer_gc.go index b5fc322cc..c6d611aa6 100644 --- a/lib/images/layer_gc.go +++ b/lib/images/layer_gc.go @@ -69,6 +69,15 @@ func (m *manager) inflightLayerRefSnapshot() map[string]struct{} { // reconcileLayerStore evicts unreferenced layer artifacts and refreshes the // cached disk usage totals so accounting reflects the removals. func (m *manager) reconcileLayerStore() { + m.createMu.Lock() + defer m.createMu.Unlock() + m.reconcileLayerStoreLocked() +} + +// reconcileLayerStoreLocked is used by lifecycle operations that already hold +// createMu. Serializing reconciliation with manifest finalization prevents an +// eviction scan from racing a newly committed layer reference. +func (m *manager) reconcileLayerStoreLocked() { m.evictUnreferencedLayerArtifacts() m.refreshDiskUsageTotals() } From 7ee2a98a5cc209cd7b63dc80b591f85d17d063f7 Mon Sep 17 00:00:00 2001 From: chruffins <23645059+chruffins@users.noreply.github.com> Date: Thu, 3 Sep 2026 12:48:07 +0000 Subject: [PATCH 06/19] Rebase storage lifecycle onto rootfs composition --- .github/workflows/test.yml | 8 +- cmd/api/api/images.go | 44 +++---- cmd/api/api/images_test.go | 114 ++++-------------- lib/devices/mdev_linux.go | 6 +- lib/diskutilization/diskutilization.go | 28 +---- lib/diskutilization/diskutilization_test.go | 16 --- lib/hypervisor/cloudhypervisor/config.go | 11 +- lib/hypervisor/cloudhypervisor/config_test.go | 10 ++ lib/hypervisor/config.go | 3 +- lib/hypervisor/qemu/config.go | 11 +- lib/hypervisor/qemu/config_test.go | 43 +++++++ lib/hypervisor/qemu/machine_test.go | 13 ++ lib/hypervisor/qemu/profile.go | 2 +- lib/images/credentials_test.go | 23 ++-- lib/images/lifecycle_test.go | 26 ++-- lib/images/manager_test.go | 16 +-- lib/images/manifest_model.go | 27 +---- lib/images/manifest_model_test.go | 6 +- lib/images/metrics_test.go | 6 +- lib/images/oci_public.go | 5 +- lib/images/recovery_regression_test.go | 9 +- lib/images/testlayers_test.go | 52 ++++++++ lib/images/testutil/testutil.go | 107 ++++++++++++++++ lib/instances/create_mdev_test.go | 6 +- lib/instances/delete_test.go | 10 ++ lib/instances/fork.go | 5 + lib/instances/guestmemory_linux_test.go | 2 +- lib/instances/manager.go | 13 -- lib/instances/manager_test.go | 8 +- lib/instances/network_test.go | 3 +- lib/instances/qemu_lifecycle_test.go | 3 +- lib/instances/query.go | 4 - lib/instances/restore.go | 6 +- lib/instances/snapshot_test.go | 113 +++++++++++++++++ lib/instances/start.go | 12 -- lib/instances/version_upgrade_test.go | 4 +- lib/instances/vm_config_validation.go | 25 ++-- lib/instances/vm_config_validation_test.go | 15 ++- lib/middleware/resolve.go | 3 +- lib/middleware/resolve_test.go | 78 +++++++----- lib/paths/paths.go | 5 - lib/resources/gpu.go | 2 +- openapi.yaml | 6 + 43 files changed, 580 insertions(+), 329 deletions(-) create mode 100644 lib/images/testlayers_test.go create mode 100644 lib/images/testutil/testutil.go diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 870b87467..7ccb6f999 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -13,7 +13,13 @@ on: required: false type: string -# Superseded branch pushes should release self-hosted test capacity. +# Every branch push triggers a full run and superseded runs used to keep +# running, holding self-hosted slots that every repo on the shared pool +# queues behind. Group by ref so a newer push cancels the older run for +# that branch. main and workflow_dispatch runs get run_id, a unique group +# each, because sharing a group is unsafe even with cancellation off: +# GitHub cancels an existing pending run when a newer one enters the +# group, which would drop a commit's only test signal. concurrency: group: test-${{ (github.event_name == 'push' && github.ref != 'refs/heads/main') && github.ref || github.run_id }} cancel-in-progress: ${{ github.event_name == 'push' && github.ref != 'refs/heads/main' }} diff --git a/cmd/api/api/images.go b/cmd/api/api/images.go index e9f6defc8..7ae20ebdc 100644 --- a/cmd/api/api/images.go +++ b/cmd/api/api/images.go @@ -94,6 +94,10 @@ func (s *ApiService) CreateImage(ctx context.Context, request oapi.CreateImageRe return oapi.CreateImage202JSONResponse(imageToOAPI(*img)), nil } +// TagImage handles POST /images/{name}/tag. +// Note: ResolveResource skips POST /images/{name}/tag, so the source is +// resolved by ImageManager.TagImage and a missing source gets the specific +// 404 body below. func (s *ApiService) TagImage(ctx context.Context, request oapi.TagImageRequestObject) (oapi.TagImageResponseObject, error) { if request.Body == nil { return oapi.TagImage400JSONResponse{ @@ -104,35 +108,25 @@ func (s *ApiService) TagImage(ctx context.Context, request oapi.TagImageRequestO img, err := s.ImageManager.TagImage(ctx, request.Name, request.Body.Target) if err != nil { - log := logger.FromContext(ctx) - switch { - case errors.Is(err, images.ErrInvalidName): - return oapi.TagImage400JSONResponse{ - Code: "invalid_name", - Message: err.Error(), - }, nil - case errors.Is(err, images.ErrNotFound): - return oapi.TagImage404JSONResponse{ - Code: "not_found", - Message: "source image not found", - }, nil - case errors.Is(err, images.ErrImageNotReady): - return oapi.TagImage409JSONResponse{ - Code: "image_not_ready", - Message: err.Error(), - }, nil - default: - log.ErrorContext(ctx, "failed to tag image", "error", err, "source", request.Name, "target", request.Body.Target) - return oapi.TagImage500JSONResponse{ - Code: "internal_error", - Message: "failed to tag image", - }, nil - } + return tagImageErrorResponse(ctx, err, request.Name, request.Body.Target), nil } - return oapi.TagImage200JSONResponse(imageToOAPI(*img)), nil } +func tagImageErrorResponse(ctx context.Context, err error, source, target string) oapi.TagImageResponseObject { + switch { + case errors.Is(err, images.ErrInvalidName): + return oapi.TagImage400JSONResponse{Code: "invalid_name", Message: err.Error()} + case errors.Is(err, images.ErrNotFound): + return oapi.TagImage404JSONResponse{Code: "not_found", Message: "source image not found"} + case errors.Is(err, images.ErrImageNotReady): + return oapi.TagImage409JSONResponse{Code: "image_not_ready", Message: err.Error()} + default: + logger.FromContext(ctx).ErrorContext(ctx, "failed to tag image", "error", err, "source", source, "target", target) + return oapi.TagImage500JSONResponse{Code: "internal_error", Message: "failed to tag image"} + } +} + // GetImage gets image details by name // Note: Resolution is handled by ResolveResource middleware func (s *ApiService) GetImage(ctx context.Context, request oapi.GetImageRequestObject) (oapi.GetImageResponseObject, error) { diff --git a/cmd/api/api/images_test.go b/cmd/api/api/images_test.go index 2a5d02367..b6a1857b5 100644 --- a/cmd/api/api/images_test.go +++ b/cmd/api/api/images_test.go @@ -2,14 +2,12 @@ package api import ( "context" - "encoding/json" "fmt" - "os" - "path/filepath" "testing" "time" "github.com/kernel/hypeman/lib/images" + "github.com/kernel/hypeman/lib/images/testutil" "github.com/kernel/hypeman/lib/oapi" "github.com/kernel/hypeman/lib/paths" "github.com/stretchr/testify/assert" @@ -515,58 +513,36 @@ func seedReadyDigestOnlyImage(t *testing.T, svc *ApiService, imageRef string, im require.NoError(t, err) require.True(t, ref.IsDigest(), "test helper expects a digest reference") - p := paths.New(svc.Config.DataDir) - digestDir := p.ImageDigestDir(ref.Repository(), ref.DigestHex()) - require.NoError(t, os.MkdirAll(digestDir, 0o755)) - require.NoError(t, os.WriteFile(p.ImageDigestPath(ref.Repository(), ref.DigestHex()), []byte("rootfs"), 0o644)) - - meta := struct { - Name string `json:"name"` - Digest string `json:"digest"` - Status string `json:"status"` - SizeBytes int64 `json:"size_bytes"` - Tags map[string]string `json:"tags,omitempty"` - CreatedAt time.Time `json:"created_at"` - }{ - Name: imageRef, - Digest: "sha256:" + ref.DigestHex(), - Status: "ready", - SizeBytes: int64(len("rootfs")), - Tags: imageTags, - CreatedAt: time.Now().UTC(), - } - - data, err := json.Marshal(meta) - require.NoError(t, err) - require.NoError(t, os.WriteFile(p.ImageMetadata(ref.Repository(), ref.DigestHex()), data, 0o644)) + testutil.SeedReadyImage(t, paths.New(svc.Config.DataDir), testutil.Seed{ + Repository: ref.Repository(), + DigestHex: ref.DigestHex(), + Name: imageRef, + Tags: imageTags, + }) } func TestTagImage_ErrorStatusMapping(t *testing.T) { t.Parallel() cases := []struct { - name string - err error - wantType any - wantCode string + name string + err error + want oapi.TagImageResponseObject }{ { - name: "invalid name -> 400", - err: fmt.Errorf("tag: %w", images.ErrInvalidName), - wantType: oapi.TagImage400JSONResponse{}, - wantCode: "invalid_name", + name: "invalid name -> 400", + err: fmt.Errorf("tag: %w", images.ErrInvalidName), + want: oapi.TagImage400JSONResponse{Code: "invalid_name", Message: "tag: invalid image name"}, }, { - name: "not found -> 404", - err: fmt.Errorf("tag: %w", images.ErrNotFound), - wantType: oapi.TagImage404JSONResponse{}, - wantCode: "not_found", + name: "not found -> 404", + err: fmt.Errorf("tag: %w", images.ErrNotFound), + want: oapi.TagImage404JSONResponse{Code: "not_found", Message: "source image not found"}, }, { - name: "not ready -> 409", - err: fmt.Errorf("tag: %w", images.ErrImageNotReady), - wantType: oapi.TagImage409JSONResponse{}, - wantCode: "image_not_ready", + name: "not ready -> 409", + err: fmt.Errorf("tag: %w", images.ErrImageNotReady), + want: oapi.TagImage409JSONResponse{Code: "image_not_ready", Message: "tag: image is not ready"}, }, } @@ -578,8 +554,7 @@ func TestTagImage_ErrorStatusMapping(t *testing.T) { Body: &oapi.TagImageRequest{Target: "docker.io/library/alpine:stable"}, }) require.NoError(t, err) - require.IsType(t, tc.wantType, resp) - require.Equal(t, tc.wantCode, tagImageErrorCode(resp)) + require.Equal(t, tc.want, resp) }) } } @@ -595,53 +570,16 @@ func TestTagImage_MissingBody(t *testing.T) { require.IsType(t, oapi.TagImage400JSONResponse{}, resp) } -func tagImageErrorCode(resp oapi.TagImageResponseObject) string { - switch r := resp.(type) { - case oapi.TagImage400JSONResponse: - return r.Code - case oapi.TagImage404JSONResponse: - return r.Code - case oapi.TagImage409JSONResponse: - return r.Code - case oapi.TagImage500JSONResponse: - return r.Code - default: - return "" - } -} - // seedReadyContentImage writes a ready image into the shared content layout // plus a repository tag reference, without pulling from a registry. func seedReadyContentImage(t *testing.T, svc *ApiService, repository, tag, digestHex string) { t.Helper() - - p := paths.New(svc.Config.DataDir) - contentDir := p.ImageContentDir(digestHex) - require.NoError(t, os.MkdirAll(contentDir, 0o755)) - require.NoError(t, os.WriteFile(p.ImageContentPath(digestHex), []byte("rootfs"), 0o644)) - - meta := struct { - Name string `json:"name"` - Digest string `json:"digest"` - Status string `json:"status"` - SizeBytes int64 `json:"size_bytes"` - CreatedAt time.Time `json:"created_at"` - }{ - Name: repository + ":" + tag, - Digest: "sha256:" + digestHex, - Status: "ready", - SizeBytes: int64(len("rootfs")), - CreatedAt: time.Now().UTC(), - } - data, err := json.Marshal(meta) - require.NoError(t, err) - require.NoError(t, os.WriteFile(p.ImageContentMetadata(digestHex), data, 0o644)) - - linkPath := p.ImageRepositoryTagSymlink(repository, tag) - target, err := filepath.Rel(filepath.Dir(linkPath), contentDir) - require.NoError(t, err) - require.NoError(t, os.MkdirAll(filepath.Dir(linkPath), 0o755)) - require.NoError(t, os.Symlink(target, linkPath)) + testutil.SeedReadyImage(t, paths.New(svc.Config.DataDir), testutil.Seed{ + Repository: repository, + Tag: tag, + DigestHex: digestHex, + Content: true, + }) } func TestTagImage_Success(t *testing.T) { diff --git a/lib/devices/mdev_linux.go b/lib/devices/mdev_linux.go index fe4ad50cb..e8495fda8 100644 --- a/lib/devices/mdev_linux.go +++ b/lib/devices/mdev_linux.go @@ -150,7 +150,7 @@ func discoverMdevVFsWith(busPath, pciPath string, listMdevs func() ([]MdevDevice vfs = append(vfs, VirtualFunction{ PCIAddress: vfAddr, ParentGPU: parentGPU, - HasMdev: hasMdev, + Allocated: hasMdev, }) } if len(vfErrs) > 0 { @@ -276,7 +276,7 @@ func countAvailableVFsForProfilesParallel(vfs []VirtualFunction, profiles []prof // Group free VFs by parent GPU (done once, shared by all goroutines) freeVFsByParent := make(map[string][]VirtualFunction) for _, vf := range vfs { - if vf.HasMdev { + if vf.Allocated { continue } freeVFsByParent[vf.ParentGPU] = append(freeVFsByParent[vf.ParentGPU], vf) @@ -476,7 +476,7 @@ func selectLeastLoadedVF(ctx context.Context, vfs []VirtualFunction, profileType allGPUs := make(map[string]bool) for _, vf := range vfs { allGPUs[vf.ParentGPU] = true - if !vf.HasMdev { + if !vf.Allocated { freeVFsByGPU[vf.ParentGPU] = append(freeVFsByGPU[vf.ParentGPU], vf) } } diff --git a/lib/diskutilization/diskutilization.go b/lib/diskutilization/diskutilization.go index a5f55d677..27fef3c81 100644 --- a/lib/diskutilization/diskutilization.go +++ b/lib/diskutilization/diskutilization.go @@ -4,7 +4,6 @@ import ( "io/fs" "os" "path/filepath" - "strings" "syscall" "github.com/kernel/hypeman/lib/paths" @@ -57,7 +56,7 @@ func Collect(p *paths.Paths) (Breakdown, error) { return false } name := entry.Name() - return name == "rootfs.erofs" || name == "rootfs.ext4" || strings.HasPrefix(name, "layer.") + return name == "rootfs.erofs" || name == "rootfs.ext4" }) if err != nil { return Breakdown{}, err @@ -179,7 +178,6 @@ func sumDirectChildFileAllocatedBytes(root string, childFile string) (int64, err func sumMatchingFilesAllocatedBytes(root string, match func(path string, entry fs.DirEntry) bool) (int64, error) { var total int64 - seen := make(map[fileIdentity]struct{}) err := filepath.WalkDir(root, func(path string, entry fs.DirEntry, err error) error { if err != nil { if os.IsNotExist(err) { @@ -187,24 +185,9 @@ func sumMatchingFilesAllocatedBytes(root string, match func(path string, entry f } return err } - if !match(path, entry) { - return nil + if match(path, entry) { + total += allocatedBytesForPath(path) } - info, statErr := os.Lstat(path) - if statErr != nil { - if os.IsNotExist(statErr) { - return nil - } - return statErr - } - if stat, ok := info.Sys().(*syscall.Stat_t); ok { - identity := fileIdentity{dev: uint64(stat.Dev), ino: uint64(stat.Ino)} - if _, exists := seen[identity]; exists { - return nil - } - seen[identity] = struct{}{} - } - total += allocatedBytesForPath(path) return nil }) if err != nil { @@ -261,11 +244,6 @@ func sumSnapshotTreeAllocatedBytes(root string, sharedExtents *sharedExtentTrack return privateTotal, sharedTotal, nil } -type fileIdentity struct { - dev uint64 - ino uint64 -} - func allocatedBytesForPath(path string) int64 { info, err := os.Lstat(path) if err != nil { diff --git a/lib/diskutilization/diskutilization_test.go b/lib/diskutilization/diskutilization_test.go index ff854438d..50a787017 100644 --- a/lib/diskutilization/diskutilization_test.go +++ b/lib/diskutilization/diskutilization_test.go @@ -98,22 +98,6 @@ func TestCollect_UsesAllocatedBytesAndClassifiesSnapshots(t *testing.T) { require.Equal(t, otherTotal, utilization.SnapshotOther) } -func TestCollect_DeduplicatesHardLinkedImagesAndCountsLayers(t *testing.T) { - p := paths.New(t.TempDir()) - imagePath := filepath.Join(p.ImagesDir(), "repo", "digest", "rootfs.erofs") - require.NoError(t, createSparseTestFile(imagePath, 8192, []sparseWrite{{offset: 0, data: []byte("image")}})) - aliasPath := filepath.Join(p.ImagesDir(), "content", "digest", "rootfs.erofs") - require.NoError(t, os.MkdirAll(filepath.Dir(aliasPath), 0755)) - require.NoError(t, os.Link(imagePath, aliasPath)) - - layerPath := filepath.Join(p.ImageLayersDir(), "layer-digest", "layer.erofs") - require.NoError(t, createSparseTestFile(layerPath, 8192, []sparseWrite{{offset: 0, data: []byte("layer")}})) - - utilization, err := Collect(p) - require.NoError(t, err) - require.Equal(t, allocatedBytesForPath(imagePath)+allocatedBytesForPath(layerPath), utilization.Images) -} - func createSparseTestFile(path string, size int64, writes []sparseWrite) error { if err := os.MkdirAll(filepath.Dir(path), 0755); err != nil { return err diff --git a/lib/hypervisor/cloudhypervisor/config.go b/lib/hypervisor/cloudhypervisor/config.go index e9f91fe4a..ca3d98a55 100644 --- a/lib/hypervisor/cloudhypervisor/config.go +++ b/lib/hypervisor/cloudhypervisor/config.go @@ -126,13 +126,20 @@ func ToVMConfig(cfg hypervisor.VMConfig) vmm.VmConfig { // Device passthrough configuration var devices *[]vmm.DeviceConfig - if len(cfg.PCIDevices) > 0 { - deviceConfigs := make([]vmm.DeviceConfig, 0, len(cfg.PCIDevices)) + deviceCount := len(cfg.PCIDevices) + if cfg.VGPUDevicePath != "" { + deviceCount++ + } + if deviceCount > 0 { + deviceConfigs := make([]vmm.DeviceConfig, 0, deviceCount) for _, path := range cfg.PCIDevices { deviceConfigs = append(deviceConfigs, vmm.DeviceConfig{ Path: path, }) } + if cfg.VGPUDevicePath != "" { + deviceConfigs = append(deviceConfigs, vmm.DeviceConfig{Path: cfg.VGPUDevicePath}) + } devices = &deviceConfigs } diff --git a/lib/hypervisor/cloudhypervisor/config_test.go b/lib/hypervisor/cloudhypervisor/config_test.go index b5cdb96e9..be39d13af 100644 --- a/lib/hypervisor/cloudhypervisor/config_test.go +++ b/lib/hypervisor/cloudhypervisor/config_test.go @@ -8,6 +8,16 @@ import ( "github.com/stretchr/testify/require" ) +func TestToVMConfigIncludesVGPUDevice(t *testing.T) { + path := "/sys/bus/mdev/devices/aa618089-8b16-4d01-a136-25a0f3c73123" + + vmCfg := ToVMConfig(hypervisor.VMConfig{VGPUDevicePath: path}) + + require.NotNil(t, vmCfg.Devices) + require.Len(t, *vmCfg.Devices, 1) + assert.Equal(t, path, (*vmCfg.Devices)[0].Path) +} + func TestToVMConfig_GuestMemoryBalloon(t *testing.T) { cfg := hypervisor.VMConfig{ VCPUs: 1, diff --git a/lib/hypervisor/config.go b/lib/hypervisor/config.go index 07f392e8d..e46aa3446 100644 --- a/lib/hypervisor/config.go +++ b/lib/hypervisor/config.go @@ -24,7 +24,8 @@ type VMConfig struct { VsockSocket string // PCI device passthrough (GPU, etc.) - PCIDevices []string + PCIDevices []string + VGPUDevicePath string // Boot configuration. Empty BootMode preserves the existing direct-kernel // behavior for Linux callers. diff --git a/lib/hypervisor/qemu/config.go b/lib/hypervisor/qemu/config.go index b3959a29a..f00b1a5af 100644 --- a/lib/hypervisor/qemu/config.go +++ b/lib/hypervisor/qemu/config.go @@ -119,13 +119,10 @@ func buildArgs(cfg hypervisor.VMConfig, machine MachineType) []string { args = append(args, "-device", fmt.Sprintf("%s,guest-cid=%d", virtioDevice(microvm, "vhost-vsock"), cfg.VsockCID)) } - // PCI device passthrough (GPU, mdev vGPU, etc.) + // Whole-device PCI passthrough (vGPU attaches via VGPUDevicePath below) for _, devicePath := range cfg.PCIDevices { var deviceArg string - if strings.HasPrefix(devicePath, "/sys/bus/mdev/devices/") { - // mdev device (vGPU) - use sysfsdev parameter - deviceArg = fmt.Sprintf("vfio-pci,sysfsdev=%s", devicePath) - } else if strings.HasPrefix(devicePath, "/sys/bus/pci/devices/") { + if strings.HasPrefix(devicePath, "/sys/bus/pci/devices/") { // Full sysfs path for regular PCI device - extract the PCI address // Using filepath.Base is more robust than manual string splitting pciAddr := filepath.Base(strings.TrimSuffix(devicePath, "/")) @@ -137,6 +134,10 @@ func buildArgs(cfg hypervisor.VMConfig, machine MachineType) []string { args = append(args, "-device", deviceArg) } + if cfg.VGPUDevicePath != "" { + args = append(args, "-device", fmt.Sprintf("vfio-pci,sysfsdev=%s", cfg.VGPUDevicePath)) + } + // Serial console output to file. Use a chardev with append=on so QEMU // opens the file with O_APPEND. Without it, QEMU writes at its internal // fd offset; if the file is externally truncated (e.g. log rotation via diff --git a/lib/hypervisor/qemu/config_test.go b/lib/hypervisor/qemu/config_test.go index 4d4ae0595..71e8080af 100644 --- a/lib/hypervisor/qemu/config_test.go +++ b/lib/hypervisor/qemu/config_test.go @@ -152,6 +152,49 @@ func TestBuildArgs_Vsock(t *testing.T) { assert.Contains(t, args, "vhost-vsock-pci,guest-cid=123") } +func TestBuildArgs_VGPU(t *testing.T) { + t.Parallel() + + for _, path := range []string{ + "/sys/bus/mdev/devices/aa618089-8b16-4d01-a136-25a0f3c73123", + "/sys/bus/pci/devices/0000:82:00.4", + } { + path := path + t.Run(path, func(t *testing.T) { + t.Parallel() + args := BuildArgs(hypervisor.VMConfig{ + VCPUs: 1, + MemoryBytes: 512 * 1024 * 1024, + VGPUDevicePath: path, + }) + assert.Contains(t, args, "vfio-pci,sysfsdev="+path) + }) + } +} + +func TestBuildArgs_VGPUAfterPCIDevices(t *testing.T) { + args := BuildArgs(hypervisor.VMConfig{ + VCPUs: 1, + MemoryBytes: 512 * 1024 * 1024, + PCIDevices: []string{"0000:01:00.0"}, + VGPUDevicePath: "/sys/bus/mdev/devices/aa618089-8b16-4d01-a136-25a0f3c73123", + }) + + pciDeviceIndex := -1 + vgpuDeviceIndex := -1 + for i, arg := range args { + switch arg { + case "vfio-pci,host=0000:01:00.0": + pciDeviceIndex = i + case "vfio-pci,sysfsdev=/sys/bus/mdev/devices/aa618089-8b16-4d01-a136-25a0f3c73123": + vgpuDeviceIndex = i + } + } + + assert.Greater(t, pciDeviceIndex, -1) + assert.Greater(t, vgpuDeviceIndex, pciDeviceIndex) +} + func TestBuildArgs_PCIPassthrough(t *testing.T) { cfg := hypervisor.VMConfig{ VCPUs: 1, diff --git a/lib/hypervisor/qemu/machine_test.go b/lib/hypervisor/qemu/machine_test.go index 9998d9280..9484cdcde 100644 --- a/lib/hypervisor/qemu/machine_test.go +++ b/lib/hypervisor/qemu/machine_test.go @@ -81,6 +81,19 @@ func TestQEMUCapabilitiesAdvertiseFork(t *testing.T) { assert.True(t, (MicroVMProfile{}).capabilities().SupportsFork) } +func TestMicroVMValidateConfigRejectsVFIODevices(t *testing.T) { + t.Parallel() + err := (MicroVMProfile{}).validateConfig(hypervisor.VMConfig{ + PCIDevices: []string{"0000:82:00.4"}, + }) + require.ErrorContains(t, err, "microvm does not support PCI devices") + + err = (MicroVMProfile{}).validateConfig(hypervisor.VMConfig{ + VGPUDevicePath: "/sys/bus/mdev/devices/aa618089-8b16-4d01-a136-25a0f3c73123", + }) + require.ErrorContains(t, err, "microvm does not support PCI devices") +} + func TestValidateConfigMicroVM(t *testing.T) { t.Parallel() if _, err := microVMMachineType(); err != nil { diff --git a/lib/hypervisor/qemu/profile.go b/lib/hypervisor/qemu/profile.go index 07485b849..0652551f9 100644 --- a/lib/hypervisor/qemu/profile.go +++ b/lib/hypervisor/qemu/profile.go @@ -55,7 +55,7 @@ func (MicroVMProfile) validateConfig(cfg hypervisor.VMConfig) error { if cfg.HotplugBytes > 0 { return fmt.Errorf("microvm does not support hotplug memory") } - if len(cfg.PCIDevices) > 0 { + if len(cfg.PCIDevices) > 0 || cfg.VGPUDevicePath != "" { return fmt.Errorf("microvm does not support PCI devices") } diff --git a/lib/images/credentials_test.go b/lib/images/credentials_test.go index 87a72c39b..7f680b980 100644 --- a/lib/images/credentials_test.go +++ b/lib/images/credentials_test.go @@ -83,8 +83,10 @@ func TestCreateImageRequestCredentialsAreNotPersisted(t *testing.T) { } func TestInflightPullRejectsDifferentCredentials(t *testing.T) { - m := newTestManager(nil) - m.borrowedCredentialsTimeout = time.Minute + m := &manager{ + inflightPulls: make(map[string]*inflightImagePull), + borrowedCredentialsTimeout: time.Minute, + } const digest = "sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" credentials := &authn.AuthConfig{Username: "AWS", Password: "token-a"} inflight := m.registerInflightPull(digest, credentials) @@ -96,8 +98,10 @@ func TestInflightPullRejectsDifferentCredentials(t *testing.T) { } func TestBorrowedCredentialsExpireWhileQueued(t *testing.T) { - m := newTestManager(nil) - m.borrowedCredentialsTimeout = time.Millisecond + m := &manager{ + inflightPulls: make(map[string]*inflightImagePull), + borrowedCredentialsTimeout: time.Millisecond, + } const digest = "sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" inflight := m.registerInflightPull(digest, &authn.AuthConfig{Username: "AWS", Password: "secret"}) defer m.releaseInflightPull(digest, inflight)() @@ -115,7 +119,7 @@ func TestBorrowedCredentialsExpireWhileQueued(t *testing.T) { } func TestBorrowedAuthRejectsReplacedInflightPull(t *testing.T) { - m := newTestManager(nil) + m := &manager{inflightPulls: make(map[string]*inflightImagePull)} const digest = "sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" first := m.registerInflightPull(digest, &authn.AuthConfig{Username: "first"}) second := m.registerInflightPull(digest, &authn.AuthConfig{Username: "second"}) @@ -170,9 +174,12 @@ func TestRecoverInterruptedCredentialedPullFailsForFreshRetry(t *testing.T) { p := paths.New(t.TempDir()) client, err := newOCIClient(p.SystemOCICache()) require.NoError(t, err) - m := newTestManager(p) - m.ociClient = client - m.queue = queue.New(1) + m := &manager{ + paths: p, + ociClient: client, + queue: queue.New(1), + readySubscribers: make(map[string][]chan StatusEvent), + } const repository = "registry.example/private/image" const digest = "sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" diff --git a/lib/images/lifecycle_test.go b/lib/images/lifecycle_test.go index 1554f1501..1519c618c 100644 --- a/lib/images/lifecycle_test.go +++ b/lib/images/lifecycle_test.go @@ -134,27 +134,35 @@ func TestSharedLayersMaterializeOnceAndEvictWithReferences(t *testing.T) { require.Empty(t, hexes, "unreferenced layer artifacts must be evicted") } +func newLifecycleTestManager(p *paths.Paths) *manager { + return &manager{ + paths: p, + inflightPulls: make(map[string]*inflightImagePull), + inflightLayerRefs: make(map[string]int), + } +} + func TestTotalImageBytesIncludesLayerArtifacts(t *testing.T) { p := paths.New(t.TempDir()) - m := newTestManager(p) + m := newLifecycleTestManager(p) digestHex := "cd01cd01cd01cd01cd01cd01cd01cd01cd01cd01cd01cd01cd01cd01cd01cd01" require.NoError(t, os.MkdirAll(p.ImageLayerDir(digestHex), 0o755)) payload := make([]byte, 4096) - require.NoError(t, os.WriteFile(p.ImageLayerArtifact(digestHex), payload, 0o644)) + require.NoError(t, os.WriteFile(p.ImageLayerArtifactForFormat(digestHex, string(DefaultImageFormat)), payload, 0o644)) - totals, err := m.getDiskUsageTotals() + readyBytes, cacheBytes, err := m.getDiskUsageTotals() require.NoError(t, err) - require.GreaterOrEqual(t, totals.layerBytes, int64(len(payload))) + require.GreaterOrEqual(t, cacheBytes, int64(len(payload))) totalBytes, err := m.TotalImageBytes(context.Background()) require.NoError(t, err) - require.Equal(t, totals.readyImageBytes+totals.layerBytes, totalBytes) + require.Equal(t, readyBytes+cacheBytes, totalBytes) } func TestCleanStaleImageTempDirsRemovesOnlyOldDirectories(t *testing.T) { p := paths.New(t.TempDir()) - m := newTestManager(p) + m := newLifecycleTestManager(p) m.layerEvictionGrace = time.Hour layersDir := p.ImageLayersDir() @@ -175,7 +183,7 @@ func TestCleanStaleImageTempDirsRemovesOnlyOldDirectories(t *testing.T) { func TestEvictionKeepsReferencedAndFreshArtifacts(t *testing.T) { p := paths.New(t.TempDir()) - m := newTestManager(p) + m := newLifecycleTestManager(p) m.layerEvictionGrace = time.Hour referencedHex := "ef01ef01ef01ef01ef01ef01ef01ef01ef01ef01ef01ef01ef01ef01ef01ef01" @@ -193,11 +201,11 @@ func TestEvictionKeepsReferencedAndFreshArtifacts(t *testing.T) { } require.NoError(t, writeManifestModel(p, referencedHex, model)) require.NoError(t, os.MkdirAll(p.ImageLayerDir(referencedHex), 0o755)) - require.NoError(t, os.WriteFile(p.ImageLayerArtifact(referencedHex), []byte("kept"), 0o644)) + require.NoError(t, os.WriteFile(p.ImageLayerArtifactForFormat(referencedHex, string(DefaultImageFormat)), []byte("kept"), 0o644)) // An unreferenced but fresh artifact is protected by the grace period. require.NoError(t, os.MkdirAll(p.ImageLayerDir(orphanFreshHex), 0o755)) - require.NoError(t, os.WriteFile(p.ImageLayerArtifact(orphanFreshHex), []byte("fresh"), 0o644)) + require.NoError(t, os.WriteFile(p.ImageLayerArtifactForFormat(orphanFreshHex, string(DefaultImageFormat)), []byte("fresh"), 0o644)) m.reconcileLayerStore() diff --git a/lib/images/manager_test.go b/lib/images/manager_test.go index 01b91b808..13a6835e2 100644 --- a/lib/images/manager_test.go +++ b/lib/images/manager_test.go @@ -17,18 +17,6 @@ import ( "github.com/stretchr/testify/require" ) -// newTestManager returns a manager with the maps NewManager initializes, so -// tests can construct one directly without nil-map guards in the manager. -func newTestManager(p *paths.Paths) *manager { - return &manager{ - paths: p, - tagGenerations: make(map[string]uint64), - inflightLayerRefs: make(map[string]int), - inflightPulls: make(map[string]*inflightImagePull), - readySubscribers: make(map[string][]chan StatusEvent), - } -} - func TestConversionFailedErr(t *testing.T) { t.Run("without detail", func(t *testing.T) { assert.EqualError(t, conversionFailedErr(nil, nil), "image conversion failed") @@ -746,9 +734,9 @@ func TestDeleteAndRecreateDuringBuildTail(t *testing.T) { require.NoError(t, err) staleRef := NewResolvedRef(normalized, digestStr) m.updateStatusByDigest(staleRef, StatusFailed, errors.New("stale build"), firstMeta.BuildID) - staleResult, _, _, err := m.ociClient.extractOCIImageDetails(digestHex) + staleBundle, err := m.ociClient.extractOCIImageBundle(digestHex) require.NoError(t, err) - require.ErrorIs(t, m.finalizeImage(staleRef, &pullResult{Metadata: staleResult}, 1, firstMeta.BuildID, ""), errStaleBuild) + require.ErrorIs(t, m.finalizeImage(staleRef, &pullResult{Metadata: staleBundle.Meta}, 1, firstMeta.BuildID, ""), errStaleBuild) currentMeta, err = readMetadata(p, repo, digestHex) require.NoError(t, err) require.Equal(t, StatusPending, currentMeta.Status) diff --git a/lib/images/manifest_model.go b/lib/images/manifest_model.go index a640f3e08..16a913ce4 100644 --- a/lib/images/manifest_model.go +++ b/lib/images/manifest_model.go @@ -66,7 +66,6 @@ func (m *imageManifestModel) blobReferences() []string { return refs } -// writeManifestModel persists the manifest model for a digest atomically. func validateManifestModel(digestHex string, model *imageManifestModel) error { if model == nil { return fmt.Errorf("manifest model is nil") @@ -170,28 +169,10 @@ func readManifestModel(p *paths.Paths, digestHex string) (*imageManifestModel, e // writeJSONAtomic writes data to path via a temp file in the same directory // followed by a rename, so readers never observe a partial document. func writeJSONAtomic(path string, data []byte) error { - if err := os.MkdirAll(filepath.Dir(path), 0755); err != nil { - return fmt.Errorf("create directory: %w", err) - } - tempFile, err := os.CreateTemp(filepath.Dir(path), "."+filepath.Base(path)+".tmp-*") - if err != nil { - return fmt.Errorf("create temp file: %w", err) - } - tempPath := tempFile.Name() - defer os.Remove(tempPath) - if err := tempFile.Chmod(0644); err != nil { - _ = tempFile.Close() - return fmt.Errorf("chmod temp file: %w", err) - } - if _, err := tempFile.Write(data); err != nil { - _ = tempFile.Close() - return fmt.Errorf("write temp file: %w", err) - } - if err := tempFile.Close(); err != nil { - return fmt.Errorf("close temp file: %w", err) - } - if err := os.Rename(tempPath, path); err != nil { - return fmt.Errorf("rename into place: %w", err) + if err := installAtomically(path, func(tempPath string) error { + return os.WriteFile(tempPath, data, 0o644) + }); err != nil { + return fmt.Errorf("write %s: %w", filepath.Base(path), err) } return nil } diff --git a/lib/images/manifest_model_test.go b/lib/images/manifest_model_test.go index d73125445..bb0ebf722 100644 --- a/lib/images/manifest_model_test.go +++ b/lib/images/manifest_model_test.go @@ -46,8 +46,9 @@ func TestExtractManifestModel(t *testing.T) { client, layoutTag := writeSyntheticLayout(t, img) - model, err := client.extractManifestModel(layoutTag) + bundle, err := client.extractOCIImageBundle(layoutTag) require.NoError(t, err) + model := bundle.Model manifest, err := img.Manifest() require.NoError(t, err) @@ -88,8 +89,9 @@ func TestExtractManifestModelPlatform(t *testing.T) { client, layoutTag := writeSyntheticLayout(t, img) - model, err := client.extractManifestModel(layoutTag) + bundle, err := client.extractOCIImageBundle(layoutTag) require.NoError(t, err) + model := bundle.Model require.Equal(t, "linux/amd64", model.Platform) } diff --git a/lib/images/metrics_test.go b/lib/images/metrics_test.go index 1f58ab48c..93007b300 100644 --- a/lib/images/metrics_test.go +++ b/lib/images/metrics_test.go @@ -16,8 +16,10 @@ import ( func TestImageBuildPhaseMetrics(t *testing.T) { reader := otelmetric.NewManualReader() provider := otelmetric.NewMeterProvider(otelmetric.WithReader(reader)) - m := newTestManager(paths.New(t.TempDir())) - m.queue = queue.New(1) + m := &manager{ + paths: paths.New(t.TempDir()), + queue: queue.New(1), + } metrics, err := newMetrics(provider.Meter("test"), m) require.NoError(t, err) diff --git a/lib/images/oci_public.go b/lib/images/oci_public.go index 554e1967f..7d336745b 100644 --- a/lib/images/oci_public.go +++ b/lib/images/oci_public.go @@ -47,10 +47,7 @@ func (c *OCIClient) InspectManifestForLinux(ctx context.Context, imageRef string // PullAndUnpack pulls an OCI image and unpacks it to a directory (public for system manager). // Always targets Linux platform since hypeman VMs are Linux guests. func (c *OCIClient) PullAndUnpack(ctx context.Context, imageRef, digest, exportDir string) error { - result, err := c.client.pullAndExport(ctx, imageRef, digest, exportDir) - if result != nil { - defer result.cleanup() - } + _, err := c.client.pullAndExport(ctx, imageRef, digest, exportDir) if err != nil { return fmt.Errorf("pull and unpack: %w", err) } diff --git a/lib/images/recovery_regression_test.go b/lib/images/recovery_regression_test.go index 398bad182..cd7d2e7ed 100644 --- a/lib/images/recovery_regression_test.go +++ b/lib/images/recovery_regression_test.go @@ -46,9 +46,12 @@ func TestRecoverInterruptedBuildsCapturedFixtureMarksBuildFailed(t *testing.T) { client, err := newOCIClient(p.SystemOCICache()) require.NoError(t, err) - m := newTestManager(p) - m.ociClient = client - m.queue = queue.New(1) + m := &manager{ + paths: p, + ociClient: client, + queue: queue.New(1), + readySubscribers: make(map[string][]chan StatusEvent), + } m.RecoverInterruptedBuilds() diff --git a/lib/images/testlayers_test.go b/lib/images/testlayers_test.go new file mode 100644 index 000000000..d56cf1015 --- /dev/null +++ b/lib/images/testlayers_test.go @@ -0,0 +1,52 @@ +package images + +import ( + "archive/tar" + "bytes" + "compress/gzip" + "io" + "testing" + + gcr "github.com/google/go-containerregistry/pkg/v1" + "github.com/google/go-containerregistry/pkg/v1/tarball" + "github.com/stretchr/testify/require" +) + +type tarEntrySpec struct { + name string + content string + isDir bool + mode int64 +} + +// specLayer builds a gzipped tar layer from entry specs in order. +func specLayer(t *testing.T, entries []tarEntrySpec) gcr.Layer { + t.Helper() + + var buf bytes.Buffer + gzw := gzip.NewWriter(&buf) + tw := tar.NewWriter(gzw) + for _, entry := range entries { + if entry.isDir { + require.NoError(t, tw.WriteHeader(&tar.Header{Name: entry.name, Typeflag: tar.TypeDir, Mode: entry.mode})) + continue + } + require.NoError(t, tw.WriteHeader(&tar.Header{ + Name: entry.name, + Typeflag: tar.TypeReg, + Mode: entry.mode, + Size: int64(len(entry.content)), + })) + _, err := tw.Write([]byte(entry.content)) + require.NoError(t, err) + } + require.NoError(t, tw.Close()) + require.NoError(t, gzw.Close()) + + data := buf.Bytes() + layer, err := tarball.LayerFromOpener(func() (io.ReadCloser, error) { + return io.NopCloser(bytes.NewReader(data)), nil + }) + require.NoError(t, err) + return layer +} diff --git a/lib/images/testutil/testutil.go b/lib/images/testutil/testutil.go new file mode 100644 index 000000000..aa28cd1e4 --- /dev/null +++ b/lib/images/testutil/testutil.go @@ -0,0 +1,107 @@ +// Package testutil provides helpers for seeding on-disk image state in tests. +package testutil + +import ( + "encoding/json" + "fmt" + "os" + "path/filepath" + "testing" + "time" + + "github.com/kernel/hypeman/lib/images" + "github.com/kernel/hypeman/lib/paths" + "github.com/stretchr/testify/require" +) + +const seedImageContent = "rootfs!" + +// Seed describes a ready image to seed directly to disk, bypassing pulls, +// builds, and conversion. +type Seed struct { + Repository string + DigestHex string + // Tag optionally creates a tag symlink pointing at the image. + Tag string + // Name overrides the metadata "name" field; it defaults to + // repository:tag, or repository@sha256:digest when Tag is empty. + Name string + // Tags records resource tags in the metadata. + Tags map[string]string + // Content writes the image into the shared content layout instead of + // the legacy per-repository digest layout. + Content bool +} + +type imageMetadata struct { + Name string `json:"name"` + Digest string `json:"digest"` + Status string `json:"status"` + SizeBytes int64 `json:"size_bytes"` + Tags map[string]string `json:"tags,omitempty"` + CreatedAt time.Time `json:"created_at"` +} + +// SeedReadyImage writes a ready image to disk per s. +func SeedReadyImage(t testing.TB, p *paths.Paths, s Seed) { + t.Helper() + require.NoError(t, seedImage(p, s)) +} + +func seedImage(p *paths.Paths, s Seed) error { + dir := p.ImageDigestDir(s.Repository, s.DigestHex) + disk := p.ImageDigestPath(s.Repository, s.DigestHex) + metadata := p.ImageMetadata(s.Repository, s.DigestHex) + linkPath := p.ImageTagSymlink(s.Repository, s.Tag) + target := s.DigestHex + if s.Content { + dir = p.ImageContentDir(s.DigestHex) + disk = p.ImageContentPath(s.DigestHex) + metadata = p.ImageContentMetadata(s.DigestHex) + linkPath = p.ImageRepositoryTagSymlink(s.Repository, s.Tag) + rel, err := filepath.Rel(filepath.Dir(linkPath), p.ImageContentDir(s.DigestHex)) + if err != nil { + return fmt.Errorf("rel content symlink target: %w", err) + } + target = rel + } + + if err := os.MkdirAll(dir, 0o755); err != nil { + return fmt.Errorf("create image dir: %w", err) + } + if err := os.WriteFile(disk, []byte(seedImageContent), 0o644); err != nil { + return fmt.Errorf("write disk image: %w", err) + } + + name := s.Name + if name == "" && s.Tag != "" { + name = s.Repository + ":" + s.Tag + } else if name == "" { + name = s.Repository + "@sha256:" + s.DigestHex + } + data, err := json.Marshal(imageMetadata{ + Name: name, + Digest: "sha256:" + s.DigestHex, + Status: images.StatusReady, + SizeBytes: int64(len(seedImageContent)), + Tags: s.Tags, + CreatedAt: time.Now().UTC(), + }) + if err != nil { + return fmt.Errorf("marshal metadata: %w", err) + } + if err := os.WriteFile(metadata, data, 0o644); err != nil { + return fmt.Errorf("write metadata: %w", err) + } + + if s.Tag == "" { + return nil + } + if err := os.MkdirAll(filepath.Dir(linkPath), 0o755); err != nil { + return fmt.Errorf("create tag dir: %w", err) + } + if err := os.Symlink(target, linkPath); err != nil { + return fmt.Errorf("create tag symlink: %w", err) + } + return nil +} diff --git a/lib/instances/create_mdev_test.go b/lib/instances/create_mdev_test.go index 4943303ab..05b3e9d8c 100644 --- a/lib/instances/create_mdev_test.go +++ b/lib/instances/create_mdev_test.go @@ -45,7 +45,7 @@ func TestCreateInstanceRejectsUnsupportedVGPUBeforeResourceReservation(t *testin assert.Zero(t, validator.reserveCalls) } -func TestWrapCreateMdevErr(t *testing.T) { +func TestWrapCreateVGPUErr(t *testing.T) { t.Parallel() for _, tc := range []struct { @@ -61,7 +61,7 @@ func TestWrapCreateMdevErr(t *testing.T) { wantInvalidRequest: true, }, { - name: "other mdev error", + name: "other vGPU error", err: errors.New("boom"), wantMessage: "create vGPU for profile profile: boom", }, @@ -69,7 +69,7 @@ func TestWrapCreateMdevErr(t *testing.T) { t.Run(tc.name, func(t *testing.T) { t.Parallel() - err := wrapCreateMdevErr("profile", tc.err) + err := wrapCreateVGPUErr("profile", tc.err) assert.ErrorIs(t, err, tc.err) if tc.wantInvalidRequest { diff --git a/lib/instances/delete_test.go b/lib/instances/delete_test.go index d13beeb5d..20a14d8c3 100644 --- a/lib/instances/delete_test.go +++ b/lib/instances/delete_test.go @@ -2,6 +2,7 @@ package instances import ( "os/exec" + "syscall" "testing" "time" @@ -18,6 +19,15 @@ func TestWaitForProcessExit_ReapsZombieChild(t *testing.T) { require.True(t, exited, "zombie child should be detected/reaped as exited") } +func TestWaitForProcessExit_EPERMProcessIsAlive(t *testing.T) { + t.Parallel() + if syscall.Kill(1, 0) == nil { + t.Skip("running as root") + } + + assert.False(t, WaitForProcessExit(1, 100*time.Millisecond)) +} + func TestWaitForProcessExit_TimesOutForRunningProcess(t *testing.T) { t.Parallel() cmd := exec.Command("sleep", "2") diff --git a/lib/instances/fork.go b/lib/instances/fork.go index fc4ed782e..4c0046c7e 100644 --- a/lib/instances/fork.go +++ b/lib/instances/fork.go @@ -298,6 +298,11 @@ func (m *manager) forkInstanceFromStoppedOrStandby(ctx context.Context, id strin // phase (Standby for snapshot forks, Stopped for stopped forks) will be // recorded by the appropriate operation when the fork is acted on. forkMeta.Phases.Reset() + // A vGPU assignment is never shared with a fork: normally stop already + // released it, and an assignment retained by a failed release must stay + // with the source so only one instance retries it. The fork acquires its + // own vGPU on start from GPUProfile. + clearStoredVGPUDevice(&forkMeta) switch source.State { case StateStandby: forkMeta.Phases.Record(phasetracking.PhaseStandby, now) diff --git a/lib/instances/guestmemory_linux_test.go b/lib/instances/guestmemory_linux_test.go index 224a74cbd..87a40992e 100644 --- a/lib/instances/guestmemory_linux_test.go +++ b/lib/instances/guestmemory_linux_test.go @@ -211,7 +211,7 @@ func requireHypervisorPID(t *testing.T, ctx context.Context, mgr *manager, insta t.Helper() inst, err := mgr.GetInstance(ctx, instanceID) require.NoError(t, err) - if inst.HypervisorPID != nil && processExists(*inst.HypervisorPID) { + if inst.HypervisorPID != nil && ProcessExists(*inst.HypervisorPID) { return *inst.HypervisorPID } if pid, err := hypervisor.ResolveProcessPID(inst.SocketPath); err == nil { diff --git a/lib/instances/manager.go b/lib/instances/manager.go index f972041c6..de5cbf8ed 100644 --- a/lib/instances/manager.go +++ b/lib/instances/manager.go @@ -662,19 +662,6 @@ func (m *manager) StopInstance(ctx context.Context, id string) (*Instance, error return nil, err } if current.State == StateStopped { - if current.GPUMdevUUID != "" { - if err := devices.DestroyMdev(ctx, current.GPUMdevUUID); err != nil { - return nil, fmt.Errorf("destroy retained mdev %s: %w", current.GPUMdevUUID, err) - } - meta, err := m.loadMetadata(id) - if err != nil { - return nil, err - } - meta.GPUMdevUUID = "" - if err := m.saveMetadata(meta); err != nil { - return nil, fmt.Errorf("save metadata after mdev cleanup: %w", err) - } - } if err := m.markRestartManualStopLocked(ctx, id); err != nil { return nil, err } diff --git a/lib/instances/manager_test.go b/lib/instances/manager_test.go index 4964363ea..6593edca4 100644 --- a/lib/instances/manager_test.go +++ b/lib/instances/manager_test.go @@ -165,7 +165,10 @@ func waitForInstanceState(ctx context.Context, mgr Manager, instanceID string, e return nil, fmt.Errorf("instance %s did not reach %s within %v (last state: %s)", instanceID, expected, timeout, lastState) } -// deleteInstanceEventually retries deletion while a hypervisor finishes dying. +// deleteInstanceEventually deletes an instance, retrying while the hypervisor +// finishes dying. Delete fails closed when the VMM has not exited within its +// short post-SIGKILL wait; on loaded CI hosts kernel-side teardown can outlast +// that wait, and the contract is that a retried delete converges. func deleteInstanceEventually(t *testing.T, ctx context.Context, mgr Manager, instanceID string) { t.Helper() deadline := time.Now().Add(integrationTestTimeout(30 * time.Second)) @@ -1663,8 +1666,7 @@ func TestStandbyAndRestore(t *testing.T) { // Cleanup (no sleep needed - DeleteInstance handles process cleanup) t.Log("Cleaning up...") - err = manager.DeleteInstance(ctx, inst.Id) - require.NoError(t, err) + deleteInstanceEventually(t, ctx, manager, inst.Id) t.Log("Standby/restore test complete!") } diff --git a/lib/instances/network_test.go b/lib/instances/network_test.go index 0c25455f0..67cf7b0f9 100644 --- a/lib/instances/network_test.go +++ b/lib/instances/network_test.go @@ -296,8 +296,7 @@ func TestCreateInstanceWithNetwork(t *testing.T) { // Cleanup t.Log("Cleaning up instance...") - err = manager.DeleteInstance(ctx, inst.Id) - require.NoError(t, err) + deleteInstanceEventually(t, ctx, manager, inst.Id) // Verify TAP deleted after instance cleanup t.Log("Verifying TAP deleted after cleanup...") diff --git a/lib/instances/qemu_lifecycle_test.go b/lib/instances/qemu_lifecycle_test.go index 114c76390..ec7da113a 100644 --- a/lib/instances/qemu_lifecycle_test.go +++ b/lib/instances/qemu_lifecycle_test.go @@ -154,8 +154,7 @@ func runQEMUStandbyAndRestore(t *testing.T, hypervisorType hypervisor.Type, inst // Cleanup t.Log("Cleaning up...") - err = manager.DeleteInstance(ctx, inst.Id) - require.NoError(t, err) + deleteInstanceEventually(t, ctx, manager, inst.Id) // Verify cleanup assert.NoDirExists(t, p.InstanceDir(inst.Id)) diff --git a/lib/instances/query.go b/lib/instances/query.go index f54b1600c..0621460da 100644 --- a/lib/instances/query.go +++ b/lib/instances/query.go @@ -569,10 +569,6 @@ func (m *manager) toInstanceWithStateDerivation(ctx context.Context, meta *metad return inst } -func processExists(pid int) bool { - return ProcessExists(pid) -} - // parseExitSentinel reads the last lines of the serial console log to find the // HYPEMAN-EXIT sentinel written by init before shutdown. // Returns the exit code, message, and whether a sentinel was found. diff --git a/lib/instances/restore.go b/lib/instances/restore.go index 8f302251b..ab27903ba 100644 --- a/lib/instances/restore.go +++ b/lib/instances/restore.go @@ -298,7 +298,8 @@ func (m *manager) restoreInstance( attribute.String("operation", "restore_from_snapshot"), ) log.InfoContext(ctx, "restoring from snapshot", "instance_id", id, "snapshot_dir", snapshotDir, "hypervisor", stored.HypervisorType) - pid, hv, err := m.restoreFromSnapshot(restoreCtx, stored, snapshotDir, restoreOptions) + // restoreFromSnapshot records the hypervisor process identity on stored. + _, hv, err := m.restoreFromSnapshot(restoreCtx, stored, snapshotDir, restoreOptions) restoreSpanEnd(err) if err != nil { log.ErrorContext(ctx, "failed to restore from snapshot", "instance_id", id, "error", err) @@ -308,9 +309,6 @@ func (m *manager) restoreInstance( return nil, err } - // Store the process identity for later cleanup. - stored.HypervisorProcessIdentity.Set(pid) - // 6. Transition: Paused → Running (resume) resumeCtx, resumeSpanEnd := m.startLifecycleStep(ctx, "resume_vm", attribute.String("instance_id", id), diff --git a/lib/instances/snapshot_test.go b/lib/instances/snapshot_test.go index 917f88d46..b23e364ee 100644 --- a/lib/instances/snapshot_test.go +++ b/lib/instances/snapshot_test.go @@ -8,6 +8,7 @@ import ( "testing" "time" + "github.com/kernel/hypeman/lib/devices" "github.com/kernel/hypeman/lib/hypervisor" "github.com/kernel/hypeman/lib/images" snapshotstore "github.com/kernel/hypeman/lib/snapshot" @@ -15,6 +16,118 @@ import ( "github.com/stretchr/testify/require" ) +func TestForkSnapshotClearsVGPUAssignment(t *testing.T) { + mgr, _ := setupTestManager(t) + ctx := context.Background() + + sourceID := "snapshot-vgpu-source" + createStoppedSnapshotSourceFixture(t, mgr, sourceID, sourceID, mgr.defaultHypervisor) + + meta, err := mgr.loadMetadata(sourceID) + require.NoError(t, err) + meta.GPUProfile = "NVIDIA L40S-2Q" + meta.GPUFramework = devices.VGPUFramework("future-framework") + meta.GPUDevicePath = "/sys/bus/pci/devices/0000:82:00.4" + meta.GPUMdevUUID = "retained-uuid" + require.NoError(t, mgr.saveMetadata(meta)) + + snapshot, err := mgr.CreateSnapshot(ctx, sourceID, CreateSnapshotRequest{ + Kind: SnapshotKindStopped, + Name: "snapshot-vgpu", + }) + require.NoError(t, err) + + forked, err := mgr.ForkSnapshot(ctx, snapshot.Id, ForkSnapshotRequest{ + Name: "snapshot-vgpu-fork", + TargetState: StateStopped, + }) + require.NoError(t, err) + assert.Equal(t, "NVIDIA L40S-2Q", forked.GPUProfile) + assert.Equal(t, devices.VGPUFrameworkNone, forked.GPUFramework) + assert.Empty(t, forked.GPUDevicePath) + assert.Empty(t, forked.GPUMdevUUID) + + source, err := mgr.loadMetadata(sourceID) + require.NoError(t, err) + assert.Equal(t, "/sys/bus/pci/devices/0000:82:00.4", source.GPUDevicePath) +} + +func TestRestoreSnapshotDoesNotResurrectStaleVGPUAssignment(t *testing.T) { + mgr, _ := setupTestManager(t) + ctx := context.Background() + + sourceID := "snapshot-vgpu-restore-stale" + createStoppedSnapshotSourceFixture(t, mgr, sourceID, sourceID, mgr.defaultHypervisor) + + meta, err := mgr.loadMetadata(sourceID) + require.NoError(t, err) + meta.GPUProfile = "NVIDIA L40S-2Q" + meta.GPUFramework = devices.VGPUFramework("future-framework") + meta.GPUDevicePath = "/sys/bus/pci/devices/0000:82:00.4" + meta.GPUMdevUUID = "retained-uuid" + require.NoError(t, mgr.saveMetadata(meta)) + + snapshot, err := mgr.CreateSnapshot(ctx, sourceID, CreateSnapshotRequest{ + Kind: SnapshotKindStopped, + Name: "snapshot-vgpu-restore-stale", + }) + require.NoError(t, err) + + // The retained assignment is released successfully after the snapshot + // was taken; a restore must not resurrect the snapshot's embedded copy. + meta, err = mgr.loadMetadata(sourceID) + require.NoError(t, err) + clearStoredVGPUDevice(&meta.StoredMetadata) + require.NoError(t, mgr.saveMetadata(meta)) + + _, err = mgr.RestoreSnapshot(ctx, sourceID, snapshot.Id, RestoreSnapshotRequest{ + TargetState: StateStopped, + TargetHypervisor: mgr.defaultHypervisor, + }) + require.NoError(t, err) + + restored, err := mgr.loadMetadata(sourceID) + require.NoError(t, err) + assert.Equal(t, devices.VGPUFrameworkNone, restored.GPUFramework) + assert.Empty(t, restored.GPUDevicePath) + assert.Empty(t, restored.GPUMdevUUID) +} + +func TestRestoreSnapshotKeepsCurrentVGPUAssignment(t *testing.T) { + mgr, _ := setupTestManager(t) + ctx := context.Background() + + sourceID := "snapshot-vgpu-restore-retained" + createStoppedSnapshotSourceFixture(t, mgr, sourceID, sourceID, mgr.defaultHypervisor) + + snapshot, err := mgr.CreateSnapshot(ctx, sourceID, CreateSnapshotRequest{ + Kind: SnapshotKindStopped, + Name: "snapshot-vgpu-restore-retained", + }) + require.NoError(t, err) + + // An assignment retained after the snapshot was taken (e.g. from a + // failed release on stop) must survive the restore for the next retry. + meta, err := mgr.loadMetadata(sourceID) + require.NoError(t, err) + meta.GPUFramework = devices.VGPUFramework("future-framework") + meta.GPUDevicePath = "/sys/bus/pci/devices/0000:82:00.4" + meta.GPUMdevUUID = "retained-uuid" + require.NoError(t, mgr.saveMetadata(meta)) + + _, err = mgr.RestoreSnapshot(ctx, sourceID, snapshot.Id, RestoreSnapshotRequest{ + TargetState: StateStopped, + TargetHypervisor: mgr.defaultHypervisor, + }) + require.NoError(t, err) + + restored, err := mgr.loadMetadata(sourceID) + require.NoError(t, err) + assert.Equal(t, devices.VGPUFramework("future-framework"), restored.GPUFramework) + assert.Equal(t, "/sys/bus/pci/devices/0000:82:00.4", restored.GPUDevicePath) + assert.Equal(t, "retained-uuid", restored.GPUMdevUUID) +} + func TestStoppedSnapshotLifecycleAndForkAfterSourceDeletion(t *testing.T) { t.Parallel() mgr, _ := setupTestManager(t) diff --git a/lib/instances/start.go b/lib/instances/start.go index 8ad5785cc..b162ad90f 100644 --- a/lib/instances/start.go +++ b/lib/instances/start.go @@ -62,18 +62,6 @@ func (m *manager) startInstance( rollbackMeta := *meta rollbackMeta.Phases = meta.Phases.Clone() - // A failed stop can retain an mdev UUID. Release it before creating the - // replacement and persist the cleared metadata before start can fail again. - if stored.GPUMdevUUID != "" { - if err := devices.DestroyMdev(ctx, stored.GPUMdevUUID); err != nil { - return nil, fmt.Errorf("destroy retained mdev before start: %w", err) - } - stored.GPUMdevUUID = "" - if err := m.saveMetadata(meta); err != nil { - return nil, fmt.Errorf("save metadata after mdev cleanup: %w", err) - } - } - // 2a. Clear stale exit info from previous run and apply command overrides stored.ExitCode = nil stored.ExitMessage = "" diff --git a/lib/instances/version_upgrade_test.go b/lib/instances/version_upgrade_test.go index 9e078d533..3b93eb54f 100644 --- a/lib/instances/version_upgrade_test.go +++ b/lib/instances/version_upgrade_test.go @@ -134,8 +134,8 @@ func TestCloudHypervisorVersionUpgradeRestore(t *testing.T) { // Cleanup t.Log("Cleaning up...") - require.NoError(t, mgr.DeleteInstance(ctx, inst.Id)) - require.NoError(t, mgr.DeleteInstance(ctx, inst2.Id)) + deleteInstanceEventually(t, ctx, mgr, inst.Id) + deleteInstanceEventually(t, ctx, mgr, inst2.Id) t.Log("Version upgrade restore test complete!") } diff --git a/lib/instances/vm_config_validation.go b/lib/instances/vm_config_validation.go index fe1530b54..61639e9b4 100644 --- a/lib/instances/vm_config_validation.go +++ b/lib/instances/vm_config_validation.go @@ -6,7 +6,10 @@ import ( "github.com/kernel/hypeman/lib/hypervisor" ) -const baseInstanceDiskCount = 3 // rootfs, writable overlay, and config disk +const ( + baseInstanceDiskCount = 3 // rootfs, writable overlay, and config disk + plannedVGPUDevicePath = "planned-vgpu-device" +) func instanceDiskCount(volumes []VolumeAttachment) int { count := baseInstanceDiskCount @@ -22,15 +25,13 @@ func instanceDiskCount(volumes []VolumeAttachment) int { // validateCreateVMConfig performs side-effect-free backend validation against // the complete device plan before image, PCI, network, or filesystem work. func (m *manager) validateCreateVMConfig(starter hypervisor.VMStarter, req CreateInstanceRequest, hvType hypervisor.Type) error { - pciDeviceCount := len(req.Devices) - if req.GPU != nil && req.GPU.Profile != "" { - pciDeviceCount++ - } + hasVGPU := req.GPU != nil && req.GPU.Profile != "" return validatePlannedVMConfig(starter, hvType, m.plannedVMConfig( req.HotplugSize, req.Volumes, req.NetworkEnabled, - pciDeviceCount, + len(req.Devices), + hasVGPU, )) } @@ -41,15 +42,13 @@ func (m *manager) validateStoredVMConfig(starter hypervisor.VMStarter, snapshotK } func (m *manager) plannedStoredVMConfig(snapshotKind SnapshotKind, meta StoredMetadata) hypervisor.VMConfig { - pciDeviceCount := len(meta.Devices) - if meta.GPUMdevUUID != "" || meta.GPUProfile != "" { - pciDeviceCount++ - } + hasVGPU := storedVGPUDevicePath(&meta) != "" || meta.GPUProfile != "" config := m.plannedVMConfig( meta.HotplugSize, meta.Volumes, meta.NetworkEnabled, - pciDeviceCount, + len(meta.Devices), + hasVGPU, ) if snapshotKind == SnapshotKindStandby { // Standby restore/fork reuses the frozen snapshot device model, so live @@ -64,6 +63,7 @@ func (m *manager) plannedVMConfig( volumes []VolumeAttachment, networkEnabled bool, pciDeviceCount int, + hasVGPU bool, ) hypervisor.VMConfig { diskCount := instanceDiskCount(volumes) @@ -77,6 +77,9 @@ func (m *manager) plannedVMConfig( if networkEnabled { config.Networks = []hypervisor.NetworkConfig{{}} } + if hasVGPU { + config.VGPUDevicePath = plannedVGPUDevicePath + } return config } diff --git a/lib/instances/vm_config_validation_test.go b/lib/instances/vm_config_validation_test.go index c7037e6ac..d25456722 100644 --- a/lib/instances/vm_config_validation_test.go +++ b/lib/instances/vm_config_validation_test.go @@ -17,21 +17,34 @@ func TestPlannedVMConfig(t *testing.T) { []VolumeAttachment{{Overlay: true}, {Overlay: false}}, true, 2, + true, ) assert.Equal(t, int64(1024), config.HotplugBytes) require.Len(t, config.Disks, 6, "three instance disks plus two overlay-volume disks plus one plain volume") require.Len(t, config.Networks, 1) require.Len(t, config.PCIDevices, 2) + assert.Equal(t, plannedVGPUDevicePath, config.VGPUDevicePath) assert.Equal(t, int64(3), config.VsockCID) } func TestPlannedVMConfigWithoutOptionalDevices(t *testing.T) { t.Parallel() - config := (&manager{}).plannedVMConfig(0, nil, false, 0) + config := (&manager{}).plannedVMConfig(0, nil, false, 0, false) require.Len(t, config.Disks, baseInstanceDiskCount) assert.Empty(t, config.Networks) assert.Empty(t, config.PCIDevices) + assert.Empty(t, config.VGPUDevicePath) +} + +func TestPlannedStoredVMConfigSeparatesVGPUFromPCIDevices(t *testing.T) { + t.Parallel() + config := (&manager{}).plannedStoredVMConfig(SnapshotKindStopped, StoredMetadata{ + Devices: []string{"pci-device"}, + GPUProfile: "gpu-profile", + }) + require.Len(t, config.PCIDevices, 1) + assert.Equal(t, plannedVGPUDevicePath, config.VGPUDevicePath) } func TestPlannedStoredVMConfigStandbyIgnoresLiveBalloonPolicy(t *testing.T) { diff --git a/lib/middleware/resolve.go b/lib/middleware/resolve.go index 5eaa75d54..2658d3ddd 100644 --- a/lib/middleware/resolve.go +++ b/lib/middleware/resolve.go @@ -86,7 +86,8 @@ func ResolveResource(resolvers Resolvers, errResponder ErrorResponder) func(http // The tag route resolves the source in the handler, which returns a // specific 404 body for a missing source; don't intercept it with // the generic one. - if r.Method == http.MethodPost && strings.HasSuffix(path, "/tag") { + rctx := chi.RouteContext(ctx) + if r.Method == http.MethodPost && rctx != nil && rctx.RoutePattern() == "/images/{name}/tag" { next.ServeHTTP(w, r) return } diff --git a/lib/middleware/resolve_test.go b/lib/middleware/resolve_test.go index e9cfa477f..2f5a5924e 100644 --- a/lib/middleware/resolve_test.go +++ b/lib/middleware/resolve_test.go @@ -93,6 +93,41 @@ func TestResolveResource_URLDecodesImageName(t *testing.T) { } } +func TestResolveResource_ResolvesBuilderByID(t *testing.T) { + // Regression test: the path-dispatch switch must include a /builders/ + // case, otherwise the Builder resolver is never invoked and resolved + // builders are missing from request context (handlers then 500). + + resolver := &mockResolver{} + + errResponder := func(w http.ResponseWriter, err error, lookup string) { + w.WriteHeader(http.StatusNotFound) + } + + middleware := ResolveResource(Resolvers{ + Builder: resolver, + }, errResponder) + + r := chi.NewRouter() + r.With(middleware).Get("/builders/{id}", func(w http.ResponseWriter, r *http.Request) { + if GetResolvedBuilder[struct{}](r.Context()) == nil { + w.WriteHeader(http.StatusInternalServerError) + return + } + w.WriteHeader(http.StatusOK) + }) + + req := httptest.NewRequest(http.MethodGet, "/builders/bld_123", nil) + w := httptest.NewRecorder() + + r.ServeHTTP(w, req) + + require.Equal(t, http.StatusOK, w.Code, + "Expected resolved builder in context, got %d", w.Code) + assert.Equal(t, "bld_123", resolver.receivedName, + "Builder resolver was not invoked with the path ID") +} + func TestResolveResource_SkipsOnlyImageTagPosts(t *testing.T) { resolver := &mockResolver{} @@ -107,6 +142,9 @@ func TestResolveResource_SkipsOnlyImageTagPosts(t *testing.T) { r.With(middleware).Post("/images/{name}", func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) }) + r.With(middleware).Post("/images/{name}/metadata/tag", func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + }) t.Run("tag route bypasses resolver", func(t *testing.T) { resolver.receivedName = "" @@ -129,39 +167,15 @@ func TestResolveResource_SkipsOnlyImageTagPosts(t *testing.T) { assert.Equal(t, "alpine:latest", resolver.receivedName, "only the tag route should bypass image resolution") }) -} - -func TestResolveResource_ResolvesBuilderByID(t *testing.T) { - // Regression test: the path-dispatch switch must include a /builders/ - // case, otherwise the Builder resolver is never invoked and resolved - // builders are missing from request context (handlers then 500). - - resolver := &mockResolver{} - errResponder := func(w http.ResponseWriter, err error, lookup string) { - w.WriteHeader(http.StatusNotFound) - } - - middleware := ResolveResource(Resolvers{ - Builder: resolver, - }, errResponder) + t.Run("other tag-suffixed route resolves", func(t *testing.T) { + resolver.receivedName = "" + req := httptest.NewRequest(http.MethodPost, "/images/alpine:latest/metadata/tag", nil) + w := httptest.NewRecorder() + r.ServeHTTP(w, req) - r := chi.NewRouter() - r.With(middleware).Get("/builders/{id}", func(w http.ResponseWriter, r *http.Request) { - if GetResolvedBuilder[struct{}](r.Context()) == nil { - w.WriteHeader(http.StatusInternalServerError) - return - } - w.WriteHeader(http.StatusOK) + require.Equal(t, http.StatusOK, w.Code, "body: %s", w.Body.String()) + assert.Equal(t, "alpine:latest", resolver.receivedName, + "only the exact image tag route should bypass resolution") }) - - req := httptest.NewRequest(http.MethodGet, "/builders/bld_123", nil) - w := httptest.NewRecorder() - - r.ServeHTTP(w, req) - - require.Equal(t, http.StatusOK, w.Code, - "Expected resolved builder in context, got %d", w.Code) - assert.Equal(t, "bld_123", resolver.receivedName, - "Builder resolver was not invoked with the path ID") } diff --git a/lib/paths/paths.go b/lib/paths/paths.go index 81844c01f..9086242e6 100644 --- a/lib/paths/paths.go +++ b/lib/paths/paths.go @@ -189,11 +189,6 @@ func (p *Paths) ImageLayerDir(layerHex string) string { return filepath.Join(p.ImageLayersDir(), layerHex) } -// ImageLayerArtifact returns the path to the default materialized layer artifact. -func (p *Paths) ImageLayerArtifact(layerHex string) string { - return p.ImageLayerArtifactForFormat(layerHex, "erofs") -} - // ImageLayerArtifactForFormat returns the path to a materialized layer artifact. func (p *Paths) ImageLayerArtifactForFormat(layerHex, format string) string { return filepath.Join(p.ImageLayerDir(layerHex), "layer."+format) diff --git a/lib/resources/gpu.go b/lib/resources/gpu.go index fba449dca..6dfd537ae 100644 --- a/lib/resources/gpu.go +++ b/lib/resources/gpu.go @@ -49,7 +49,7 @@ func getVGPUStatus(ctx context.Context, framework devices.VGPUFramework, vfs []d usedSlots := 0 // Count used VFs (those with a vGPU assigned) for _, vf := range vfs { - if vf.HasMdev { + if vf.Allocated { usedSlots++ } } diff --git a/openapi.yaml b/openapi.yaml index c896bbbc1..3621c3e8c 100644 --- a/openapi.yaml +++ b/openapi.yaml @@ -2709,6 +2709,12 @@ paths: application/json: schema: $ref: "#/components/schemas/Error" + 401: + description: Unauthorized + content: + application/json: + schema: + $ref: "#/components/schemas/Error" 500: description: Internal server error content: From 2a7e118bffd135daceee7ec5975208c77d864203 Mon Sep 17 00:00:00 2001 From: chruffins <23645059+chruffins@users.noreply.github.com> Date: Thu, 3 Sep 2026 12:50:13 +0000 Subject: [PATCH 07/19] Include layer cache in image disk totals --- lib/images/manager.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/images/manager.go b/lib/images/manager.go index 428e143c5..f99b866f9 100644 --- a/lib/images/manager.go +++ b/lib/images/manager.go @@ -857,11 +857,11 @@ func (m *manager) deleteTaggedImage(repository, tag string) error { // TotalImageBytes returns the total size of all ready images on disk. func (m *manager) TotalImageBytes(ctx context.Context) (int64, error) { - readyImageBytes, _, err := m.getDiskUsageTotals() + readyImageBytes, cacheBytes, err := m.getDiskUsageTotals() if err != nil { return 0, err } - return readyImageBytes, nil + return readyImageBytes + cacheBytes, nil } // TotalOCICacheBytes returns the total size of the OCI and materialized layer caches. From 9be1b27c77d9b18d808f4f7614a8e4e5759bfba2 Mon Sep 17 00:00:00 2001 From: chruffins <23645059+chruffins@users.noreply.github.com> Date: Thu, 3 Sep 2026 13:54:23 +0000 Subject: [PATCH 08/19] Reconcile lifecycle with context-aware layer storage --- lib/images/layer_gc.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/lib/images/layer_gc.go b/lib/images/layer_gc.go index c6d611aa6..baf1215d6 100644 --- a/lib/images/layer_gc.go +++ b/lib/images/layer_gc.go @@ -124,14 +124,13 @@ func (m *manager) evictUnreferencedLayerArtifacts() { } // tryEvictLayerArtifact removes one unreferenced layer artifact if it is still -// stale and no build is materializing it. The per-digest lock is taken with +// stale and no build is materializing it. The layer-store lock is taken with // TryLock so eviction never blocks behind an in-flight conversion. func (m *manager) tryEvictLayerArtifact(digestHex, dirPath string, cutoff time.Time) (int64, bool) { - unlock, ok := m.layerLocks.tryLock(digestHex) - if !ok { + if !m.layerStoreMu.TryLock() { return 0, false } - defer unlock() + defer m.layerStoreMu.Unlock() // The candidate was selected outside the lock; re-check that a build has // not retained the digest and the artifact has not been rewritten since. From 7ca50a3bafe6bab0a67e884e7f22fba2142d7ed4 Mon Sep 17 00:00:00 2001 From: chruffins <23645059+chruffins@users.noreply.github.com> Date: Thu, 3 Sep 2026 14:06:27 +0000 Subject: [PATCH 09/19] Integrate layer materialization with lifecycle GC --- lib/images/layer_gc.go | 8 ++------ lib/images/manager.go | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 6 deletions(-) diff --git a/lib/images/layer_gc.go b/lib/images/layer_gc.go index baf1215d6..6392f19e3 100644 --- a/lib/images/layer_gc.go +++ b/lib/images/layer_gc.go @@ -124,13 +124,9 @@ func (m *manager) evictUnreferencedLayerArtifacts() { } // tryEvictLayerArtifact removes one unreferenced layer artifact if it is still -// stale and no build is materializing it. The layer-store lock is taken with -// TryLock so eviction never blocks behind an in-flight conversion. +// stale and no build is materializing it. Reconciliation holds createMu while +// scanning and eviction, so new builds cannot begin without being retained. func (m *manager) tryEvictLayerArtifact(digestHex, dirPath string, cutoff time.Time) (int64, bool) { - if !m.layerStoreMu.TryLock() { - return 0, false - } - defer m.layerStoreMu.Unlock() // The candidate was selected outside the lock; re-check that a build has // not retained the digest and the artifact has not been rewritten since. diff --git a/lib/images/manager.go b/lib/images/manager.go index f99b866f9..f6eed8294 100644 --- a/lib/images/manager.go +++ b/lib/images/manager.go @@ -77,6 +77,9 @@ type manager struct { queue *queue.Queue createMu sync.Mutex layerFlights singleflight.Group + layerRefMu sync.Mutex + inflightLayerRefs map[string]int + layerEvictionGrace time.Duration diskUsageMu sync.RWMutex tagGenerations map[string]uint64 requestedTags map[string]string // newest pull's digest per requested tag @@ -105,6 +108,8 @@ func NewManager(p *paths.Paths, maxConcurrentBuilds int, meter metric.Meter) (Ma ociClient: ociClient, queue: queue.New(maxConcurrentBuilds), inflightPulls: make(map[string]*inflightImagePull), + inflightLayerRefs: make(map[string]int), + layerEvictionGrace: layerEvictionGracePeriod, borrowedCredentialsTimeout: DefaultBorrowedCredentialsTimeout, readySubscribers: make(map[string][]chan StatusEvent), tagGenerations: make(map[string]uint64), @@ -494,6 +499,11 @@ func (m *manager) buildImage(ctx context.Context, ref *ResolvedRef, credentials } m.recordPullMetrics(ctx, "success") + if err := m.materializeLayerArtifacts(ctx, result); err != nil { + m.updateStatusByDigest(ref, StatusFailed, fmt.Errorf("materialize layers: %w", err), buildID) + return + } + // Check if this digest already exists and is ready (deduplication) if meta, err := readMetadata(m.paths, ref.Repository(), ref.DigestHex()); err == nil { if meta.Status == StatusReady { @@ -541,6 +551,35 @@ func (m *manager) buildImage(ctx context.Context, ref *ResolvedRef, credentials buildStatus = "success" } +func (m *manager) materializeLayerArtifacts(ctx context.Context, result *pullResult) error { + if result == nil || result.Manifest == nil { + return nil + } + for _, desc := range result.Manifest.Layers { + digestHex := strings.TrimPrefix(desc.Digest, "sha256:") + m.createMu.Lock() + m.layerRefMu.Lock() + m.inflightLayerRefs[digestHex]++ + m.layerRefMu.Unlock() + m.createMu.Unlock() + + _, err := m.materializeLayerArtifactContext(ctx, desc) + + m.createMu.Lock() + m.layerRefMu.Lock() + m.inflightLayerRefs[digestHex]-- + if m.inflightLayerRefs[digestHex] == 0 { + delete(m.inflightLayerRefs, digestHex) + } + m.layerRefMu.Unlock() + m.createMu.Unlock() + if err != nil { + return err + } + } + return nil +} + func (m *manager) finalizeImage(ref *ResolvedRef, result *pullResult, diskSize int64, buildID, diskTempPath string) error { m.createMu.Lock() defer m.createMu.Unlock() From 8d77b5c7aaa4aa6879143b60fce7e02881e8a418 Mon Sep 17 00:00:00 2001 From: chruffins <23645059+chruffins@users.noreply.github.com> Date: Thu, 3 Sep 2026 14:32:04 +0000 Subject: [PATCH 10/19] Use renamed materializeLayerArtifact --- lib/images/manager.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/images/manager.go b/lib/images/manager.go index f6eed8294..1ddc177db 100644 --- a/lib/images/manager.go +++ b/lib/images/manager.go @@ -563,7 +563,7 @@ func (m *manager) materializeLayerArtifacts(ctx context.Context, result *pullRes m.layerRefMu.Unlock() m.createMu.Unlock() - _, err := m.materializeLayerArtifactContext(ctx, desc) + _, err := m.materializeLayerArtifact(ctx, desc) m.createMu.Lock() m.layerRefMu.Lock() From ba892bc0f2dee9f8024434ceb93ea38e5d1bbdd2 Mon Sep 17 00:00:00 2001 From: chruffins <23645059+chruffins@users.noreply.github.com> Date: Fri, 4 Sep 2026 16:20:24 +0000 Subject: [PATCH 11/19] Adapt layer lifecycle to current composition validation Skip layer materialization for image formats without layer artifact support (cpio), and set the rootfs type and config media type that the manifest model validation now requires in the lifecycle test fixture. --- lib/images/lifecycle_test.go | 6 ++++-- lib/images/manager.go | 3 +++ 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/lib/images/lifecycle_test.go b/lib/images/lifecycle_test.go index 1519c618c..eb74960d1 100644 --- a/lib/images/lifecycle_test.go +++ b/lib/images/lifecycle_test.go @@ -193,9 +193,11 @@ func TestEvictionKeepsReferencedAndFreshArtifacts(t *testing.T) { model := &imageManifestModel{ SchemaVersion: manifestModelSchemaVersion, Digest: "sha256:" + referencedHex, + RootFSType: "layers", Config: manifestConfigRef{ - Digest: "sha256:" + strings.Repeat("c", 64), - DiffIDs: []string{"sha256:" + referencedHex}, + Digest: "sha256:" + strings.Repeat("c", 64), + MediaType: "application/vnd.oci.image.config.v1+json", + DiffIDs: []string{"sha256:" + referencedHex}, }, Layers: []layerDescriptor{{Digest: "sha256:" + referencedHex, DiffID: "sha256:" + referencedHex}}, } diff --git a/lib/images/manager.go b/lib/images/manager.go index 1ddc177db..06222a390 100644 --- a/lib/images/manager.go +++ b/lib/images/manager.go @@ -555,6 +555,9 @@ func (m *manager) materializeLayerArtifacts(ctx context.Context, result *pullRes if result == nil || result.Manifest == nil { return nil } + if layerArtifactFormat() == "" { + return nil + } for _, desc := range result.Manifest.Layers { digestHex := strings.TrimPrefix(desc.Digest, "sha256:") m.createMu.Lock() From 7296e44f771afaa5de685da05042deadb6e1d2da Mon Sep 17 00:00:00 2001 From: chruffins <23645059+chruffins@users.noreply.github.com> Date: Fri, 4 Sep 2026 17:10:35 +0000 Subject: [PATCH 12/19] Wire layer lifecycle reconciliation and fix review findings Invoke layer store reconciliation on image deletion and at manager startup so reference-protected eviction and stale temp-dir sweeping actually run; previously the GC had no production call sites. Hold in-flight layer references until the manifest model protecting them is durable in finalizeImage, closing a window where reconciliation could evict cache-hit layers a build was about to reference. Fix the remaining review findings: abort eviction when the manifest reference walk fails instead of evicting against a truncated set, use removePath so eviction handles read-only layer trees, batch in-flight reference registration per build, drop the redundant layerRefMu, delete the dead totalLayerArtifactBytes duplicate, sweep .tag-stage-* by walking the whole images tree, and revert TotalImageBytes to ready-only bytes so consumers summing it with TotalOCICacheBytes no longer double count the cache. --- lib/images/layer_gc.go | 108 ++++++++++++++++++++--------------- lib/images/lifecycle_test.go | 11 +++- lib/images/manager.go | 75 ++++++++++++++---------- lib/resources/resource.go | 2 +- 4 files changed, 116 insertions(+), 80 deletions(-) diff --git a/lib/images/layer_gc.go b/lib/images/layer_gc.go index 6392f19e3..ae18fe01a 100644 --- a/lib/images/layer_gc.go +++ b/lib/images/layer_gc.go @@ -21,7 +21,7 @@ const layerEvictionGracePeriod = 10 * time.Minute // currently referenced by in-flight builds. Layer artifacts in this set are // protected from eviction. Unreadable manifest models are skipped with a // warning so one corrupt record cannot disable eviction entirely. -func (m *manager) referencedLayerDigests() map[string]struct{} { +func (m *manager) referencedLayerDigests() (map[string]struct{}, error) { refs := m.inflightLayerRefSnapshot() contentRoot := filepath.Join(m.paths.ImagesDir(), "content") err := filepath.WalkDir(contentRoot, func(path string, entry fs.DirEntry, err error) error { @@ -29,6 +29,8 @@ func (m *manager) referencedLayerDigests() map[string]struct{} { if os.IsNotExist(err) { return nil } + // An incomplete walk means an incomplete reference set; the caller + // must not evict against it. return err } if entry.IsDir() || entry.Name() != "manifest.json" { @@ -49,16 +51,14 @@ func (m *manager) referencedLayerDigests() map[string]struct{} { return nil }) if err != nil && !os.IsNotExist(err) { - slog.Warn("failed to walk content manifests for layer eviction", "error", err) + return nil, fmt.Errorf("walk content manifests: %w", err) } - return refs + return refs, nil } // inflightLayerRefSnapshot returns the layer digests currently retained by -// in-flight builds. +// in-flight builds. Callers hold createMu, which guards the map. func (m *manager) inflightLayerRefSnapshot() map[string]struct{} { - m.layerRefMu.Lock() - defer m.layerRefMu.Unlock() refs := make(map[string]struct{}, len(m.inflightLayerRefs)) for digestHex := range m.inflightLayerRefs { refs[digestHex] = struct{}{} @@ -66,6 +66,36 @@ func (m *manager) inflightLayerRefSnapshot() map[string]struct{} { return refs } +// retainInflightLayers registers one in-flight reference per digest so +// reconciliation cannot evict layers a build is materializing. +func (m *manager) retainInflightLayers(digestHexes []string) { + m.createMu.Lock() + for _, digestHex := range digestHexes { + m.inflightLayerRefs[digestHex]++ + } + m.createMu.Unlock() +} + +// releaseInflightLayers drops the in-flight references taken by +// retainInflightLayers. +func (m *manager) releaseInflightLayers(digestHexes []string) { + m.createMu.Lock() + m.releaseInflightLayersLocked(digestHexes) + m.createMu.Unlock() +} + +// releaseInflightLayersLocked is releaseInflightLayers for callers already +// holding createMu, such as finalizeImage. +func (m *manager) releaseInflightLayersLocked(digestHexes []string) { + for _, digestHex := range digestHexes { + if m.inflightLayerRefs[digestHex] <= 1 { + delete(m.inflightLayerRefs, digestHex) + } else { + m.inflightLayerRefs[digestHex]-- + } + } +} + // reconcileLayerStore evicts unreferenced layer artifacts and refreshes the // cached disk usage totals so accounting reflects the removals. func (m *manager) reconcileLayerStore() { @@ -86,7 +116,13 @@ func (m *manager) reconcileLayerStoreLocked() { // manifest model references, deleting the digest directory entirely. Artifacts // newer than the grace period are kept so in-flight builds never lose work. func (m *manager) evictUnreferencedLayerArtifacts() { - refs := m.referencedLayerDigests() + refs, err := m.referencedLayerDigests() + if err != nil { + // Evicting against a truncated reference set would delete artifacts + // belonging to images the walk never reached. + slog.Warn("skipping layer eviction: incomplete reference scan", "error", err) + return + } layersDir := m.paths.ImageLayersDir() entries, err := os.ReadDir(layersDir) @@ -125,12 +161,10 @@ func (m *manager) evictUnreferencedLayerArtifacts() { // tryEvictLayerArtifact removes one unreferenced layer artifact if it is still // stale and no build is materializing it. Reconciliation holds createMu while -// scanning and eviction, so new builds cannot begin without being retained. +// scanning and eviction, so builds cannot register or drop references mid-pass +// and the inflight check below cannot flip between scan and removal. func (m *manager) tryEvictLayerArtifact(digestHex, dirPath string, cutoff time.Time) (int64, bool) { - - // The candidate was selected outside the lock; re-check that a build has - // not retained the digest and the artifact has not been rewritten since. - if _, referenced := m.inflightLayerRefSnapshot()[digestHex]; referenced { + if m.inflightLayerRefs[digestHex] > 0 { return 0, false } info, statErr := os.Stat(dirPath) @@ -141,7 +175,9 @@ func (m *manager) tryEvictLayerArtifact(digestHex, dirPath string, cutoff time.T if err != nil { slog.Warn("failed to measure layer artifact size", "digest", digestHex, "error", err) } - if err := os.RemoveAll(dirPath); err != nil { + // removePath clears read-only directories restored from layer metadata, + // which os.RemoveAll cannot unlink through. + if err := removePath(dirPath); err != nil { slog.Warn("failed to evict unreferenced layer artifact", "digest", digestHex, "error", err) return 0, false } @@ -150,12 +186,18 @@ func (m *manager) tryEvictLayerArtifact(digestHex, dirPath string, cutoff time.T // cleanStaleImageTempDirs removes temp directories left behind by builds that // were interrupted mid-install, mid-materialization, or mid-tag promotion. -// Only directories older than the grace period are removed so live builds are +// The walk covers the whole images tree: layer and content staging dirs plus +// .tag-stage-* dirs, which are created under images//. Only +// directories older than the grace period are removed so live builds are // never disturbed. +// +// This must stay a startup-only sweep: a staging dir's own mtime only moves +// when its direct children change, so a deep extraction running longer than +// the grace period can look stale while actively writing. A periodic sweep +// would need a heartbeat or a live-build registry first. func (m *manager) cleanStaleImageTempDirs() { roots := []string{ - m.paths.ImageLayersDir(), - filepath.Join(m.paths.ImagesDir(), "content"), + m.paths.ImagesDir(), } cutoff := time.Now().Add(-m.layerEvictionGrace) for _, root := range roots { @@ -175,7 +217,9 @@ func (m *manager) cleanStaleImageTempDirs() { } info, statErr := os.Stat(path) if statErr == nil && info.ModTime().Before(cutoff) { - _ = os.RemoveAll(path) + if err := os.RemoveAll(path); err != nil { + slog.Warn("failed to remove stale image temp dir", "dir", path, "error", err) + } } return fs.SkipDir }) @@ -184,33 +228,3 @@ func (m *manager) cleanStaleImageTempDirs() { } } } - -// totalLayerArtifactBytes sums the bytes held by materialized layer -// artifacts, matching what diskutilization.Collect counts for the same store. -func totalLayerArtifactBytes(layersDir string) (int64, error) { - var total int64 - err := filepath.WalkDir(layersDir, func(path string, entry fs.DirEntry, err error) error { - if err != nil { - if os.IsNotExist(err) { - return nil - } - return err - } - if entry.IsDir() { - return nil - } - if !strings.HasPrefix(entry.Name(), "layer.") { - return nil - } - info, statErr := entry.Info() - if statErr != nil { - return nil - } - total += info.Size() - return nil - }) - if err != nil && !os.IsNotExist(err) { - return 0, fmt.Errorf("walk layer artifacts: %w", err) - } - return total, nil -} diff --git a/lib/images/lifecycle_test.go b/lib/images/lifecycle_test.go index eb74960d1..2b55eaf57 100644 --- a/lib/images/lifecycle_test.go +++ b/lib/images/lifecycle_test.go @@ -142,7 +142,10 @@ func newLifecycleTestManager(p *paths.Paths) *manager { } } -func TestTotalImageBytesIncludesLayerArtifacts(t *testing.T) { +// TestLayerArtifactsCountedInOneBucket verifies the accounting contract: ready +// image bytes and the OCI+layer cache total stay disjoint, so consumers summing +// TotalImageBytes and TotalOCICacheBytes count layer bytes exactly once. +func TestLayerArtifactsCountedInOneBucket(t *testing.T) { p := paths.New(t.TempDir()) m := newLifecycleTestManager(p) @@ -157,7 +160,11 @@ func TestTotalImageBytesIncludesLayerArtifacts(t *testing.T) { totalBytes, err := m.TotalImageBytes(context.Background()) require.NoError(t, err) - require.Equal(t, readyBytes+cacheBytes, totalBytes) + require.Equal(t, readyBytes, totalBytes) + + cacheTotal, err := m.TotalOCICacheBytes(context.Background()) + require.NoError(t, err) + require.Equal(t, cacheBytes, cacheTotal) } func TestCleanStaleImageTempDirsRemovesOnlyOldDirectories(t *testing.T) { diff --git a/lib/images/manager.go b/lib/images/manager.go index 06222a390..7f67ebb05 100644 --- a/lib/images/manager.go +++ b/lib/images/manager.go @@ -77,7 +77,6 @@ type manager struct { queue *queue.Queue createMu sync.Mutex layerFlights singleflight.Group - layerRefMu sync.Mutex inflightLayerRefs map[string]int layerEvictionGrace time.Duration diskUsageMu sync.RWMutex @@ -126,6 +125,10 @@ func NewManager(p *paths.Paths, maxConcurrentBuilds int, meter metric.Meter) (Ma } m.RecoverInterruptedBuilds() + // Sweep temp dirs and evict layer orphans an unclean shutdown may have + // left behind. Age-gated, so builds re-enqueued above are untouched. + m.cleanStaleImageTempDirs() + m.reconcileLayerStore() // Keep legacy images readable in their existing layout and promote them only // when an operation needs shared content, such as a cross-repository tag. // Avoiding a startup-wide migration keeps startup bounded and independent of @@ -499,10 +502,22 @@ func (m *manager) buildImage(ctx context.Context, ref *ResolvedRef, credentials } m.recordPullMetrics(ctx, "success") - if err := m.materializeLayerArtifacts(ctx, result); err != nil { + materialized, err := m.materializeLayerArtifacts(ctx, result) + if err != nil { m.updateStatusByDigest(ref, StatusFailed, fmt.Errorf("materialize layers: %w", err), buildID) return } + releaseLayerRefs := func() {} + if len(materialized) > 0 { + // Hold the in-flight references until the manifest model protecting + // the layers is durable: on the cache-hit path the artifacts' mtimes + // are too old for the eviction grace period to cover the gap. + var releaseOnce sync.Once + releaseLayerRefs = func() { + releaseOnce.Do(func() { m.releaseInflightLayers(materialized) }) + } + defer releaseLayerRefs() + } // Check if this digest already exists and is ready (deduplication) if meta, err := readMetadata(m.paths, ref.Repository(), ref.DigestHex()); err == nil { @@ -551,36 +566,25 @@ func (m *manager) buildImage(ctx context.Context, ref *ResolvedRef, credentials buildStatus = "success" } -func (m *manager) materializeLayerArtifacts(ctx context.Context, result *pullResult) error { +func (m *manager) materializeLayerArtifacts(ctx context.Context, result *pullResult) ([]string, error) { if result == nil || result.Manifest == nil { - return nil + return nil, nil } if layerArtifactFormat() == "" { - return nil + return nil, nil } + digestHexes := make([]string, 0, len(result.Manifest.Layers)) for _, desc := range result.Manifest.Layers { - digestHex := strings.TrimPrefix(desc.Digest, "sha256:") - m.createMu.Lock() - m.layerRefMu.Lock() - m.inflightLayerRefs[digestHex]++ - m.layerRefMu.Unlock() - m.createMu.Unlock() - - _, err := m.materializeLayerArtifact(ctx, desc) - - m.createMu.Lock() - m.layerRefMu.Lock() - m.inflightLayerRefs[digestHex]-- - if m.inflightLayerRefs[digestHex] == 0 { - delete(m.inflightLayerRefs, digestHex) - } - m.layerRefMu.Unlock() - m.createMu.Unlock() - if err != nil { - return err + digestHexes = append(digestHexes, strings.TrimPrefix(desc.Digest, "sha256:")) + } + m.retainInflightLayers(digestHexes) + for _, desc := range result.Manifest.Layers { + if _, err := m.materializeLayerArtifact(ctx, desc); err != nil { + m.releaseInflightLayers(digestHexes) + return nil, err } } - return nil + return digestHexes, nil } func (m *manager) finalizeImage(ref *ResolvedRef, result *pullResult, diskSize int64, buildID, diskTempPath string) error { @@ -626,6 +630,15 @@ func (m *manager) finalizeImage(ref *ResolvedRef, result *pullResult, diskSize i return rollbackFinalization(layout, modelPath, diskInstalled, modelWritten, fmt.Errorf("write manifest model: %w", err)) } modelWritten = true + // The model now protects these layers on disk, so the in-flight refs + // can go while still holding createMu: a delete racing the ready + // notification then reconciles against durable state. The caller's + // deferred release is a no-op after this. + layerHexes := make([]string, 0, len(model.Layers)) + for _, desc := range model.Layers { + layerHexes = append(layerHexes, strings.TrimPrefix(desc.Digest, "sha256:")) + } + m.releaseInflightLayersLocked(layerHexes) } meta.Status = StatusReady @@ -859,7 +872,7 @@ func (m *manager) deleteDigestImage(repository, digestHex string) error { return err } m.clearRequestedDigest(digestHex) - m.refreshDiskUsageTotals() + m.reconcileLayerStoreLocked() return nil } @@ -893,17 +906,19 @@ func (m *manager) deleteTaggedImage(repository, tag string) error { if err := removeDigestIfUnreferenced(m.paths, repository, digestHex, true); err != nil { return fmt.Errorf("delete orphaned digest %s: %w", digestHex, err) } - m.refreshDiskUsageTotals() + m.reconcileLayerStoreLocked() return nil } -// TotalImageBytes returns the total size of all ready images on disk. +// TotalImageBytes returns the total size of all ready images on disk. Shared +// layer artifacts are accounted separately via TotalOCICacheBytes so the two +// totals can be summed without double-counting. func (m *manager) TotalImageBytes(ctx context.Context) (int64, error) { - readyImageBytes, cacheBytes, err := m.getDiskUsageTotals() + readyImageBytes, _, err := m.getDiskUsageTotals() if err != nil { return 0, err } - return readyImageBytes + cacheBytes, nil + return readyImageBytes, nil } // TotalOCICacheBytes returns the total size of the OCI and materialized layer caches. diff --git a/lib/resources/resource.go b/lib/resources/resource.go index 5943cb0bc..8af7e32d7 100644 --- a/lib/resources/resource.go +++ b/lib/resources/resource.go @@ -889,7 +889,7 @@ func (m *Manager) MaxImageStorageBytes() int64 { return int64(float64(capacity) * fraction) } -// CurrentImageStorageBytes returns the current image storage usage (OCI cache + rootfs). +// CurrentImageStorageBytes returns the current image storage usage (ready rootfs + OCI cache and shared layers). func (m *Manager) CurrentImageStorageBytes(ctx context.Context) (int64, error) { if m.imageLister == nil { return 0, nil From 5cd56a013e07cc46cc7153ae4ede3fff930c7944 Mon Sep 17 00:00:00 2001 From: chruffins <23645059+chruffins@users.noreply.github.com> Date: Sat, 5 Sep 2026 18:15:57 +0000 Subject: [PATCH 13/19] Fix double release of in-flight layer refs and legacy-model eviction finalizeImage released the in-flight layer refs and buildImage's deferred release fired again afterwards, so two builds sharing a layer could drop each other's protection mid-materialization; the release is now an idempotent handle shared by both sites, with finalization releasing under createMu before the ready notification. The eviction reference scan also missed manifest models written at legacy layout paths, so ready legacy-layout images could lose their layers; the scan now walks the whole images tree, which covers both layouts. Also make layer materialization best effort as documented (log and continue, since the rootfs is already composed from blobs), drop the redundant inflight snapshot helper and single-element roots slice, move the eviction metric behind the usual recorder, and reuse writeLayerTestLayout for the shared-layout test fixture. --- lib/images/layer_artifact_test.go | 18 +++-- lib/images/layer_gc.go | 124 ++++++++++++++++-------------- lib/images/lifecycle_test.go | 40 +++------- lib/images/manager.go | 51 ++++++------ lib/images/manager_test.go | 2 +- lib/images/metrics.go | 8 ++ 6 files changed, 124 insertions(+), 119 deletions(-) diff --git a/lib/images/layer_artifact_test.go b/lib/images/layer_artifact_test.go index 28e3cca7e..f0d7f6f0c 100644 --- a/lib/images/layer_artifact_test.go +++ b/lib/images/layer_artifact_test.go @@ -32,17 +32,19 @@ const whiteoutPrefix = ".wh." const testTarGzMediaType = "application/vnd.oci.image.layer.v1.tar+gzip" -// writeLayerTestLayout writes img into the shared OCI cache of p tagged with -// the image's digest, mirroring pullToOCILayout. -func writeLayerTestLayout(t *testing.T, p *paths.Paths, img gcr.Image) { +// writeLayerTestLayout writes images into the shared OCI cache of p tagged +// with each image's digest, mirroring pullToOCILayout. +func writeLayerTestLayout(t *testing.T, p *paths.Paths, imgs ...gcr.Image) { t.Helper() - digest, err := img.Digest() - require.NoError(t, err) layoutPath, err := layout.Write(p.SystemOCICache(), empty.Index) require.NoError(t, err) - require.NoError(t, layoutPath.AppendImage(img, layout.WithAnnotations(map[string]string{ - "org.opencontainers.image.ref.name": digestToLayoutTag(digest.String()), - }))) + for _, img := range imgs { + digest, err := img.Digest() + require.NoError(t, err) + require.NoError(t, layoutPath.AppendImage(img, layout.WithAnnotations(map[string]string{ + "org.opencontainers.image.ref.name": digestToLayoutTag(digest.String()), + }))) + } } func layerDescFromImage(t *testing.T, img gcr.Image, index int) layerDescriptor { diff --git a/lib/images/layer_gc.go b/lib/images/layer_gc.go index ae18fe01a..a6144ca38 100644 --- a/lib/images/layer_gc.go +++ b/lib/images/layer_gc.go @@ -8,6 +8,7 @@ import ( "os" "path/filepath" "strings" + "sync" "time" ) @@ -17,14 +18,21 @@ import ( const layerEvictionGracePeriod = 10 * time.Minute // referencedLayerDigests returns the set of layer blob digests referenced by -// the manifest models of every image in the content layout, plus the digests -// currently referenced by in-flight builds. Layer artifacts in this set are -// protected from eviction. Unreadable manifest models are skipped with a -// warning so one corrupt record cannot disable eviction entirely. +// the manifest models of every image in the images tree — both content and +// legacy layouts write their model as a manifest.json with the digest as its +// parent directory — plus the digests currently referenced by in-flight +// builds. Layer artifacts in this set are protected from eviction. Unreadable +// manifest models are skipped with a warning so one corrupt record cannot +// disable eviction entirely. +// +// Callers must hold createMu so the in-flight map read is ordered with model +// writes during finalization. func (m *manager) referencedLayerDigests() (map[string]struct{}, error) { - refs := m.inflightLayerRefSnapshot() - contentRoot := filepath.Join(m.paths.ImagesDir(), "content") - err := filepath.WalkDir(contentRoot, func(path string, entry fs.DirEntry, err error) error { + refs := make(map[string]struct{}, len(m.inflightLayerRefs)) + for digestHex := range m.inflightLayerRefs { + refs[digestHex] = struct{}{} + } + err := filepath.WalkDir(m.paths.ImagesDir(), func(path string, entry fs.DirEntry, err error) error { if err != nil { if os.IsNotExist(err) { return nil @@ -50,43 +58,43 @@ func (m *manager) referencedLayerDigests() (map[string]struct{}, error) { } return nil }) - if err != nil && !os.IsNotExist(err) { + if err != nil { return nil, fmt.Errorf("walk content manifests: %w", err) } return refs, nil } -// inflightLayerRefSnapshot returns the layer digests currently retained by -// in-flight builds. Callers hold createMu, which guards the map. -func (m *manager) inflightLayerRefSnapshot() map[string]struct{} { - refs := make(map[string]struct{}, len(m.inflightLayerRefs)) - for digestHex := range m.inflightLayerRefs { - refs[digestHex] = struct{}{} - } - return refs +// inflightLayerRef is the handle returned by retainInflightLayers. Its +// release is idempotent: finalization releases the refs as soon as the +// manifest model is durable, and the build's deferred release becomes a +// no-op afterwards. +type inflightLayerRef struct { + once sync.Once + digestHexes []string +} + +func (r *inflightLayerRef) release(m *manager) { + r.once.Do(func() { m.releaseInflightLayerRefsLocked(r.digestHexes) }) +} + +// releaseLocked is release for callers already holding createMu, such as +// finalizeImage. +func (r *inflightLayerRef) releaseLocked(m *manager) { + r.once.Do(func() { m.releaseInflightLayerRefsLocked(r.digestHexes) }) } // retainInflightLayers registers one in-flight reference per digest so // reconciliation cannot evict layers a build is materializing. -func (m *manager) retainInflightLayers(digestHexes []string) { +func (m *manager) retainInflightLayers(digestHexes []string) *inflightLayerRef { m.createMu.Lock() for _, digestHex := range digestHexes { m.inflightLayerRefs[digestHex]++ } m.createMu.Unlock() + return &inflightLayerRef{digestHexes: digestHexes} } -// releaseInflightLayers drops the in-flight references taken by -// retainInflightLayers. -func (m *manager) releaseInflightLayers(digestHexes []string) { - m.createMu.Lock() - m.releaseInflightLayersLocked(digestHexes) - m.createMu.Unlock() -} - -// releaseInflightLayersLocked is releaseInflightLayers for callers already -// holding createMu, such as finalizeImage. -func (m *manager) releaseInflightLayersLocked(digestHexes []string) { +func (m *manager) releaseInflightLayerRefsLocked(digestHexes []string) { for _, digestHex := range digestHexes { if m.inflightLayerRefs[digestHex] <= 1 { delete(m.inflightLayerRefs, digestHex) @@ -153,9 +161,7 @@ func (m *manager) evictUnreferencedLayerArtifacts() { } if evicted > 0 { slog.Info("evicted unreferenced layer artifacts", "count", evicted, "bytes", evictedBytes) - if m.metrics != nil { - m.metrics.layerArtifactsEvicted.Add(context.Background(), int64(evicted)) - } + m.recordLayerArtifactsEvicted(context.Background(), int64(evicted)) } } @@ -184,6 +190,17 @@ func (m *manager) tryEvictLayerArtifact(digestHex, dirPath string, cutoff time.T return size, true } +// isStaleTempDirName reports whether a directory name matches the temp +// prefixes builds use for staging, installs, and tag promotion. +func isStaleTempDirName(name string) bool { + for _, prefix := range []string{".unpack-", ".install-", ".tag-stage-"} { + if strings.HasPrefix(name, prefix) { + return true + } + } + return false +} + // cleanStaleImageTempDirs removes temp directories left behind by builds that // were interrupted mid-install, mid-materialization, or mid-tag promotion. // The walk covers the whole images tree: layer and content staging dirs plus @@ -196,35 +213,30 @@ func (m *manager) tryEvictLayerArtifact(digestHex, dirPath string, cutoff time.T // the grace period can look stale while actively writing. A periodic sweep // would need a heartbeat or a live-build registry first. func (m *manager) cleanStaleImageTempDirs() { - roots := []string{ - m.paths.ImagesDir(), - } cutoff := time.Now().Add(-m.layerEvictionGrace) - for _, root := range roots { - err := filepath.WalkDir(root, func(path string, entry fs.DirEntry, err error) error { - if err != nil { - if os.IsNotExist(err) { - return nil - } - return err - } - if !entry.IsDir() { - return nil - } - name := entry.Name() - if !strings.HasPrefix(name, ".unpack-") && !strings.HasPrefix(name, ".install-") && !strings.HasPrefix(name, ".tag-stage-") { + err := filepath.WalkDir(m.paths.ImagesDir(), func(path string, entry fs.DirEntry, err error) error { + if err != nil { + if os.IsNotExist(err) { return nil } - info, statErr := os.Stat(path) - if statErr == nil && info.ModTime().Before(cutoff) { - if err := os.RemoveAll(path); err != nil { - slog.Warn("failed to remove stale image temp dir", "dir", path, "error", err) - } + return err + } + if !entry.IsDir() { + return nil + } + name := entry.Name() + if !isStaleTempDirName(name) { + return nil + } + info, statErr := os.Stat(path) + if statErr == nil && info.ModTime().Before(cutoff) { + if err := os.RemoveAll(path); err != nil { + slog.Warn("failed to remove stale image temp dir", "dir", path, "error", err) } - return fs.SkipDir - }) - if err != nil && !os.IsNotExist(err) { - slog.Warn("failed to clean stale image temp dirs", "root", root, "error", err) } + return fs.SkipDir + }) + if err != nil { + slog.Warn("failed to clean stale image temp dirs", "root", m.paths.ImagesDir(), "error", err) } } diff --git a/lib/images/lifecycle_test.go b/lib/images/lifecycle_test.go index 2b55eaf57..7bc78b2f7 100644 --- a/lib/images/lifecycle_test.go +++ b/lib/images/lifecycle_test.go @@ -9,35 +9,13 @@ import ( "testing" "time" - gcr "github.com/google/go-containerregistry/pkg/v1" "github.com/google/go-containerregistry/pkg/v1/empty" - "github.com/google/go-containerregistry/pkg/v1/layout" "github.com/google/go-containerregistry/pkg/v1/mutate" "github.com/kernel/hypeman/lib/paths" "github.com/stretchr/testify/require" ) -// writeSharedLayout writes several images into one OCI layout cache, each -// annotated with its own digest tag. -func writeSharedLayout(t *testing.T, p *paths.Paths, imgs ...gcr.Image) []string { - t.Helper() - - layoutPath, err := layout.Write(p.SystemOCICache(), empty.Index) - require.NoError(t, err) - - digests := make([]string, 0, len(imgs)) - for _, img := range imgs { - digest, err := img.Digest() - require.NoError(t, err) - require.NoError(t, layoutPath.AppendImage(img, layout.WithAnnotations(map[string]string{ - "org.opencontainers.image.ref.name": digestToLayoutTag(digest.String()), - }))) - digests = append(digests, digest.String()) - } - return digests -} - -func layerHexes(t *testing.T, p *paths.Paths) map[string]struct{} { +func layerStoreHexes(t *testing.T, p *paths.Paths) map[string]struct{} { t.Helper() entries, err := os.ReadDir(p.ImageLayersDir()) require.NoError(t, err) @@ -74,8 +52,12 @@ func TestSharedLayersMaterializeOnceAndEvictWithReferences(t *testing.T) { imgB, err := mutate.AppendLayers(empty.Image, base, topB) require.NoError(t, err) - digests := writeSharedLayout(t, p, imgA, imgB) - digestA, digestB := digests[0], digests[1] + writeLayerTestLayout(t, p, imgA, imgB) + digestAHash, err := imgA.Digest() + require.NoError(t, err) + digestBHash, err := imgB.Digest() + require.NoError(t, err) + digestA, digestB := digestAHash.String(), digestBHash.String() baseManifest, err := imgA.Manifest() require.NoError(t, err) @@ -114,7 +96,7 @@ func TestSharedLayersMaterializeOnceAndEvictWithReferences(t *testing.T) { } // The shared base layer materialized exactly once, alongside the two tops. - hexes := layerHexes(t, p) + hexes := layerStoreHexes(t, p) require.Len(t, hexes, 3) require.Contains(t, hexes, baseHex) require.Contains(t, hexes, topAHex) @@ -122,7 +104,7 @@ func TestSharedLayersMaterializeOnceAndEvictWithReferences(t *testing.T) { // Deleting image A evicts only its unique layer; the shared base survives. require.NoError(t, m.DeleteImage(ctx, repoA+"@"+digestA)) - hexes = layerHexes(t, p) + hexes = layerStoreHexes(t, p) require.Len(t, hexes, 2) require.Contains(t, hexes, baseHex, "shared base must survive while referenced") require.Contains(t, hexes, topBHex) @@ -130,7 +112,7 @@ func TestSharedLayersMaterializeOnceAndEvictWithReferences(t *testing.T) { // Deleting image B removes the last references: everything is evicted. require.NoError(t, m.DeleteImage(ctx, repoB+"@"+digestB)) - hexes = layerHexes(t, p) + hexes = layerStoreHexes(t, p) require.Empty(t, hexes, "unreferenced layer artifacts must be evicted") } @@ -218,7 +200,7 @@ func TestEvictionKeepsReferencedAndFreshArtifacts(t *testing.T) { m.reconcileLayerStore() - hexes := layerHexes(t, p) + hexes := layerStoreHexes(t, p) require.Contains(t, hexes, referencedHex) require.Contains(t, hexes, orphanFreshHex) } diff --git a/lib/images/manager.go b/lib/images/manager.go index 7f67ebb05..c4bb46088 100644 --- a/lib/images/manager.go +++ b/lib/images/manager.go @@ -126,7 +126,10 @@ func NewManager(p *paths.Paths, maxConcurrentBuilds int, meter metric.Meter) (Ma m.RecoverInterruptedBuilds() // Sweep temp dirs and evict layer orphans an unclean shutdown may have - // left behind. Age-gated, so builds re-enqueued above are untouched. + // left behind. Recovered builds re-enqueued above run concurrently and + // hold in-flight references before materializing; artifacts from their + // previous attempts are unreferenced garbage and get evicted, to be + // re-materialized from the blob cache on demand. m.cleanStaleImageTempDirs() m.reconcileLayerStore() // Keep legacy images readable in their existing layout and promote them only @@ -503,20 +506,19 @@ func (m *manager) buildImage(ctx context.Context, ref *ResolvedRef, credentials m.recordPullMetrics(ctx, "success") materialized, err := m.materializeLayerArtifacts(ctx, result) - if err != nil { - m.updateStatusByDigest(ref, StatusFailed, fmt.Errorf("materialize layers: %w", err), buildID) - return - } - releaseLayerRefs := func() {} - if len(materialized) > 0 { + if materialized != nil { // Hold the in-flight references until the manifest model protecting // the layers is durable: on the cache-hit path the artifacts' mtimes // are too old for the eviction grace period to cover the gap. - var releaseOnce sync.Once - releaseLayerRefs = func() { - releaseOnce.Do(func() { m.releaseInflightLayers(materialized) }) - } - defer releaseLayerRefs() + // finalizeImage releases them under createMu; this defer only covers + // paths that never reach finalization. + defer materialized.release(m) + } + if err != nil { + // The rootfs is already composed from blobs, so materialization is + // best effort: log and continue with degraded sharing. + slog.Warn("layer materialization failed; continuing without shared artifacts", + "digest", ref.DigestHex(), "error", err) } // Check if this digest already exists and is ready (deduplication) @@ -553,7 +555,7 @@ func (m *manager) buildImage(ctx context.Context, ref *ResolvedRef, credentials } finalizeStart := time.Now() - err = m.finalizeImage(ref, result, diskSize, buildID, diskTempPath) + err = m.finalizeImage(ref, result, diskSize, buildID, diskTempPath, materialized) m.recordImageBuildPhase(ctx, ref.Digest(), "finalize", time.Since(finalizeStart), phaseStatus(err), "not_applicable") if err != nil { if errors.Is(err, errStaleBuild) { @@ -566,8 +568,8 @@ func (m *manager) buildImage(ctx context.Context, ref *ResolvedRef, credentials buildStatus = "success" } -func (m *manager) materializeLayerArtifacts(ctx context.Context, result *pullResult) ([]string, error) { - if result == nil || result.Manifest == nil { +func (m *manager) materializeLayerArtifacts(ctx context.Context, result *pullResult) (*inflightLayerRef, error) { + if result.Manifest == nil { return nil, nil } if layerArtifactFormat() == "" { @@ -577,17 +579,18 @@ func (m *manager) materializeLayerArtifacts(ctx context.Context, result *pullRes for _, desc := range result.Manifest.Layers { digestHexes = append(digestHexes, strings.TrimPrefix(desc.Digest, "sha256:")) } - m.retainInflightLayers(digestHexes) + handle := m.retainInflightLayers(digestHexes) for _, desc := range result.Manifest.Layers { if _, err := m.materializeLayerArtifact(ctx, desc); err != nil { - m.releaseInflightLayers(digestHexes) - return nil, err + // The handle's release is idempotent: the caller keeps the partial + // set protected until its path finishes with it. + return handle, err } } - return digestHexes, nil + return handle, nil } -func (m *manager) finalizeImage(ref *ResolvedRef, result *pullResult, diskSize int64, buildID, diskTempPath string) error { +func (m *manager) finalizeImage(ref *ResolvedRef, result *pullResult, diskSize int64, buildID, diskTempPath string, materialized *inflightLayerRef) error { m.createMu.Lock() defer m.createMu.Unlock() @@ -632,13 +635,11 @@ func (m *manager) finalizeImage(ref *ResolvedRef, result *pullResult, diskSize i modelWritten = true // The model now protects these layers on disk, so the in-flight refs // can go while still holding createMu: a delete racing the ready - // notification then reconciles against durable state. The caller's + // notification then reconciles against durable state. The build's // deferred release is a no-op after this. - layerHexes := make([]string, 0, len(model.Layers)) - for _, desc := range model.Layers { - layerHexes = append(layerHexes, strings.TrimPrefix(desc.Digest, "sha256:")) + if materialized != nil { + materialized.releaseLocked(m) } - m.releaseInflightLayersLocked(layerHexes) } meta.Status = StatusReady diff --git a/lib/images/manager_test.go b/lib/images/manager_test.go index 13a6835e2..6f24e6390 100644 --- a/lib/images/manager_test.go +++ b/lib/images/manager_test.go @@ -736,7 +736,7 @@ func TestDeleteAndRecreateDuringBuildTail(t *testing.T) { m.updateStatusByDigest(staleRef, StatusFailed, errors.New("stale build"), firstMeta.BuildID) staleBundle, err := m.ociClient.extractOCIImageBundle(digestHex) require.NoError(t, err) - require.ErrorIs(t, m.finalizeImage(staleRef, &pullResult{Metadata: staleBundle.Meta}, 1, firstMeta.BuildID, ""), errStaleBuild) + require.ErrorIs(t, m.finalizeImage(staleRef, &pullResult{Metadata: staleBundle.Meta}, 1, firstMeta.BuildID, "", nil), errStaleBuild) currentMeta, err = readMetadata(p, repo, digestHex) require.NoError(t, err) require.Equal(t, StatusPending, currentMeta.Status) diff --git a/lib/images/metrics.go b/lib/images/metrics.go index 75164a6cd..e58075905 100644 --- a/lib/images/metrics.go +++ b/lib/images/metrics.go @@ -179,3 +179,11 @@ func (m *manager) recordOCIImageMetrics(ctx context.Context, layerCount int, com m.metrics.ociLayerCount.Record(ctx, int64(layerCount), attrs) m.metrics.ociCompressedBytes.Record(ctx, compressedBytes, attrs) } + +// recordLayerArtifactsEvicted counts layer artifacts removed by eviction. +func (m *manager) recordLayerArtifactsEvicted(ctx context.Context, evicted int64) { + if m.metrics == nil { + return + } + m.metrics.layerArtifactsEvicted.Add(ctx, evicted) +} From 1899a2869d9d9e60e25048996dd7ef3c8614a02c Mon Sep 17 00:00:00 2001 From: chruffins <23645059+chruffins@users.noreply.github.com> Date: Sat, 5 Sep 2026 18:29:54 +0000 Subject: [PATCH 14/19] Guard releases with createMu and read legacy models during eviction The deferred layer-ref release mutated the in-flight map without createMu, racing retention and reconciliation reads; release now takes the lock while finalization keeps the already-locked variant. The eviction walk resolved every manifest model through the content-layout path, so legacy-layout models were skipped and their layers left unprotected; the walk now reads the model at the path it found. tryEvictLayerArtifact also drops its unreachable in-flight check, which the reference set already covers. --- lib/images/layer_gc.go | 19 ++++++++++--------- lib/images/manifest_model.go | 8 +++++++- 2 files changed, 17 insertions(+), 10 deletions(-) diff --git a/lib/images/layer_gc.go b/lib/images/layer_gc.go index a6144ca38..bf7bf808e 100644 --- a/lib/images/layer_gc.go +++ b/lib/images/layer_gc.go @@ -45,7 +45,7 @@ func (m *manager) referencedLayerDigests() (map[string]struct{}, error) { return nil } digestHex := filepath.Base(filepath.Dir(path)) - model, readErr := readManifestModel(m.paths, digestHex) + model, readErr := readManifestModelAt(path, digestHex) if readErr != nil { slog.Warn("skipping unreadable manifest model for layer eviction", "digest", digestHex, "error", readErr) return nil @@ -74,7 +74,11 @@ type inflightLayerRef struct { } func (r *inflightLayerRef) release(m *manager) { - r.once.Do(func() { m.releaseInflightLayerRefsLocked(r.digestHexes) }) + r.once.Do(func() { + m.createMu.Lock() + defer m.createMu.Unlock() + m.releaseInflightLayerRefsLocked(r.digestHexes) + }) } // releaseLocked is release for callers already holding createMu, such as @@ -165,14 +169,11 @@ func (m *manager) evictUnreferencedLayerArtifacts() { } } -// tryEvictLayerArtifact removes one unreferenced layer artifact if it is still -// stale and no build is materializing it. Reconciliation holds createMu while -// scanning and eviction, so builds cannot register or drop references mid-pass -// and the inflight check below cannot flip between scan and removal. +// tryEvictLayerArtifact removes one unreferenced layer artifact if it is +// stale. Reconciliation holds createMu while scanning and eviction, and the +// reference set already contains the in-flight digests, so no build can +// register or drop a reference mid-pass. func (m *manager) tryEvictLayerArtifact(digestHex, dirPath string, cutoff time.Time) (int64, bool) { - if m.inflightLayerRefs[digestHex] > 0 { - return 0, false - } info, statErr := os.Stat(dirPath) if statErr != nil || info.ModTime().After(cutoff) { return 0, false diff --git a/lib/images/manifest_model.go b/lib/images/manifest_model.go index 16a913ce4..5cdfd8f30 100644 --- a/lib/images/manifest_model.go +++ b/lib/images/manifest_model.go @@ -149,7 +149,13 @@ func writeManifestModelAt(path, digestHex string, model *imageManifestModel) err // Missing models return (nil, nil): images converted before the manifest model // existed only have a flattened rootfs. func readManifestModel(p *paths.Paths, digestHex string) (*imageManifestModel, error) { - data, err := os.ReadFile(p.ImageContentManifestModel(digestHex)) + return readManifestModelAt(p.ImageContentManifestModel(digestHex), digestHex) +} + +// readManifestModelAt loads the manifest model at path, if present. Missing +// files return (nil, nil). +func readManifestModelAt(path, digestHex string) (*imageManifestModel, error) { + data, err := os.ReadFile(path) if err != nil { if os.IsNotExist(err) { return nil, nil From 214256fd1f2df258e8fe9e9247d738fbd838cb1c Mon Sep 17 00:00:00 2001 From: chruffins <23645059+chruffins@users.noreply.github.com> Date: Sat, 5 Sep 2026 18:40:59 +0000 Subject: [PATCH 15/19] Reconcile layers on replaced-tag cleanup and tidy gc details cleanupReplacedTag now evicts the replaced image's unreferenced layers like the other delete paths instead of leaving them until the next delete or restart, the manifest-walk error names what it walks, and the compose doc no longer claims .compose-* staging gets the same startup sweep as .unpack-* dirs. --- lib/images/compose.go | 4 ++-- lib/images/layer_gc.go | 2 +- lib/images/tag.go | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/images/compose.go b/lib/images/compose.go index 9df8c6175..ba3dffa83 100644 --- a/lib/images/compose.go +++ b/lib/images/compose.go @@ -15,8 +15,8 @@ import ( // concurrently, and a failure between the remove and the rename leaves dest // absent. The export root is always 0755 regardless of the last layer's tar // root entry, matching the mode the previous unpack path created. A crash -// can also strand .compose-* staging directories in dest's parent, the same -// way .unpack-* directories can strand under layer builds. +// can also strand .compose-* staging directories in dest's parent build +// directory, which the next successful build of the same digest removes. func (c *ociClient) composeRootfs(ctx context.Context, dest, layoutTag string, model *imageManifestModel) error { if err := validateManifestModel(layoutTag, model); err != nil { return fmt.Errorf("validate manifest model: %w", err) diff --git a/lib/images/layer_gc.go b/lib/images/layer_gc.go index bf7bf808e..890b16aba 100644 --- a/lib/images/layer_gc.go +++ b/lib/images/layer_gc.go @@ -59,7 +59,7 @@ func (m *manager) referencedLayerDigests() (map[string]struct{}, error) { return nil }) if err != nil { - return nil, fmt.Errorf("walk content manifests: %w", err) + return nil, fmt.Errorf("walk image manifests: %w", err) } return refs, nil } diff --git a/lib/images/tag.go b/lib/images/tag.go index 302263d7e..c64415b43 100644 --- a/lib/images/tag.go +++ b/lib/images/tag.go @@ -92,7 +92,7 @@ func (m *manager) cleanupReplacedTag(ref *NormalizedRef, previousDigest, digestH if err := removeDigestIfUnreferenced(m.paths, ref.Repository(), previousDigest, true); err != nil { slog.Warn("failed to collect replaced image content", "repository", ref.Repository(), "digest", previousDigest, "error", err) } - m.refreshDiskUsageTotals() + m.reconcileLayerStoreLocked() } func parseTagReferences(source, target string) (*NormalizedRef, *NormalizedRef, error) { From 92060fa1e93663d87654036824d886862198dd63 Mon Sep 17 00:00:00 2001 From: chruffins <23645059+chruffins@users.noreply.github.com> Date: Sat, 5 Sep 2026 18:52:41 +0000 Subject: [PATCH 16/19] Update stale layer-artifact comments after production integration --- lib/images/layer_artifact.go | 5 ++--- lib/images/manager.go | 4 ++-- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/lib/images/layer_artifact.go b/lib/images/layer_artifact.go index e2faf8093..11e401789 100644 --- a/lib/images/layer_artifact.go +++ b/lib/images/layer_artifact.go @@ -154,9 +154,8 @@ func discardLayerCache(p *paths.Paths, layerHex string) error { // The layer is unpacked into an isolated temp directory, converted to the // default image format, and installed atomically. Normal failures remove the // temp directory; a crash mid-build can leave a stale .unpack-* directory -// behind, which reconciliation landing with the pull integration is expected -// to sweep. No production caller yet: pull integration and -// composition land in later changes. +// behind, which the startup sweep in layer_gc.go removes once it ages past +// the eviction grace period. // // Concurrent callers share one build. The build itself is detached from the // initiating caller's cancellation so one cancelled pull cannot fail every diff --git a/lib/images/manager.go b/lib/images/manager.go index c4bb46088..3bac5178f 100644 --- a/lib/images/manager.go +++ b/lib/images/manager.go @@ -510,8 +510,8 @@ func (m *manager) buildImage(ctx context.Context, ref *ResolvedRef, credentials // Hold the in-flight references until the manifest model protecting // the layers is durable: on the cache-hit path the artifacts' mtimes // are too old for the eviction grace period to cover the gap. - // finalizeImage releases them under createMu; this defer only covers - // paths that never reach finalization. + // finalizeImage releases them under createMu once the model is + // written; this defer covers every path that returns before that. defer materialized.release(m) } if err != nil { From 96b47aa58fdfea33fc54e63517661a8c6bc9daec Mon Sep 17 00:00:00 2001 From: chruffins <23645059+chruffins@users.noreply.github.com> Date: Sat, 5 Sep 2026 19:01:40 +0000 Subject: [PATCH 17/19] Use removePath for stale image temp dir sweep Umoci extraction restores read-only directory modes from layer metadata, so os.RemoveAll cannot unlink through a stale .unpack-* dir and the startup sweep would leave it behind indefinitely. --- lib/images/layer_gc.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/images/layer_gc.go b/lib/images/layer_gc.go index 890b16aba..d05d7ac89 100644 --- a/lib/images/layer_gc.go +++ b/lib/images/layer_gc.go @@ -231,7 +231,9 @@ func (m *manager) cleanStaleImageTempDirs() { } info, statErr := os.Stat(path) if statErr == nil && info.ModTime().Before(cutoff) { - if err := os.RemoveAll(path); err != nil { + // removePath clears the read-only directories umoci restores from + // layer metadata, which os.RemoveAll cannot unlink through. + if err := removePath(path); err != nil { slog.Warn("failed to remove stale image temp dir", "dir", path, "error", err) } } From 2c54ae6cd843206e837a1fbfe9ca3cb1d5e6bf5d Mon Sep 17 00:00:00 2001 From: chruffins <23645059+chruffins@users.noreply.github.com> Date: Sat, 5 Sep 2026 19:10:38 +0000 Subject: [PATCH 18/19] Clear stale compose staging before composing A crashed build can strand a .compose-* staging directory in the digest-keyed build directory, invisible to disk accounting; each compose attempt now removes stale siblings before creating its own staging directory. --- lib/images/compose.go | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/lib/images/compose.go b/lib/images/compose.go index ba3dffa83..a799d240c 100644 --- a/lib/images/compose.go +++ b/lib/images/compose.go @@ -16,7 +16,8 @@ import ( // absent. The export root is always 0755 regardless of the last layer's tar // root entry, matching the mode the previous unpack path created. A crash // can also strand .compose-* staging directories in dest's parent build -// directory, which the next successful build of the same digest removes. +// directory; the next compose attempt for the same digest removes stale +// ones before creating its own. func (c *ociClient) composeRootfs(ctx context.Context, dest, layoutTag string, model *imageManifestModel) error { if err := validateManifestModel(layoutTag, model); err != nil { return fmt.Errorf("validate manifest model: %w", err) @@ -25,6 +26,14 @@ func (c *ociClient) composeRootfs(ctx context.Context, dest, layoutTag string, m if err := os.MkdirAll(parent, 0755); err != nil { return fmt.Errorf("create compose parent: %w", err) } + // The build directory is digest-keyed, so any leftover .compose-* sibling + // is garbage from a crashed build of the same digest. + leftovers, _ := filepath.Glob(filepath.Join(parent, ".compose-*")) + for _, leftover := range leftovers { + if err := removePath(leftover); err != nil { + slog.Warn("failed to remove stale compose staging directory", "dir", leftover, "error", err) + } + } staging, err := os.MkdirTemp(parent, ".compose-*") if err != nil { return fmt.Errorf("create compose directory: %w", err) From 61e87a7aa664f0580aa78a85d8137739ddcc74dc Mon Sep 17 00:00:00 2001 From: chruffins <23645059+chruffins@users.noreply.github.com> Date: Sat, 5 Sep 2026 19:50:25 +0000 Subject: [PATCH 19/19] Collapse single-caller eviction helper and compress lifecycle tests Inline tryEvictLayerArtifact into the eviction loop, use the walk entry's cached info instead of a second stat in the temp-dir sweep, and share the import-and-wait and digest-extraction sequences in the lifecycle tests through small helpers. --- lib/images/layer_gc.go | 43 +++++++++----------------- lib/images/lifecycle_test.go | 59 ++++++++++++++++++------------------ lib/images/manager.go | 12 +++----- 3 files changed, 48 insertions(+), 66 deletions(-) diff --git a/lib/images/layer_gc.go b/lib/images/layer_gc.go index d05d7ac89..9f7a071a9 100644 --- a/lib/images/layer_gc.go +++ b/lib/images/layer_gc.go @@ -81,8 +81,7 @@ func (r *inflightLayerRef) release(m *manager) { }) } -// releaseLocked is release for callers already holding createMu, such as -// finalizeImage. +// releaseLocked is release for callers already holding createMu. func (r *inflightLayerRef) releaseLocked(m *manager) { r.once.Do(func() { m.releaseInflightLayerRefsLocked(r.digestHexes) }) } @@ -156,8 +155,19 @@ func (m *manager) evictUnreferencedLayerArtifacts() { if _, referenced := refs[digestHex]; referenced { continue } - size, removed := m.tryEvictLayerArtifact(digestHex, filepath.Join(layersDir, digestHex), cutoff) - if !removed { + dirPath := filepath.Join(layersDir, digestHex) + info, statErr := os.Stat(dirPath) + if statErr != nil || info.ModTime().After(cutoff) { + continue + } + size, err := dirSize(dirPath) + if err != nil { + slog.Warn("failed to measure layer artifact size", "digest", digestHex, "error", err) + } + // removePath clears read-only directories restored from layer + // metadata, which os.RemoveAll cannot unlink through. + if err := removePath(dirPath); err != nil { + slog.Warn("failed to evict unreferenced layer artifact", "digest", digestHex, "error", err) continue } evicted++ @@ -169,28 +179,6 @@ func (m *manager) evictUnreferencedLayerArtifacts() { } } -// tryEvictLayerArtifact removes one unreferenced layer artifact if it is -// stale. Reconciliation holds createMu while scanning and eviction, and the -// reference set already contains the in-flight digests, so no build can -// register or drop a reference mid-pass. -func (m *manager) tryEvictLayerArtifact(digestHex, dirPath string, cutoff time.Time) (int64, bool) { - info, statErr := os.Stat(dirPath) - if statErr != nil || info.ModTime().After(cutoff) { - return 0, false - } - size, err := dirSize(dirPath) - if err != nil { - slog.Warn("failed to measure layer artifact size", "digest", digestHex, "error", err) - } - // removePath clears read-only directories restored from layer metadata, - // which os.RemoveAll cannot unlink through. - if err := removePath(dirPath); err != nil { - slog.Warn("failed to evict unreferenced layer artifact", "digest", digestHex, "error", err) - return 0, false - } - return size, true -} - // isStaleTempDirName reports whether a directory name matches the temp // prefixes builds use for staging, installs, and tag promotion. func isStaleTempDirName(name string) bool { @@ -229,8 +217,7 @@ func (m *manager) cleanStaleImageTempDirs() { if !isStaleTempDirName(name) { return nil } - info, statErr := os.Stat(path) - if statErr == nil && info.ModTime().Before(cutoff) { + if info, err := entry.Info(); err == nil && info.ModTime().Before(cutoff) { // removePath clears the read-only directories umoci restores from // layer metadata, which os.RemoveAll cannot unlink through. if err := removePath(path); err != nil { diff --git a/lib/images/lifecycle_test.go b/lib/images/lifecycle_test.go index 7bc78b2f7..6ac8a06dc 100644 --- a/lib/images/lifecycle_test.go +++ b/lib/images/lifecycle_test.go @@ -9,6 +9,7 @@ import ( "testing" "time" + gcr "github.com/google/go-containerregistry/pkg/v1" "github.com/google/go-containerregistry/pkg/v1/empty" "github.com/google/go-containerregistry/pkg/v1/mutate" "github.com/kernel/hypeman/lib/paths" @@ -28,6 +29,30 @@ func layerStoreHexes(t *testing.T, p *paths.Paths) map[string]struct{} { return hexes } +func imageDigest(t *testing.T, img gcr.Image) string { + t.Helper() + digest, err := img.Digest() + require.NoError(t, err) + return digest.String() +} + +// importAndWait imports an image and blocks until its build reaches ready. +func importAndWait(t *testing.T, m *manager, ctx context.Context, repo, tag, digest string) { + t.Helper() + events := make(chan StatusEvent, 2) + layoutTag := digestToLayoutTag(digest) + m.subscribeToReady(layoutTag, events) + defer m.unsubscribeFromReady(layoutTag, events) + _, err := m.ImportLocalImage(ctx, repo, tag, digest) + require.NoError(t, err) + select { + case event := <-events: + require.Equal(t, StatusReady, event.Status) + case <-time.After(30 * time.Second): + t.Fatalf("image %s did not become ready", repo) + } +} + // TestSharedLayersMaterializeOnceAndEvictWithReferences is the end-to-end // lifecycle: two images share a base layer, the shared artifact is created // once, survives the deletion of one image, and is evicted only when its last @@ -53,16 +78,11 @@ func TestSharedLayersMaterializeOnceAndEvictWithReferences(t *testing.T) { require.NoError(t, err) writeLayerTestLayout(t, p, imgA, imgB) - digestAHash, err := imgA.Digest() - require.NoError(t, err) - digestBHash, err := imgB.Digest() - require.NoError(t, err) - digestA, digestB := digestAHash.String(), digestBHash.String() + digestA, digestB := imageDigest(t, imgA), imageDigest(t, imgB) baseManifest, err := imgA.Manifest() require.NoError(t, err) - baseHex := baseManifest.Layers[0].Digest.Hex - topAHex := baseManifest.Layers[1].Digest.Hex + baseHex, topAHex := baseManifest.Layers[0].Digest.Hex, baseManifest.Layers[1].Digest.Hex topBManifest, err := imgB.Manifest() require.NoError(t, err) topBHex := topBManifest.Layers[1].Digest.Hex @@ -71,29 +91,8 @@ func TestSharedLayersMaterializeOnceAndEvictWithReferences(t *testing.T) { const repoA = "kernel.local/apps/app-a" const repoB = "kernel.local/apps/app-b" - eventsA := make(chan StatusEvent, 2) - m.subscribeToReady(digestToLayoutTag(digestA), eventsA) - defer m.unsubscribeFromReady(digestToLayoutTag(digestA), eventsA) - _, err = m.ImportLocalImage(ctx, repoA, "v1", digestA) - require.NoError(t, err) - select { - case event := <-eventsA: - require.Equal(t, StatusReady, event.Status) - case <-time.After(30 * time.Second): - t.Fatal("image A did not become ready") - } - - eventsB := make(chan StatusEvent, 2) - m.subscribeToReady(digestToLayoutTag(digestB), eventsB) - defer m.unsubscribeFromReady(digestToLayoutTag(digestB), eventsB) - _, err = m.ImportLocalImage(ctx, repoB, "v1", digestB) - require.NoError(t, err) - select { - case event := <-eventsB: - require.Equal(t, StatusReady, event.Status) - case <-time.After(30 * time.Second): - t.Fatal("image B did not become ready") - } + importAndWait(t, m, ctx, repoA, "v1", digestA) + importAndWait(t, m, ctx, repoB, "v1", digestB) // The shared base layer materialized exactly once, alongside the two tops. hexes := layerStoreHexes(t, p) diff --git a/lib/images/manager.go b/lib/images/manager.go index 3bac5178f..66bb149bd 100644 --- a/lib/images/manager.go +++ b/lib/images/manager.go @@ -126,10 +126,9 @@ func NewManager(p *paths.Paths, maxConcurrentBuilds int, meter metric.Meter) (Ma m.RecoverInterruptedBuilds() // Sweep temp dirs and evict layer orphans an unclean shutdown may have - // left behind. Recovered builds re-enqueued above run concurrently and - // hold in-flight references before materializing; artifacts from their - // previous attempts are unreferenced garbage and get evicted, to be - // re-materialized from the blob cache on demand. + // left behind. Recovered builds re-enqueued above run concurrently; they + // re-materialize from the blob cache if this pass evicts their previous + // attempt's unreferenced artifacts. m.cleanStaleImageTempDirs() m.reconcileLayerStore() // Keep legacy images readable in their existing layout and promote them only @@ -569,10 +568,7 @@ func (m *manager) buildImage(ctx context.Context, ref *ResolvedRef, credentials } func (m *manager) materializeLayerArtifacts(ctx context.Context, result *pullResult) (*inflightLayerRef, error) { - if result.Manifest == nil { - return nil, nil - } - if layerArtifactFormat() == "" { + if result.Manifest == nil || layerArtifactFormat() == "" { return nil, nil } digestHexes := make([]string, 0, len(result.Manifest.Layers))