Skip to content
Open
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions agent/bootstrap_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"time"

boshlogstarprovider "github.com/cloudfoundry/bosh-agent/v2/agent/logstarprovider"
"github.com/cloudfoundry/bosh-agent/v2/infrastructure/devicepathresolver"

. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
Expand Down Expand Up @@ -1132,6 +1133,7 @@ var _ = Describe("bootstrap", func() {
monitRetryStrategy := boshretry.NewAttemptRetryStrategy(10, 1*time.Second, monitRetryable, logger)

devicePathResolver := fakedevicepathresolver.NewFakeDevicePathResolver()
symlinkDeviceResolver := devicepathresolver.NewSymlinkDeviceResolver(fs, udev, logger)

fakeUUIDGenerator := boshuuid.NewGenerator()
routesSearcher := boshnet.NewRoutesSearcher(logger, runner, nil)
Expand All @@ -1153,6 +1155,7 @@ var _ = Describe("bootstrap", func() {
ubuntuCertManager,
monitRetryStrategy,
devicePathResolver,
symlinkDeviceResolver,
state,
linuxOptions,
logger,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import (
)

type FakeDevicePathResolver struct {
GetRealDevicePathDiskSettings boshsettings.DiskSettings
GetRealDevicePathDiskSettings []boshsettings.DiskSettings
RealDevicePath string
GetRealDevicePathStub func(boshsettings.DiskSettings) (string, bool, error)
GetRealDevicePathTimedOut bool
Expand All @@ -17,15 +17,15 @@ func NewFakeDevicePathResolver() *FakeDevicePathResolver {
}

func (r *FakeDevicePathResolver) GetRealDevicePath(diskSettings boshsettings.DiskSettings) (string, bool, error) {
r.GetRealDevicePathDiskSettings = diskSettings

if r.GetRealDevicePathErr != nil {
return "", r.GetRealDevicePathTimedOut, r.GetRealDevicePathErr
}
r.GetRealDevicePathDiskSettings = append(r.GetRealDevicePathDiskSettings, diskSettings)

if r.GetRealDevicePathStub != nil {
return r.GetRealDevicePathStub(diskSettings)
}

if r.GetRealDevicePathErr != nil {
return "", r.GetRealDevicePathTimedOut, r.GetRealDevicePathErr
}

return r.RealDevicePath, false, nil
}
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ var _ = Describe("FallbackDevicePathResolver", func() {
It("does not call the secondary resolver", func() {
_, _, err := pathResolver.GetRealDevicePath(diskSettings)
Expect(err).ToNot(HaveOccurred())
Expect(secondaryResolver.GetRealDevicePathDiskSettings).To(Equal(boshsettings.DiskSettings{}))
Expect(secondaryResolver.GetRealDevicePathDiskSettings).To(BeEmpty())
})
})

Expand All @@ -68,7 +68,7 @@ var _ = Describe("FallbackDevicePathResolver", func() {
Expect(timeout).To(BeFalse())
Expect(realPath).To(Equal("/dev/sdc"))

Expect(secondaryResolver.GetRealDevicePathDiskSettings).To(Equal(diskSettings))
Expect(secondaryResolver.GetRealDevicePathDiskSettings).To(ContainElement(diskSettings))
})

Context("when secondary resolver also errors", func() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ var _ = Describe("scsiDevicePathResolver", func() {
Expect(timeout).To(BeFalse())
Expect(realPath).To(Equal("fake-id-resolved-device-path"))

Expect(scsiIDDevicePathResolver.GetRealDevicePathDiskSettings).To(Equal(diskSettings))
Expect(scsiIDDevicePathResolver.GetRealDevicePathDiskSettings).To(ContainElement(diskSettings))
})
})

Expand All @@ -60,7 +60,7 @@ var _ = Describe("scsiDevicePathResolver", func() {
Expect(timeout).To(BeFalse())
Expect(realPath).To(Equal("fake-volume-id-resolved-device-path"))

Expect(scsiVolumeIDDevicePathResolver.GetRealDevicePathDiskSettings).To(Equal(diskSettings))
Expect(scsiVolumeIDDevicePathResolver.GetRealDevicePathDiskSettings).To(ContainElement(diskSettings))
})
})

Expand All @@ -80,7 +80,7 @@ var _ = Describe("scsiDevicePathResolver", func() {
Expect(timeout).To(BeFalse())
Expect(realPath).To(Equal("fake-lun-resolved-device-path"))

Expect(scsiLunDevicePathResolver.GetRealDevicePathDiskSettings).To(Equal(diskSettings))
Expect(scsiLunDevicePathResolver.GetRealDevicePathDiskSettings).To(ContainElement(diskSettings))
})
})

Expand Down
101 changes: 101 additions & 0 deletions infrastructure/devicepathresolver/symlink_device_resolver.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
package devicepathresolver

import (
bosherr "github.com/cloudfoundry/bosh-utils/errors"
boshlog "github.com/cloudfoundry/bosh-utils/logger"
boshsys "github.com/cloudfoundry/bosh-utils/system"

boshudev "github.com/cloudfoundry/bosh-agent/v2/platform/udevdevice"
)

const (
// NVMeDevicePattern is a glob pattern matching NVMe namespace devices.
NVMeDevicePattern = "/dev/nvme*n1"

// NVMeDevicePathPrefix is the common prefix for NVMe device paths.
// Used to detect if a device path is an NVMe device.
NVMeDevicePathPrefix = "/dev/nvme"
)

type SymlinkDeviceResolver struct {
fs boshsys.FileSystem
udev boshudev.UdevDevice
logger boshlog.Logger
logTag string
}

// NewSymlinkDeviceResolver creates a new symlink device resolver.
func NewSymlinkDeviceResolver(
fs boshsys.FileSystem,
udev boshudev.UdevDevice,
logger boshlog.Logger,
) *SymlinkDeviceResolver {
return &SymlinkDeviceResolver{
fs: fs,
udev: udev,
logger: logger,
logTag: "SymlinkDeviceResolver",
}
}

// ResolveSymlinksToDevices resolves all symlinks matching the given pattern
// and returns a map of resolved device paths -> symlink paths.
//
// udevadm trigger and settle are called before globbing to avoid a race condition:
// NVMe block devices (/dev/nvme*) appear synchronously at boot, but the
// /dev/disk/by-id/ symlinks are created asynchronously by udev. Without waiting,
// globbing may return no symlinks, causing all NVMe devices to be misidentified
// as instance storage (instead of EBS/managed volumes).
func (r *SymlinkDeviceResolver) ResolveSymlinksToDevices(symlinkPattern string) (map[string]string, error) {
if err := r.udev.Trigger(); err != nil {
return nil, bosherr.WrapError(err, "Running udevadm trigger")
}
if err := r.udev.Settle(); err != nil {
return nil, bosherr.WrapError(err, "Running udevadm settle")
}

symlinks, err := r.fs.Glob(symlinkPattern)
if err != nil {
return nil, bosherr.WrapErrorf(err, "Globbing symlinks with pattern '%s'", symlinkPattern)
}

result := make(map[string]string)
for _, symlink := range symlinks {
absPath, err := r.fs.ReadAndFollowLink(symlink)
if err != nil {
return nil, bosherr.WrapErrorf(err, "Resolving managed volume symlink '%s'", symlink)
}

r.logger.Debug(r.logTag, "Resolved symlink: %s -> %s", symlink, absPath)
result[absPath] = symlink
}

return result, nil
}

// GetDevicesByPattern returns all devices matching the given pattern.
func (r *SymlinkDeviceResolver) GetDevicesByPattern(devicePattern string) ([]string, error) {
devices, err := r.fs.Glob(devicePattern)
if err != nil {
return nil, bosherr.WrapErrorf(err, "Globbing devices with pattern '%s'", devicePattern)
}

r.logger.Debug(r.logTag, "Found devices matching '%s': %v", devicePattern, devices)
return devices, nil
}

// FilterDevices returns devices that are NOT in the exclusion map.
// This is used to filter out IaaS-managed volumes (EBS, Azure Managed Disks, etc.)
// from the list of all NVMe devices, leaving only instance/ephemeral storage.
func (r *SymlinkDeviceResolver) FilterDevices(allDevices []string, excludeDevices map[string]string) []string {
var filtered []string
for _, device := range allDevices {
if _, excluded := excludeDevices[device]; !excluded {
filtered = append(filtered, device)
r.logger.Debug(r.logTag, "Including device: %s", device)
} else {
r.logger.Debug(r.logTag, "Excluding device: %s (symlink: %s)", device, excludeDevices[device])
}
}
return filtered
}
157 changes: 157 additions & 0 deletions infrastructure/devicepathresolver/symlink_device_resolver_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,157 @@
package devicepathresolver_test

import (
"errors"
"os"
"runtime"

. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"

boshlog "github.com/cloudfoundry/bosh-utils/logger"
fakesys "github.com/cloudfoundry/bosh-utils/system/fakes"

. "github.com/cloudfoundry/bosh-agent/v2/infrastructure/devicepathresolver"
fakeudev "github.com/cloudfoundry/bosh-agent/v2/platform/udevdevice/fakes"
)

var _ = Describe("SymlinkDeviceResolver", func() {
var (
fs *fakesys.FakeFileSystem
udev *fakeudev.FakeUdevDevice
logger boshlog.Logger
resolver *SymlinkDeviceResolver
)

BeforeEach(func() {
if runtime.GOOS == "windows" {
Skip("Not applicable on Windows")
}

fs = fakesys.NewFakeFileSystem()
udev = fakeudev.NewFakeUdevDevice()
logger = boshlog.NewLogger(boshlog.LevelNone)
resolver = NewSymlinkDeviceResolver(fs, udev, logger)
})

Describe("ResolveSymlinksToDevices", func() {
It("returns empty map when no symlinks match the pattern", func() {
fs.SetGlob("/dev/disk/by-id/nvme-*", []string{})

result, err := resolver.ResolveSymlinksToDevices("/dev/disk/by-id/nvme-*")
Expect(err).ToNot(HaveOccurred())
Expect(result).To(BeEmpty())
})

It("resolves symlinks to their target device paths", func() {
err := fs.MkdirAll("/dev/disk/by-id", os.FileMode(0750))
Expect(err).ToNot(HaveOccurred())

// Create target device files
err = fs.WriteFileString("/dev/nvme1n1", "")
Expect(err).ToNot(HaveOccurred())
err = fs.WriteFileString("/dev/nvme2n1", "")
Expect(err).ToNot(HaveOccurred())

fs.SetGlob("/dev/disk/by-id/nvme-Amazon_Elastic_Block_Store_*", []string{
"/dev/disk/by-id/nvme-Amazon_Elastic_Block_Store_vol123",
"/dev/disk/by-id/nvme-Amazon_Elastic_Block_Store_vol456",
})
err = fs.Symlink("/dev/nvme1n1", "/dev/disk/by-id/nvme-Amazon_Elastic_Block_Store_vol123")
Expect(err).ToNot(HaveOccurred())
err = fs.Symlink("/dev/nvme2n1", "/dev/disk/by-id/nvme-Amazon_Elastic_Block_Store_vol456")
Expect(err).ToNot(HaveOccurred())

result, err := resolver.ResolveSymlinksToDevices("/dev/disk/by-id/nvme-Amazon_Elastic_Block_Store_*")
Expect(err).ToNot(HaveOccurred())
Expect(result).To(HaveLen(2))
Expect(result["/dev/nvme1n1"]).To(Equal("/dev/disk/by-id/nvme-Amazon_Elastic_Block_Store_vol123"))
Expect(result["/dev/nvme2n1"]).To(Equal("/dev/disk/by-id/nvme-Amazon_Elastic_Block_Store_vol456"))
})

It("returns error when a managed volume symlink cannot be resolved", func() {
err := fs.MkdirAll("/dev/disk/by-id", os.FileMode(0750))
Expect(err).ToNot(HaveOccurred())

// Create target device file for valid symlink
err = fs.WriteFileString("/dev/nvme1n1", "")
Expect(err).ToNot(HaveOccurred())

fs.SetGlob("/dev/disk/by-id/nvme-*", []string{
"/dev/disk/by-id/nvme-valid",
"/dev/disk/by-id/nvme-invalid",
})
err = fs.Symlink("/dev/nvme1n1", "/dev/disk/by-id/nvme-valid")
Expect(err).ToNot(HaveOccurred())
// nvme-invalid has no symlink target

_, err = resolver.ResolveSymlinksToDevices("/dev/disk/by-id/nvme-*")
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(ContainSubstring("nvme-invalid"))
Comment on lines +88 to +90
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

Assert result is nil on resolution failure.

This test now validates fail-closed errors, but it should also assert no partial mapping is returned (contract safety on error paths).

Suggested test tightening
-			_, err = resolver.ResolveSymlinksToDevices("/dev/disk/by-id/nvme-*")
+			result, err := resolver.ResolveSymlinksToDevices("/dev/disk/by-id/nvme-*")
 			Expect(err).To(HaveOccurred())
+			Expect(result).To(BeNil())
 			Expect(err.Error()).To(ContainSubstring("nvme-invalid"))
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@infrastructure/devicepathresolver/symlink_device_resolver_test.go` around
lines 88 - 90, The test currently expects an error from
resolver.ResolveSymlinksToDevices("/dev/disk/by-id/nvme-*") but doesn't assert
the returned mapping is nil; update the test to also assert that the returned
result (the value assigned from ResolveSymlinksToDevices) is nil on failure to
ensure no partial mapping is returned. Locate the call to
ResolveSymlinksToDevices and the variables holding its return values (e.g.,
result, err) and add an assertion like Expect(result).To(BeNil()) immediately
after Expect(err).To(HaveOccurred()) /
Expect(err.Error()).To(ContainSubstring("nvme-invalid")) so the contract on
error paths is enforced.

})

It("returns error when glob fails", func() {
fs.GlobErr = errors.New("glob error")

_, err := resolver.ResolveSymlinksToDevices("/dev/disk/by-id/nvme-*")
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(ContainSubstring("glob error"))
})
})

Describe("GetDevicesByPattern", func() {
It("returns devices matching the pattern", func() {
fs.SetGlob("/dev/nvme*n1", []string{"/dev/nvme0n1", "/dev/nvme1n1", "/dev/nvme2n1"})

devices, err := resolver.GetDevicesByPattern("/dev/nvme*n1")
Expect(err).ToNot(HaveOccurred())
Expect(devices).To(ConsistOf("/dev/nvme0n1", "/dev/nvme1n1", "/dev/nvme2n1"))
})

It("returns empty slice when no devices match", func() {
fs.SetGlob("/dev/nvme*n1", []string{})

devices, err := resolver.GetDevicesByPattern("/dev/nvme*n1")
Expect(err).ToNot(HaveOccurred())
Expect(devices).To(BeEmpty())
})

It("returns error when glob fails", func() {
fs.GlobErr = errors.New("glob error")

_, err := resolver.GetDevicesByPattern("/dev/nvme*n1")
Expect(err).To(HaveOccurred())
})
})

Describe("FilterDevices", func() {
It("returns devices not in the exclusion map", func() {
allDevices := []string{"/dev/nvme0n1", "/dev/nvme1n1", "/dev/nvme2n1", "/dev/nvme3n1"}
excludeDevices := map[string]string{
"/dev/nvme1n1": "/dev/disk/by-id/ebs-vol1",
"/dev/nvme2n1": "/dev/disk/by-id/ebs-vol2",
}

filtered := resolver.FilterDevices(allDevices, excludeDevices)
Expect(filtered).To(ConsistOf("/dev/nvme0n1", "/dev/nvme3n1"))
})

It("returns all devices when exclusion map is empty", func() {
allDevices := []string{"/dev/nvme0n1", "/dev/nvme1n1"}
excludeDevices := map[string]string{}

filtered := resolver.FilterDevices(allDevices, excludeDevices)
Expect(filtered).To(ConsistOf("/dev/nvme0n1", "/dev/nvme1n1"))
})

It("returns empty slice when all devices are excluded", func() {
allDevices := []string{"/dev/nvme0n1"}
excludeDevices := map[string]string{
"/dev/nvme0n1": "/dev/disk/by-id/ebs-vol1",
}

filtered := resolver.FilterDevices(allDevices, excludeDevices)
Expect(filtered).To(BeEmpty())
})
})
})
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ var _ = Describe("VirtioDevicePathResolver", func() {
Expect(timeout).To(BeFalse())
Expect(realPath).To(Equal("fake-mapped-resolved-device-path"))

Expect(mappedDevicePathResolver.GetRealDevicePathDiskSettings).To(Equal(diskSettings))
Expect(mappedDevicePathResolver.GetRealDevicePathDiskSettings).To(ContainElement(diskSettings))
})

Context("when mappedDevicePathResolver times out", func() {
Expand Down
Loading
Loading