diff --git a/api/v1alpha1/lvm_volume_group.go b/api/v1alpha1/lvm_volume_group.go index 54f1b66f9..da9210ae8 100644 --- a/api/v1alpha1/lvm_volume_group.go +++ b/api/v1alpha1/lvm_volume_group.go @@ -40,11 +40,18 @@ type LVMVolumeGroup struct { // +k8s:deepcopy-gen=true type LVMVolumeGroupSpec struct { - ActualVGNameOnTheNode string `json:"actualVGNameOnTheNode"` - BlockDeviceSelector *metav1.LabelSelector `json:"blockDeviceSelector"` - ThinPools []LVMVolumeGroupThinPoolSpec `json:"thinPools"` - Type string `json:"type"` - Local LVMVolumeGroupLocalSpec `json:"local"` + ActualVGNameOnTheNode string `json:"actualVGNameOnTheNode"` + BlockDeviceSelector *metav1.LabelSelector `json:"blockDeviceSelector,omitempty"` + ThinPools []LVMVolumeGroupThinPoolSpec `json:"thinPools"` + Type string `json:"type"` + Local LVMVolumeGroupLocalSpec `json:"local"` + FileDevices []LVMVolumeGroupFileDeviceSpec `json:"fileDevices,omitempty"` +} + +// +k8s:deepcopy-gen=true +type LVMVolumeGroupFileDeviceSpec struct { + Directory string `json:"directory"` + Size resource.Quantity `json:"size"` } // +k8s:deepcopy-gen=true @@ -73,8 +80,17 @@ type LVMVolumeGroupDevice struct { // +k8s:deepcopy-gen=true type LVMVolumeGroupNode struct { - Devices []LVMVolumeGroupDevice `json:"devices"` - Name string `json:"name"` + Devices []LVMVolumeGroupDevice `json:"devices"` + FileDevices []LVMVolumeGroupFileDevice `json:"fileDevices,omitempty"` + Name string `json:"name"` +} + +// +k8s:deepcopy-gen=true +type LVMVolumeGroupFileDevice struct { + FilePath string `json:"filePath"` + LoopDevice string `json:"loopDevice"` + Size resource.Quantity `json:"size"` + PVUuid string `json:"pvUUID"` } // +k8s:deepcopy-gen=true diff --git a/api/v1alpha1/lvm_volume_group_set.go b/api/v1alpha1/lvm_volume_group_set.go index 0a4a9c951..07b9536a0 100644 --- a/api/v1alpha1/lvm_volume_group_set.go +++ b/api/v1alpha1/lvm_volume_group_set.go @@ -48,11 +48,12 @@ type LVMVolumeGroupSetSpec struct { // +k8s:deepcopy-gen=true type LVMVolumeGroupTemplate struct { - Metadata LVMVolumeGroupTemplateMeta `json:"metadata"` - BlockDeviceSelector *metav1.LabelSelector `json:"blockDeviceSelector"` - ActualVGNameOnTheNode string `json:"actualVGNameOnTheNode"` - ThinPools []LVMVolumeGroupThinPoolSpec `json:"thinPools"` - Type string `json:"type"` + Metadata LVMVolumeGroupTemplateMeta `json:"metadata"` + BlockDeviceSelector *metav1.LabelSelector `json:"blockDeviceSelector,omitempty"` + ActualVGNameOnTheNode string `json:"actualVGNameOnTheNode"` + ThinPools []LVMVolumeGroupThinPoolSpec `json:"thinPools"` + Type string `json:"type"` + FileDevices []LVMVolumeGroupFileDeviceSpec `json:"fileDevices,omitempty"` } // +k8s:deepcopy-gen=true diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 7821a2bee..8a5cbaaa9 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -532,6 +532,38 @@ func (in *LVMVolumeGroupLocalSpec) DeepCopy() *LVMVolumeGroupLocalSpec { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *LVMVolumeGroupFileDevice) DeepCopyInto(out *LVMVolumeGroupFileDevice) { + *out = *in + out.Size = in.Size.DeepCopy() +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new LVMVolumeGroupFileDevice. +func (in *LVMVolumeGroupFileDevice) DeepCopy() *LVMVolumeGroupFileDevice { + if in == nil { + return nil + } + out := new(LVMVolumeGroupFileDevice) + in.DeepCopyInto(out) + return out +} + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *LVMVolumeGroupFileDeviceSpec) DeepCopyInto(out *LVMVolumeGroupFileDeviceSpec) { + *out = *in + out.Size = in.Size.DeepCopy() +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new LVMVolumeGroupFileDeviceSpec. +func (in *LVMVolumeGroupFileDeviceSpec) DeepCopy() *LVMVolumeGroupFileDeviceSpec { + if in == nil { + return nil + } + out := new(LVMVolumeGroupFileDeviceSpec) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *LVMVolumeGroupNode) DeepCopyInto(out *LVMVolumeGroupNode) { *out = *in @@ -542,6 +574,13 @@ func (in *LVMVolumeGroupNode) DeepCopyInto(out *LVMVolumeGroupNode) { (*in)[i].DeepCopyInto(&(*out)[i]) } } + if in.FileDevices != nil { + in, out := &in.FileDevices, &out.FileDevices + *out = make([]LVMVolumeGroupFileDevice, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new LVMVolumeGroupNode. @@ -683,6 +722,13 @@ func (in *LVMVolumeGroupSpec) DeepCopyInto(out *LVMVolumeGroupSpec) { copy(*out, *in) } out.Local = in.Local + if in.FileDevices != nil { + in, out := &in.FileDevices, &out.FileDevices + *out = make([]LVMVolumeGroupFileDeviceSpec, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new LVMVolumeGroupSpec. @@ -749,6 +795,13 @@ func (in *LVMVolumeGroupTemplate) DeepCopyInto(out *LVMVolumeGroupTemplate) { *out = make([]LVMVolumeGroupThinPoolSpec, len(*in)) copy(*out, *in) } + if in.FileDevices != nil { + in, out := &in.FileDevices, &out.FileDevices + *out = make([]LVMVolumeGroupFileDeviceSpec, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new LVMVolumeGroupTemplate. diff --git a/crds/lvmvolumegroup.yaml b/crds/lvmvolumegroup.yaml index cb3e58718..3e2c33d15 100644 --- a/crds/lvmvolumegroup.yaml +++ b/crds/lvmvolumegroup.yaml @@ -38,9 +38,13 @@ spec: - rule: | (self.type == "Local" && has(self.local)) || self.type != "Local" message: "The 'local' field is required when the 'type' field is 'Local'." + - rule: | + has(self.blockDeviceSelector) || (has(self.fileDevices) && size(self.fileDevices) > 0) + message: "At least one of 'blockDeviceSelector' or 'fileDevices' must be specified." + - rule: '!has(oldSelf.fileDevices) || oldSelf.fileDevices.all(o, has(self.fileDevices) && self.fileDevices.exists(n, n.directory == o.directory && n.size == o.size))' + message: 'Existing fileDevices entries are immutable. Add a new entry instead of editing directory or size.' required: - type - - blockDeviceSelector - actualVGNameOnTheNode properties: type: @@ -120,6 +124,60 @@ spec: x-kubernetes-validations: - rule: self == oldSelf message: "The actualVGNameOnTheNode field is immutable." + fileDevices: + type: array + maxItems: 32 + x-kubernetes-list-type: map + x-kubernetes-list-map-keys: + - directory + - size + description: | + File-backed block devices to create as LVM PVs for this Volume Group. + + The agent will create a preallocated file in the specified directory, + attach it as a loop device, and use it as a Physical Volume. + File devices and block devices can be used together in the same VG. + + Existing entries are immutable: `directory` and `size` cannot be changed + after creation, because the backing file's path is derived from them and + the loop device captures its size at attach time. To increase capacity, + add a new `fileDevices` entry instead of editing an existing one. + items: + type: object + required: + - directory + - size + properties: + directory: + type: string + maxLength: 4096 + description: | + Absolute directory on the node's filesystem where the backing file + will be created. + + Must be the module's configured base directory + (fileDevicesDirectory, default /opt/deckhouse/sds/file-devices) or a + subdirectory of it; the agent rejects paths outside that subtree so a + stray entry cannot fill an arbitrary host path. + + The agent creates the directory automatically (mkdir -p) if it does + not exist. The path must be absolute and must not contain '..' + segments. Provisioning fails if the path is on a read-only + filesystem, a non-directory component is in the way, or there is not + enough free space for the requested size. + size: + type: string + maxLength: 32 + pattern: '^[0-9]+(\.[0-9]+)?(E|P|T|G|M|k|Ei|Pi|Ti|Gi|Mi|Ki)?$' + description: | + Size of the preallocated backing file. Must be at least 1Gi. + + Prefer binary units (`Gi`, `Mi`). Decimal units are accepted + by the format but are smaller than they look: `1G` is 10^9 bytes, + which is below the 1Gi (2^30 bytes) minimum and will be rejected + by the agent. Because an existing entry's `size` is immutable and + cannot be removed, a too-small value can only be corrected by + deleting and recreating the LVMVolumeGroup, so set it correctly. thinPools: type: array description: | @@ -312,6 +370,32 @@ spec: type: string description: | Name of the corresponding block device resource. + fileDevices: + type: array + description: | + Information about the file-backed devices used in the Volume Group on the current node. + items: + type: object + properties: + filePath: + type: string + description: | + Path to the backing file on the node. + loopDevice: + type: string + description: | + Loop device path (e.g., `/dev/loop0`). + size: + type: string + description: | + Size of the Physical Volume created on the backing file. + This is slightly smaller than the requested `spec.fileDevices[].size` + because LVM reserves space for PV metadata (the same way + `status.nodes[].devices[].pvSize` differs from the raw device size). + pvUUID: + type: string + description: | + LVM Physical Volume UUID. subresources: status: {} additionalPrinterColumns: diff --git a/crds/lvmvolumegroupset.yaml b/crds/lvmvolumegroupset.yaml index 902cff710..8740e74b7 100644 --- a/crds/lvmvolumegroupset.yaml +++ b/crds/lvmvolumegroupset.yaml @@ -86,10 +86,15 @@ spec: type: object description: | Common template for LVMVolumeGroup resources provided by the set. + x-kubernetes-validations: + - rule: | + has(self.blockDeviceSelector) || (has(self.fileDevices) && size(self.fileDevices) > 0) + message: "At least one of 'blockDeviceSelector' or 'fileDevices' must be specified." + - rule: '!has(oldSelf.fileDevices) || oldSelf.fileDevices.all(o, has(self.fileDevices) && self.fileDevices.exists(n, n.directory == o.directory && n.size == o.size))' + message: 'Existing fileDevices entries are immutable. Add a new entry instead of editing directory or size.' required: - type - actualVGNameOnTheNode - - blockDeviceSelector properties: blockDeviceSelector: type: object @@ -162,6 +167,48 @@ spec: x-kubernetes-validations: - rule: self == oldSelf message: "The actualVGNameOnTheNode field is immutable." + fileDevices: + type: array + maxItems: 32 + x-kubernetes-list-type: map + x-kubernetes-list-map-keys: + - directory + - size + description: | + File-backed block devices to create as LVM PVs in LVMVolumeGroups. + + The agent will create a preallocated file in the specified directory, + attach it as a loop device, and use it as a Physical Volume. + + Existing entries are immutable: `directory` and `size` cannot be changed + after creation. To increase capacity, add a new entry instead. + + > Note that the configuration will be common for every LVMVolumeGroup created by the set. + items: + type: object + required: + - directory + - size + properties: + directory: + type: string + maxLength: 4096 + description: | + Absolute directory on the node's filesystem where the backing file + will be created. The agent creates the directory automatically + (mkdir -p) if it does not exist; the path must be absolute and + must not contain '..' segments. + size: + type: string + maxLength: 32 + pattern: '^[0-9]+(\.[0-9]+)?(E|P|T|G|M|k|Ei|Pi|Ti|Gi|Mi|Ki)?$' + description: | + Size of the preallocated backing file. Must be at least 1Gi. + + Prefer binary units (`Gi`, `Mi`). Decimal units are accepted + by the format but are smaller than they look: `1G` is 10^9 bytes, + which is below the 1Gi (2^30 bytes) minimum and will be rejected + by the agent. thinPools: type: array description: | diff --git a/docs/FAQ.md b/docs/FAQ.md index dd37be7ec..e50cfb583 100644 --- a/docs/FAQ.md +++ b/docs/FAQ.md @@ -151,6 +151,47 @@ If the Volume Group has active Logical Volumes, perform the following steps: If necessary, the command can be added to the `cloud-init` script for automatic execution when creating virtual machines. +## How do file-backed devices (fileDevices) work? + +File-backed devices allow you to allocate part of an existing filesystem for LVM without dedicated block devices. The agent creates a preallocated file in the specified directory, attaches it as a loop device via `losetup`, and uses it as an LVM Physical Volume. + +### Limitations + +- **Confined to a base directory**: Each `directory` must be the `fileDevicesDirectory` module setting (default `/opt/deckhouse/sds/file-devices`) or a subdirectory of it. Paths outside this subtree are rejected so an arbitrary host path cannot be filled up. Point `fileDevicesDirectory` at a dedicated data disk to use a different location. +- **Directory auto-created**: The agent creates the backing directory automatically (`mkdir -p`) on the node if it does not exist. The path must be absolute and free of `..` segments; provisioning fails only if the path is on a read-only filesystem or a non-directory component is in the way. +- **No resize**: File device size cannot be changed after creation. To increase capacity, add a new `fileDevices` entry. +- **Preallocated only**: Files are created with `fallocate`, which preallocates space on the filesystem. The agent refuses to create a backing file larger than the directory's free space, so a too-large entry is reported on the resource instead of filling the node. +- **Minimum size**: Each file device must be at least 1Gi. +- **Performance overhead**: LVM on a loop device over a filesystem adds double indirection. Use file-backed devices only when dedicated disks are not available. +- **Host LVM filter**: NodeGroupConfiguration adds loop devices to the host-wide LVM `global_filter`, so unprivileged `lvm`/`pvs` on the node do not see them. The agent re-attaches managed backing files at startup and uses its own LVM config for managed Volume Groups. + +### Reclaiming space + +Backing files are preallocated with `fallocate`, so a file occupies its full size on the node's filesystem from the moment it is created and does **not** grow or shrink automatically as data is written or deleted inside the volume. + +Space can still be returned to the node's filesystem through the discard (TRIM) chain, which works end to end for these devices: + +`filesystem on the volume` → `thin LV` → `thin pool` (created with `discards=passdown` by default) → `/dev/loopN` → backing file. + +A loop device translates discards into `FALLOC_FL_PUNCH_HOLE` on its backing file, turning the reclaimed regions into holes and making the file sparse. To trigger reclamation after deleting data: + +- run `fstrim ` on the volume's filesystem periodically (for example via the `fstrim.timer` systemd unit), or +- mount the volume with `-o discard` for continuous (online) discard. + +Caveats: + +- Only whole thin-pool chunks are reclaimed, so the effect depends on the pool's chunk size and alignment. +- Snapshots pin the chunks they reference, so space shared with a snapshot is not freed until the snapshot is removed. +- The backing file only shrinks after a write → delete → trim cycle; a freshly created file always occupies its full preallocated size. + +### Reboot recovery + +After a node reboot, the agent automatically re-establishes loop device mappings before activating Volume Groups. The backing file path is stored in `status.nodes[].fileDevices[].filePath`. + +### Deletion + +When an LVMVolumeGroup with file-backed devices is deleted, the agent detaches the loop devices and removes the backing files. + ## What labels are added by the controller to BlockDevice resources? - `status.blockdevice.storage.deckhouse.io/type`: LVM type. diff --git a/docs/RESOURCES.md b/docs/RESOURCES.md index 9472e16ee..4948fe53a 100644 --- a/docs/RESOURCES.md +++ b/docs/RESOURCES.md @@ -195,6 +195,60 @@ The `spec.local` field is mandatory for the `Local` type. If there's a discrepan All selected block devices must belong to the same node for [LVMVolumeGroup](./cr.html#lvmvolumegroup) with type `Local`. {{< /alert >}} +#### Creating an LVMVolumeGroup with file-backed devices + +When dedicated block devices are not available, you can allocate part of an existing filesystem for LVM by using file-backed devices (`spec.fileDevices`). The agent will create a preallocated file, attach it as a loop device, and use it as an LVM Physical Volume. + +Each `spec.fileDevices[].directory` must be inside the base directory configured by the `fileDevicesDirectory` module setting (default `/opt/deckhouse/sds/file-devices`); the agent rejects paths outside it so a stray entry cannot fill an arbitrary host path. To place backing files on a dedicated data disk, mount the disk on the node and set `fileDevicesDirectory` to its mount point in the module configuration. + +Creating a Volume Group backed by a 50Gi file on an existing filesystem: + +```yaml +apiVersion: storage.deckhouse.io/v1alpha1 +kind: LVMVolumeGroup +metadata: + name: "vg-file-on-node-0" +spec: + type: Local + local: + nodeName: "node-0" + actualVGNameOnTheNode: "vg-file" + fileDevices: + - directory: /opt/deckhouse/sds/file-devices + size: 50Gi +``` + +Combining block devices and file-backed devices in a single Volume Group: + +```yaml +apiVersion: storage.deckhouse.io/v1alpha1 +kind: LVMVolumeGroup +metadata: + name: "vg-mixed-on-node-0" +spec: + type: Local + local: + nodeName: "node-0" + blockDeviceSelector: + matchExpressions: + - key: kubernetes.io/metadata.name + operator: In + values: + - dev-07ad52cef2348996b72db262011f1b5f896bb68f + actualVGNameOnTheNode: "vg-mixed" + fileDevices: + - directory: /opt/deckhouse/sds/file-devices + size: 100Gi +``` + +{{< alert level="warning" >}} +File-backed devices add overhead due to double indirection (LVM on loop device on filesystem). Use them only when dedicated block devices are not available. +{{< /alert >}} + +{{< alert level="info" >}} +The agent automatically recovers loop device mappings after a node reboot. File device resize is not supported in the current version. +{{< /alert >}} + ### Updating an LVMVolumeGroup resource To change the desired state of a Volume Group or thin pool on nodes, modify the `spec` field of the corresponding [LVMVolumeGroup](./cr.html#lvmvolumegroup) resource. The controller automatically validates the new data and, if correct, makes the necessary changes to entities on the node. diff --git a/e2e/README.md b/e2e/README.md index 353758f74..9c5951590 100644 --- a/e2e/README.md +++ b/e2e/README.md @@ -99,6 +99,20 @@ ginkgo -v --progress --label-filter=e2e-tests --focus="Should schedule Pod with - **BlockDevice discovery**: появление диска; корректные `status.nodeName`, `status.path`, `status.size`, `consumable`. - **LVMVolumeGroup**: создание на основе BlockDevice, статус и capacity. +### LVMVolumeGroup на файлах (fileDevices) + +Context `LVMVolumeGroup with file-backed devices` (Ginkgo-лейбл **`e2e-tests`**, файл `sds_node_configurator_test.go`, хелперы — `sds_node_configurator_helpers_filedevices_test.go`). В отличие от остальных сценариев **не требует отдельного диска / nested-VM**: LVG создаётся с `spec.fileDevices` на любой ноде с работающим агентом; проверки на ноде идут по SSH. Файлы кладутся в дефолтный базовый каталог `/opt/deckhouse/sds/file-devices`. + +Сценарии: + +- **Создание и удаление**: file-only VG (`1Gi`) → `Ready`; в `status.nodes[].fileDevices` — `filePath` под базовым каталогом, `loopDevice` `/dev/loop*`, `pvUUID`; на ноде существуют backing-файл, loop-устройство и VG. После удаления LVG — backing-файл удалён, loop отсоединён. +- **Thin-pool на файле**: file-only VG (`2Gi`) + thin-pool `50%` → `Ready`, data-LV thin-pool присутствует на ноде. +- **Расширение**: добавление второй записи `fileDevices` → в статусе две записи, `vgSize` растёт, `Ready`. +- **Reattach после рестарта агента**: рестарт пода агента → VG остаётся `Ready`, файл переподключён (loop снова привязан). +- **Негатив (allowlist)**: `directory` вне базового каталога (`/tmp/...`) → `VGConfigurationApplied=False`, reason `ValidationFailed`, VG и каталог на ноде не создаются. + +> Требования: кластер собран из этой ветки (поле `fileDevices` и настройка `fileDevicesDirectory` существуют только здесь); на выбранной ноде в `/opt/deckhouse/sds` должно быть ≥3Gi свободного места (иначе сработает проверка свободного места агента). + ### Stress: максимум VG на одной ноде Spec в `sds_node_configurator_stress_max_vgs_test.go`, Ginkgo-лейбл **`stress-test`** (остальные e2e — **`e2e-tests`**). CI smoke по умолчанию: `-ginkgo.label-filter=e2e-tests`. Подробнее — [E2E_USAGE.md](E2E_USAGE.md). diff --git a/e2e/tests/sds_node_configurator_helpers_filedevices_test.go b/e2e/tests/sds_node_configurator_helpers_filedevices_test.go new file mode 100644 index 000000000..2796d22fb --- /dev/null +++ b/e2e/tests/sds_node_configurator_helpers_filedevices_test.go @@ -0,0 +1,149 @@ +/* + Copyright 2026 Flant JSC + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +package tests + +import ( + "context" + "fmt" + "strings" + "time" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/rest" + "sigs.k8s.io/controller-runtime/pkg/client" + + "github.com/deckhouse/sds-node-configurator/api/v1alpha1" +) + +const ( + // e2eFileDevicesBaseDir must match the fileDevicesDirectory module default + // (openapi/config-values.yaml) and DefaultFileDevicesDirectory in the agent. + // File-backed tests place backing files here so the agent's base-dir + // allowlist accepts them. + e2eFileDevicesBaseDir = "/opt/deckhouse/sds/file-devices" + + e2eSNCNamespace = "d8-sds-node-configurator" + e2eSNCAgentApp = "sds-node-configurator" +) + +// e2ePickNodeRunningAgent returns the node name of a Ready, Running +// sds-node-configurator agent pod. File-backed LVMVolumeGroups need no +// dedicated disk, so any node with a healthy agent can host the test VG. +func e2ePickNodeRunningAgent(ctx context.Context, cl client.Client) string { + var chosen string + Eventually(func(g Gomega) { + var pods corev1.PodList + g.Expect(cl.List(ctx, &pods, client.InNamespace(e2eSNCNamespace), client.MatchingLabels{"app": e2eSNCAgentApp})).To(Succeed()) + for i := range pods.Items { + p := &pods.Items[i] + if p.Spec.NodeName == "" || p.DeletionTimestamp != nil || p.Status.Phase != corev1.PodRunning || !isPodReady(p) { + continue + } + chosen = p.Spec.NodeName + return + } + g.Expect(chosen).NotTo(BeEmpty(), "no Ready sds-node-configurator agent pod found in %s", e2eSNCNamespace) + }, 2*time.Minute, 5*time.Second).Should(Succeed()) + return chosen +} + +// e2eFileDevicesForNode returns the file-device status entries the LVG reports +// for node (nil if the node is absent or has no file devices yet). +func e2eFileDevicesForNode(lvg *v1alpha1.LVMVolumeGroup, node string) []v1alpha1.LVMVolumeGroupFileDevice { + for i := range lvg.Status.Nodes { + if lvg.Status.Nodes[i].Name == node { + return lvg.Status.Nodes[i].FileDevices + } + } + return nil +} + +// e2eNodePathExists reports whether path exists on the node (checked as root, +// because backing files live under root-owned /opt/deckhouse/sds). +func e2eNodePathExists(ctx context.Context, kubeconfig *rest.Config, node, sshUser, path string) (bool, error) { + out, err := e2eExecOnTestClusterNodeSSH(ctx, kubeconfig, node, sshUser, + fmt.Sprintf("sudo -n sh -c 'test -e %q && echo EXISTS || echo MISSING'", path)) + if err != nil { + return false, err + } + return strings.Contains(out, "EXISTS"), nil +} + +// e2eLoopBoundToFileOnNode reports whether any loop device is currently +// attached to path on the node (via `losetup -j`). +func e2eLoopBoundToFileOnNode(ctx context.Context, kubeconfig *rest.Config, node, sshUser, path string) (bool, string, error) { + out, err := e2eExecOnTestClusterNodeSSH(ctx, kubeconfig, node, sshUser, + fmt.Sprintf("sudo -n losetup -j %q 2>/dev/null || losetup -j %q 2>/dev/null", path, path)) + if err != nil { + return false, out, err + } + return strings.TrimSpace(out) != "", out, nil +} + +// e2eVGListedOnNode reports whether vgName is visible to vgs on the node. +func e2eVGListedOnNode(ctx context.Context, kubeconfig *rest.Config, node, sshUser, vgName string) (bool, string, error) { + out, err := e2eExecOnTestClusterNodeSSH(ctx, kubeconfig, node, sshUser, + "vgs -o vg_name --noheadings 2>/dev/null || sudo -n vgs -o vg_name --noheadings 2>/dev/null") + if err != nil { + return false, out, err + } + return e2eVgNameListedInVgsOutput(out, vgName), out, nil +} + +// e2eCreateFileBackedLVGAndWaitReady creates lvg and waits (up to +// e2eLVMVolumeGroupReadyTimeout) until it reports Phase Ready, dumping +// conditions while it waits. Returns the refreshed object. +func e2eCreateFileBackedLVGAndWaitReady(ctx context.Context, cl client.Client, lvg *v1alpha1.LVMVolumeGroup) *v1alpha1.LVMVolumeGroup { + Expect(cl.Create(ctx, lvg)).To(Succeed()) + var created v1alpha1.LVMVolumeGroup + Eventually(func(g Gomega) { + g.Expect(cl.Get(ctx, client.ObjectKeyFromObject(lvg), &created)).To(Succeed()) + if created.Status.Phase != v1alpha1.PhaseReady { + GinkgoWriter.Printf("LVMVolumeGroup %s phase=%s (waiting for Ready)\n", lvg.Name, created.Status.Phase) + for _, c := range created.Status.Conditions { + GinkgoWriter.Printf(" condition %s status=%s reason=%s msg=%s\n", c.Type, c.Status, c.Reason, c.Message) + } + } + g.Expect(created.Status.Phase).To(Equal(v1alpha1.PhaseReady), "Phase should be Ready, got %s", created.Status.Phase) + }, e2eLVMVolumeGroupReadyTimeout, 10*time.Second).Should(Succeed()) + return &created +} + +// e2eExpectNoFalseConditions fails if any status condition is False. +func e2eExpectNoFalseConditions(lvg *v1alpha1.LVMVolumeGroup) { + for _, c := range lvg.Status.Conditions { + Expect(c.Status).NotTo(Equal(metav1.ConditionFalse), + "condition %s has status False: reason=%s message=%s", c.Type, c.Reason, c.Message) + } +} + +// e2eDeleteLVGAndWaitGone deletes the LVMVolumeGroup and waits until its CR is +// gone. The finalizer is only cleared after the agent detaches the loop +// devices and removes the backing files, so a successful delete also confirms +// node-side cleanup ran. +func e2eDeleteLVGAndWaitGone(ctx context.Context, cl client.Client, name string, timeout time.Duration) { + Expect(client.IgnoreNotFound(cl.Delete(ctx, &v1alpha1.LVMVolumeGroup{ObjectMeta: metav1.ObjectMeta{Name: name}}))).To(Succeed()) + Eventually(func(g Gomega) { + var cur v1alpha1.LVMVolumeGroup + err := cl.Get(ctx, client.ObjectKey{Name: name}, &cur) + g.Expect(apierrors.IsNotFound(err)).To(BeTrue(), "LVMVolumeGroup %s should be deleted", name) + }, timeout, 8*time.Second).Should(Succeed()) +} diff --git a/e2e/tests/sds_node_configurator_test.go b/e2e/tests/sds_node_configurator_test.go index 8088561a9..50c87961b 100644 --- a/e2e/tests/sds_node_configurator_test.go +++ b/e2e/tests/sds_node_configurator_test.go @@ -2446,13 +2446,13 @@ var _ = Describe("sds-node-configurator module e2e", Label("e2e-tests"), Ordered targetVM := clusterVMs[rand.Intn(len(clusterVMs))] type multiDisk struct { - diskName string - diskSize string - att *kubernetes.VirtualDiskAttachmentResult - bd *v1alpha1.BlockDevice - meta string - lvgName string - vgName string + diskName string + diskSize string + att *kubernetes.VirtualDiskAttachmentResult + bd *v1alpha1.BlockDevice + meta string + lvgName string + vgName string } disks := make([]multiDisk, 0, nDisks) for i := 0; i < nDisks; i++ { @@ -3077,6 +3077,258 @@ sudo -n vgchange "$VG" --addtag storage.deckhouse.io/enabled=true 2>&1 }) }) + Context("LVMVolumeGroup with file-backed devices", Label("e2e-tests"), func() { + const ( + lvgConditionVGConfigurationApplied = "VGConfigurationApplied" + reasonValidationFailed = "ValidationFailed" + ) + + var vmSSH string + + BeforeEach(func() { + ensureE2EK8sClient(testClusterResources, &k8sClient, e2eCtx) + vmSSH = e2eConfigVMSSHUser() + }) + + // Helper: build a file-only LVMVolumeGroup (no blockDeviceSelector) on node. + newFileLVG := func(node, lvgName, vgName string, fds []v1alpha1.LVMVolumeGroupFileDeviceSpec, thinPools []v1alpha1.LVMVolumeGroupThinPoolSpec) *v1alpha1.LVMVolumeGroup { + return &v1alpha1.LVMVolumeGroup{ + ObjectMeta: metav1.ObjectMeta{Name: lvgName}, + Spec: v1alpha1.LVMVolumeGroupSpec{ + ActualVGNameOnTheNode: vgName, + Type: "Local", + Local: v1alpha1.LVMVolumeGroupLocalSpec{NodeName: node}, + FileDevices: fds, + ThinPools: thinPools, + }, + } + } + + It("Should create a file-backed LVMVolumeGroup and remove backing files on delete", func() { + node := e2ePickNodeRunningAgent(e2eCtx, k8sClient) + runID := strconv.FormatInt(time.Now().UnixNano(), 10) + nodeSuffix := strings.NewReplacer(".", "-", "_", "-").Replace(node) + lvgName := "e2e-lvg-fd-" + runID + "-" + nodeSuffix + vgName := "e2e-vg-fd-" + runID + + lvg := newFileLVG(node, lvgName, vgName, + []v1alpha1.LVMVolumeGroupFileDeviceSpec{{Directory: e2eFileDevicesBaseDir, Size: resource.MustParse("1Gi")}}, nil) + + By(fmt.Sprintf("Creating file-backed LVMVolumeGroup %s on node %s (dir %s)", lvgName, node, e2eFileDevicesBaseDir)) + deleted := false + defer func() { + if !deleted { + e2eDeleteLVGAndWaitGone(e2eCtx, k8sClient, lvgName, 10*time.Minute) + } + }() + created := e2eCreateFileBackedLVGAndWaitReady(e2eCtx, k8sClient, lvg) + + By("Verifying conditions (no errors)") + e2eExpectNoFalseConditions(created) + + By("Verifying status.nodes[].fileDevices") + fds := e2eFileDevicesForNode(created, node) + Expect(fds).To(HaveLen(1), "expected exactly one file device in status for node %s", node) + fd := fds[0] + Expect(fd.FilePath).To(HavePrefix(e2eFileDevicesBaseDir+"/"), "backing file must live under the base dir") + Expect(fd.LoopDevice).To(HavePrefix("/dev/loop"), "loop device path") + Expect(fd.PVUuid).NotTo(BeEmpty(), "file device should be a PV") + printLVMVolumeGroupInfo(created) + + By("Verifying backing file, loop attachment and VG exist on the node") + exists, err := e2eNodePathExists(e2eCtx, testClusterResources.Kubeconfig, node, vmSSH, fd.FilePath) + Expect(err).NotTo(HaveOccurred()) + Expect(exists).To(BeTrue(), "backing file %s should exist on node", fd.FilePath) + bound, loopOut, err := e2eLoopBoundToFileOnNode(e2eCtx, testClusterResources.Kubeconfig, node, vmSSH, fd.FilePath) + Expect(err).NotTo(HaveOccurred()) + Expect(bound).To(BeTrue(), "a loop device should be attached to %s; losetup:\n%s", fd.FilePath, loopOut) + vgListed, vgsOut, err := e2eVGListedOnNode(e2eCtx, testClusterResources.Kubeconfig, node, vmSSH, vgName) + Expect(err).NotTo(HaveOccurred()) + Expect(vgListed).To(BeTrue(), "VG %s should be visible on node; vgs:\n%s", vgName, vgsOut) + + By("Deleting the LVMVolumeGroup and verifying node-side cleanup") + filePath := fd.FilePath + e2eDeleteLVGAndWaitGone(e2eCtx, k8sClient, lvgName, 10*time.Minute) + deleted = true + Eventually(func(g Gomega) { + stillThere, err := e2eNodePathExists(e2eCtx, testClusterResources.Kubeconfig, node, vmSSH, filePath) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(stillThere).To(BeFalse(), "backing file %s should be removed after delete", filePath) + }, 3*time.Minute, 10*time.Second).Should(Succeed()) + stillBound, _, err := e2eLoopBoundToFileOnNode(e2eCtx, testClusterResources.Kubeconfig, node, vmSSH, filePath) + Expect(err).NotTo(HaveOccurred()) + Expect(stillBound).To(BeFalse(), "loop device should be detached from %s after delete", filePath) + + By("✓ File-backed VG created (file+loop+PV+VG), then fully cleaned up on delete") + }) + + It("Should create a thin-pool on a file-backed device", func() { + node := e2ePickNodeRunningAgent(e2eCtx, k8sClient) + runID := strconv.FormatInt(time.Now().UnixNano(), 10) + nodeSuffix := strings.NewReplacer(".", "-", "_", "-").Replace(node) + lvgName := "e2e-lvg-fdtp-" + runID + "-" + nodeSuffix + vgName := "e2e-vg-fdtp-" + runID + thinPoolName := "e2e-thin-fd" + + lvg := newFileLVG(node, lvgName, vgName, + []v1alpha1.LVMVolumeGroupFileDeviceSpec{{Directory: e2eFileDevicesBaseDir, Size: resource.MustParse("2Gi")}}, + []v1alpha1.LVMVolumeGroupThinPoolSpec{{Name: thinPoolName, Size: "50%", AllocationLimit: "100%"}}) + + By(fmt.Sprintf("Creating file-backed LVMVolumeGroup %s with thin-pool %s on node %s", lvgName, thinPoolName, node)) + defer func() { e2eDeleteLVGAndWaitGone(e2eCtx, k8sClient, lvgName, 10*time.Minute) }() + created := e2eCreateFileBackedLVGAndWaitReady(e2eCtx, k8sClient, lvg) + e2eExpectNoFalseConditions(created) + printLVMVolumeGroupInfo(created) + + By("Verifying the thin-pool is Ready in status") + var tp *v1alpha1.LVMVolumeGroupThinPoolStatus + for i := range created.Status.ThinPools { + if created.Status.ThinPools[i].Name == thinPoolName { + tp = &created.Status.ThinPools[i] + break + } + } + Expect(tp).NotTo(BeNil(), "thin-pool %q not found in status", thinPoolName) + Expect(tp.Ready).To(BeTrue(), "thin-pool should be Ready") + + By("Verifying the thin-pool data LV exists on the node") + present, out, err := e2eThinPoolDataLVPresentOnNode(e2eCtx, testClusterResources.Kubeconfig, node, vmSSH, vgName, thinPoolName) + Expect(err).NotTo(HaveOccurred()) + Expect(present).To(BeTrue(), "thin-pool data LV for %s/%s should exist on node; lvs:\n%s", vgName, thinPoolName, out) + + By("✓ Thin-pool created on a file-backed VG and reported Ready") + }) + + It("Should extend a file-backed VG by adding a fileDevices entry", func() { + node := e2ePickNodeRunningAgent(e2eCtx, k8sClient) + runID := strconv.FormatInt(time.Now().UnixNano(), 10) + nodeSuffix := strings.NewReplacer(".", "-", "_", "-").Replace(node) + lvgName := "e2e-lvg-fdext-" + runID + "-" + nodeSuffix + vgName := "e2e-vg-fdext-" + runID + + lvg := newFileLVG(node, lvgName, vgName, + []v1alpha1.LVMVolumeGroupFileDeviceSpec{{Directory: e2eFileDevicesBaseDir, Size: resource.MustParse("1Gi")}}, nil) + + By(fmt.Sprintf("Creating file-backed LVMVolumeGroup %s with one file device on node %s", lvgName, node)) + defer func() { e2eDeleteLVGAndWaitGone(e2eCtx, k8sClient, lvgName, 10*time.Minute) }() + created := e2eCreateFileBackedLVGAndWaitReady(e2eCtx, k8sClient, lvg) + Expect(e2eFileDevicesForNode(created, node)).To(HaveLen(1)) + vgSizeBefore := created.Status.VGSize.Value() + + By("Appending a second fileDevices entry (immutable existing entry + new one)") + Eventually(func(g Gomega) { + var cur v1alpha1.LVMVolumeGroup + g.Expect(k8sClient.Get(e2eCtx, client.ObjectKey{Name: lvgName}, &cur)).To(Succeed()) + // A different size yields a different backing-file path, so the + // new entry does not collide with the existing 1Gi one. + cur.Spec.FileDevices = append(cur.Spec.FileDevices, + v1alpha1.LVMVolumeGroupFileDeviceSpec{Directory: e2eFileDevicesBaseDir, Size: resource.MustParse("2Gi")}) + g.Expect(k8sClient.Update(e2eCtx, &cur)).To(Succeed()) + }, 1*time.Minute, 5*time.Second).Should(Succeed()) + + By("Waiting for the VG to grow with the second file device") + var extended v1alpha1.LVMVolumeGroup + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(e2eCtx, client.ObjectKey{Name: lvgName}, &extended)).To(Succeed()) + g.Expect(extended.Status.Phase).To(Equal(v1alpha1.PhaseReady), "phase should return to Ready after extend") + g.Expect(e2eFileDevicesForNode(&extended, node)).To(HaveLen(2), "status should list two file devices") + g.Expect(extended.Status.VGSize.Value()).To(BeNumerically(">", vgSizeBefore), "VG size should grow after extend") + }, e2eLVMVolumeGroupReadyTimeout, 10*time.Second).Should(Succeed()) + e2eExpectNoFalseConditions(&extended) + printLVMVolumeGroupInfo(&extended) + + By("✓ File-backed VG extended by adding a new fileDevices entry") + }) + + It("Should reattach file devices after the agent restarts", func() { + node := e2ePickNodeRunningAgent(e2eCtx, k8sClient) + runID := strconv.FormatInt(time.Now().UnixNano(), 10) + nodeSuffix := strings.NewReplacer(".", "-", "_", "-").Replace(node) + lvgName := "e2e-lvg-fdreat-" + runID + "-" + nodeSuffix + vgName := "e2e-vg-fdreat-" + runID + + lvg := newFileLVG(node, lvgName, vgName, + []v1alpha1.LVMVolumeGroupFileDeviceSpec{{Directory: e2eFileDevicesBaseDir, Size: resource.MustParse("1Gi")}}, nil) + + By(fmt.Sprintf("Creating file-backed LVMVolumeGroup %s on node %s", lvgName, node)) + defer func() { e2eDeleteLVGAndWaitGone(e2eCtx, k8sClient, lvgName, 10*time.Minute) }() + created := e2eCreateFileBackedLVGAndWaitReady(e2eCtx, k8sClient, lvg) + fdsBefore := e2eFileDevicesForNode(created, node) + Expect(fdsBefore).To(HaveLen(1)) + filePath := fdsBefore[0].FilePath + + By("Restarting the sds-node-configurator agent on the node (triggers ReattachFileDevices at startup)") + restartSDSNodeConfiguratorAgentOnNode(e2eCtx, k8sClient, node) + + By("Verifying the VG stays Ready and the file device is reattached") + var afterRestart v1alpha1.LVMVolumeGroup + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(e2eCtx, client.ObjectKey{Name: lvgName}, &afterRestart)).To(Succeed()) + g.Expect(afterRestart.Status.Phase).To(Equal(v1alpha1.PhaseReady), "phase should stay Ready after agent restart") + fds := e2eFileDevicesForNode(&afterRestart, node) + g.Expect(fds).To(HaveLen(1), "file device should still be reported after restart") + g.Expect(fds[0].FilePath).To(Equal(filePath), "backing file path should be unchanged") + g.Expect(fds[0].LoopDevice).To(HavePrefix("/dev/loop"), "loop device should be reattached") + }, 5*time.Minute, 10*time.Second).Should(Succeed()) + e2eExpectNoFalseConditions(&afterRestart) + + bound, loopOut, err := e2eLoopBoundToFileOnNode(e2eCtx, testClusterResources.Kubeconfig, node, vmSSH, filePath) + Expect(err).NotTo(HaveOccurred()) + Expect(bound).To(BeTrue(), "loop device should be attached to %s after restart; losetup:\n%s", filePath, loopOut) + printLVMVolumeGroupInfo(&afterRestart) + + By("✓ File device reattached and VG stayed Ready after agent restart") + }) + + It("Should reject a fileDevices directory outside the allowed base dir", func() { + node := e2ePickNodeRunningAgent(e2eCtx, k8sClient) + runID := strconv.FormatInt(time.Now().UnixNano(), 10) + nodeSuffix := strings.NewReplacer(".", "-", "_", "-").Replace(node) + lvgName := "e2e-lvg-fdbad-" + runID + "-" + nodeSuffix + vgName := "e2e-vg-fdbad-" + runID + badDir := "/tmp/e2e-filedevices-" + runID + + lvg := newFileLVG(node, lvgName, vgName, + []v1alpha1.LVMVolumeGroupFileDeviceSpec{{Directory: badDir, Size: resource.MustParse("1Gi")}}, nil) + + By(fmt.Sprintf("Creating LVMVolumeGroup %s with an out-of-base directory %s", lvgName, badDir)) + Expect(k8sClient.Create(e2eCtx, lvg)).To(Succeed()) + defer func() { e2eDeleteLVGAndWaitGone(e2eCtx, k8sClient, lvgName, 5*time.Minute) }() + + By("Expecting VGConfigurationApplied=False with ValidationFailed and the base-dir message") + Eventually(func(g Gomega) { + var cur v1alpha1.LVMVolumeGroup + g.Expect(k8sClient.Get(e2eCtx, client.ObjectKey{Name: lvgName}, &cur)).To(Succeed()) + g.Expect(cur.Status.Phase).NotTo(Equal(v1alpha1.PhaseReady)) + var cfg *metav1.Condition + for i := range cur.Status.Conditions { + if cur.Status.Conditions[i].Type == lvgConditionVGConfigurationApplied { + cfg = &cur.Status.Conditions[i] + break + } + } + g.Expect(cfg).NotTo(BeNil(), "VGConfigurationApplied condition should be present") + g.Expect(cfg.Status).To(Equal(metav1.ConditionFalse)) + g.Expect(cfg.Reason).To(Equal(reasonValidationFailed)) + g.Expect(cfg.Message).To(ContainSubstring("or a subdirectory of it")) + }, 3*time.Minute, 8*time.Second).Should(Succeed()) + + By("Verifying no VG and no backing directory were created on the node") + vgListed, vgsOut, err := e2eVGListedOnNode(e2eCtx, testClusterResources.Kubeconfig, node, vmSSH, vgName) + Expect(err).NotTo(HaveOccurred()) + Expect(vgListed).To(BeFalse(), "VG %s must not be created for a rejected spec; vgs:\n%s", vgName, vgsOut) + dirExists, err := e2eNodePathExists(e2eCtx, testClusterResources.Kubeconfig, node, vmSSH, badDir) + Expect(err).NotTo(HaveOccurred()) + Expect(dirExists).To(BeFalse(), "the out-of-base directory %s must not be created", badDir) + + var dump v1alpha1.LVMVolumeGroup + Expect(k8sClient.Get(e2eCtx, client.ObjectKey{Name: lvgName}, &dump)).To(Succeed()) + printLVMVolumeGroupInfo(&dump) + + By("✓ Out-of-base fileDevices directory rejected; nothing provisioned on the node") + }) + }) + ///////////////////////////////////////////////////// ---=== TESTS END HERE ===--- ///////////////////////////////////////////////////// }) // Describe: Sds Node Configurator diff --git a/images/agent/cmd/main.go b/images/agent/cmd/main.go index 00e3b827b..adabfa04d 100644 --- a/images/agent/cmd/main.go +++ b/images/agent/cmd/main.go @@ -23,6 +23,7 @@ import ( "os/signal" goruntime "runtime" "syscall" + "time" v1 "k8s.io/api/core/v1" sv1 "k8s.io/api/storage/v1" @@ -146,6 +147,20 @@ func run() int { log.Error(err, "[main] unable to run ReTag") } + log.Info("[main] ReattachFileDevices starts") + // Best-effort fast path: re-establish loop mappings for file-backed + // PVs so ActivateVGs can see them right after a reboot. A failure + // here must NOT suppress ActivateAllManagedVGs — that would hold + // every healthy VG on the node (including pure block-device ones) + // hostage to one unrelated file device. The LVG reconciler retries + // reattach idempotently via provisionFileDevices on its next pass, + // so a transient losetup failure recovers without a pod restart. + if err := reattachFileDevicesAtStartup(ctx, log, mgr, commands, cfgParams.NodeName, cfgParams.CmdDeadlineDuration); err != nil { + log.Error(err, "[main] file device reattach failed; the LVG reconciler will retry, continuing to ActivateVGs") + } else { + log.Info("[main] ReattachFileDevices completed") + } + log.Info("[main] ActivateVGs starts") if err := utils.ActivateAllManagedVGs(ctx, log, commands, metrics, cfgParams.CmdDeadlineDuration); err != nil { log.Error(err, "[main] unable to activate managed VGs") @@ -231,6 +246,8 @@ func run() int { NodeName: cfgParams.NodeName, VolumeGroupScanInterval: cfgParams.VolumeGroupScanInterval, BlockDeviceScanInterval: cfgParams.BlockDeviceScanInterval, + CmdDeadlineDuration: cfgParams.CmdDeadlineDuration, + FileDevicesDirectory: cfgParams.FileDevicesDirectory, }, ), ) @@ -319,3 +336,72 @@ func run() int { log.Info("[main] successfully starts the manager") return 0 } + +func reattachFileDevicesAtStartup(ctx context.Context, log logger.Logger, mgr manager.Manager, commands utils.Commands, nodeName string, cmdTimeout time.Duration) error { + var lvgList v1alpha1.LVMVolumeGroupList + if err := mgr.GetAPIReader().List(ctx, &lvgList); err != nil { + log.Error(err, "[reattachFileDevicesAtStartup] unable to list LVMVolumeGroups") + return err + } + + var items []utils.LVGWithFileDevices + for _, lvg := range lvgList.Items { + if lvg.Spec.Local.NodeName != nodeName { + continue + } + + // Collect backing-file paths from BOTH the observed status and the + // desired spec. Status alone is not enough: a node can reboot after + // the LVG was provisioned but before the discoverer wrote + // status.nodes[].fileDevices, in which case reattach would find + // nothing and ActivateVGs would bring the VG up missing its loop PV. + // The spec path is deterministic (BuildFileDevicePath), so it lets + // reattach recover even without a status entry. ReattachFileDevices + // never creates files (only losetup --find on an existing backing + // file), so a spec entry whose file does not exist yet is a harmless + // best-effort miss the LVG reconciler provisions on its next pass. + seen := make(map[string]struct{}) + fds := make([]utils.FileDeviceStatus, 0) + for _, node := range lvg.Status.Nodes { + if node.Name != nodeName { + continue + } + for _, fd := range node.FileDevices { + if fd.FilePath == "" { + continue + } + if _, ok := seen[fd.FilePath]; ok { + continue + } + seen[fd.FilePath] = struct{}{} + fds = append(fds, utils.FileDeviceStatus{ + FilePath: fd.FilePath, + LoopDevice: fd.LoopDevice, + }) + } + } + for _, fd := range lvg.Spec.FileDevices { + path := utils.BuildFileDevicePath(fd.Directory, lvg.Name, fd.Size) + if _, ok := seen[path]; ok { + continue + } + seen[path] = struct{}{} + fds = append(fds, utils.FileDeviceStatus{FilePath: path}) + } + + if len(fds) == 0 { + continue + } + items = append(items, utils.LVGWithFileDevices{ + LVGName: lvg.Name, + FileDevices: fds, + }) + } + + if len(items) == 0 { + log.Debug("[reattachFileDevicesAtStartup] no file devices to reattach") + return nil + } + + return utils.ReattachFileDevices(ctx, log, commands, cmdTimeout, items) +} diff --git a/images/agent/internal/config/config.go b/images/agent/internal/config/config.go index 79a6a762d..db6a9cd15 100644 --- a/images/agent/internal/config/config.go +++ b/images/agent/internal/config/config.go @@ -39,6 +39,13 @@ const ( CmdDeadlineDuration = "CMD_DEADLINE_DURATION" DefaultHealthProbeBindAddressEnvName = "HEALTH_PROBE_BIND_ADDRESS" DefaultHealthProbeBindAddress = ":4228" + FileDevicesDirectory = "FILE_DEVICES_DIRECTORY" + // DefaultFileDevicesDirectory is the base directory backing files are + // confined to when the module config does not override it. It lives under + // the module's own /opt/deckhouse/sds tree so a stray fileDevices entry + // cannot fill an arbitrary host path. Keep in sync with the + // fileDevicesDirectory default in openapi/config-values.yaml. + DefaultFileDevicesDirectory = "/opt/deckhouse/sds/file-devices" ) type Config struct { @@ -53,6 +60,7 @@ type Config struct { ThrottleInterval time.Duration CmdDeadlineDuration time.Duration HealthProbeBindAddress string + FileDevicesDirectory string } func NewConfig() (*Config, error) { @@ -86,6 +94,11 @@ func NewConfig() (*Config, error) { cfg.HealthProbeBindAddress = DefaultHealthProbeBindAddress } + cfg.FileDevicesDirectory = os.Getenv(FileDevicesDirectory) + if cfg.FileDevicesDirectory == "" { + cfg.FileDevicesDirectory = DefaultFileDevicesDirectory + } + scanInt := os.Getenv(ScanInterval) if scanInt == "" { cfg.BlockDeviceScanInterval = 5 * time.Second diff --git a/images/agent/internal/config/config_test.go b/images/agent/internal/config/config_test.go index 0fc4aba21..496483173 100644 --- a/images/agent/internal/config/config_test.go +++ b/images/agent/internal/config/config_test.go @@ -109,6 +109,34 @@ func TestNewConfig(t *testing.T) { assert.EqualError(t, err, expErrorMsg) }) + t.Run("FileDevicesDirectoryNotSet_ReturnsDefault", func(t *testing.T) { + err := os.Setenv(NodeName, "test-node") + assert.NoError(t, err) + err = os.Setenv(MachineID, "test-id") + assert.NoError(t, err) + defer os.Clearenv() + + opts, err := NewConfig() + if assert.NoError(t, err) { + assert.Equal(t, DefaultFileDevicesDirectory, opts.FileDevicesDirectory) + } + }) + + t.Run("FileDevicesDirectorySet_IsHonoured", func(t *testing.T) { + err := os.Setenv(NodeName, "test-node") + assert.NoError(t, err) + err = os.Setenv(MachineID, "test-id") + assert.NoError(t, err) + err = os.Setenv(FileDevicesDirectory, "/mnt/data/file-devices") + assert.NoError(t, err) + defer os.Clearenv() + + opts, err := NewConfig() + if assert.NoError(t, err) { + assert.Equal(t, "/mnt/data/file-devices", opts.FileDevicesDirectory) + } + }) + t.Run("MetricsPortNotSet_ReturnsDefaultPort", func(t *testing.T) { expNodeName := "test-node" expMetricsPort := ":4202" diff --git a/images/agent/internal/const.go b/images/agent/internal/const.go index 5d5d24814..955085d89 100644 --- a/images/agent/internal/const.go +++ b/images/agent/internal/const.go @@ -43,8 +43,15 @@ const ( // LVMGlobalFilter is passed via `lvm --config` for every LVM // subcommand the agent runs. It rejects canonical names of block // devices that always belong to a foreign storage layer (Ceph RBD, - // DRBD, NBD, loopback) so lvm does not even read PV labels - // from them when udev integration is unavailable. + // DRBD, NBD) so lvm does not even read PV labels from them + // when udev integration is unavailable. + // + // Loop devices are intentionally NOT rejected: the agent manages + // file-backed loop devices as LVM PVs (spec.fileDevices). Unmanaged + // loop-backed VGs never become candidates (the discoverer only adopts + // VGs tagged storage.deckhouse.io/enabled=true) and are additionally + // dropped from the cache by utils.FilterForeignLoopPVs so they cannot + // collide by name with a managed VG. // // There is intentionally no blanket "a|.*|" accept rule. When a // device matches none of the reject patterns, LVM accepts it by @@ -57,7 +64,7 @@ const ( // The authoritative foreign-PV filter (FilterForeignPVs) still runs // after lvm returns and catches any PVs that slip through // via /dev/block/MAJ:MIN or /dev/disk/by-id/... aliases. - LVMGlobalFilter = `devices/global_filter=["r|^/dev/rbd|","r|^/dev/drbd|","r|^/dev/nbd|","r|^/dev/loop|"]` + LVMGlobalFilter = `devices/global_filter=["r|^/dev/rbd|","r|^/dev/drbd|","r|^/dev/nbd|"]` // LVMArchiveRetention caps the size of /etc/lvm/archive: keep at // most the last 10 metadata snapshots and at most 7 days of history. @@ -79,6 +86,13 @@ const ( ReasonScanFailed = "ScanFailed" ReasonUpdated = "Updated" ReasonApplied = "Applied" + // ReasonAliasResolutionFailed is set when the agent has repeatedly failed + // to canonicalize the alias-form PV names it needs to decide whether a + // file-backed loop device is already part of the VG. Unlike the transient + // ReasonUpdating requeue, this reason signals a stuck resolver (e.g. a + // missing nsenter binary or a genuinely broken alias) that will not clear + // on its own, so it can be alerted on distinctly. + ReasonAliasResolutionFailed = "AliasResolutionFailed" MetadataNameLabelKey = "kubernetes.io/metadata.name" HostNameLabelKey = "kubernetes.io/hostname" @@ -94,6 +108,20 @@ const ( DeletionProtectionAnnotation = "storage.deckhouse.io/deletion-protection" LVMVolumeGroupTag = "storage.deckhouse.io/lvmVolumeGroupName" LVGMetadataNameLabelKey = "kubernetes.io/metadata.name" + + // FileDeviceImageSuffix is the trailing component the agent appends + // to every backing file it creates for spec.fileDevices entries. + FileDeviceImageSuffix = ".img" + + // FileDevicePrefix anchors the basename of every backing file the + // agent creates. The full pattern is `sds--.img` and + // is the sole owner marker: the discoverer treats a loop PV as a + // managed file device only when its backing file's basename matches + // this pattern (see utils.IsManagedFileDevicePath). Reusing the LVG + // name in the basename means a foreign loop device backed by an + // unrelated file (a libvirt qcow2, a snap, …) is never misidentified + // as ours during discovery or cleanup. + FileDevicePrefix = "sds-" ) var ( @@ -110,11 +138,17 @@ var ( // rbd - Ceph RBD (kernel rbd module, major 251) // drbd - DRBD (sds-replicated-volume, major 147) // nbd - network block device (major 43) - // loop - loopback (major 7) — typically backs QEMU/file-based VM disks + // + // Loop devices (major 7) are NOT in this list: the agent manages + // file-backed loop devices as LVM PVs via spec.fileDevices. + // Unmanaged loop PVs forming a whole VG are dropped from the cache by + // utils.FilterForeignLoopPVs so they cannot collide by name with a + // managed VG; the discoverer additionally only acts on VGs tagged with + // storage.deckhouse.io/enabled=true. // // Used after lvm returns the PV list, against the canonical // path resolved via readlink -f in the host mount namespace. - ForeignDeviceBasePrefixes = []string{"rbd", "drbd", "nbd", "loop"} + ForeignDeviceBasePrefixes = []string{"rbd", "drbd", "nbd"} ) const ( diff --git a/images/agent/internal/controller/lvg/discoverer.go b/images/agent/internal/controller/lvg/discoverer.go index 008ea1013..14e3c5743 100644 --- a/images/agent/internal/controller/lvg/discoverer.go +++ b/images/agent/internal/controller/lvg/discoverer.go @@ -131,7 +131,7 @@ func (d *Discoverer) LVMVolumeGroupDiscoverReconcile(ctx context.Context) bool { d.sdsCache.StoreManagedVGs(maps.Keys(filteredLVGs)) d.log.Debug("[RunLVMVolumeGroupDiscoverController] tries to get LVMVolumeGroup candidates") - candidates, err := d.GetLVMVolumeGroupCandidates(blockDevices) + candidates, err := d.GetLVMVolumeGroupCandidates(ctx, blockDevices) if err != nil { d.log.Error(err, "[RunLVMVolumeGroupDiscoverController] unable to run GetLVMVolumeGroupCandidates") for _, lvg := range filteredLVGs { @@ -355,7 +355,7 @@ func (d *Discoverer) ReconcileUnhealthyLVMVolumeGroups( return candidates, nil } -func (d *Discoverer) GetLVMVolumeGroupCandidates(bds map[string]v1alpha1.BlockDevice) ([]internal.LVMVolumeGroupCandidate, error) { +func (d *Discoverer) GetLVMVolumeGroupCandidates(ctx context.Context, bds map[string]v1alpha1.BlockDevice) ([]internal.LVMVolumeGroupCandidate, error) { vgs, vgErrs := d.sdsCache.GetVGs() vgWithTag := filterVGByTag(vgs, internal.LVMTags) candidates := make([]internal.LVMVolumeGroupCandidate, 0, len(vgWithTag)) @@ -427,8 +427,8 @@ func (d *Discoverer) GetLVMVolumeGroupCandidates(bds map[string]v1alpha1.BlockDe VGFree: *resource.NewQuantity(vg.VGFree.Value(), resource.BinarySI), VGUUID: vg.VGUUID, ExtentSize: *resource.NewQuantity(vg.VGExtentSize.Value(), resource.BinarySI), - Nodes: d.configureCandidateNodeDevices(sortedPVs, sortedBDs, vg, d.cfg.NodeName), } + candidate.Nodes, candidate.FileDeviceNodes = d.configureCandidateNodeDevices(ctx, sortedPVs, sortedBDs, vg, d.cfg.NodeName) candidates = append(candidates, candidate) } @@ -460,7 +460,7 @@ func (d *Discoverer) CreateLVMVolumeGroupByCandidate( }, Status: v1alpha1.LVMVolumeGroupStatus{ AllocatedSize: candidate.AllocatedSize, - Nodes: convertLVMVGNodes(candidate.Nodes), + Nodes: convertLVMVGNodes(candidate.Nodes, candidate.FileDeviceNodes), ThinPools: thinPools, VGSize: candidate.VGSize, VGUuid: candidate.VGUUID, @@ -545,7 +545,7 @@ func (d *Discoverer) UpdateLVMVolumeGroupByCandidate( } lvg.Status.AllocatedSize = candidate.AllocatedSize - lvg.Status.Nodes = convertLVMVGNodes(candidate.Nodes) + lvg.Status.Nodes = convertLVMVGNodes(candidate.Nodes, candidate.FileDeviceNodes) lvg.Status.ThinPools = thinPools lvg.Status.VGSize = candidate.VGSize lvg.Status.VGFree = candidate.VGFree @@ -569,22 +569,50 @@ func (d *Discoverer) UpdateLVMVolumeGroupByCandidate( return err } -func (d *Discoverer) configureCandidateNodeDevices(pvs map[string][]internal.PVData, bds map[string][]v1alpha1.BlockDevice, vg internal.VGData, currentNode string) map[string][]internal.LVMVGDevice { +func (d *Discoverer) configureCandidateNodeDevices(ctx context.Context, pvs map[string][]internal.PVData, bds map[string][]v1alpha1.BlockDevice, vg internal.VGData, currentNode string) (map[string][]internal.LVMVGDevice, map[string][]internal.LVMVGFileDevice) { filteredPV := pvs[vg.VGName+vg.VGUUID] filteredBds := bds[vg.VGName+vg.VGUUID] bdPathStatus := make(map[string]v1alpha1.BlockDevice, len(bds)) result := make(map[string][]internal.LVMVGDevice, len(filteredPV)) + fileResult := make(map[string][]internal.LVMVGFileDevice) for _, blockDevice := range filteredBds { bdPathStatus[blockDevice.Status.Path] = blockDevice } + // The agent stamps the owning LVMVolumeGroup name onto its VGs via + // the storage.deckhouse.io/lvmVolumeGroupName tag — use it as the + // authoritative owner for any loop PV that lives in this VG. An + // untagged VG (foreign LVM imported by hand and then tagged enabled) + // must never be claimed; in that case we leave file-device discovery + // off entirely for safety. + _, ownerLVGName := utils.ReadValueFromTags(vg.VGTags, internal.LVMVolumeGroupTag) + for _, pv := range filteredPV { + if strings.HasPrefix(pv.PVName, "/dev/loop") { + fileDev := d.buildFileDeviceFromLoopPV(ctx, pv, ownerLVGName, false) + if fileDev != nil { + fileResult[currentNode] = append(fileResult[currentNode], *fileDev) + } + continue + } + bd, exist := bdPathStatus[pv.PVName] - // this is very rare case which might occurred while VG extend operation goes. In this case, in the cache the controller - // sees a new PV included in the VG, but BlockDeviceDiscover did not update the corresponding BlockDevice resource on time, - // so the BlockDevice resource does not have any info, that it is in the VG. if !exist { + // When udev integration is unavailable LVM may report a managed + // loop PV under a /dev/block/MAJ:MIN or /dev/disk/by-id alias + // instead of /dev/loopN (the same aliasing FilterForeignPVs + // resolves). Probe it as a loop device before giving up: + // buildFileDeviceFromLoopPV refuses anything whose backing file + // is not one of ours, and a non-loop device simply yields no + // backing file. The probe is quiet (Debug) so a genuine + // not-yet-registered BlockDevice does not spam warnings. + if ownerLVGName != "" { + if fileDev := d.buildFileDeviceFromLoopPV(ctx, pv, ownerLVGName, true); fileDev != nil { + fileResult[currentNode] = append(fileResult[currentNode], *fileDev) + continue + } + } d.log.Warning(fmt.Sprintf("[configureCandidateNodeDevices] no BlockDevice resource is yet configured for PV %s in VG %s, retry on the next iteration", pv.PVName, vg.VGName)) continue } @@ -601,7 +629,98 @@ func (d *Discoverer) configureCandidateNodeDevices(pvs map[string][]internal.PVD result[currentNode] = append(result[currentNode], device) } - return result + return result, fileResult +} + +// buildFileDeviceFromLoopPV resolves the backing file for a loop PV and +// returns a managed file-device record iff the basename matches the +// owner pattern produced by utils.BuildFileDevicePath. Any other loop +// PV — a foreign losetup-attached qcow2, a snap squashfs loop that +// somehow ended up in a VG, a manual experiment — is skipped with a +// warning so the agent never writes its path into status (and therefore +// never tries to rm it later during cleanup). +// +// expectedLVGName is the owner name pulled from the VG tag; when empty +// the function refuses to claim any loop PV. +// +// probe is set when the PV is not a /dev/loopN node but an alias we are +// only speculatively checking (it was not found among BlockDevices): in +// that mode "not a loop" / "not ours" outcomes are expected and logged at +// Debug instead of Warning, so an ordinary not-yet-registered BlockDevice +// does not produce misleading warnings. +// +// NOTE: this is one of two places that canonicalize an alias-reported loop +// PV. Here we resolve backing-file → canonical /dev/loopN via losetup (we +// have the backing file and need the loop). The reconciler does the inverse +// in Reconciler.loopAlreadyRegisteredAsPV (canonical loop is known, alias PV +// names are resolved via readlink). They use different methods because their +// inputs differ; keep their ownership/aliasing assumptions in sync. +func (d *Discoverer) buildFileDeviceFromLoopPV(ctx context.Context, pv internal.PVData, expectedLVGName string, probe bool) *internal.LVMVGFileDevice { + if expectedLVGName == "" { + d.log.Warning(fmt.Sprintf("[buildFileDeviceFromLoopPV] VG of loop PV %s carries no %s tag; skipping file-device discovery", pv.PVName, internal.LVMVolumeGroupTag)) + return nil + } + + // A short timeout protects the discoverer's reconcile loop from a + // hung losetup: this call runs once per loop PV per cycle. + ctx, cancel := context.WithTimeout(ctx, 10*time.Second) + defer cancel() + + cmd, backingFile, err := d.commands.GetLoopBackingFile(ctx, pv.PVName) + d.log.Debug(cmd) + if err != nil { + msg := fmt.Sprintf("[buildFileDeviceFromLoopPV] unable to read backing file for %s: %v", pv.PVName, err) + if probe { + d.log.Debug(msg) + } else { + d.log.Warning(msg) + } + return nil + } + if backingFile == "" { + return nil + } + + if !utils.IsManagedFileDevicePath(backingFile, expectedLVGName) { + msg := fmt.Sprintf("[buildFileDeviceFromLoopPV] backing file %q of loop PV %s does not match managed pattern for LVG %s; skipping", backingFile, pv.PVName, expectedLVGName) + if probe { + d.log.Debug(msg) + } else { + d.log.Warning(msg) + } + return nil + } + + // Status must record the canonical /dev/loopN device, never an alias. + // In the probe path pv.PVName is a /dev/disk/by-id or /dev/block alias; + // recording it verbatim makes status.loopDevice flip-flop between the + // alias and /dev/loopN across reconciles (whichever lvm happens to + // report), churning the resource with no-op updates. Re-resolve the + // canonical loop from the backing file; keep pv.PVName only if that fails. + loopDevice := pv.PVName + if !strings.HasPrefix(loopDevice, "/dev/loop") { + findCmd, canonical, ferr := d.commands.FindLoopDeviceByFile(ctx, backingFile) + d.log.Debug(findCmd) + if ferr != nil { + d.log.Debug(fmt.Sprintf("[buildFileDeviceFromLoopPV] unable to resolve canonical loop for %s: %v; keeping %s", backingFile, ferr, pv.PVName)) + } else if canonical != "" { + loopDevice = canonical + } + } + + return &internal.LVMVGFileDevice{ + FilePath: backingFile, + // Size is the PV size, not the raw backing-file size: it is what lvm + // reports for this device and is always slightly smaller than the + // requested spec.fileDevices[].size (LVM metadata overhead). This + // mirrors how block devices expose status...devices[].pvSize, and keeps + // the value self-consistent across reconciles (hasStatusNodesDiff + // compares PV size against PV size, so it never churns). Callers that + // need the requested size must read spec, not status. + LoopDevice: loopDevice, + Size: *resource.NewQuantity(pv.PVSize.Value(), resource.BinarySI), + PVUUID: pv.PVUuid, + } } func checkVGHealth(vgIssues map[string]string, pvIssues map[string][]string, lvIssues map[string]map[string]string, vg internal.VGData) (health, message string) { @@ -879,7 +998,7 @@ func hasLVMVolumeGroupDiff(log logger.Logger, lvg v1alpha1.LVMVolumeGroup, candi } log.Trace(fmt.Sprintf(`VGSize, candidate: %s, lvg: %s`, candidate.VGSize.String(), lvg.Status.VGSize.String())) log.Trace(fmt.Sprintf(`VGUUID, candidate: %s, lvg: %s`, candidate.VGUUID, lvg.Status.VGUuid)) - log.Trace(fmt.Sprintf(`Nodes, candidate: %+v, lvg: %+v`, convertLVMVGNodes(candidate.Nodes), lvg.Status.Nodes)) + log.Trace(fmt.Sprintf(`Nodes, candidate: %+v, lvg: %+v`, convertLVMVGNodes(candidate.Nodes, candidate.FileDeviceNodes), lvg.Status.Nodes)) return candidate.AllocatedSize.Value() != lvg.Status.AllocatedSize.Value() || hasStatusPoolDiff(convertedStatusPools, lvg.Status.ThinPools) || @@ -887,7 +1006,7 @@ func hasLVMVolumeGroupDiff(log logger.Logger, lvg v1alpha1.LVMVolumeGroup, candi candidate.VGFree.Value() != lvg.Status.VGFree.Value() || candidate.VGUUID != lvg.Status.VGUuid || candidate.ExtentSize.Value() != lvg.Status.ExtentSize.Value() || - hasStatusNodesDiff(log, convertLVMVGNodes(candidate.Nodes), lvg.Status.Nodes) + hasStatusNodesDiff(log, convertLVMVGNodes(candidate.Nodes, candidate.FileDeviceNodes), lvg.Status.Nodes) } func hasStatusNodesDiff(log logger.Logger, first, second []v1alpha1.LVMVolumeGroupNode) bool { @@ -915,6 +1034,45 @@ func hasStatusNodesDiff(log logger.Logger, first, second []v1alpha1.LVMVolumeGro return true } } + + // File devices must be diffed too: a loop minor can change across + // reboots (ReattachFileDevices re-attaches via `losetup --find`), so + // without this the discoverer would never refresh a stale + // loopDevice/pvUUID in status, and cleanup on delete would later act + // on the stale value. + // + // Match by FilePath rather than by slice position: the candidate + // slice is built in raw `lvm pvs` report order (sortPVsByVG does not + // sort within a VG), and a loop PV can be reported under a /dev/loopN + // or an alias on different scans, so the two slices may list the same + // devices in a different order. A positional comparison would then + // flag a spurious diff and rewrite status on every reconcile. + if hasStatusFileDevicesDiff(first[i].FileDevices, second[i].FileDevices) { + return true + } + } + + return false +} + +func hasStatusFileDevicesDiff(first, second []v1alpha1.LVMVolumeGroupFileDevice) bool { + if len(first) != len(second) { + return true + } + + byPath := make(map[string]v1alpha1.LVMVolumeGroupFileDevice, len(second)) + for _, fd := range second { + byPath[fd.FilePath] = fd + } + + for _, fd := range first { + other, ok := byPath[fd.FilePath] + if !ok || + fd.LoopDevice != other.LoopDevice || + fd.PVUuid != other.PVUuid || + fd.Size.Value() != other.Size.Value() { + return true + } } return false @@ -964,19 +1122,58 @@ func configureBlockDeviceSelector(candidate internal.LVMVolumeGroupCandidate) *m } } -func convertLVMVGNodes(nodes map[string][]internal.LVMVGDevice) []v1alpha1.LVMVolumeGroupNode { - lvmvgNodes := make([]v1alpha1.LVMVolumeGroupNode, 0, len(nodes)) +func convertLVMVGNodes(nodes map[string][]internal.LVMVGDevice, fileNodes map[string][]internal.LVMVGFileDevice) []v1alpha1.LVMVolumeGroupNode { + // Fast path for the overwhelmingly common case of no file devices + // (every pure block-device LVG): a single pass over nodes, no extra + // node-name set to allocate. This runs per LVG on every discover + // reconcile via hasLVMVolumeGroupDiff, so the allocation matters. + if len(fileNodes) == 0 { + lvmvgNodes := make([]v1alpha1.LVMVolumeGroupNode, 0, len(nodes)) + for nodeName, nodeDevices := range nodes { + lvmvgNodes = append(lvmvgNodes, v1alpha1.LVMVolumeGroupNode{ + Devices: convertLVMVGDevices(nodeDevices), + Name: nodeName, + }) + } + return lvmvgNodes + } + + allNodeNames := make(map[string]struct{}, len(nodes)+len(fileNodes)) + for n := range nodes { + allNodeNames[n] = struct{}{} + } + for n := range fileNodes { + allNodeNames[n] = struct{}{} + } - for nodeName, nodeDevices := range nodes { - lvmvgNodes = append(lvmvgNodes, v1alpha1.LVMVolumeGroupNode{ - Devices: convertLVMVGDevices(nodeDevices), + lvmvgNodes := make([]v1alpha1.LVMVolumeGroupNode, 0, len(allNodeNames)) + for nodeName := range allNodeNames { + node := v1alpha1.LVMVolumeGroupNode{ + Devices: convertLVMVGDevices(nodes[nodeName]), Name: nodeName, - }) + } + if fds := fileNodes[nodeName]; len(fds) > 0 { + node.FileDevices = convertLVMVGFileDevices(fds) + } + lvmvgNodes = append(lvmvgNodes, node) } return lvmvgNodes } +func convertLVMVGFileDevices(devices []internal.LVMVGFileDevice) []v1alpha1.LVMVolumeGroupFileDevice { + result := make([]v1alpha1.LVMVolumeGroupFileDevice, 0, len(devices)) + for _, dev := range devices { + result = append(result, v1alpha1.LVMVolumeGroupFileDevice{ + FilePath: dev.FilePath, + LoopDevice: dev.LoopDevice, + Size: dev.Size, + PVUuid: dev.PVUUID, + }) + } + return result +} + func convertLVMVGDevices(devices []internal.LVMVGDevice) []v1alpha1.LVMVolumeGroupDevice { convertedDevices := make([]v1alpha1.LVMVolumeGroupDevice, 0, len(devices)) diff --git a/images/agent/internal/controller/lvg/discoverer_test.go b/images/agent/internal/controller/lvg/discoverer_test.go index 81a4b4c1e..ea2209c2b 100644 --- a/images/agent/internal/controller/lvg/discoverer_test.go +++ b/images/agent/internal/controller/lvg/discoverer_test.go @@ -334,7 +334,7 @@ func TestLVMVolumeGroupDiscover(t *testing.T) { mp := map[string][]v1alpha1.BlockDevice{vgName + vgUUID: bds} ar := map[string][]internal.PVData{vgName + vgUUID: pvs} - actual := setupDiscoverer(nil).configureCandidateNodeDevices(ar, mp, vg, nodeName) + actual, _ := setupDiscoverer(nil).configureCandidateNodeDevices(context.Background(), ar, mp, vg, nodeName) assert.Equal(t, expected, actual) }) @@ -501,7 +501,7 @@ func TestLVMVolumeGroupDiscover(t *testing.T) { }, Status: v1alpha1.LVMVolumeGroupStatus{ AllocatedSize: size10G, - Nodes: convertLVMVGNodes(nodes), + Nodes: convertLVMVGNodes(nodes, nil), ThinPools: thinPools, VGSize: size10G, VGUuid: VGUUID, @@ -857,7 +857,7 @@ func TestLVMVolumeGroupDiscover(t *testing.T) { }, Status: v1alpha1.LVMVolumeGroupStatus{ AllocatedSize: resource.MustParse("9765625Ki"), - Nodes: convertLVMVGNodes(nodes), + Nodes: convertLVMVGNodes(nodes, nil), ThinPools: thinPools, VGSize: resource.MustParse("9765625Ki"), }, @@ -922,7 +922,7 @@ func TestLVMVolumeGroupDiscover(t *testing.T) { lvmVolumeGroup := v1alpha1.LVMVolumeGroup{ Status: v1alpha1.LVMVolumeGroupStatus{ AllocatedSize: allocatedSize, - Nodes: convertLVMVGNodes(nodes), + Nodes: convertLVMVGNodes(nodes, nil), ThinPools: thinPools, VGSize: vgSize, VGFree: *resource.NewQuantity(vgFree.Value()+10000, resource.BinarySI), @@ -1001,3 +1001,51 @@ func setupDiscoverer(opts *DiscovererConfig) *Discoverer { return NewDiscoverer(cl, log, metrics, cache.New(), utils.NewCommands(), *opts) } + +// hasStatusFileDevicesDiff must match file devices by FilePath, not by slice +// position: the candidate slice is built in raw `lvm pvs` report order while +// status holds a previous reconcile's order, and a loop PV can be reported +// under /dev/loopN or an alias on different scans. A positional comparison +// would flag a spurious diff and churn status on every reconcile. +func TestHasStatusFileDevicesDiff(t *testing.T) { + fdA := v1alpha1.LVMVolumeGroupFileDevice{ + FilePath: "/data/sds-vg-a-aaaaaaaaaaaa.img", + LoopDevice: "/dev/loop0", + PVUuid: "uuid-a", + Size: resource.MustParse("10Gi"), + } + fdB := v1alpha1.LVMVolumeGroupFileDevice{ + FilePath: "/data/sds-vg-a-bbbbbbbbbbbb.img", + LoopDevice: "/dev/loop1", + PVUuid: "uuid-b", + Size: resource.MustParse("20Gi"), + } + + t.Run("same_set_different_order_is_not_a_diff", func(t *testing.T) { + first := []v1alpha1.LVMVolumeGroupFileDevice{fdA, fdB} + second := []v1alpha1.LVMVolumeGroupFileDevice{fdB, fdA} + assert.False(t, hasStatusFileDevicesDiff(first, second)) + }) + + t.Run("changed_loop_device_is_a_diff", func(t *testing.T) { + moved := fdA + moved.LoopDevice = "/dev/loop7" + first := []v1alpha1.LVMVolumeGroupFileDevice{moved, fdB} + second := []v1alpha1.LVMVolumeGroupFileDevice{fdA, fdB} + assert.True(t, hasStatusFileDevicesDiff(first, second)) + }) + + t.Run("different_length_is_a_diff", func(t *testing.T) { + first := []v1alpha1.LVMVolumeGroupFileDevice{fdA, fdB} + second := []v1alpha1.LVMVolumeGroupFileDevice{fdA} + assert.True(t, hasStatusFileDevicesDiff(first, second)) + }) + + t.Run("different_file_path_is_a_diff", func(t *testing.T) { + renamed := fdB + renamed.FilePath = "/data/sds-vg-a-cccccccccccc.img" + first := []v1alpha1.LVMVolumeGroupFileDevice{fdA, renamed} + second := []v1alpha1.LVMVolumeGroupFileDevice{fdA, fdB} + assert.True(t, hasStatusFileDevicesDiff(first, second)) + }) +} diff --git a/images/agent/internal/controller/lvg/reconciler.go b/images/agent/internal/controller/lvg/reconciler.go index 8da45ab30..190523873 100644 --- a/images/agent/internal/controller/lvg/reconciler.go +++ b/images/agent/internal/controller/lvg/reconciler.go @@ -20,9 +20,12 @@ import ( "context" "errors" "fmt" + "path/filepath" "reflect" "slices" + "sort" "strings" + "sync" "time" "k8s.io/apimachinery/pkg/api/resource" @@ -68,6 +71,31 @@ func extentSizeForThinPoolAlign(lvg *v1alpha1.LVMVolumeGroup, vg *internal.VGDat return lvmDefaultPhysicalExtent } +// alignThinPoolSizeForValidation aligns a thin-pool's requested size to the +// extent boundary for the create-time capacity check. +// +// Absolute sizes are rounded UP — that is the real number of extents LVM will +// consume, so an oversized absolute pool must still be rejected. +// +// Percentage sizes (e.g. "100%") are rounded DOWN instead. During create +// validation the VG size is the raw block-device/file sum, which is not +// extent-aligned, so a "100%" request rounded up lands one extent past the VG +// and would wrongly fail the capacity check — even though the pool is created +// with %FREE and fits. A percentage of the VG can never legitimately exceed +// it, so flooring is always safe and also makes split percentages (e.g. +// "50%"+"50%") sum to at most the VG size. +func alignThinPoolSizeForValidation(specSize string, requested, extentSize resource.Quantity) (resource.Quantity, error) { + if utils.IsPercentSize(specSize) { + extentBytes := extentSize.Value() + if extentBytes <= 0 { + return resource.Quantity{}, fmt.Errorf("extent size must be positive, got %d", extentBytes) + } + floored := (requested.Value() / extentBytes) * extentBytes + return *resource.NewQuantity(floored, resource.BinarySI), nil + } + return utils.AlignSizeToExtent(requested, extentSize) +} + type Reconciler struct { cl client.Client log logger.Logger @@ -77,12 +105,39 @@ type Reconciler struct { sdsCache *cache.Cache cfg ReconcilerConfig commands utils.Commands + // resolver maps a /dev/* PV name to its canonical block device. It is + // only used to recognise a managed loop PV that lvm.static reported + // under a /dev/disk/by-id or /dev/block/MAJ:MIN alias. Defaults to + // utils.HostNsenterCanonicalResolver; overridable in tests. + resolver utils.CanonicalPathResolver + + // aliasResolveFailures counts, per LVG name, how many consecutive + // extendFileDevicesIfNeeded rounds made no progress purely because + // alias-form PV names could not be resolved. It escalates the condition + // from a generic "Updating" retry to ReasonAliasResolutionFailed once the + // failures look persistent, so a stuck resolver is alertable instead of + // looking like an ordinary in-flight update. Reset on any round that makes + // progress or genuinely has nothing to do. The reconciler runs with + // MaxConcurrentReconciles==1, but the mutex keeps the map safe if that ever + // changes. + aliasResolveFailuresMu sync.Mutex + aliasResolveFailures map[string]int } +// aliasResolveFailureEscalationThreshold is the number of consecutive +// resolver-only failed rounds after which extendFileDevicesIfNeeded switches +// the VGConfigurationApplied reason to ReasonAliasResolutionFailed. +const aliasResolveFailureEscalationThreshold = 3 + type ReconcilerConfig struct { NodeName string BlockDeviceScanInterval time.Duration VolumeGroupScanInterval time.Duration + CmdDeadlineDuration time.Duration + // FileDevicesDirectory is the base directory backing files are confined + // to; spec.fileDevices[].directory must be this path or a subdirectory of + // it. Empty means "no restriction" (used by unit tests that do not care). + FileDevicesDirectory string } func NewReconciler( @@ -103,11 +158,13 @@ func NewReconciler( cfg.NodeName, ReconcilerName, ), - bdCl: repository.NewBDClient(cl, metrics), - metrics: metrics, - sdsCache: sdsCache, - cfg: cfg, - commands: commands, + bdCl: repository.NewBDClient(cl, metrics), + metrics: metrics, + sdsCache: sdsCache, + cfg: cfg, + commands: commands, + resolver: utils.HostNsenterCanonicalResolver, + aliasResolveFailures: make(map[string]int), } } @@ -176,47 +233,59 @@ func (r *Reconciler) Reconcile(ctx context.Context, request controller.Reconcile r.log.Debug(fmt.Sprintf("[RunLVMVolumeGroupWatcherController] successfully removed the label %s from the LVMVolumeGroup %s", internal.LVGUpdateTriggerLabel, lvg.Name)) } - r.log.Debug(fmt.Sprintf("[RunLVMVolumeGroupWatcherController] tries to get block device resources for the LVMVolumeGroup %s by the selector %v", lvg.Name, lvg.Spec.BlockDeviceSelector)) - blockDevices, err := r.bdCl.GetAPIBlockDevices(ctx, ReconcilerName, lvg.Spec.BlockDeviceSelector) - if err != nil { - r.log.Error(err, fmt.Sprintf("[RunLVMVolumeGroupWatcherController] unable to get BlockDevices. Retry in %s", r.cfg.BlockDeviceScanInterval.String())) - err = r.lvgCl.UpdateLVGConditionIfNeeded( - ctx, - lvg, - v1.ConditionFalse, - internal.TypeVGConfigurationApplied, - "NoBlockDevices", - fmt.Sprintf("unable to get block devices resources, err: %s", err.Error()), - ) + // blockDeviceSelector is optional: a file-only LVMVolumeGroup (only + // spec.fileDevices) carries no selector. Listing block devices with a + // nil selector would match EVERY BlockDevice on the node (the repository + // maps an empty selector to "select all"), and validateSpecBlockDevices + // dereferences the nil selector. So skip block-device discovery and + // validation entirely when there is no selector, and reconcile only the + // file devices. + blockDevices := make(map[string]v1alpha1.BlockDevice) + if lvg.Spec.BlockDeviceSelector != nil { + r.log.Debug(fmt.Sprintf("[RunLVMVolumeGroupWatcherController] tries to get block device resources for the LVMVolumeGroup %s by the selector %v", lvg.Name, lvg.Spec.BlockDeviceSelector)) + blockDevices, err = r.bdCl.GetAPIBlockDevices(ctx, ReconcilerName, lvg.Spec.BlockDeviceSelector) if err != nil { - r.log.Error(err, fmt.Sprintf("[RunLVMVolumeGroupWatcherController] unable to add a condition %s to the LVMVolumeGroup %s. Retry in %s", internal.TypeVGConfigurationApplied, lvg.Name, r.cfg.BlockDeviceScanInterval.String())) - } - - return controller.Result{RequeueAfter: r.cfg.BlockDeviceScanInterval}, nil - } - r.log.Debug(fmt.Sprintf("[RunLVMVolumeGroupWatcherController] successfully got block device resources for the LVMVolumeGroup %s by the selector %v", lvg.Name, lvg.Spec.BlockDeviceSelector)) - - blockDevices = filterBlockDevicesByNodeName(blockDevices, lvg.Spec.Local.NodeName) + r.log.Error(err, fmt.Sprintf("[RunLVMVolumeGroupWatcherController] unable to get BlockDevices. Retry in %s", r.cfg.BlockDeviceScanInterval.String())) + err = r.lvgCl.UpdateLVGConditionIfNeeded( + ctx, + lvg, + v1.ConditionFalse, + internal.TypeVGConfigurationApplied, + "NoBlockDevices", + fmt.Sprintf("unable to get block devices resources, err: %s", err.Error()), + ) + if err != nil { + r.log.Error(err, fmt.Sprintf("[RunLVMVolumeGroupWatcherController] unable to add a condition %s to the LVMVolumeGroup %s. Retry in %s", internal.TypeVGConfigurationApplied, lvg.Name, r.cfg.BlockDeviceScanInterval.String())) + } - valid, reason := validateSpecBlockDevices(lvg, blockDevices) - if !valid { - r.log.Warning(fmt.Sprintf("[RunLVMVolumeGroupController] validation failed for the LVMVolumeGroup %s, reason: %s", lvg.Name, reason)) - err = r.lvgCl.UpdateLVGConditionIfNeeded( - ctx, - lvg, - v1.ConditionFalse, - internal.TypeVGConfigurationApplied, - internal.ReasonValidationFailed, - reason, - ) - if err != nil { - r.log.Error(err, fmt.Sprintf("[RunLVMVolumeGroupWatcherController] unable to add a condition %s to the LVMVolumeGroup %s. Retry in %s", internal.TypeVGConfigurationApplied, lvg.Name, r.cfg.VolumeGroupScanInterval.String())) - return controller.Result{}, err + return controller.Result{RequeueAfter: r.cfg.BlockDeviceScanInterval}, nil } + r.log.Debug(fmt.Sprintf("[RunLVMVolumeGroupWatcherController] successfully got block device resources for the LVMVolumeGroup %s by the selector %v", lvg.Name, lvg.Spec.BlockDeviceSelector)) + + blockDevices = filterBlockDevicesByNodeName(blockDevices, lvg.Spec.Local.NodeName) + + valid, reason := validateSpecBlockDevices(lvg, blockDevices) + if !valid { + r.log.Warning(fmt.Sprintf("[RunLVMVolumeGroupController] validation failed for the LVMVolumeGroup %s, reason: %s", lvg.Name, reason)) + err = r.lvgCl.UpdateLVGConditionIfNeeded( + ctx, + lvg, + v1.ConditionFalse, + internal.TypeVGConfigurationApplied, + internal.ReasonValidationFailed, + reason, + ) + if err != nil { + r.log.Error(err, fmt.Sprintf("[RunLVMVolumeGroupWatcherController] unable to add a condition %s to the LVMVolumeGroup %s. Retry in %s", internal.TypeVGConfigurationApplied, lvg.Name, r.cfg.VolumeGroupScanInterval.String())) + return controller.Result{}, err + } - return controller.Result{}, nil + return controller.Result{}, nil + } + r.log.Debug(fmt.Sprintf("[RunLVMVolumeGroupWatcherController] successfully validated BlockDevices of the LVMVolumeGroup %s", lvg.Name)) + } else { + r.log.Debug(fmt.Sprintf("[RunLVMVolumeGroupWatcherController] the LVMVolumeGroup %s has no blockDeviceSelector (file-only); skipping block device discovery and validation", lvg.Name)) } - r.log.Debug(fmt.Sprintf("[RunLVMVolumeGroupWatcherController] successfully validated BlockDevices of the LVMVolumeGroup %s", lvg.Name)) r.log.Debug(fmt.Sprintf("[RunLVMVolumeGroupWatcherController] tries to add label %s to the LVMVolumeGroup %s", internal.LVGMetadataNameLabelKey, r.cfg.NodeName)) added, err = r.addLVGLabelIfNeeded(ctx, lvg, internal.LVGMetadataNameLabelKey, lvg.Name) @@ -361,6 +430,15 @@ func (r *Reconciler) reconcileLVGDeleteFunc(ctx context.Context, lvg *v1alpha1.L return true, err } + if err := r.cleanupFileDevices(ctx, lvg); err != nil { + r.log.Error(err, fmt.Sprintf("[reconcileLVGDeleteFunc] unable to clean up file devices for the LVMVolumeGroup %s", lvg.Name)) + condErr := r.lvgCl.UpdateLVGConditionIfNeeded(ctx, lvg, v1.ConditionFalse, internal.TypeVGConfigurationApplied, internal.ReasonTerminating, err.Error()) + if condErr != nil { + r.log.Error(condErr, fmt.Sprintf("[reconcileLVGDeleteFunc] unable to add the condition %s to the LVMVolumeGroup %s", internal.TypeVGConfigurationApplied, lvg.Name)) + } + return true, err + } + removed, err := r.removeLVGFinalizerIfExist(ctx, lvg) if err != nil { r.log.Error(err, fmt.Sprintf("[reconcileLVGDeleteFunc] unable to remove a finalizer %s from the LVMVolumeGroup %s", internal.SdsNodeConfiguratorFinalizer, lvg.Name)) @@ -383,6 +461,7 @@ func (r *Reconciler) reconcileLVGDeleteFunc(ctx context.Context, lvg *v1alpha1.L return true, err } + r.resetAliasResolveFailure(lvg.Name) r.log.Info(fmt.Sprintf("[reconcileLVGDeleteFunc] successfully reconciled VG %s of the LVMVolumeGroup %s", lvg.Spec.ActualVGNameOnTheNode, lvg.Name)) return false, nil } @@ -396,7 +475,7 @@ func (r *Reconciler) reconcileLVGUpdateFunc( r.log.Debug(fmt.Sprintf("[reconcileLVGUpdateFunc] tries to validate the LVMVolumeGroup %s", lvg.Name)) pvs, _ := r.sdsCache.GetPVs() - valid, reason := r.validateLVGForUpdateFunc(lvg, blockDevices) + valid, reason := r.validateLVGForUpdateFunc(ctx, lvg, blockDevices) if !valid { r.log.Warning(fmt.Sprintf("[reconcileLVGUpdateFunc] the LVMVolumeGroup %s is not valid", lvg.Name)) err := r.lvgCl.UpdateLVGConditionIfNeeded(ctx, lvg, v1.ConditionFalse, internal.TypeVGConfigurationApplied, internal.ReasonValidationFailed, reason) @@ -464,6 +543,19 @@ func (r *Reconciler) reconcileLVGUpdateFunc( } r.log.Debug(fmt.Sprintf("[reconcileLVGUpdateFunc] successfully ended the extend operation for VG of the LVMVolumeGroup %s", lvg.Name)) + r.log.Debug(fmt.Sprintf("[reconcileLVGUpdateFunc] starts to extend VG %s of the LVMVolumeGroup %s with file devices", vg.VGName, lvg.Name)) + err = r.extendFileDevicesIfNeeded(ctx, lvg, vg, pvs) + if err != nil { + r.log.Error(err, fmt.Sprintf("[reconcileLVGUpdateFunc] unable to extend VG of the LVMVolumeGroup %s with file devices", lvg.Name)) + err = r.lvgCl.UpdateLVGConditionIfNeeded(ctx, lvg, v1.ConditionFalse, internal.TypeVGConfigurationApplied, "VGExtendFailed", fmt.Sprintf("unable to extend VG with file devices, err: %s", err.Error())) + if err != nil { + r.log.Error(err, fmt.Sprintf("[reconcileLVGUpdateFunc] unable to add a condition %s to the LVMVolumeGroup %s", internal.TypeVGConfigurationApplied, lvg.Name)) + } + + return true, err + } + r.log.Debug(fmt.Sprintf("[reconcileLVGUpdateFunc] successfully ended the file-device extend operation for VG of the LVMVolumeGroup %s", lvg.Name)) + if lvg.Spec.ThinPools != nil { r.log.Debug(fmt.Sprintf("[reconcileLVGUpdateFunc] starts to reconcile thin-pools of the LVMVolumeGroup %s", lvg.Name)) lvs, _ := r.sdsCache.GetLVs() @@ -517,7 +609,7 @@ func (r *Reconciler) reconcileLVGCreateFunc( } r.log.Debug(fmt.Sprintf("[reconcileLVGCreateFunc] tries to validate the LVMVolumeGroup %s", lvg.Name)) - valid, reason := r.validateLVGForCreateFunc(lvg, blockDevices) + valid, reason := r.validateLVGForCreateFunc(ctx, lvg, blockDevices) if !valid { r.log.Warning(fmt.Sprintf("[reconcileLVGCreateFunc] validation fails for the LVMVolumeGroup %s", lvg.Name)) err := r.lvgCl.UpdateLVGConditionIfNeeded(ctx, lvg, v1.ConditionFalse, internal.TypeVGConfigurationApplied, internal.ReasonValidationFailed, reason) @@ -554,7 +646,14 @@ func (r *Reconciler) reconcileLVGCreateFunc( extentForThinPools := extentSizeForThinPoolAlign(lvg, vgAfterCreate) for _, tp := range lvg.Spec.ThinPools { + // vgSize must account for file-backed PVs too: a file-only VG has + // no block devices, so countVGSizeByBlockDevices alone returns 0, + // which collapses every percentage thin-pool to 0 and forces every + // absolute-sized thin-pool into the full-VG-space branch (taking the + // whole VG instead of the requested size). Add the spec.fileDevices + // capacity, mirroring how validateLVGForCreateFunc computes totalVGSize. vgSize := countVGSizeByBlockDevices(blockDevices) + vgSize.Add(countVGSizeByFileDevices(lvg)) tpRequestedSize, err := utils.GetRequestedSizeFromString(tp.Size, vgSize) if err != nil { r.log.Error(err, fmt.Sprintf("[reconcileLVGCreateFunc] unable to get thin-pool %s requested size of the LVMVolumeGroup %s", tp.Name, lvg.Name)) @@ -751,6 +850,7 @@ func (r *Reconciler) deleteLVGIfNeeded(ctx context.Context, lvg *v1alpha1.LVMVol } func (r *Reconciler) validateLVGForCreateFunc( + ctx context.Context, lvg *v1alpha1.LVMVolumeGroup, blockDevices map[string]v1alpha1.BlockDevice, ) (bool, string) { @@ -771,6 +871,8 @@ func (r *Reconciler) validateLVGForCreateFunc( r.log.Debug(fmt.Sprintf("[validateLVGForCreateFunc] all BlockDevices of the LVMVolumeGroup %s are consumable", lvg.Name)) } + r.validateFileDevices(ctx, lvg, &reason, &totalVGSize) + if lvg.Spec.ThinPools != nil { r.log.Debug(fmt.Sprintf("[validateLVGForCreateFunc] the LVMVolumeGroup %s has thin-pools. Validate if VG size has enough space for the thin-pools", lvg.Name)) r.log.Trace(fmt.Sprintf("[validateLVGForCreateFunc] the LVMVolumeGroup %s has thin-pools %v", lvg.Name, lvg.Spec.ThinPools)) @@ -789,7 +891,7 @@ func (r *Reconciler) validateLVGForCreateFunc( continue } - alignedTpSize, alignErr := utils.AlignSizeToExtent(tpRequestedSize, extentSizeForThinPoolAlign(lvg, nil)) + alignedTpSize, alignErr := alignThinPoolSizeForValidation(tp.Size, tpRequestedSize, extentSizeForThinPoolAlign(lvg, nil)) if alignErr != nil { reason.WriteString(fmt.Sprintf("Unable to align thin-pool %s size: %s. ", tp.Name, alignErr.Error())) continue @@ -818,7 +920,96 @@ func (r *Reconciler) validateLVGForCreateFunc( return true, "" } +var minFileDeviceSize = resource.MustParse("1Gi") + +// isWithinBaseDir reports whether dir is base or a descendant of it. Both are +// cleaned before comparison so trailing slashes and redundant separators do +// not matter. dir is expected to be absolute and already free of '..' +// segments (validateFileDevice enforces that first), so the prefix check +// cannot be fooled into escaping base. +func isWithinBaseDir(dir, base string) bool { + base = filepath.Clean(base) + dir = filepath.Clean(dir) + return dir == base || strings.HasPrefix(dir, base+string(filepath.Separator)) +} + +func (r *Reconciler) validateFileDevices( + ctx context.Context, + lvg *v1alpha1.LVMVolumeGroup, + reason *strings.Builder, + totalVGSize *resource.Quantity, +) { + seen := make(map[string]int, len(lvg.Spec.FileDevices)) + for i, fd := range lvg.Spec.FileDevices { + key := utils.BuildFileDevicePath(fd.Directory, lvg.Name, fd.Size) + if prev, ok := seen[key]; ok { + fmt.Fprintf(reason, "fileDevices[%d] collides with fileDevices[%d]: same backing file would be created. ", i, prev) + continue + } + seen[key] = i + r.validateFileDevice(ctx, fd, i, reason, totalVGSize) + } +} + +func (r *Reconciler) validateFileDevice( + ctx context.Context, + fd v1alpha1.LVMVolumeGroupFileDeviceSpec, + index int, + reason *strings.Builder, + totalVGSize *resource.Quantity, +) { + if fd.Directory == "" { + fmt.Fprintf(reason, "fileDevices[%d].directory is empty. ", index) + return + } + + // The agent runs in PID 1's mount namespace; an absolute path with + // no `..` segment is the minimum sanity check that keeps `fallocate` + // from creating runaway files outside whatever directory the cluster + // admin intended. + cleaned := filepath.Clean(fd.Directory) + if !filepath.IsAbs(cleaned) { + fmt.Fprintf(reason, "fileDevices[%d].directory %q must be an absolute path. ", index, fd.Directory) + return + } + if cleaned != fd.Directory && strings.Contains(fd.Directory, "..") { + fmt.Fprintf(reason, "fileDevices[%d].directory %q must not contain '..' segments. ", index, fd.Directory) + return + } + + // Confine backing files to the configured base directory (module config + // fileDevicesDirectory, default /opt/deckhouse/sds/file-devices). Without + // this an arbitrary host path — `/`, `/etc`, `/var/lib/kubelet` — could be + // targeted, and one oversized file there fills the node's filesystem and + // trips kubelet DiskPressure eviction. An empty base disables the check + // (unit tests that do not exercise the allowlist). + if r.cfg.FileDevicesDirectory != "" && !isWithinBaseDir(cleaned, r.cfg.FileDevicesDirectory) { + fmt.Fprintf(reason, "fileDevices[%d].directory %q must be %q or a subdirectory of it. ", index, fd.Directory, r.cfg.FileDevicesDirectory) + return + } + + if fd.Size.Value() < minFileDeviceSize.Value() { + // A common cause is a decimal unit (e.g. "1G" = 10^9 bytes) where a + // binary unit was meant ("1Gi" = 2^30 bytes); the former is below the + // minimum. Point the user at binary units so they do not get stuck on + // an immutable, too-small entry. + fmt.Fprintf(reason, "fileDevices[%d].size %s is less than the minimum %s; use a binary unit such as Gi. ", index, fd.Size.String(), minFileDeviceSize.String()) + return + } + + // The backing directory is created on demand by provisionFileDevices + // (mkdir -p in PID 1's mount namespace), so validation only enforces the + // structural rules above. A genuinely unusable path (read-only FS, a file + // where a directory is expected, …) surfaces as a provisioning error and + // is reported on the VGConfigurationApplied condition. + + if totalVGSize != nil { + totalVGSize.Add(fd.Size) + } +} + func (r *Reconciler) validateLVGForUpdateFunc( + ctx context.Context, lvg *v1alpha1.LVMVolumeGroup, blockDevices map[string]v1alpha1.BlockDevice, ) (bool, string) { @@ -880,6 +1071,42 @@ func (r *Reconciler) validateLVGForUpdateFunc( } } + r.validateFileDevices(ctx, lvg, &reason, nil) + + // additionFileDeviceSpace mirrors additionBlockDeviceSpace for file + // devices: it accounts for spec.fileDevices entries that are not yet PVs + // in the VG (i.e. not yet reflected in status.nodes[].fileDevices). Without + // it, a combined "append a fileDevices entry + grow a thin-pool" edit would + // be validated against the current VG size, wrongly rejecting a valid + // request or forcing an absolute-sized thin-pool into the full-VG-space + // branch — the same defect the create path avoids via countVGSizeByFileDevices. + var additionFileDeviceSpace int64 + if len(lvg.Spec.FileDevices) > 0 { + // Match by basename, not full path: status.nodes[].fileDevices[].FilePath + // is the loop's backing file as reported by `losetup --output BACK-FILE`, + // which canonicalizes symlink components of the directory (e.g. a spec + // directory /data symlinked to /mnt/disk1/data is reported as + // /mnt/disk1/data/...), while BuildFileDevicePath keeps the literal spec + // directory. The basename `sds--.img` is identical on both + // sides (the hash is derived from the literal spec directory string and + // losetup leaves the basename untouched), so a full-path compare would + // miss an already-provisioned device whenever the directory is a symlink + // and count it as new on every reconcile, inflating the VG size used for + // thin-pool validation. + existingBasenames := make(map[string]struct{}) + for _, n := range lvg.Status.Nodes { + for _, fd := range n.FileDevices { + existingBasenames[filepath.Base(fd.FilePath)] = struct{}{} + } + } + for _, fd := range lvg.Spec.FileDevices { + base := filepath.Base(utils.BuildFileDevicePath(fd.Directory, lvg.Name, fd.Size)) + if _, ok := existingBasenames[base]; !ok { + additionFileDeviceSpace += fd.Size.Value() + } + } + } + if lvg.Spec.ThinPools != nil { r.log.Debug(fmt.Sprintf("[validateLVGForUpdateFunc] the LVMVolumeGroup %s has thin-pools. Validate them", lvg.Name)) actualThinPools := make(map[string]internal.LVData, len(lvg.Spec.ThinPools)) @@ -907,7 +1134,7 @@ func (r *Reconciler) validateLVGForUpdateFunc( return false, reason.String() } - newTotalVGSize := resource.NewQuantity(vg.VGSize.Value()+additionBlockDeviceSpace, resource.BinarySI) + newTotalVGSize := resource.NewQuantity(vg.VGSize.Value()+additionBlockDeviceSpace+additionFileDeviceSpace, resource.BinarySI) for _, specTp := range lvg.Spec.ThinPools { // might be a case when Thin-pool is already created, but is not shown in status tpRequestedSize, err := utils.GetRequestedSizeFromString(specTp.Size, *newTotalVGSize) @@ -1199,6 +1426,229 @@ func (r *Reconciler) extendVGIfNeeded( return nil } +// extendFileDevicesIfNeeded provisions any spec.fileDevices entries that +// are not yet part of the VG and adds their loop devices as PVs. It is the +// update-path counterpart of provisionFileDevices+createVGComplex and is +// what makes the documented "add a new fileDevices entry to grow the VG" +// flow actually take effect — without it, appending an entry would pass +// validation and silently do nothing. +// +// provisionFileDevices is idempotent (it reuses already-attached loops and +// only creates missing files), so calling it on every update is safe; only +// loop devices that are not already PVs in the VG are handed to vgextend. +func (r *Reconciler) extendFileDevicesIfNeeded( + ctx context.Context, + lvg *v1alpha1.LVMVolumeGroup, + vg internal.VGData, + pvs []internal.PVData, +) (retErr error) { + if len(lvg.Spec.FileDevices) == 0 { + return nil + } + + loopPaths, provisioned, err := r.provisionFileDevices(ctx, lvg) + if err != nil { + return fmt.Errorf("file device provisioning failed: %w", err) + } + + // provisionFileDevices only rolls back artifacts it created within its own + // call; once it returns, the new loops/files survive. Unlike createVGComplex + // (which can roll back the whole VG via cleanupFileDevices because the VG is + // brand new), here the VG already holds healthy file devices we must NOT + // touch. So if a later step fails (condition update, vgextend), detach and + // remove ONLY the devices this provision call just created — they are not + // yet PVs in the VG. Otherwise a failed extend leaks a loop device and a + // preallocated file on the node (and orphans them entirely if the admin + // then removes the failing spec entry). Runs on a detached context because + // the failure is frequently the reconcile ctx being cancelled. + defer func() { + if retErr == nil || len(provisioned) == 0 { + return + } + rollbackCtx, cancel := r.newRollbackContext() + defer cancel() + r.rollbackProvisionedFileDevices(rollbackCtx, provisioned) + }() + + // Build the set of current PVs from BOTH the caller's snapshot (captured at + // the top of reconcileLVGUpdateFunc) and a fresh cache read. A loop may have + // become a canonical /dev/loopN PV after that snapshot (a prior reconcile + // pvcreated it but failed before the VG was assembled, or the create-rollback + // kept it); the stale snapshot would not list it, the exact-name check below + // would miss it, and loopAlreadyRegisteredAsPV only inspects /dev/disk/ + // /dev/block alias-form PVs (not canonical names) — so the loop would be + // handed to pvcreate again and fail "already a PV", wedging the condition + // every reconcile. Taking the union never drops a known PV, so the skip + // decision stays safe. createVGComplex re-reads GetPVs() before pvcreate for + // the same reason. + if freshPVs, _ := r.sdsCache.GetPVs(); len(freshPVs) > 0 { + pvs = append(append([]internal.PVData(nil), pvs...), freshPVs...) + } + pvsMap := make(map[string]struct{}, len(pvs)) + for _, pv := range pvs { + pvsMap[pv.PVName] = struct{}{} + } + + // provisionFileDevices always returns canonical /dev/loopN names, but + // lvm.static has no udev integration and frequently reports a managed + // loop PV under a /dev/disk/by-id or /dev/block/MAJ:MIN alias instead + // (the same aliasing the discoverer resolves). A literal name match + // would therefore miss an already-attached loop PV and wrongly hand it + // to pvcreate/vgextend again — pvcreate then fails because the device + // is already a PV, wedging the VGConfigurationApplied condition on + // every reconcile. Resolve alias-form PV names before deciding a loop + // is new. + pvsToExtend := make([]string, 0, len(loopPaths)) + skippedOnResolverFailure := false + for _, loop := range loopPaths { + if _, exist := pvsMap[loop]; exist { + continue + } + registered, resolveFailed := r.loopAlreadyRegisteredAsPV(ctx, loop, pvs) + if registered { + if resolveFailed { + skippedOnResolverFailure = true + r.log.Warning(fmt.Sprintf("[extendFileDevicesIfNeeded] loop %s skipped because an alias PV could not be resolved; will retry on the next reconcile", loop)) + } else { + r.log.Debug(fmt.Sprintf("[extendFileDevicesIfNeeded] loop %s is already a PV of VG %s under an alias; skipping", loop, vg.VGName)) + } + continue + } + pvsToExtend = append(pvsToExtend, loop) + } + + if len(pvsToExtend) == 0 { + // A skip forced by a resolver failure is not a real "nothing to do": + // the loop might genuinely not be a PV yet, but we could not confirm + // it this round. Returning nil here would mark the configuration + // applied and the new file device would never join the VG, silently. + // Surface it on the condition and return an error so the reconcile + // requeues. The rollback defer is a no-op in this case: a skipped loop + // was already attached, so provisionFileDevices reused it and recorded + // nothing in `provisioned`. + if skippedOnResolverFailure { + // Escalate once the failure looks persistent: a resolver that stays + // broken (missing nsenter binary, a genuinely dangling alias) would + // otherwise requeue forever under the generic "Updating" reason, + // indistinguishable from an ordinary in-flight update. After a few + // consecutive no-progress rounds switch to a dedicated reason and + // log at Error level so it can be alerted on. + streak := r.noteAliasResolveFailure(lvg.Name) + reason := internal.ReasonUpdating + msg := "unable to resolve alias PV names to decide whether file-backed loop devices are already part of the VG; retrying" + if streak >= aliasResolveFailureEscalationThreshold { + reason = internal.ReasonAliasResolutionFailed + msg = fmt.Sprintf("unable to resolve alias PV names for %d consecutive reconciles; file devices cannot be added to VG %s until path resolution recovers (check the nsenter binary and PV aliases on the node)", streak, vg.VGName) + r.log.Error(fmt.Errorf("alias PV resolution stuck"), fmt.Sprintf("[extendFileDevicesIfNeeded] %s (LVMVolumeGroup %s)", msg, lvg.Name)) + } + if err := r.lvgCl.UpdateLVGConditionIfNeeded(ctx, lvg, v1.ConditionFalse, internal.TypeVGConfigurationApplied, reason, msg); err != nil { + r.log.Error(err, fmt.Sprintf("[extendFileDevicesIfNeeded] unable to add the condition %s to the LVMVolumeGroup %s", internal.TypeVGConfigurationApplied, lvg.Name)) + } + return fmt.Errorf("unable to resolve alias PV names for VG %s (attempt %d); retrying", vg.VGName, streak) + } + r.resetAliasResolveFailure(lvg.Name) + r.log.Debug(fmt.Sprintf("[extendFileDevicesIfNeeded] VG %s of the LVMVolumeGroup %s has no new file devices to add", vg.VGName, lvg.Name)) + return nil + } + + // We resolved enough to make progress this round; clear any prior + // resolver-failure streak so a transient blip does not eventually escalate. + r.resetAliasResolveFailure(lvg.Name) + + if isApplied(lvg) { + if err := r.lvgCl.UpdateLVGConditionIfNeeded(ctx, lvg, v1.ConditionFalse, internal.TypeVGConfigurationApplied, internal.ReasonUpdating, "trying to apply the configuration"); err != nil { + r.log.Error(err, fmt.Sprintf("[extendFileDevicesIfNeeded] unable to add the condition %s to the LVMVolumeGroup %s", internal.TypeVGConfigurationApplied, lvg.Name)) + return err + } + } + + r.log.Info(fmt.Sprintf("[extendFileDevicesIfNeeded] VG %s of the LVMVolumeGroup %s should be extended with file devices %v", vg.VGName, lvg.Name, pvsToExtend)) + if err := r.extendVGComplex(ctx, pvsToExtend, vg.VGName); err != nil { + r.log.Error(err, fmt.Sprintf("[extendFileDevicesIfNeeded] unable to extend VG %s of the LVMVolumeGroup %s with file devices", vg.VGName, lvg.Name)) + return err + } + r.log.Info(fmt.Sprintf("[extendFileDevicesIfNeeded] VG %s of the LVMVolumeGroup %s was extended with file devices", vg.VGName, lvg.Name)) + + return nil +} + +// noteAliasResolveFailure records one more consecutive resolver-only failed +// round for lvgName and returns the new streak length. +func (r *Reconciler) noteAliasResolveFailure(lvgName string) int { + r.aliasResolveFailuresMu.Lock() + defer r.aliasResolveFailuresMu.Unlock() + r.aliasResolveFailures[lvgName]++ + return r.aliasResolveFailures[lvgName] +} + +// resetAliasResolveFailure clears the resolver-failure streak for lvgName +// after a round that made progress or genuinely had nothing to do. +func (r *Reconciler) resetAliasResolveFailure(lvgName string) { + r.aliasResolveFailuresMu.Lock() + defer r.aliasResolveFailuresMu.Unlock() + delete(r.aliasResolveFailures, lvgName) +} + +// loopAlreadyRegisteredAsPV reports whether the canonical loop device is +// already present in pvs under an alias name (e.g. /dev/disk/by-id/... or +// /dev/block/MAJ:MIN). It only resolves alias-form PV names, so the common +// case (lvm reports the canonical /dev/loopN) costs no extra host command. +// +// It returns two booleans: registered (skip the extend for this loop) and +// resolveFailed (the skip was forced by an unresolvable alias rather than a +// confirmed match). A resolver failure is treated conservatively as a match: +// we cannot rule out that the unresolved alias IS this loop already +// registered as a PV, and handing a possibly-already-registered loop to +// pvcreate fails ("already a PV") and wedges the VGConfigurationApplied +// condition. It is safer to skip the extend this round and let the next +// reconcile retry once the resolver recovers; provisionFileDevices keeps the +// loop attached idempotently in the meantime. The caller uses resolveFailed +// to surface a retrying condition instead of silently reporting "nothing to +// do" when every loop was skipped only because resolution failed. +// +// NOTE: this is one of two places that canonicalize an alias-reported loop +// PV. Here the canonical /dev/loopN is known and alias PV names are resolved +// via readlink (r.resolver). The discoverer does the inverse in +// Discoverer.buildFileDeviceFromLoopPV (backing-file → canonical loop via +// losetup). They use different methods because their inputs differ; keep +// their ownership/aliasing assumptions in sync. +func (r *Reconciler) loopAlreadyRegisteredAsPV(ctx context.Context, loop string, pvs []internal.PVData) (registered, resolveFailed bool) { + for _, pv := range pvs { + if !strings.HasPrefix(pv.PVName, "/dev/disk/") && !strings.HasPrefix(pv.PVName, "/dev/block/") { + continue + } + resolved, err := utils.RunWithTimeout(ctx, r.cfg.CmdDeadlineDuration, func(ctx context.Context) (string, error) { + return r.resolver(ctx, pv.PVName) + }) + if err != nil { + r.log.Warning(fmt.Sprintf("[loopAlreadyRegisteredAsPV] unable to resolve canonical path for PV %s: %v; treating loop %s as already registered to avoid a duplicate pvcreate", pv.PVName, err, loop)) + resolveFailed = true + continue + } + if resolved == loop { + return true, false + } + } + // No confirmed match. If at least one alias PV could not be resolved, + // stay on the safe side and report the loop as already registered so the + // caller skips a potentially duplicate pvcreate; the next reconcile retries. + return resolveFailed, resolveFailed +} + +// newRollbackContext returns a fresh, detached context (bounded by the +// configured command deadline when set) for file-device cleanup that must run +// even when the reconcile context is already cancelled. The failure that +// triggers a rollback is frequently the reconcile ctx being cancelled (SIGTERM, +// deadline), and exec.CommandContext refuses to start a process under an +// already-cancelled context — which would strand the loop device and backing +// file we just created. The returned cancel func is always safe to defer. +func (r *Reconciler) newRollbackContext() (context.Context, context.CancelFunc) { + if r.cfg.CmdDeadlineDuration > 0 { + return context.WithTimeout(context.Background(), r.cfg.CmdDeadlineDuration) + } + return context.Background(), func() {} +} + func (r *Reconciler) tryGetVG(vgName string) (bool, internal.VGData) { vgs, _ := r.sdsCache.GetVGs() for _, vg := range vgs { @@ -1351,11 +1801,429 @@ func (r *Reconciler) triggerUdevForPaths(parent context.Context, paths []string) } } -func (r *Reconciler) createVGComplex(ctx context.Context, lvg *v1alpha1.LVMVolumeGroup, blockDevices map[string]v1alpha1.BlockDevice) error { +// provisionFileDevices creates one preallocated backing file per +// spec.fileDevices entry and attaches each as a loop device. It is +// idempotent across reconcile retries: if a loop device is already +// attached to the target file, it reuses that loop device instead of +// creating a fresh one (`losetup --find --show` would otherwise hand +// out a new minor on every call, slowly leaking up to the system-wide +// loop limit). +// +// On any error mid-way the function rolls back everything it created +// in *this* invocation — detaches loop devices it just attached and +// removes files it just created — so a transient failure does not +// leave dangling files in the data directory. Loop devices and files +// that pre-existed (e.g. left behind by an earlier successful step) +// are preserved on purpose: they will be picked up by the next +// reconcile as "already attached". +// provisionedFileDevice records a backing file + loop device that +// provisionFileDevices newly created (fallocate + losetup) within a single +// call. Reused, already-attached devices are NOT included, so a caller can +// roll back exactly what this call added if a later step fails, without +// touching pre-existing healthy file devices of the same LVG. +type provisionedFileDevice struct { + filePath string + loopDev string +} + +func (r *Reconciler) provisionFileDevices(ctx context.Context, lvg *v1alpha1.LVMVolumeGroup) (loopPaths []string, provisioned []provisionedFileDevice, retErr error) { + if len(lvg.Spec.FileDevices) == 0 { + return nil, nil, nil + } + + // Track resources we (and only we) just created so we can undo them + // on failure. We deliberately do NOT roll back pre-existing artifacts. + type rollback struct { + filePath string + createdFile bool + loopDev string + attachedLoopOK bool + } + created := make([]rollback, 0, len(lvg.Spec.FileDevices)) + defer func() { + if retErr == nil { + return + } + rollbackCtx, cancel := r.newRollbackContext() + defer cancel() + for i := len(created) - 1; i >= 0; i-- { + rb := created[i] + if rb.attachedLoopOK && rb.loopDev != "" { + if cmd, err := r.commands.DetachLoopDevice(rollbackCtx, rb.loopDev); err != nil { + r.log.Warning(fmt.Sprintf("[provisionFileDevices][rollback] unable to detach %s: %v (cmd: %s)", rb.loopDev, err, cmd)) + } + } + if rb.createdFile && rb.filePath != "" { + if cmd, err := r.commands.RemoveFileDevice(rollbackCtx, rb.filePath); err != nil { + r.log.Warning(fmt.Sprintf("[provisionFileDevices][rollback] unable to remove %s: %v (cmd: %s)", rb.filePath, err, cmd)) + } + } + } + }() + + loopPaths = make([]string, 0, len(lvg.Spec.FileDevices)) + provisioned = make([]provisionedFileDevice, 0, len(lvg.Spec.FileDevices)) + seenLoops := make(map[string]struct{}, len(lvg.Spec.FileDevices)) + for _, fd := range lvg.Spec.FileDevices { + if err := ctx.Err(); err != nil { + return nil, nil, err + } + + filePath := utils.BuildFileDevicePath(fd.Directory, lvg.Name, fd.Size) + sizeBytes := fd.Size.Value() + + type findResult struct { + cmd string + loopDev string + } + findRes, err := utils.RunWithTimeout(ctx, r.cfg.CmdDeadlineDuration, func(ctx context.Context) (findResult, error) { + cmd, existing, err := r.commands.FindLoopDeviceByFile(ctx, filePath) + return findResult{cmd: cmd, loopDev: existing}, err + }) + r.log.Debug(findRes.cmd) + if err != nil { + return nil, nil, fmt.Errorf("query loop for %s: %w", filePath, err) + } + if findRes.loopDev != "" { + r.log.Info(fmt.Sprintf("[provisionFileDevices] %s already attached to %s; reusing", filePath, findRes.loopDev)) + if _, ok := seenLoops[findRes.loopDev]; !ok { + seenLoops[findRes.loopDev] = struct{}{} + loopPaths = append(loopPaths, findRes.loopDev) + } + continue + } + + r.log.Info(fmt.Sprintf("[provisionFileDevices] creating file device %s (%d bytes) for LVMVolumeGroup %s", filePath, sizeBytes, lvg.Name)) + + // Create the backing directory on demand (mkdir -p, idempotent) so + // the admin does not have to pre-create it on every node. A failure + // here (read-only FS, a non-directory in the path) aborts the + // provision and is reported on the VGConfigurationApplied condition. + mkdirCmd, err := utils.RunWithTimeout(ctx, r.cfg.CmdDeadlineDuration, func(ctx context.Context) (string, error) { + return r.commands.EnsureFileDeviceDirectory(ctx, fd.Directory) + }) + r.log.Debug(mkdirCmd) + if err != nil { + r.log.Error(err, fmt.Sprintf("[provisionFileDevices] unable to create directory %s", fd.Directory)) + return nil, nil, err + } + + // Refuse to allocate a backing file larger than the free space of the + // node's filesystem. `fallocate -l` preallocates the full size, so + // without this guard a single oversized fileDevices entry (or a typo in + // `directory`/`size`) can fill the node's root filesystem and push + // kubelet into DiskPressure eviction — a node-level outage, not a mere + // condition error. The check is best-effort: if we cannot determine the + // free space we log and fall through to fallocate, which still fails + // cleanly on a genuine ENOSPC. + availableBytes, err := utils.RunWithTimeout(ctx, r.cfg.CmdDeadlineDuration, func(ctx context.Context) (int64, error) { + cmd, available, err := r.commands.GetAvailableBytes(ctx, fd.Directory) + r.log.Debug(cmd) + return available, err + }) + if err != nil { + r.log.Warning(fmt.Sprintf("[provisionFileDevices] unable to check free space in %s, proceeding (fallocate will still fail on ENOSPC): %v", fd.Directory, err)) + } else if availableBytes < sizeBytes { + return nil, nil, fmt.Errorf( + "not enough free space in %q to create backing file %s: %d bytes available, %d bytes requested", + fd.Directory, filePath, availableBytes, sizeBytes, + ) + } + + rb := rollback{filePath: filePath} + createCmd, err := utils.RunWithTimeout(ctx, r.cfg.CmdDeadlineDuration, func(ctx context.Context) (string, error) { + return r.commands.CreateFileDevice(ctx, filePath, sizeBytes) + }) + r.log.Debug(createCmd) + if err != nil { + r.log.Error(err, fmt.Sprintf("[provisionFileDevices] unable to create file %s", filePath)) + created = append(created, rb) + return nil, nil, err + } + rb.createdFile = true + + type setupResult struct { + cmd string + loopDev string + } + setupRes, err := utils.RunWithTimeout(ctx, r.cfg.CmdDeadlineDuration, func(ctx context.Context) (setupResult, error) { + cmd, loopDev, err := r.commands.SetupLoopDevice(ctx, filePath) + return setupResult{cmd: cmd, loopDev: loopDev}, err + }) + r.log.Debug(setupRes.cmd) + if err != nil { + r.log.Error(err, fmt.Sprintf("[provisionFileDevices] unable to setup loop device for %s", filePath)) + created = append(created, rb) + return nil, nil, err + } + rb.loopDev = setupRes.loopDev + rb.attachedLoopOK = true + created = append(created, rb) + provisioned = append(provisioned, provisionedFileDevice{filePath: filePath, loopDev: setupRes.loopDev}) + + r.log.Info(fmt.Sprintf("[provisionFileDevices] file %s attached to %s", filePath, setupRes.loopDev)) + if _, ok := seenLoops[setupRes.loopDev]; !ok { + seenLoops[setupRes.loopDev] = struct{}{} + loopPaths = append(loopPaths, setupRes.loopDev) + } + } + return loopPaths, provisioned, nil +} + +// rollbackProvisionedFileDevices tears down ONLY the file devices this reconcile +// just provisioned (created a fresh backing file and attached a fresh loop), +// after a later step (pvcreate/vgcreate/vgextend/condition update) failed. +// +// It is the create/extend-path counterpart to cleanupFileDevices and MUST be +// used instead of it on those paths: cleanupFileDevices walks spec+status and +// would remove the backing file of a loop that another, concurrent reconcile — +// or a pvcreate/vgcreate that materially succeeded but returned a non-zero +// status — has already turned into a live PV of the VG. Removing such a file +// leaves a PV backed by a deleted file while the VG keeps using it; the next +// reconcile then re-provisions a second loop and the VG silently doubles in +// size (observed on real clusters as one backing file attached to two loops, +// one shown "(deleted)"). +// +// As a hard safety net it lists the current PVs once, authoritatively (a fresh +// `lvm pvs`, not the possibly-stale cache, because the result gates a +// destructive teardown), and SKIPS any provisioned loop that is already an LVM +// PV (matched canonically or via the /dev/disk//dev/block alias the discoverer +// also resolves). If the PV listing fails it tears nothing down — a leaked +// loop/file is recoverable (the next reconcile reuses it via +// FindLoopDeviceByFile), corrupting a live VG is not — and it never removes a +// backing file whose loop it could not detach, since the loop may still +// reference it. +func (r *Reconciler) rollbackProvisionedFileDevices(ctx context.Context, provisioned []provisionedFileDevice) { + if len(provisioned) == 0 { + return + } + + pvs, cmd, _, err := r.commands.GetAllPVs(ctx) + r.log.Debug(cmd) + if err != nil { + r.log.Warning(fmt.Sprintf("[rollbackProvisionedFileDevices] unable to list PVs to confirm rollback is safe; leaving %d provisioned file device(s) in place (a leak is recoverable, corrupting a live VG is not): %v", len(provisioned), err)) + return + } + pvNames := make(map[string]struct{}, len(pvs)) + for _, pv := range pvs { + pvNames[pv.PVName] = struct{}{} + } + + for i := len(provisioned) - 1; i >= 0; i-- { + p := provisioned[i] + if p.loopDev != "" { + _, isPV := pvNames[p.loopDev] + if !isPV { + // lvm.static without udev may report the loop PV under a + // /dev/disk or /dev/block alias instead of /dev/loopN. + if registered, resolveFailed := r.loopAlreadyRegisteredAsPV(ctx, p.loopDev, pvs); registered || resolveFailed { + isPV = true + } + } + if isPV { + r.log.Warning(fmt.Sprintf("[rollbackProvisionedFileDevices] loop %s (file %s) is already an LVM PV; skipping rollback so a concurrent or partially-succeeded create does not lose its backing storage", p.loopDev, p.filePath)) + continue + } + if cmd, derr := r.commands.DetachLoopDevice(ctx, p.loopDev); derr != nil { + r.log.Warning(fmt.Sprintf("[rollbackProvisionedFileDevices] unable to detach %s: %v (cmd: %s); keeping backing file %s in place", p.loopDev, derr, cmd, p.filePath)) + continue + } + } + if p.filePath != "" { + if cmd, rerr := r.commands.RemoveFileDevice(ctx, p.filePath); rerr != nil { + r.log.Warning(fmt.Sprintf("[rollbackProvisionedFileDevices] unable to remove %s: %v (cmd: %s)", p.filePath, rerr, cmd)) + } + } + } +} + +// cleanupFileDevices detaches loop devices and removes backing files +// recorded for this LVG. It walks the union of status.nodes[].fileDevices +// (what the discoverer last observed) and spec.fileDevices (what the +// user asked for), so it cannot leak files that were created but never +// reflected in status — e.g. when the agent crashed mid-provision. +// +// `rm` errors are logged as warnings and do not abort the cleanup +// (a stale ENOENT after manual cleanup is harmless), but every +// `losetup -d` failure is reported back as an error: a busy loop +// device means there is still a live reference to the file (a mount, +// an LV, a sidecar) and removing the LVG resource at this point would +// strand state on the node. +func (r *Reconciler) cleanupFileDevices(ctx context.Context, lvg *v1alpha1.LVMVolumeGroup) error { + type target struct { + filePath string + loopDevice string + } + seen := make(map[string]target) + add := func(t target) { + if t.filePath == "" && t.loopDevice == "" { + return + } + key := t.filePath + if key == "" { + key = "loop:" + t.loopDevice + } + // Prefer the entry that carries both fields. + if prev, ok := seen[key]; ok { + if prev.loopDevice == "" && t.loopDevice != "" { + prev.loopDevice = t.loopDevice + seen[key] = prev + } + return + } + seen[key] = t + } + for _, node := range lvg.Status.Nodes { + for _, fd := range node.FileDevices { + add(target{filePath: fd.FilePath, loopDevice: fd.LoopDevice}) + } + } + for _, fd := range lvg.Spec.FileDevices { + add(target{filePath: utils.BuildFileDevicePath(fd.Directory, lvg.Name, fd.Size)}) + } + + keys := make([]string, 0, len(seen)) + for k := range seen { + keys = append(keys, k) + } + sort.Strings(keys) + + var detachErrs []error + for _, key := range keys { + t := seen[key] + // Defense in depth: never act on a path whose basename does not + // match the agent's managed pattern. If it slipped into status + // via a foreign loop PV (or a bug), refuse to rm and warn. + if t.filePath != "" && !utils.IsManagedFileDevicePath(t.filePath, lvg.Name) { + r.log.Warning(fmt.Sprintf("[cleanupFileDevices] refusing to act on unmanaged path %q for LVG %s", t.filePath, lvg.Name)) + continue + } + + // The loop minor backing a file is NOT stable: after a reboot + // ReattachFileDevices re-attaches via `losetup --find` and may pick a + // different minor, and the kernel can later hand a freed minor to an + // unrelated file. So the loopDevice recorded in status can be stale or + // even point at a foreign device. Whenever we know the backing file, + // re-resolve the loop from it and never detach a device we have not + // just confirmed backs THIS file. + if t.filePath != "" { + loop, err := utils.RunWithTimeout(ctx, r.cfg.CmdDeadlineDuration, func(ctx context.Context) (string, error) { + cmd, loop, err := r.commands.FindLoopDeviceByFile(ctx, t.filePath) + r.log.Debug(cmd) + return loop, err + }) + if err != nil { + detachErrs = append(detachErrs, fmt.Errorf("query loop for %s: %w", t.filePath, err)) + r.log.Error(err, fmt.Sprintf("[cleanupFileDevices] unable to query loop for %s", t.filePath)) + continue + } + t.loopDevice = loop + } else if t.loopDevice != "" { + // A loop-only target (no backing-file path known) must be + // confirmed managed before we touch it: read its backing file and + // refuse unless the basename matches our owner pattern, so a + // stale/foreign minor recorded in status is never detached. + backing, err := utils.RunWithTimeout(ctx, r.cfg.CmdDeadlineDuration, func(ctx context.Context) (string, error) { + cmd, backing, err := r.commands.GetLoopBackingFile(ctx, t.loopDevice) + r.log.Debug(cmd) + return backing, err + }) + if err != nil { + detachErrs = append(detachErrs, fmt.Errorf("read backing file for %s: %w", t.loopDevice, err)) + r.log.Error(err, fmt.Sprintf("[cleanupFileDevices] unable to read backing file for loop %s", t.loopDevice)) + continue + } + if !utils.IsManagedFileDevicePath(backing, lvg.Name) { + r.log.Warning(fmt.Sprintf("[cleanupFileDevices] refusing to detach loop %s backed by unmanaged file %q for LVG %s", t.loopDevice, backing, lvg.Name)) + continue + } + } + + if t.loopDevice != "" { + cmd, err := utils.RunWithTimeout(ctx, r.cfg.CmdDeadlineDuration, func(ctx context.Context) (string, error) { + return r.commands.DetachLoopDevice(ctx, t.loopDevice) + }) + r.log.Debug(cmd) + if err != nil { + detachErrs = append(detachErrs, fmt.Errorf("detach %s: %w", t.loopDevice, err)) + r.log.Error(err, fmt.Sprintf("[cleanupFileDevices] unable to detach loop %s", t.loopDevice)) + continue + } + } + if t.filePath != "" { + cmd, err := utils.RunWithTimeout(ctx, r.cfg.CmdDeadlineDuration, func(ctx context.Context) (string, error) { + return r.commands.RemoveFileDevice(ctx, t.filePath) + }) + r.log.Debug(cmd) + if err != nil { + r.log.Warning(fmt.Sprintf("[cleanupFileDevices] unable to remove file %s: %v", t.filePath, err)) + } + } + } + if len(detachErrs) > 0 { + return errors.Join(detachErrs...) + } + return nil +} + +func (r *Reconciler) createVGComplex(ctx context.Context, lvg *v1alpha1.LVMVolumeGroup, blockDevices map[string]v1alpha1.BlockDevice) (retErr error) { paths := extractPathsFromBlockDevices(nil, blockDevices) + loopPaths, provisioned, err := r.provisionFileDevices(ctx, lvg) + if err != nil { + return fmt.Errorf("file device provisioning failed: %w", err) + } + paths = append(paths, loopPaths...) + + // If a later step (pvcreate/vgcreate) fails, tear down ONLY the file devices + // this call provisioned — and never one that already became a PV. This must + // NOT use the broad cleanupFileDevices (the delete-path cleanup): it walks + // spec+status and could remove the backing file of a loop that a concurrent + // reconcile, or a pvcreate/vgcreate that materially succeeded but returned a + // non-zero status, had already turned into a live PV of the VG — which the + // next reconcile then re-provisions with a second loop, doubling the VG. + // See rollbackProvisionedFileDevices. Runs on a detached context because the + // failure is frequently the reconcile ctx being cancelled. + if len(provisioned) > 0 { + defer func() { + if retErr == nil { + return + } + rollbackCtx, cancel := r.newRollbackContext() + defer cancel() + r.rollbackProvisionedFileDevices(rollbackCtx, provisioned) + }() + } + r.log.Trace(fmt.Sprintf("[CreateVGComplex] LVMVolumeGroup %s devices paths %v", lvg.Name, paths)) + + // Skip pvcreate for any device that is already an LVM PV. This guards a + // retry after a create that pvcreated a device but failed before vgcreate + // (SIGTERM, ctx cancel, a rollback whose detach failed): provisionFileDevices + // reuses the still-attached loop, and handing an already-PV device to + // pvcreate fails ("already a PV") and wedges the VGConfigurationApplied + // condition. The vgcreate below still consumes an existing PV, so skipping + // the redundant pvcreate is safe. Mirrors the already-a-PV guard in + // extendFileDevicesIfNeeded; loop PVs reported under an alias are resolved + // via loopAlreadyRegisteredAsPV. + existingPVs, _ := r.sdsCache.GetPVs() + existingPVNames := make(map[string]struct{}, len(existingPVs)) + for _, pv := range existingPVs { + existingPVNames[pv.PVName] = struct{}{} + } + for _, path := range paths { + if _, ok := existingPVNames[path]; ok { + r.log.Info(fmt.Sprintf("[CreateVGComplex] %s is already a PV; skipping pvcreate", path)) + continue + } + if strings.HasPrefix(path, "/dev/loop") { + if registered, _ := r.loopAlreadyRegisteredAsPV(ctx, path, existingPVs); registered { + r.log.Info(fmt.Sprintf("[CreateVGComplex] loop %s is already a PV (under an alias); skipping pvcreate", path)) + continue + } + } + start := time.Now() command, err := r.commands.CreatePV(path) r.metrics.UtilsCommandsDuration(ReconcilerName, "pvcreate").Observe(r.metrics.GetEstimatedTimeInSeconds(start)) @@ -1533,6 +2401,14 @@ func validateSpecBlockDevices(lvg *v1alpha1.LVMVolumeGroup, blockDevices map[str } } + // A file-only LVMVolumeGroup has no blockDeviceSelector; there are no + // match expressions to validate, and dereferencing the nil selector + // would panic. (The production caller already skips this function for + // file-only groups; this guard keeps it safe if called directly.) + if lvg.Spec.BlockDeviceSelector == nil { + return true, "" + } + for _, me := range lvg.Spec.BlockDeviceSelector.MatchExpressions { if me.Key == internal.MetadataNameLabelKey && me.Operator == v1.LabelSelectorOpIn { if len(me.Values) != len(blockDevices) { @@ -1591,3 +2467,15 @@ func countVGSizeByBlockDevices(blockDevices map[string]v1alpha1.BlockDevice) res } return *resource.NewQuantity(totalVGSize, resource.BinarySI) } + +// countVGSizeByFileDevices sums the capacity contributed by spec.fileDevices. +// File-backed PVs are part of the VG just like block devices, so thin-pool +// sizing on the create path must include them; otherwise a file-only VG is +// treated as zero-sized. +func countVGSizeByFileDevices(lvg *v1alpha1.LVMVolumeGroup) resource.Quantity { + var totalSize int64 + for _, fd := range lvg.Spec.FileDevices { + totalSize += fd.Size.Value() + } + return *resource.NewQuantity(totalSize, resource.BinarySI) +} diff --git a/images/agent/internal/controller/lvg/reconciler_test.go b/images/agent/internal/controller/lvg/reconciler_test.go index c85807442..f1a3a16d4 100644 --- a/images/agent/internal/controller/lvg/reconciler_test.go +++ b/images/agent/internal/controller/lvg/reconciler_test.go @@ -19,10 +19,14 @@ package lvg import ( "bytes" "context" + "errors" + "fmt" + "strings" "testing" "time" "github.com/stretchr/testify/assert" + "go.uber.org/mock/gomock" errors2 "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/resource" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -33,6 +37,7 @@ import ( "github.com/deckhouse/sds-node-configurator/images/agent/internal" "github.com/deckhouse/sds-node-configurator/images/agent/internal/cache" "github.com/deckhouse/sds-node-configurator/images/agent/internal/logger" + "github.com/deckhouse/sds-node-configurator/images/agent/internal/mock_utils" "github.com/deckhouse/sds-node-configurator/images/agent/internal/monitoring" "github.com/deckhouse/sds-node-configurator/images/agent/internal/test_utils" "github.com/deckhouse/sds-node-configurator/images/agent/internal/utils" @@ -95,7 +100,7 @@ func TestLVMVolumeGroupWatcherCtrl(t *testing.T) { r.sdsCache.StorePVs(pvs, bytes.Buffer{}) - valid, reason := r.validateLVGForUpdateFunc(lvg, bds) + valid, reason := r.validateLVGForUpdateFunc(ctx, lvg, bds) if assert.True(t, valid) { assert.Equal(t, "", reason) } @@ -155,7 +160,7 @@ func TestLVMVolumeGroupWatcherCtrl(t *testing.T) { r.sdsCache.StorePVs(pvs, bytes.Buffer{}) // new block device is not consumable - valid, _ := r.validateLVGForUpdateFunc(lvg, bds) + valid, _ := r.validateLVGForUpdateFunc(ctx, lvg, bds) assert.False(t, valid) }) @@ -225,7 +230,7 @@ func TestLVMVolumeGroupWatcherCtrl(t *testing.T) { r.sdsCache.StorePVs(pvs, bytes.Buffer{}) r.sdsCache.StoreVGs(vgs, bytes.Buffer{}) - valid, reason := r.validateLVGForUpdateFunc(lvg, bds) + valid, reason := r.validateLVGForUpdateFunc(ctx, lvg, bds) if assert.True(t, valid) { assert.Equal(t, "", reason) } @@ -302,7 +307,7 @@ func TestLVMVolumeGroupWatcherCtrl(t *testing.T) { r.sdsCache.StorePVs(pvs, bytes.Buffer{}) r.sdsCache.StoreVGs(vgs, bytes.Buffer{}) - valid, _ := r.validateLVGForUpdateFunc(lvg, bds) + valid, _ := r.validateLVGForUpdateFunc(ctx, lvg, bds) assert.False(t, valid) }) }) @@ -340,7 +345,7 @@ func TestLVMVolumeGroupWatcherCtrl(t *testing.T) { Spec: v1alpha1.LVMVolumeGroupSpec{}, } - valid, reason := r.validateLVGForCreateFunc(lvg, bds) + valid, reason := r.validateLVGForCreateFunc(ctx, lvg, bds) if assert.True(t, valid) { assert.Equal(t, "", reason) } @@ -367,7 +372,7 @@ func TestLVMVolumeGroupWatcherCtrl(t *testing.T) { Spec: v1alpha1.LVMVolumeGroupSpec{}, } - valid, _ := r.validateLVGForCreateFunc(lvg, bds) + valid, _ := r.validateLVGForCreateFunc(ctx, lvg, bds) assert.False(t, valid) }) @@ -407,7 +412,7 @@ func TestLVMVolumeGroupWatcherCtrl(t *testing.T) { }, } - valid, reason := r.validateLVGForCreateFunc(lvg, bds) + valid, reason := r.validateLVGForCreateFunc(ctx, lvg, bds) if assert.True(t, valid) { assert.Equal(t, "", reason) } @@ -449,7 +454,62 @@ func TestLVMVolumeGroupWatcherCtrl(t *testing.T) { }, } - valid, _ := r.validateLVGForCreateFunc(lvg, bds) + valid, _ := r.validateLVGForCreateFunc(ctx, lvg, bds) + assert.False(t, valid) + }) + + t.Run("with_full_vg_percent_thin_pool_returns_true", func(t *testing.T) { + // A single "100%" thin pool must pass validation. The raw VG size + // (sum of block devices) is not extent-aligned, so rounding the + // percentage up would land one extent past the VG and wrongly fail; + // the pool is created with %FREE and fits. + r := setupReconciler() + bds := map[string]v1alpha1.BlockDevice{ + "first": { + ObjectMeta: v1.ObjectMeta{Name: "first"}, + Status: v1alpha1.BlockDeviceStatus{ + // deliberately not a multiple of the 4Mi extent size + Size: resource.MustParse("20481Mi"), + Consumable: true, + }, + }, + } + lvg := &v1alpha1.LVMVolumeGroup{ + Spec: v1alpha1.LVMVolumeGroupSpec{ + ThinPools: []v1alpha1.LVMVolumeGroupThinPoolSpec{ + {Name: "thin", Size: "100%"}, + }, + }, + } + + valid, reason := r.validateLVGForCreateFunc(ctx, lvg, bds) + if assert.True(t, valid) { + assert.Equal(t, "", reason) + } + }) + + t.Run("with_full_vg_percent_thin_pool_and_another_returns_false", func(t *testing.T) { + // "100%" alongside any other thin pool cannot fit. + r := setupReconciler() + bds := map[string]v1alpha1.BlockDevice{ + "first": { + ObjectMeta: v1.ObjectMeta{Name: "first"}, + Status: v1alpha1.BlockDeviceStatus{ + Size: resource.MustParse("20481Mi"), + Consumable: true, + }, + }, + } + lvg := &v1alpha1.LVMVolumeGroup{ + Spec: v1alpha1.LVMVolumeGroupSpec{ + ThinPools: []v1alpha1.LVMVolumeGroupThinPoolSpec{ + {Name: "thin", Size: "100%"}, + {Name: "thin-2", Size: "1Gi"}, + }, + }, + } + + valid, _ := r.validateLVGForCreateFunc(ctx, lvg, bds) assert.False(t, valid) }) }) @@ -887,6 +947,29 @@ func TestLVMVolumeGroupWatcherCtrl(t *testing.T) { valid, _ := validateSpecBlockDevices(lvg, bds) assert.False(t, valid) }) + + t.Run("validation_passes_for_file_only_lvg_without_selector", func(t *testing.T) { + // A file-only LVMVolumeGroup carries no blockDeviceSelector. + // validateSpecBlockDevices must not dereference the nil selector + // (regression: it used to panic in Reconcile). + lvg := &v1alpha1.LVMVolumeGroup{ + Spec: v1alpha1.LVMVolumeGroupSpec{ + Local: v1alpha1.LVMVolumeGroupLocalSpec{NodeName: "nodeName"}, + FileDevices: []v1alpha1.LVMVolumeGroupFileDeviceSpec{ + {Directory: "/data", Size: resource.MustParse("10Gi")}, + }, + }, + } + bds := map[string]v1alpha1.BlockDevice{ + "first": {ObjectMeta: v1.ObjectMeta{Name: "first"}}, + } + + assert.NotPanics(t, func() { + valid, reason := validateSpecBlockDevices(lvg, bds) + assert.True(t, valid) + assert.Empty(t, reason) + }) + }) }) t.Run("syncThinPoolsAllocationLimit", func(t *testing.T) { @@ -1480,5 +1563,907 @@ func setupReconciler() *Reconciler { monitoring.GetMetrics(""), cache.New(), utils.NewCommands(), - ReconcilerConfig{NodeName: "test_node"}) + ReconcilerConfig{NodeName: "test_node", CmdDeadlineDuration: 30 * time.Second}) +} + +// countVGSizeByFileDevices must sum spec.fileDevices capacity so the +// create-path thin-pool sizing does not treat a file-only VG as zero-sized +// (which would collapse percentage pools to 0 and force absolute pools into +// the full-VG-space branch). +func TestCountVGSizeByFileDevices(t *testing.T) { + t.Run("no_file_devices", func(t *testing.T) { + lvg := &v1alpha1.LVMVolumeGroup{} + got := countVGSizeByFileDevices(lvg) + assert.Equal(t, int64(0), got.Value()) + }) + + t.Run("sums_all_entries", func(t *testing.T) { + lvg := &v1alpha1.LVMVolumeGroup{ + Spec: v1alpha1.LVMVolumeGroupSpec{ + FileDevices: []v1alpha1.LVMVolumeGroupFileDeviceSpec{ + {Directory: "/data", Size: resource.MustParse("10Gi")}, + {Directory: "/data", Size: resource.MustParse("20Gi")}, + }, + }, + } + want := resource.MustParse("30Gi") + got := countVGSizeByFileDevices(lvg) + assert.Equal(t, want.Value(), got.Value()) + }) +} + +func TestValidateFileDevice(t *testing.T) { + ctx := context.Background() + + t.Run("valid_file_device", func(t *testing.T) { + // Validation is purely structural now: the backing directory is + // created on demand at provision time, so no host command is invoked. + r := setupReconciler() + + var reason strings.Builder + totalSize := resource.MustParse("0") + fd := v1alpha1.LVMVolumeGroupFileDeviceSpec{ + Directory: "/data", + Size: resource.MustParse("10Gi"), + } + r.validateFileDevice(ctx, fd, 0, &reason, &totalSize) + assert.Empty(t, reason.String()) + want := resource.MustParse("10Gi") + assert.Equal(t, want.Value(), totalSize.Value()) + }) + + t.Run("empty_directory", func(t *testing.T) { + r := setupReconciler() + var reason strings.Builder + totalSize := resource.MustParse("0") + fd := v1alpha1.LVMVolumeGroupFileDeviceSpec{ + Directory: "", + Size: resource.MustParse("10Gi"), + } + r.validateFileDevice(ctx, fd, 0, &reason, &totalSize) + assert.Contains(t, reason.String(), "directory is empty") + }) + + t.Run("size_too_small", func(t *testing.T) { + r := setupReconciler() + var reason strings.Builder + totalSize := resource.MustParse("0") + fd := v1alpha1.LVMVolumeGroupFileDeviceSpec{ + Directory: "/data", + Size: resource.MustParse("500Mi"), + } + r.validateFileDevice(ctx, fd, 0, &reason, &totalSize) + assert.Contains(t, reason.String(), "less than the minimum") + }) + + t.Run("size_adds_to_total_vg_size", func(t *testing.T) { + r := setupReconciler() + + var reason strings.Builder + totalSize := resource.MustParse("5Gi") + fd := v1alpha1.LVMVolumeGroupFileDeviceSpec{ + Directory: "/data", + Size: resource.MustParse("10Gi"), + } + r.validateFileDevice(ctx, fd, 0, &reason, &totalSize) + assert.Empty(t, reason.String()) + expected := resource.MustParse("5Gi") + add := resource.MustParse("10Gi") + expected.Add(add) + assert.Equal(t, expected.Value(), totalSize.Value()) + }) + + t.Run("rejects_relative_directory", func(t *testing.T) { + r := setupReconciler() + var reason strings.Builder + totalSize := resource.MustParse("0") + fd := v1alpha1.LVMVolumeGroupFileDeviceSpec{ + Directory: "data/lvm", + Size: resource.MustParse("10Gi"), + } + r.validateFileDevice(ctx, fd, 0, &reason, &totalSize) + assert.Contains(t, reason.String(), "must be an absolute path") + }) + + t.Run("rejects_dot_dot_segment", func(t *testing.T) { + r := setupReconciler() + var reason strings.Builder + totalSize := resource.MustParse("0") + fd := v1alpha1.LVMVolumeGroupFileDeviceSpec{ + Directory: "/data/../etc", + Size: resource.MustParse("10Gi"), + } + r.validateFileDevice(ctx, fd, 0, &reason, &totalSize) + assert.Contains(t, reason.String(), "must not contain '..'") + }) + + t.Run("rejects_directory_outside_base", func(t *testing.T) { + r := setupReconciler() + r.cfg.FileDevicesDirectory = "/opt/deckhouse/sds/file-devices" + var reason strings.Builder + totalSize := resource.MustParse("0") + fd := v1alpha1.LVMVolumeGroupFileDeviceSpec{ + Directory: "/var/lib/kubelet", + Size: resource.MustParse("10Gi"), + } + r.validateFileDevice(ctx, fd, 0, &reason, &totalSize) + assert.Contains(t, reason.String(), "must be \"/opt/deckhouse/sds/file-devices\" or a subdirectory") + }) + + t.Run("rejects_base_prefix_sibling", func(t *testing.T) { + // A path that shares the base as a string prefix but is not actually + // inside it (e.g. "/opt/deckhouse/sds/file-devices-evil") must be + // rejected — the separator check guards against this. + r := setupReconciler() + r.cfg.FileDevicesDirectory = "/opt/deckhouse/sds/file-devices" + var reason strings.Builder + totalSize := resource.MustParse("0") + fd := v1alpha1.LVMVolumeGroupFileDeviceSpec{ + Directory: "/opt/deckhouse/sds/file-devices-evil", + Size: resource.MustParse("10Gi"), + } + r.validateFileDevice(ctx, fd, 0, &reason, &totalSize) + assert.Contains(t, reason.String(), "or a subdirectory of it") + }) + + t.Run("accepts_base_directory_itself", func(t *testing.T) { + r := setupReconciler() + r.cfg.FileDevicesDirectory = "/opt/deckhouse/sds/file-devices" + var reason strings.Builder + totalSize := resource.MustParse("0") + fd := v1alpha1.LVMVolumeGroupFileDeviceSpec{ + Directory: "/opt/deckhouse/sds/file-devices", + Size: resource.MustParse("10Gi"), + } + r.validateFileDevice(ctx, fd, 0, &reason, &totalSize) + assert.Empty(t, reason.String()) + }) + + t.Run("accepts_subdirectory_of_base", func(t *testing.T) { + r := setupReconciler() + r.cfg.FileDevicesDirectory = "/opt/deckhouse/sds/file-devices" + var reason strings.Builder + totalSize := resource.MustParse("0") + fd := v1alpha1.LVMVolumeGroupFileDeviceSpec{ + Directory: "/opt/deckhouse/sds/file-devices/vg-a", + Size: resource.MustParse("10Gi"), + } + r.validateFileDevice(ctx, fd, 0, &reason, &totalSize) + assert.Empty(t, reason.String()) + }) + + t.Run("rejects_duplicate_backing_file", func(t *testing.T) { + r := setupReconciler() + lvg := &v1alpha1.LVMVolumeGroup{ + ObjectMeta: v1.ObjectMeta{Name: "vg-a"}, + Spec: v1alpha1.LVMVolumeGroupSpec{ + FileDevices: []v1alpha1.LVMVolumeGroupFileDeviceSpec{ + {Directory: "/data", Size: resource.MustParse("10Gi")}, + {Directory: "/data", Size: resource.MustParse("10Gi")}, + }, + }, + } + var reason strings.Builder + r.validateFileDevices(ctx, lvg, &reason, nil) + assert.Contains(t, reason.String(), "collides with fileDevices") + }) +} + +// reconcilerWithMockedCommands wires a Reconciler around a gomock +// Commands so we can exercise provisionFileDevices / cleanupFileDevices +// against deterministic expectations. +func reconcilerWithMockedCommands(t *testing.T, mc *mock_utils.MockCommands) *Reconciler { + t.Helper() + return NewReconciler( + test_utils.NewFakeClient(&v1alpha1.LVMVolumeGroup{}, &v1alpha1.LVMLogicalVolume{}), + logger.Logger{}, + monitoring.GetMetrics(""), + cache.New(), + mc, + ReconcilerConfig{NodeName: "test_node", CmdDeadlineDuration: 30 * time.Second}, + ) +} + +// provisionFileDevices is idempotent: when a backing file is already +// attached to a loop device, the existing loop must be reused. +// `losetup --find --show` would otherwise pick a fresh minor on every +// reconcile retry and slowly leak loops up to the system-wide limit. +func TestProvisionFileDevices_ReusesExistingLoop(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + mc := mock_utils.NewMockCommands(ctrl) + r := reconcilerWithMockedCommands(t, mc) + ctx := context.Background() + + lvg := &v1alpha1.LVMVolumeGroup{ + ObjectMeta: v1.ObjectMeta{Name: "vg-a"}, + Spec: v1alpha1.LVMVolumeGroupSpec{ + FileDevices: []v1alpha1.LVMVolumeGroupFileDeviceSpec{ + {Directory: "/data", Size: resource.MustParse("10Gi")}, + }, + }, + } + want := utils.BuildFileDevicePath("/data", "vg-a", resource.MustParse("10Gi")) + + mc.EXPECT().FindLoopDeviceByFile(gomock.Any(), want).Return("losetup -j ...", "/dev/loop7", nil) + // CreateFileDevice / SetupLoopDevice MUST NOT be invoked. + + got, _, err := r.provisionFileDevices(ctx, lvg) + assert.NoError(t, err) + assert.Equal(t, []string{"/dev/loop7"}, got) +} + +// When the second file's losetup fails, the first file's freshly +// created loop and backing file must be rolled back so a retry sees a +// clean state instead of a half-attached carcass. +func TestProvisionFileDevices_RollsBackOnPartialFailure(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + mc := mock_utils.NewMockCommands(ctrl) + r := reconcilerWithMockedCommands(t, mc) + ctx := context.Background() + + fdA := v1alpha1.LVMVolumeGroupFileDeviceSpec{Directory: "/data", Size: resource.MustParse("10Gi")} + fdB := v1alpha1.LVMVolumeGroupFileDeviceSpec{Directory: "/data", Size: resource.MustParse("20Gi")} + lvg := &v1alpha1.LVMVolumeGroup{ + ObjectMeta: v1.ObjectMeta{Name: "vg-a"}, + Spec: v1alpha1.LVMVolumeGroupSpec{FileDevices: []v1alpha1.LVMVolumeGroupFileDeviceSpec{fdA, fdB}}, + } + pathA := utils.BuildFileDevicePath(fdA.Directory, "vg-a", fdA.Size) + pathB := utils.BuildFileDevicePath(fdB.Directory, "vg-a", fdB.Size) + + sizeA := fdA.Size.Value() + sizeB := fdB.Size.Value() + gomock.InOrder( + mc.EXPECT().FindLoopDeviceByFile(gomock.Any(), pathA).Return("losetup -j", "", nil), + mc.EXPECT().EnsureFileDeviceDirectory(gomock.Any(), fdA.Directory).Return("mkdir -p ...", nil), + mc.EXPECT().GetAvailableBytes(gomock.Any(), fdA.Directory).Return("stat -f ...", int64(1)<<50, nil), + mc.EXPECT().CreateFileDevice(gomock.Any(), pathA, sizeA).Return("fallocate ...", nil), + mc.EXPECT().SetupLoopDevice(gomock.Any(), pathA).Return("losetup --find ...", "/dev/loop0", nil), + mc.EXPECT().FindLoopDeviceByFile(gomock.Any(), pathB).Return("losetup -j", "", nil), + mc.EXPECT().EnsureFileDeviceDirectory(gomock.Any(), fdB.Directory).Return("mkdir -p ...", nil), + mc.EXPECT().GetAvailableBytes(gomock.Any(), fdB.Directory).Return("stat -f ...", int64(1)<<50, nil), + mc.EXPECT().CreateFileDevice(gomock.Any(), pathB, sizeB).Return("fallocate ...", nil), + mc.EXPECT().SetupLoopDevice(gomock.Any(), pathB).Return("losetup --find ...", "", errors.New("ENOSPC")), + // rollback in LIFO order: first pathB (file only, loop never attached), then pathA (loop + file). + mc.EXPECT().RemoveFileDevice(gomock.Any(), pathB).Return("rm ...", nil), + mc.EXPECT().DetachLoopDevice(gomock.Any(), "/dev/loop0").Return("losetup -d ...", nil), + mc.EXPECT().RemoveFileDevice(gomock.Any(), pathA).Return("rm ...", nil), + ) + + _, _, err := r.provisionFileDevices(ctx, lvg) + if assert.Error(t, err) { + assert.Contains(t, err.Error(), "ENOSPC") + } +} + +// A backing file larger than the filesystem's free space must be refused +// before fallocate runs, so a typo in directory/size cannot fill the node's +// root filesystem and trip kubelet DiskPressure eviction. The directory is +// still created (mkdir -p), but no file is allocated and no loop is attached. +func TestProvisionFileDevices_RefusesWhenNotEnoughFreeSpace(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + mc := mock_utils.NewMockCommands(ctrl) + r := reconcilerWithMockedCommands(t, mc) + ctx := context.Background() + + fd := v1alpha1.LVMVolumeGroupFileDeviceSpec{Directory: "/data", Size: resource.MustParse("100Gi")} + lvg := &v1alpha1.LVMVolumeGroup{ + ObjectMeta: v1.ObjectMeta{Name: "vg-a"}, + Spec: v1alpha1.LVMVolumeGroupSpec{FileDevices: []v1alpha1.LVMVolumeGroupFileDeviceSpec{fd}}, + } + path := utils.BuildFileDevicePath(fd.Directory, "vg-a", fd.Size) + + gomock.InOrder( + mc.EXPECT().FindLoopDeviceByFile(gomock.Any(), path).Return("losetup -j", "", nil), + mc.EXPECT().EnsureFileDeviceDirectory(gomock.Any(), fd.Directory).Return("mkdir -p ...", nil), + // Only 1Gi free for a 100Gi request → reject before fallocate. + mc.EXPECT().GetAvailableBytes(gomock.Any(), fd.Directory).Return("stat -f ...", int64(1)<<30, nil), + ) + // CreateFileDevice / SetupLoopDevice MUST NOT be invoked. + + _, _, err := r.provisionFileDevices(ctx, lvg) + if assert.Error(t, err) { + assert.Contains(t, err.Error(), "not enough free space") + } +} + +// When free space cannot be determined the provision proceeds best-effort: +// fallocate still fails cleanly on a genuine ENOSPC, so a transient stat +// failure must not block provisioning by itself. +func TestProvisionFileDevices_ProceedsWhenFreeSpaceUnknown(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + mc := mock_utils.NewMockCommands(ctrl) + r := reconcilerWithMockedCommands(t, mc) + ctx := context.Background() + + fd := v1alpha1.LVMVolumeGroupFileDeviceSpec{Directory: "/data", Size: resource.MustParse("10Gi")} + lvg := &v1alpha1.LVMVolumeGroup{ + ObjectMeta: v1.ObjectMeta{Name: "vg-a"}, + Spec: v1alpha1.LVMVolumeGroupSpec{FileDevices: []v1alpha1.LVMVolumeGroupFileDeviceSpec{fd}}, + } + path := utils.BuildFileDevicePath(fd.Directory, "vg-a", fd.Size) + + gomock.InOrder( + mc.EXPECT().FindLoopDeviceByFile(gomock.Any(), path).Return("losetup -j", "", nil), + mc.EXPECT().EnsureFileDeviceDirectory(gomock.Any(), fd.Directory).Return("mkdir -p ...", nil), + mc.EXPECT().GetAvailableBytes(gomock.Any(), fd.Directory).Return("stat -f ...", int64(0), errors.New("stat: command not found")), + mc.EXPECT().CreateFileDevice(gomock.Any(), path, fd.Size.Value()).Return("fallocate ...", nil), + mc.EXPECT().SetupLoopDevice(gomock.Any(), path).Return("losetup --find ...", "/dev/loop0", nil), + ) + + loops, _, err := r.provisionFileDevices(ctx, lvg) + assert.NoError(t, err) + assert.Equal(t, []string{"/dev/loop0"}, loops) +} + +// cleanupFileDevices must walk the union of spec.fileDevices and +// status.nodes[].fileDevices so files that were created mid-provision +// (and therefore never landed in status) are not leaked when the +// resource is deleted before the discoverer ran. +func TestCleanupFileDevices_UnionOfSpecAndStatus(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + mc := mock_utils.NewMockCommands(ctrl) + r := reconcilerWithMockedCommands(t, mc) + ctx := context.Background() + + fdSpec := v1alpha1.LVMVolumeGroupFileDeviceSpec{Directory: "/data", Size: resource.MustParse("10Gi")} + pathSpec := utils.BuildFileDevicePath(fdSpec.Directory, "vg-a", fdSpec.Size) + // status has a different (older) entry — still managed-named. + fdStatusPath := utils.BuildFileDevicePath("/data", "vg-a", resource.MustParse("20Gi")) + + lvg := &v1alpha1.LVMVolumeGroup{ + ObjectMeta: v1.ObjectMeta{Name: "vg-a"}, + Spec: v1alpha1.LVMVolumeGroupSpec{FileDevices: []v1alpha1.LVMVolumeGroupFileDeviceSpec{fdSpec}}, + Status: v1alpha1.LVMVolumeGroupStatus{ + Nodes: []v1alpha1.LVMVolumeGroupNode{{ + Name: "n", + FileDevices: []v1alpha1.LVMVolumeGroupFileDevice{ + {FilePath: fdStatusPath, LoopDevice: "/dev/loop9"}, + }, + }}, + }, + } + + // Both targets carry a backing file, so cleanup always re-resolves the + // loop from the file (the status-recorded minor can be stale after a + // reboot) before detaching. + mc.EXPECT().FindLoopDeviceByFile(gomock.Any(), fdStatusPath).Return("losetup -j ...", "/dev/loop9", nil) + mc.EXPECT().DetachLoopDevice(gomock.Any(), "/dev/loop9").Return("losetup -d ...", nil) + mc.EXPECT().RemoveFileDevice(gomock.Any(), fdStatusPath).Return("rm ...", nil) + // The spec-derived entry resolves via losetup -j too; here nothing is attached. + mc.EXPECT().FindLoopDeviceByFile(gomock.Any(), pathSpec).Return("losetup -j ...", "", nil) + mc.EXPECT().RemoveFileDevice(gomock.Any(), pathSpec).Return("rm ...", nil) + + assert.NoError(t, r.cleanupFileDevices(ctx, lvg)) +} + +// A busy loop device must surface as an error so the finalizer stays +// in place and the operator can retry — silently leaving a busy loop +// stranded on the node when the LVG object is gone is the worst case. +func TestCleanupFileDevices_DetachErrorBlocksFinalizer(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + mc := mock_utils.NewMockCommands(ctrl) + r := reconcilerWithMockedCommands(t, mc) + ctx := context.Background() + + pathStatus := utils.BuildFileDevicePath("/data", "vg-a", resource.MustParse("20Gi")) + lvg := &v1alpha1.LVMVolumeGroup{ + ObjectMeta: v1.ObjectMeta{Name: "vg-a"}, + Status: v1alpha1.LVMVolumeGroupStatus{ + Nodes: []v1alpha1.LVMVolumeGroupNode{{ + Name: "n", + FileDevices: []v1alpha1.LVMVolumeGroupFileDevice{ + {FilePath: pathStatus, LoopDevice: "/dev/loop9"}, + }, + }}, + }, + } + + gomock.InOrder( + mc.EXPECT().FindLoopDeviceByFile(gomock.Any(), pathStatus).Return("losetup -j ...", "/dev/loop9", nil), + mc.EXPECT().DetachLoopDevice(gomock.Any(), "/dev/loop9").Return("losetup -d ...", errors.New("EBUSY")), + ) + // RemoveFileDevice MUST NOT be called when detach failed: we don't + // want to rm the backing file while the kernel still has it open. + + err := r.cleanupFileDevices(ctx, lvg) + if assert.Error(t, err) { + assert.Contains(t, err.Error(), "EBUSY") + } +} + +// A foreign-looking path that somehow slipped into status (bug, manual +// edit, …) must be skipped, not rm-ed, even if the LVG is being +// deleted. This is the hard owner check that protects unrelated host +// files from being clobbered. +func TestCleanupFileDevices_RefusesUnmanagedPath(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + mc := mock_utils.NewMockCommands(ctrl) + r := reconcilerWithMockedCommands(t, mc) + ctx := context.Background() + + lvg := &v1alpha1.LVMVolumeGroup{ + ObjectMeta: v1.ObjectMeta{Name: "vg-a"}, + Status: v1alpha1.LVMVolumeGroupStatus{ + Nodes: []v1alpha1.LVMVolumeGroupNode{{ + Name: "n", + FileDevices: []v1alpha1.LVMVolumeGroupFileDevice{ + {FilePath: "/var/lib/libvirt/images/disk0.qcow2", LoopDevice: "/dev/loop3"}, + }, + }}, + }, + } + // No EXPECTations on DetachLoopDevice / RemoveFileDevice: the + // cleanup must refuse to touch this path. + + assert.NoError(t, r.cleanupFileDevices(ctx, lvg)) +} + +// When a backing file is known only from spec (status never recorded a +// loop device, e.g. a crash mid-provision) cleanup must resolve the loop +// via losetup -j and detach it BEFORE removing the file, otherwise the +// loop device is stranded on the node bound to a deleted file. +func TestCleanupFileDevices_SpecDerivedDetachesDiscoveredLoop(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + mc := mock_utils.NewMockCommands(ctrl) + r := reconcilerWithMockedCommands(t, mc) + ctx := context.Background() + + fdSpec := v1alpha1.LVMVolumeGroupFileDeviceSpec{Directory: "/data", Size: resource.MustParse("10Gi")} + pathSpec := utils.BuildFileDevicePath(fdSpec.Directory, "vg-a", fdSpec.Size) + lvg := &v1alpha1.LVMVolumeGroup{ + ObjectMeta: v1.ObjectMeta{Name: "vg-a"}, + Spec: v1alpha1.LVMVolumeGroupSpec{FileDevices: []v1alpha1.LVMVolumeGroupFileDeviceSpec{fdSpec}}, + } + + gomock.InOrder( + mc.EXPECT().FindLoopDeviceByFile(gomock.Any(), pathSpec).Return("losetup -j ...", "/dev/loop5", nil), + mc.EXPECT().DetachLoopDevice(gomock.Any(), "/dev/loop5").Return("losetup -d ...", nil), + mc.EXPECT().RemoveFileDevice(gomock.Any(), pathSpec).Return("rm ...", nil), + ) + + assert.NoError(t, r.cleanupFileDevices(ctx, lvg)) +} + +// The loop minor recorded in status can go stale: after a reboot the file +// is re-attached via `losetup --find` to a DIFFERENT minor, and the old +// minor may have been handed to an unrelated device. cleanup must detach +// the minor that currently backs the file (resolved from the backing +// file), never the stale one from status — otherwise it strands our loop +// and may detach a foreign device. +func TestCleanupFileDevices_ReResolvesStaleStatusLoop(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + mc := mock_utils.NewMockCommands(ctrl) + r := reconcilerWithMockedCommands(t, mc) + ctx := context.Background() + + path := utils.BuildFileDevicePath("/data", "vg-a", resource.MustParse("10Gi")) + lvg := &v1alpha1.LVMVolumeGroup{ + ObjectMeta: v1.ObjectMeta{Name: "vg-a"}, + Status: v1alpha1.LVMVolumeGroupStatus{ + Nodes: []v1alpha1.LVMVolumeGroupNode{{ + Name: "n", + FileDevices: []v1alpha1.LVMVolumeGroupFileDevice{ + // status says /dev/loop0, but the file is really on /dev/loop3 now. + {FilePath: path, LoopDevice: "/dev/loop0"}, + }, + }}, + }, + } + + gomock.InOrder( + mc.EXPECT().FindLoopDeviceByFile(gomock.Any(), path).Return("losetup -j ...", "/dev/loop3", nil), + mc.EXPECT().DetachLoopDevice(gomock.Any(), "/dev/loop3").Return("losetup -d ...", nil), + mc.EXPECT().RemoveFileDevice(gomock.Any(), path).Return("rm ...", nil), + ) + // DetachLoopDevice(/dev/loop0) MUST NOT be called — that is the stale minor. + + assert.NoError(t, r.cleanupFileDevices(ctx, lvg)) +} + +// extendFileDevicesIfNeeded is the update-path entry point that makes +// "add a new fileDevices entry to grow the VG" actually work: it +// provisions the new backing file, attaches it, and vgextends with the +// resulting loop device (which is not yet a PV of the VG). +func TestExtendFileDevicesIfNeeded_AddsNewLoopAsPV(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + mc := mock_utils.NewMockCommands(ctrl) + r := reconcilerWithMockedCommands(t, mc) + ctx := context.Background() + + fd := v1alpha1.LVMVolumeGroupFileDeviceSpec{Directory: "/data", Size: resource.MustParse("10Gi")} + lvg := &v1alpha1.LVMVolumeGroup{ + ObjectMeta: v1.ObjectMeta{Name: "vg-a"}, + Spec: v1alpha1.LVMVolumeGroupSpec{FileDevices: []v1alpha1.LVMVolumeGroupFileDeviceSpec{fd}}, + } + path := utils.BuildFileDevicePath(fd.Directory, "vg-a", fd.Size) + vg := internal.VGData{VGName: "vg-test"} + + gomock.InOrder( + mc.EXPECT().FindLoopDeviceByFile(gomock.Any(), path).Return("losetup -j", "", nil), + mc.EXPECT().EnsureFileDeviceDirectory(gomock.Any(), fd.Directory).Return("mkdir -p ...", nil), + mc.EXPECT().GetAvailableBytes(gomock.Any(), fd.Directory).Return("stat -f ...", int64(1)<<50, nil), + mc.EXPECT().CreateFileDevice(gomock.Any(), path, fd.Size.Value()).Return("fallocate ...", nil), + mc.EXPECT().SetupLoopDevice(gomock.Any(), path).Return("losetup --find ...", "/dev/loop4", nil), + mc.EXPECT().CreatePV("/dev/loop4").Return("pvcreate ...", nil), + mc.EXPECT().ExtendVG("vg-test", []string{"/dev/loop4"}).Return("vgextend ...", nil), + mc.EXPECT().UdevadmTrigger(gomock.Any(), []string{"/dev/loop4"}).Return("udevadm ...", nil), + ) + + assert.NoError(t, r.extendFileDevicesIfNeeded(ctx, lvg, vg, nil)) +} + +// A loop device that is already a PV of the VG must not be re-added. +func TestExtendFileDevicesIfNeeded_SkipsExistingPV(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + mc := mock_utils.NewMockCommands(ctrl) + r := reconcilerWithMockedCommands(t, mc) + ctx := context.Background() + + fd := v1alpha1.LVMVolumeGroupFileDeviceSpec{Directory: "/data", Size: resource.MustParse("10Gi")} + lvg := &v1alpha1.LVMVolumeGroup{ + ObjectMeta: v1.ObjectMeta{Name: "vg-a"}, + Spec: v1alpha1.LVMVolumeGroupSpec{FileDevices: []v1alpha1.LVMVolumeGroupFileDeviceSpec{fd}}, + } + path := utils.BuildFileDevicePath(fd.Directory, "vg-a", fd.Size) + vg := internal.VGData{VGName: "vg-test"} + + // Already attached and already a PV — no CreatePV/ExtendVG expected. + mc.EXPECT().FindLoopDeviceByFile(gomock.Any(), path).Return("losetup -j", "/dev/loop4", nil) + + pvs := []internal.PVData{{PVName: "/dev/loop4"}} + assert.NoError(t, r.extendFileDevicesIfNeeded(ctx, lvg, vg, pvs)) +} + +// extendFileDevicesIfNeeded must consult a FRESH cache read, not only the +// caller's PV snapshot. A loop can become a canonical /dev/loopN PV after the +// snapshot was captured at the top of reconcileLVGUpdateFunc (e.g. a prior +// reconcile pvcreated it but failed before the VG was assembled, or the +// create-rollback kept it). Here the caller's snapshot is empty (nil) but the +// cache reports /dev/loop4 as a PV, so no CreatePV/ExtendVG must be issued — +// otherwise pvcreate would fail "already a PV" and wedge the condition. +func TestExtendFileDevicesIfNeeded_SkipsPVKnownOnlyToCache(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + mc := mock_utils.NewMockCommands(ctrl) + r := reconcilerWithMockedCommands(t, mc) + ctx := context.Background() + + fd := v1alpha1.LVMVolumeGroupFileDeviceSpec{Directory: "/data", Size: resource.MustParse("10Gi")} + lvg := &v1alpha1.LVMVolumeGroup{ + ObjectMeta: v1.ObjectMeta{Name: "vg-a"}, + Spec: v1alpha1.LVMVolumeGroupSpec{FileDevices: []v1alpha1.LVMVolumeGroupFileDeviceSpec{fd}}, + } + path := utils.BuildFileDevicePath(fd.Directory, "vg-a", fd.Size) + vg := internal.VGData{VGName: "vg-test"} + + mc.EXPECT().FindLoopDeviceByFile(gomock.Any(), path).Return("losetup -j", "/dev/loop4", nil) + // Cache knows /dev/loop4 is a PV; the caller's snapshot (nil) does not. + r.sdsCache.StorePVs([]internal.PVData{{PVName: "/dev/loop4"}}, bytes.Buffer{}) + + assert.NoError(t, r.extendFileDevicesIfNeeded(ctx, lvg, vg, nil)) +} + +// lvm.static has no udev integration, so an already-attached managed loop +// PV is frequently reported under a /dev/disk/by-id (or /dev/block/MAJ:MIN) +// alias rather than /dev/loopN. provisionFileDevices always returns the +// canonical /dev/loopN, so a literal name match would miss it and wrongly +// re-pvcreate the device. extendFileDevicesIfNeeded must resolve the alias +// and skip the loop instead. +func TestExtendFileDevicesIfNeeded_SkipsExistingPVUnderAlias(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + mc := mock_utils.NewMockCommands(ctrl) + r := reconcilerWithMockedCommands(t, mc) + // Resolve the alias PV name to the canonical loop device. + r.resolver = func(_ context.Context, path string) (string, error) { + if path == "/dev/disk/by-id/lvm-pv-uuid-xyz" { + return "/dev/loop4", nil + } + return path, nil + } + ctx := context.Background() + + fd := v1alpha1.LVMVolumeGroupFileDeviceSpec{Directory: "/data", Size: resource.MustParse("10Gi")} + lvg := &v1alpha1.LVMVolumeGroup{ + ObjectMeta: v1.ObjectMeta{Name: "vg-a"}, + Spec: v1alpha1.LVMVolumeGroupSpec{FileDevices: []v1alpha1.LVMVolumeGroupFileDeviceSpec{fd}}, + } + path := utils.BuildFileDevicePath(fd.Directory, "vg-a", fd.Size) + vg := internal.VGData{VGName: "vg-test"} + + // Loop is already attached; the cache reports it only under an alias. + // No CreatePV/ExtendVG must be issued. + mc.EXPECT().FindLoopDeviceByFile(gomock.Any(), path).Return("losetup -j", "/dev/loop4", nil) + + pvs := []internal.PVData{{PVName: "/dev/disk/by-id/lvm-pv-uuid-xyz"}} + assert.NoError(t, r.extendFileDevicesIfNeeded(ctx, lvg, vg, pvs)) +} + +// When the canonical loop cannot be resolved from an alias PV (transient +// readlink/nsenter failure), extendFileDevicesIfNeeded must NOT hand the loop +// to pvcreate: the alias might already be this loop registered as a PV, and a +// duplicate pvcreate would wedge the VGConfigurationApplied condition. The +// loop is skipped this round, but because the skip was forced by a resolver +// failure (not a confirmed already-a-PV), the function returns an error so the +// reconcile requeues instead of silently reporting the configuration applied +// while the new file device never joined the VG. +func TestExtendFileDevicesIfNeeded_SkipsOnResolverFailure(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + mc := mock_utils.NewMockCommands(ctrl) + r := reconcilerWithMockedCommands(t, mc) + // The resolver fails for the alias PV, so the loop's PV membership is + // undeterminable this round. + r.resolver = func(_ context.Context, _ string) (string, error) { + return "", errors.New("nsenter readlink timed out") + } + ctx := context.Background() + + fd := v1alpha1.LVMVolumeGroupFileDeviceSpec{Directory: "/data", Size: resource.MustParse("10Gi")} + lvg := &v1alpha1.LVMVolumeGroup{ + ObjectMeta: v1.ObjectMeta{Name: "vg-a"}, + Spec: v1alpha1.LVMVolumeGroupSpec{FileDevices: []v1alpha1.LVMVolumeGroupFileDeviceSpec{fd}}, + } + path := utils.BuildFileDevicePath(fd.Directory, "vg-a", fd.Size) + vg := internal.VGData{VGName: "vg-test"} + + // Loop is already attached. With the resolver failing, CreatePV/ExtendVG + // MUST NOT be issued (no duplicate pvcreate wedge). + mc.EXPECT().FindLoopDeviceByFile(gomock.Any(), path).Return("losetup -j", "/dev/loop4", nil) + + pvs := []internal.PVData{{PVName: "/dev/disk/by-id/lvm-pv-uuid-xyz"}} + assert.Error(t, r.extendFileDevicesIfNeeded(ctx, lvg, vg, pvs)) +} + +// A resolver that stays broken must not requeue silently forever: after a few +// consecutive no-progress rounds the failure streak escalates so the condition +// switches to ReasonAliasResolutionFailed and the error reports the attempt +// count. This is the observable "resolver broken" signal from review #6. +func TestExtendFileDevicesIfNeeded_EscalatesAfterRepeatedResolverFailure(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + mc := mock_utils.NewMockCommands(ctrl) + r := reconcilerWithMockedCommands(t, mc) + r.resolver = func(_ context.Context, _ string) (string, error) { + return "", errors.New("nsenter readlink timed out") + } + ctx := context.Background() + + fd := v1alpha1.LVMVolumeGroupFileDeviceSpec{Directory: "/data", Size: resource.MustParse("10Gi")} + lvg := &v1alpha1.LVMVolumeGroup{ + ObjectMeta: v1.ObjectMeta{Name: "vg-a"}, + Spec: v1alpha1.LVMVolumeGroupSpec{FileDevices: []v1alpha1.LVMVolumeGroupFileDeviceSpec{fd}}, + } + path := utils.BuildFileDevicePath(fd.Directory, "vg-a", fd.Size) + vg := internal.VGData{VGName: "vg-test"} + pvs := []internal.PVData{{PVName: "/dev/disk/by-id/lvm-pv-uuid-xyz"}} + + mc.EXPECT().FindLoopDeviceByFile(gomock.Any(), path).Return("losetup -j", "/dev/loop4", nil).AnyTimes() + + for attempt := 1; attempt <= aliasResolveFailureEscalationThreshold; attempt++ { + err := r.extendFileDevicesIfNeeded(ctx, lvg, vg, pvs) + if assert.Error(t, err) { + assert.Contains(t, err.Error(), fmt.Sprintf("attempt %d", attempt)) + } + assert.Equal(t, attempt, r.aliasResolveFailures[lvg.Name]) + } + // At the threshold the streak reached the escalation point. + assert.GreaterOrEqual(t, r.aliasResolveFailures[lvg.Name], aliasResolveFailureEscalationThreshold) +} + +// A transient resolver blip must not accumulate toward escalation: once a +// round makes progress (or has nothing to do), the streak resets so the next +// failure starts counting from one again. +func TestExtendFileDevicesIfNeeded_ResolverFailureStreakResetsOnProgress(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + mc := mock_utils.NewMockCommands(ctrl) + r := reconcilerWithMockedCommands(t, mc) + ctx := context.Background() + + fd := v1alpha1.LVMVolumeGroupFileDeviceSpec{Directory: "/data", Size: resource.MustParse("10Gi")} + lvg := &v1alpha1.LVMVolumeGroup{ + ObjectMeta: v1.ObjectMeta{Name: "vg-a"}, + Spec: v1alpha1.LVMVolumeGroupSpec{FileDevices: []v1alpha1.LVMVolumeGroupFileDeviceSpec{fd}}, + } + path := utils.BuildFileDevicePath(fd.Directory, "vg-a", fd.Size) + vg := internal.VGData{VGName: "vg-test"} + pvs := []internal.PVData{{PVName: "/dev/disk/by-id/lvm-pv-uuid-xyz"}} + + mc.EXPECT().FindLoopDeviceByFile(gomock.Any(), path).Return("losetup -j", "/dev/loop4", nil).AnyTimes() + + // Two failing rounds build a streak of 2. + r.resolver = func(_ context.Context, _ string) (string, error) { + return "", errors.New("nsenter readlink timed out") + } + assert.Error(t, r.extendFileDevicesIfNeeded(ctx, lvg, vg, pvs)) + assert.Error(t, r.extendFileDevicesIfNeeded(ctx, lvg, vg, pvs)) + assert.Equal(t, 2, r.aliasResolveFailures[lvg.Name]) + + // A round that makes no-progress-but-not-due-to-resolver (the loop is a + // known canonical PV in the cache) clears the streak. + r.sdsCache.StorePVs([]internal.PVData{{PVName: "/dev/loop4"}}, bytes.Buffer{}) + assert.NoError(t, r.extendFileDevicesIfNeeded(ctx, lvg, vg, nil)) + assert.Equal(t, 0, r.aliasResolveFailures[lvg.Name]) +} + +// When the extend step (pvcreate/vgextend) fails after a new file device was +// freshly provisioned, the loop device and backing file created in THIS call +// must be rolled back so a failed extend does not leak them on the node (and +// orphan them entirely if the admin then removes the failing spec entry). +func TestExtendFileDevicesIfNeeded_RollsBackProvisionedOnExtendFailure(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + mc := mock_utils.NewMockCommands(ctrl) + r := reconcilerWithMockedCommands(t, mc) + ctx := context.Background() + + fd := v1alpha1.LVMVolumeGroupFileDeviceSpec{Directory: "/data", Size: resource.MustParse("10Gi")} + lvg := &v1alpha1.LVMVolumeGroup{ + ObjectMeta: v1.ObjectMeta{Name: "vg-a"}, + Spec: v1alpha1.LVMVolumeGroupSpec{FileDevices: []v1alpha1.LVMVolumeGroupFileDeviceSpec{fd}}, + } + path := utils.BuildFileDevicePath(fd.Directory, "vg-a", fd.Size) + vg := internal.VGData{VGName: "vg-test"} + + gomock.InOrder( + mc.EXPECT().FindLoopDeviceByFile(gomock.Any(), path).Return("losetup -j", "", nil), + mc.EXPECT().EnsureFileDeviceDirectory(gomock.Any(), fd.Directory).Return("mkdir -p ...", nil), + mc.EXPECT().GetAvailableBytes(gomock.Any(), fd.Directory).Return("stat -f ...", int64(1)<<50, nil), + mc.EXPECT().CreateFileDevice(gomock.Any(), path, fd.Size.Value()).Return("fallocate ...", nil), + mc.EXPECT().SetupLoopDevice(gomock.Any(), path).Return("losetup --find ...", "/dev/loop4", nil), + // Extend fails on pvcreate... + mc.EXPECT().CreatePV("/dev/loop4").Return("pvcreate ...", errors.New("pvcreate refused")), + // ...rollback first confirms the loop is not already a PV (it is not)... + mc.EXPECT().GetAllPVs(gomock.Any()).Return(nil, "pvs", bytes.Buffer{}, nil), + // ...then tears down the just-provisioned loop + file. + mc.EXPECT().DetachLoopDevice(gomock.Any(), "/dev/loop4").Return("losetup -d ...", nil), + mc.EXPECT().RemoveFileDevice(gomock.Any(), path).Return("rm ...", nil), + ) + + assert.Error(t, r.extendFileDevicesIfNeeded(ctx, lvg, vg, nil)) +} + +// rollbackProvisionedFileDevices must NOT detach the loop or remove the backing +// file of a device whose loop has already become a live PV — by a concurrent +// reconcile or by a pvcreate/vgcreate that materially succeeded but returned a +// non-zero status. Tearing it down deletes the backing file out from under a +// PV the VG is using, which on real clusters left the single backing file +// attached to two loops (one "(deleted)") and doubled the VG. Here CreatePV +// reports failure yet the loop shows up as a PV in the authoritative pvs list, +// so rollback must skip it entirely. +func TestExtendFileDevicesIfNeeded_DoesNotRollBackLoopThatBecamePV(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + mc := mock_utils.NewMockCommands(ctrl) + r := reconcilerWithMockedCommands(t, mc) + ctx := context.Background() + + fd := v1alpha1.LVMVolumeGroupFileDeviceSpec{Directory: "/data", Size: resource.MustParse("10Gi")} + lvg := &v1alpha1.LVMVolumeGroup{ + ObjectMeta: v1.ObjectMeta{Name: "vg-a"}, + Spec: v1alpha1.LVMVolumeGroupSpec{FileDevices: []v1alpha1.LVMVolumeGroupFileDeviceSpec{fd}}, + } + path := utils.BuildFileDevicePath(fd.Directory, "vg-a", fd.Size) + vg := internal.VGData{VGName: "vg-test"} + + gomock.InOrder( + mc.EXPECT().FindLoopDeviceByFile(gomock.Any(), path).Return("losetup -j", "", nil), + mc.EXPECT().EnsureFileDeviceDirectory(gomock.Any(), fd.Directory).Return("mkdir -p ...", nil), + mc.EXPECT().GetAvailableBytes(gomock.Any(), fd.Directory).Return("stat -f ...", int64(1)<<50, nil), + mc.EXPECT().CreateFileDevice(gomock.Any(), path, fd.Size.Value()).Return("fallocate ...", nil), + mc.EXPECT().SetupLoopDevice(gomock.Any(), path).Return("losetup --find ...", "/dev/loop4", nil), + mc.EXPECT().CreatePV("/dev/loop4").Return("pvcreate ...", errors.New("fd leaked on lvm invocation")), + // The pvcreate actually wrote the PV label despite the error, so the + // authoritative pvs list reports /dev/loop4 as a PV of the VG. + mc.EXPECT().GetAllPVs(gomock.Any()).Return( + []internal.PVData{{PVName: "/dev/loop4", VGName: "vg-test"}}, "pvs", bytes.Buffer{}, nil), + ) + // No DetachLoopDevice / RemoveFileDevice expectations: rollback must skip the + // in-use loop. gomock fails the test if either is called. + + assert.Error(t, r.extendFileDevicesIfNeeded(ctx, lvg, vg, nil)) +} + +// validateLVGForUpdateFunc must count the capacity of newly-added +// spec.fileDevices entries (not yet PVs in the VG) when validating thin-pool +// sizes — mirroring additionBlockDeviceSpace for block devices. Without it, a +// combined "append a fileDevices entry + grow thin-pools" edit is validated +// against the pre-extend VG size and wrongly rejected. +// +// Two thin-pools are used on purpose: with a single pool, a request that +// exceeds the VG size is silently accepted as a "100% of VG" pool, so the +// missing-capacity bug would not surface. With two pools, the too-small VG +// size pushes the first pool into the full-VG branch and produces a rejection +// ("requests size of full VG space, but there are any other thin-pools"), +// which the fix avoids. +func TestValidateLVGForUpdateFunc_CountsNewFileDeviceSpaceForThinPools(t *testing.T) { + const vgName = "test-vg-file" + + makeLVG := func(status v1alpha1.LVMVolumeGroupStatus) *v1alpha1.LVMVolumeGroup { + return &v1alpha1.LVMVolumeGroup{ + ObjectMeta: v1.ObjectMeta{Name: "vg-file"}, + Spec: v1alpha1.LVMVolumeGroupSpec{ + ActualVGNameOnTheNode: vgName, + FileDevices: []v1alpha1.LVMVolumeGroupFileDeviceSpec{ + {Directory: "/data", Size: resource.MustParse("5G")}, + }, + ThinPools: []v1alpha1.LVMVolumeGroupThinPoolSpec{ + {Name: "tp1", Size: "2G"}, + {Name: "tp2", Size: "2G"}, + }, + }, + Status: status, + } + } + + t.Run("new_file_device_capacity_is_counted", func(t *testing.T) { + r := setupReconciler() + // VG currently holds 1G; the 5G file device is new (absent from status), + // so newTotalVGSize = 1G + 5G = 6G and both 2G pools fit. + vgs := []internal.VGData{{VGName: vgName, VGSize: resource.MustParse("1G"), VGFree: resource.MustParse("1G")}} + r.sdsCache.StoreVGs(vgs, bytes.Buffer{}) + r.sdsCache.StorePVs(nil, bytes.Buffer{}) + + valid, reason := r.validateLVGForUpdateFunc(context.Background(), makeLVG(v1alpha1.LVMVolumeGroupStatus{}), map[string]v1alpha1.BlockDevice{}) + if assert.True(t, valid) { + assert.Equal(t, "", reason) + } + }) + + t.Run("existing_file_device_is_not_double_counted", func(t *testing.T) { + r := setupReconciler() + // The same 5G file device is already in status (already part of the 1G + // VG for the purpose of the test), so it is NOT added again: + // newTotalVGSize stays 1G and the two 2G pools no longer fit. + existingPath := utils.BuildFileDevicePath("/data", "vg-file", resource.MustParse("5G")) + status := v1alpha1.LVMVolumeGroupStatus{ + Nodes: []v1alpha1.LVMVolumeGroupNode{{ + Name: "test_node", + FileDevices: []v1alpha1.LVMVolumeGroupFileDevice{{FilePath: existingPath}}, + }}, + } + vgs := []internal.VGData{{VGName: vgName, VGSize: resource.MustParse("1G"), VGFree: resource.MustParse("1G")}} + r.sdsCache.StoreVGs(vgs, bytes.Buffer{}) + r.sdsCache.StorePVs(nil, bytes.Buffer{}) + + valid, _ := r.validateLVGForUpdateFunc(context.Background(), makeLVG(status), map[string]v1alpha1.BlockDevice{}) + assert.False(t, valid) + }) + + t.Run("symlink_canonicalized_status_path_still_matches", func(t *testing.T) { + r := setupReconciler() + // losetup reports the loop's backing file with the directory symlink + // resolved (/data -> /mnt/disk1/data), so status FilePath differs from + // BuildFileDevicePath in its directory but shares the basename. The + // existing device must still be recognized (basename match) and NOT + // counted as new — so newTotalVGSize stays 1G and the two 2G pools no + // longer fit, exactly like the same-path case above. A full-path compare + // would miss it, inflate the VG size, and wrongly pass validation. + specPath := utils.BuildFileDevicePath("/data", "vg-file", resource.MustParse("5G")) + canonicalStatusPath := "/mnt/disk1" + specPath // /mnt/disk1/data/sds-...img + status := v1alpha1.LVMVolumeGroupStatus{ + Nodes: []v1alpha1.LVMVolumeGroupNode{{ + Name: "test_node", + FileDevices: []v1alpha1.LVMVolumeGroupFileDevice{{FilePath: canonicalStatusPath}}, + }}, + } + vgs := []internal.VGData{{VGName: vgName, VGSize: resource.MustParse("1G"), VGFree: resource.MustParse("1G")}} + r.sdsCache.StoreVGs(vgs, bytes.Buffer{}) + r.sdsCache.StorePVs(nil, bytes.Buffer{}) + + valid, _ := r.validateLVGForUpdateFunc(context.Background(), makeLVG(status), map[string]v1alpha1.BlockDevice{}) + assert.False(t, valid) + }) } diff --git a/images/agent/internal/mock_utils/block_device.go b/images/agent/internal/mock_utils/block_device.go index 07b0e5b3f..29b475bcc 100644 --- a/images/agent/internal/mock_utils/block_device.go +++ b/images/agent/internal/mock_utils/block_device.go @@ -29,9 +29,8 @@ import ( fs "io/fs" reflect "reflect" - gomock "go.uber.org/mock/gomock" - utils "github.com/deckhouse/sds-node-configurator/images/agent/internal/utils" + gomock "go.uber.org/mock/gomock" ) // MockDiscarder is a mock of Discarder interface. diff --git a/images/agent/internal/mock_utils/commands.go b/images/agent/internal/mock_utils/commands.go index 469e3c550..3f8540f98 100644 --- a/images/agent/internal/mock_utils/commands.go +++ b/images/agent/internal/mock_utils/commands.go @@ -45,6 +45,52 @@ func (m *MockCommands) EXPECT() *MockCommandsMockRecorder { return m.recorder } +// EnsureFileDeviceDirectory mocks base method. +func (m *MockCommands) EnsureFileDeviceDirectory(ctx context.Context, directory string) (string, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "EnsureFileDeviceDirectory", ctx, directory) + ret0, _ := ret[0].(string) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// EnsureFileDeviceDirectory indicates an expected call of EnsureFileDeviceDirectory. +func (mr *MockCommandsMockRecorder) EnsureFileDeviceDirectory(ctx, directory any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "EnsureFileDeviceDirectory", reflect.TypeOf((*MockCommands)(nil).EnsureFileDeviceDirectory), ctx, directory) +} + +// GetAvailableBytes mocks base method. +func (m *MockCommands) GetAvailableBytes(ctx context.Context, directory string) (string, int64, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetAvailableBytes", ctx, directory) + ret0, _ := ret[0].(string) + ret1, _ := ret[1].(int64) + ret2, _ := ret[2].(error) + return ret0, ret1, ret2 +} + +// GetAvailableBytes indicates an expected call of GetAvailableBytes. +func (mr *MockCommandsMockRecorder) GetAvailableBytes(ctx, directory any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetAvailableBytes", reflect.TypeOf((*MockCommands)(nil).GetAvailableBytes), ctx, directory) +} + +// CreateFileDevice mocks base method. +func (m *MockCommands) CreateFileDevice(ctx context.Context, path string, sizeBytes int64) (string, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "CreateFileDevice", ctx, path, sizeBytes) + ret0, _ := ret[0].(string) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// CreateFileDevice indicates an expected call of CreateFileDevice. +func (mr *MockCommandsMockRecorder) CreateFileDevice(ctx, path, sizeBytes any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateFileDevice", reflect.TypeOf((*MockCommands)(nil).CreateFileDevice), ctx, path, sizeBytes) +} + // CreatePV mocks base method. func (m *MockCommands) CreatePV(path string) (string, error) { m.ctrl.T.Helper() @@ -180,6 +226,21 @@ func (mr *MockCommandsMockRecorder) CreateVGShared(vgName, lvmVolumeGroupName, p return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateVGShared", reflect.TypeOf((*MockCommands)(nil).CreateVGShared), vgName, lvmVolumeGroupName, pvNames) } +// DetachLoopDevice mocks base method. +func (m *MockCommands) DetachLoopDevice(ctx context.Context, loopDev string) (string, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "DetachLoopDevice", ctx, loopDev) + ret0, _ := ret[0].(string) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// DetachLoopDevice indicates an expected call of DetachLoopDevice. +func (mr *MockCommandsMockRecorder) DetachLoopDevice(ctx, loopDev any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DetachLoopDevice", reflect.TypeOf((*MockCommands)(nil).DetachLoopDevice), ctx, loopDev) +} + // ExtendLV mocks base method. func (m *MockCommands) ExtendLV(size int64, vgName, lvName string) (string, error) { m.ctrl.T.Helper() @@ -225,6 +286,22 @@ func (mr *MockCommandsMockRecorder) ExtendVG(vgName, paths any) *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ExtendVG", reflect.TypeOf((*MockCommands)(nil).ExtendVG), vgName, paths) } +// FindLoopDeviceByFile mocks base method. +func (m *MockCommands) FindLoopDeviceByFile(ctx context.Context, filePath string) (string, string, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "FindLoopDeviceByFile", ctx, filePath) + ret0, _ := ret[0].(string) + ret1, _ := ret[1].(string) + ret2, _ := ret[2].(error) + return ret0, ret1, ret2 +} + +// FindLoopDeviceByFile indicates an expected call of FindLoopDeviceByFile. +func (mr *MockCommandsMockRecorder) FindLoopDeviceByFile(ctx, filePath any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "FindLoopDeviceByFile", reflect.TypeOf((*MockCommands)(nil).FindLoopDeviceByFile), ctx, filePath) +} + // GetAllLVs mocks base method. func (m *MockCommands) GetAllLVs(ctx context.Context) ([]internal.LVData, string, bytes.Buffer, error) { m.ctrl.T.Helper() @@ -310,6 +387,22 @@ func (mr *MockCommandsMockRecorder) GetLV(vgName, lvName any) *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetLV", reflect.TypeOf((*MockCommands)(nil).GetLV), vgName, lvName) } +// GetLoopBackingFile mocks base method. +func (m *MockCommands) GetLoopBackingFile(ctx context.Context, loopDev string) (string, string, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetLoopBackingFile", ctx, loopDev) + ret0, _ := ret[0].(string) + ret1, _ := ret[1].(string) + ret2, _ := ret[2].(error) + return ret0, ret1, ret2 +} + +// GetLoopBackingFile indicates an expected call of GetLoopBackingFile. +func (mr *MockCommandsMockRecorder) GetLoopBackingFile(ctx, loopDev any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetLoopBackingFile", reflect.TypeOf((*MockCommands)(nil).GetLoopBackingFile), ctx, loopDev) +} + // GetPV mocks base method. func (m *MockCommands) GetPV(pvName string) (internal.PVData, string, bytes.Buffer, error) { m.ctrl.T.Helper() @@ -389,21 +482,6 @@ func (mr *MockCommandsMockRecorder) PVScan(ctx any) *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "PVScan", reflect.TypeOf((*MockCommands)(nil).PVScan), ctx) } -// UdevadmTrigger mocks base method. -func (m *MockCommands) UdevadmTrigger(ctx context.Context, paths []string) (string, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "UdevadmTrigger", ctx, paths) - ret0, _ := ret[0].(string) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// UdevadmTrigger indicates an expected call of UdevadmTrigger. -func (mr *MockCommandsMockRecorder) UdevadmTrigger(ctx, paths any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UdevadmTrigger", reflect.TypeOf((*MockCommands)(nil).UdevadmTrigger), ctx, paths) -} - // ReTag mocks base method. func (m *MockCommands) ReTag(ctx context.Context, log logger.Logger, metrics *monitoring.Metrics, ctrlName string, cmdTimeout time.Duration) error { m.ctrl.T.Helper() @@ -418,6 +496,21 @@ func (mr *MockCommandsMockRecorder) ReTag(ctx, log, metrics, ctrlName, cmdTimeou return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ReTag", reflect.TypeOf((*MockCommands)(nil).ReTag), ctx, log, metrics, ctrlName, cmdTimeout) } +// RemoveFileDevice mocks base method. +func (m *MockCommands) RemoveFileDevice(ctx context.Context, path string) (string, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "RemoveFileDevice", ctx, path) + ret0, _ := ret[0].(string) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// RemoveFileDevice indicates an expected call of RemoveFileDevice. +func (mr *MockCommandsMockRecorder) RemoveFileDevice(ctx, path any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RemoveFileDevice", reflect.TypeOf((*MockCommands)(nil).RemoveFileDevice), ctx, path) +} + // RemoveLV mocks base method. func (m *MockCommands) RemoveLV(vgName, lvName string) (string, error) { m.ctrl.T.Helper() @@ -478,6 +571,37 @@ func (mr *MockCommandsMockRecorder) ResizePV(pvName any) *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ResizePV", reflect.TypeOf((*MockCommands)(nil).ResizePV), pvName) } +// SetupLoopDevice mocks base method. +func (m *MockCommands) SetupLoopDevice(ctx context.Context, filePath string) (string, string, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "SetupLoopDevice", ctx, filePath) + ret0, _ := ret[0].(string) + ret1, _ := ret[1].(string) + ret2, _ := ret[2].(error) + return ret0, ret1, ret2 +} + +// SetupLoopDevice indicates an expected call of SetupLoopDevice. +func (mr *MockCommandsMockRecorder) SetupLoopDevice(ctx, filePath any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetupLoopDevice", reflect.TypeOf((*MockCommands)(nil).SetupLoopDevice), ctx, filePath) +} + +// UdevadmTrigger mocks base method. +func (m *MockCommands) UdevadmTrigger(ctx context.Context, paths []string) (string, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "UdevadmTrigger", ctx, paths) + ret0, _ := ret[0].(string) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// UdevadmTrigger indicates an expected call of UdevadmTrigger. +func (mr *MockCommandsMockRecorder) UdevadmTrigger(ctx, paths any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UdevadmTrigger", reflect.TypeOf((*MockCommands)(nil).UdevadmTrigger), ctx, paths) +} + // UnmarshalDevices mocks base method. func (m *MockCommands) UnmarshalDevices(out []byte) ([]internal.Device, error) { m.ctrl.T.Helper() diff --git a/images/agent/internal/mock_utils/syscall.go b/images/agent/internal/mock_utils/syscall.go index 18ae434d1..6514c831c 100644 --- a/images/agent/internal/mock_utils/syscall.go +++ b/images/agent/internal/mock_utils/syscall.go @@ -28,9 +28,8 @@ package mock_utils import ( reflect "reflect" - gomock "go.uber.org/mock/gomock" - utils "github.com/deckhouse/sds-node-configurator/images/agent/internal/utils" + gomock "go.uber.org/mock/gomock" ) // MockSysCall is a mock of SysCall interface. diff --git a/images/agent/internal/scanner/scanner.go b/images/agent/internal/scanner/scanner.go index 7d0d4f602..8c380adfe 100644 --- a/images/agent/internal/scanner/scanner.go +++ b/images/agent/internal/scanner/scanner.go @@ -257,11 +257,17 @@ func (s *scanner) fillTheCache(ctx context.Context, log logger.Logger, cache *ca // lvm.static bundled in /opt/deckhouse/sds/bin is built without udev // integration and so reports PVs found on any block device it sees in - // /dev, including Ceph RBD images and loopback-mounted guest VM disks - // that happen to carry LVM signatures from nested LVM (see ADR / PR - // description). Filter those out before feeding the cache so that the - // LVG/BD reconcile logic never sees duplicate VG names produced by - // foreign storage layers. + // /dev, including Ceph RBD/DRBD/NBD images that happen to carry LVM + // signatures from nested LVM (see ADR / PR description). Filter those out + // before feeding the cache so that the LVG/BD reconcile logic never sees + // duplicate VG names produced by foreign storage layers. + // + // FilterForeignPVs does NOT reject loop devices: the agent manages + // file-backed loop devices as LVM PVs (spec.fileDevices). Unmanaged loop + // PVs that form an entire VG are dropped just below by FilterForeignLoopPVs + // so a guest VM's nested-LVM loop VG cannot collide by name with a managed + // VG. A managed loop PV is kept because the agent tags every VG it creates + // with storage.deckhouse.io/enabled=true. // // cfg.CmdDeadlineDuration bounds every per-PV nsenter+readlink call: // a hung resolver on a single foreign device cannot block the entire @@ -270,7 +276,22 @@ func (s *scanner) fillTheCache(ctx context.Context, log logger.Logger, cache *ca beforePV := len(pvs) pvs = utils.FilterForeignPVs(ctx, log, nil, pvs, cfg.CmdDeadlineDuration) if dropped := beforePV - len(pvs); dropped > 0 { - log.Info(fmt.Sprintf("[fillTheCache] dropped %d foreign PV(s) backed by rbd/drbd/nbd/loop devices", dropped)) + log.Info(fmt.Sprintf("[fillTheCache] dropped %d foreign PV(s) backed by rbd/drbd/nbd devices", dropped)) + } + + // Also drop PVs of unmanaged, purely loop-backed VGs (nested LVM inside a + // guest VM's file-backed disk attached via losetup). Loop PVs are not + // rejected by FilterForeignPVs because the agent manages its own + // file-backed loop devices, but an unmanaged loop VG that shares a name + // with a managed VG would otherwise be detected as a duplicate by + // findDuplicateVGNames and take the managed LVMVolumeGroup offline. + beforeLoopPV := len(pvs) + pvs = utils.FilterForeignLoopPVs(log, vgs, pvs) + if dropped := beforeLoopPV - len(pvs); dropped > 0 { + log.Info(fmt.Sprintf("[fillTheCache] dropped %d PV(s) of unmanaged loop-backed VG(s)", dropped)) + } + + if len(pvs) < beforePV { beforeVG := len(vgs) vgs = utils.FilterVGsByPresentPVs(vgs, pvs) beforeLV := len(lvs) diff --git a/images/agent/internal/type.go b/images/agent/internal/type.go index fbbed3bb0..56da0b33f 100644 --- a/images/agent/internal/type.go +++ b/images/agent/internal/type.go @@ -38,6 +38,7 @@ type LVMVolumeGroupCandidate struct { VGUUID string ExtentSize resource.Quantity Nodes map[string][]LVMVGDevice + FileDeviceNodes map[string][]LVMVGFileDevice } type LVMVGStatusThinPool struct { @@ -57,6 +58,13 @@ type LVMVGDevice struct { BlockDevice string } +type LVMVGFileDevice struct { + FilePath string + LoopDevice string + Size resource.Quantity + PVUUID string +} + type Devices struct { BlockDevices []Device `json:"blockdevices"` } diff --git a/images/agent/internal/utils/activation.go b/images/agent/internal/utils/activation.go index 376a85aeb..19e9b779b 100644 --- a/images/agent/internal/utils/activation.go +++ b/images/agent/internal/utils/activation.go @@ -18,6 +18,7 @@ package utils import ( "context" + "errors" "fmt" "strings" "time" @@ -62,10 +63,10 @@ func FilterVGsByTag(vgs []internal.VGData, tags []string) []internal.VGData { return filtered } -// runWithTimeout invokes fn under a child context with the given timeout, so a +// RunWithTimeout invokes fn under a child context with the given timeout, so a // stuck nsenter-backed LVM command does not block the caller indefinitely. // A non-positive timeout disables the deadline and is treated as "no timeout". -func runWithTimeout[T any](ctx context.Context, timeout time.Duration, fn func(context.Context) (T, error)) (T, error) { +func RunWithTimeout[T any](ctx context.Context, timeout time.Duration, fn func(context.Context) (T, error)) (T, error) { if timeout <= 0 { return fn(ctx) } @@ -74,14 +75,94 @@ func runWithTimeout[T any](ctx context.Context, timeout time.Duration, fn func(c return fn(ctx) } +// ReattachFileDevices re-establishes loop device mappings for file-backed +// PVs after a node reboot. It iterates status.nodes[].fileDevices of every +// LVMVolumeGroup belonging to this node and runs `losetup --find --show` +// for files whose loop device is not attached yet. Must be called BEFORE +// ActivateAllManagedVGs so that LVM can see the PVs. +// +// On any partial failure the function still tries every remaining entry +// (best-effort) and returns a non-nil error joining all per-device +// failures so the caller can log/surface it. A non-nil return is NOT +// fatal: the startup caller deliberately continues to +// ActivateAllManagedVGs anyway, because holding every healthy VG on the +// node (including pure block-device ones) hostage to one unrelated file +// device would be worse. The LVG reconciler re-attaches idempotently via +// provisionFileDevices on its next pass, so a transient losetup failure +// recovers without a pod restart. +func ReattachFileDevices(ctx context.Context, log logger.Logger, commands Commands, cmdTimeout time.Duration, lvgs []LVGWithFileDevices) error { + var failures []error + for _, item := range lvgs { + for _, fd := range item.FileDevices { + if fd.FilePath == "" { + continue + } + if err := ctx.Err(); err != nil { + return errors.Join(append(failures, fmt.Errorf("aborted: %w", err))...) + } + + type findResult struct { + cmd string + loopDev string + } + findRes, err := RunWithTimeout(ctx, cmdTimeout, func(ctx context.Context) (findResult, error) { + cmd, existing, err := commands.FindLoopDeviceByFile(ctx, fd.FilePath) + return findResult{cmd: cmd, loopDev: existing}, err + }) + log.Debug(findRes.cmd) + if err != nil { + failures = append(failures, fmt.Errorf("LVG %s: query loop for %s: %w", item.LVGName, fd.FilePath, err)) + log.Warning(fmt.Sprintf("[ReattachFileDevices] unable to query loop for %s: %v", fd.FilePath, err)) + continue + } + if findRes.loopDev != "" { + log.Debug(fmt.Sprintf("[ReattachFileDevices] %s already attached to %s", fd.FilePath, findRes.loopDev)) + continue + } + + type setupResult struct { + cmd string + loopDev string + } + setupRes, err := RunWithTimeout(ctx, cmdTimeout, func(ctx context.Context) (setupResult, error) { + cmd, loopDev, err := commands.SetupLoopDevice(ctx, fd.FilePath) + return setupResult{cmd: cmd, loopDev: loopDev}, err + }) + log.Debug(setupRes.cmd) + if err != nil { + failures = append(failures, fmt.Errorf("LVG %s: setup loop for %s: %w", item.LVGName, fd.FilePath, err)) + log.Error(err, fmt.Sprintf("[ReattachFileDevices] unable to reattach %s", fd.FilePath)) + continue + } + log.Info(fmt.Sprintf("[ReattachFileDevices] reattached %s → %s (LVG %s)", fd.FilePath, setupRes.loopDev, item.LVGName)) + } + } + if len(failures) > 0 { + return errors.Join(failures...) + } + return nil +} + +// LVGWithFileDevices carries the minimal data needed by ReattachFileDevices. +type LVGWithFileDevices struct { + LVGName string + FileDevices []FileDeviceStatus +} + +// FileDeviceStatus is the status-level record of a single file device. +type FileDeviceStatus struct { + FilePath string + LoopDevice string +} + func ActivateAllManagedVGs(ctx context.Context, log logger.Logger, commands Commands, metrics *monitoring.Metrics, cmdTimeout time.Duration) error { log.Info("[ActivateVGs] refreshing LVM metadata cache") - if cmd, err := runWithTimeout(ctx, cmdTimeout, func(ctx context.Context) (string, error) { + if cmd, err := RunWithTimeout(ctx, cmdTimeout, func(ctx context.Context) (string, error) { return commands.PVScan(ctx) }); err != nil { log.Warning(fmt.Sprintf("[ActivateVGs] pvscan --cache failed (cmd: %s): %v", cmd, err)) } - if cmd, err := runWithTimeout(ctx, cmdTimeout, func(ctx context.Context) (string, error) { + if cmd, err := RunWithTimeout(ctx, cmdTimeout, func(ctx context.Context) (string, error) { return commands.VGScan(ctx) }); err != nil { log.Warning(fmt.Sprintf("[ActivateVGs] vgscan --cache failed (cmd: %s): %v", cmd, err)) @@ -91,7 +172,7 @@ func ActivateAllManagedVGs(ctx context.Context, log logger.Logger, commands Comm data []internal.VGData cmdStr string } - res, err := runWithTimeout(ctx, cmdTimeout, func(ctx context.Context) (vgsResult, error) { + res, err := RunWithTimeout(ctx, cmdTimeout, func(ctx context.Context) (vgsResult, error) { data, cmdStr, _, err := commands.GetAllVGs(ctx) return vgsResult{data: data, cmdStr: cmdStr}, err }) @@ -111,7 +192,7 @@ func ActivateAllManagedVGs(ctx context.Context, log logger.Logger, commands Comm var activationErrors []error for _, vg := range managedVGs { shared := vg.VGShared != "" - cmd, err := runWithTimeout(ctx, cmdTimeout, func(ctx context.Context) (string, error) { + cmd, err := RunWithTimeout(ctx, cmdTimeout, func(ctx context.Context) (string, error) { return commands.VGActivate(ctx, vg.VGName, shared) }) if err != nil { @@ -176,7 +257,7 @@ func EnsureVGActivation( activated := false for _, vg := range vgsToActivate { shared := vg.VGShared != "" - cmd, err := runWithTimeout(ctx, cmdTimeout, func(ctx context.Context) (string, error) { + cmd, err := RunWithTimeout(ctx, cmdTimeout, func(ctx context.Context) (string, error) { return commands.VGActivate(ctx, vg.VGName, shared) }) if err != nil { diff --git a/images/agent/internal/utils/activation_filedevices_test.go b/images/agent/internal/utils/activation_filedevices_test.go new file mode 100644 index 000000000..0d8be6d71 --- /dev/null +++ b/images/agent/internal/utils/activation_filedevices_test.go @@ -0,0 +1,98 @@ +/* +Copyright 2026 Flant JSC + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package utils_test + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "go.uber.org/mock/gomock" + + "github.com/deckhouse/sds-node-configurator/images/agent/internal/logger" + "github.com/deckhouse/sds-node-configurator/images/agent/internal/mock_utils" + "github.com/deckhouse/sds-node-configurator/images/agent/internal/utils" +) + +func testLogger(t *testing.T) logger.Logger { + t.Helper() + log, err := logger.NewLogger(logger.ErrorLevel) + if err != nil { + t.Fatalf("init logger: %v", err) + } + return log +} + +// ReattachFileDevices must reuse an existing loop attachment instead of +// allocating a fresh one — otherwise a node reboot followed by a +// successful reattach leaks loop minors on every restart. +func TestReattachFileDevices_SkipsAlreadyAttached(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + mc := mock_utils.NewMockCommands(ctrl) + + mc.EXPECT().FindLoopDeviceByFile(gomock.Any(), "/data/sds-vg-a-deadbeef0011.img").Return("losetup -j ...", "/dev/loop0", nil) + // SetupLoopDevice MUST NOT be called when the file is already attached. + + err := utils.ReattachFileDevices(context.Background(), testLogger(t), mc, 30*time.Second, []utils.LVGWithFileDevices{ + {LVGName: "vg-a", FileDevices: []utils.FileDeviceStatus{{FilePath: "/data/sds-vg-a-deadbeef0011.img", LoopDevice: "/dev/loop0"}}}, + }) + assert.NoError(t, err) +} + +// When losetup fails on a single file, ReattachFileDevices must still +// process every other entry and return a joined error so the caller can +// abort the activation step. +func TestReattachFileDevices_AggregatesErrors(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + mc := mock_utils.NewMockCommands(ctrl) + + mc.EXPECT().FindLoopDeviceByFile(gomock.Any(), "/data/sds-vg-a-aaaaaaaaaaaa.img").Return("losetup -j ...", "", nil) + mc.EXPECT().SetupLoopDevice(gomock.Any(), "/data/sds-vg-a-aaaaaaaaaaaa.img").Return("losetup --find --show ...", "", errors.New("ENOENT")) + mc.EXPECT().FindLoopDeviceByFile(gomock.Any(), "/data/sds-vg-a-bbbbbbbbbbbb.img").Return("losetup -j ...", "", nil) + mc.EXPECT().SetupLoopDevice(gomock.Any(), "/data/sds-vg-a-bbbbbbbbbbbb.img").Return("losetup --find --show ...", "/dev/loop3", nil) + + err := utils.ReattachFileDevices(context.Background(), testLogger(t), mc, 30*time.Second, []utils.LVGWithFileDevices{ + {LVGName: "vg-a", FileDevices: []utils.FileDeviceStatus{ + {FilePath: "/data/sds-vg-a-aaaaaaaaaaaa.img"}, + {FilePath: "/data/sds-vg-a-bbbbbbbbbbbb.img"}, + }}, + }) + if assert.Error(t, err) { + assert.Contains(t, err.Error(), "aaaaaaaaaaaa.img") + } +} + +// FindLoopDeviceByFile failures must also be surfaced (and remaining +// files still processed best-effort). +func TestReattachFileDevices_QueryFailure(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + mc := mock_utils.NewMockCommands(ctrl) + + mc.EXPECT().FindLoopDeviceByFile(gomock.Any(), "/data/sds-vg-a-cafebabec0de.img").Return("losetup -j ...", "", errors.New("EIO")) + + err := utils.ReattachFileDevices(context.Background(), testLogger(t), mc, 30*time.Second, []utils.LVGWithFileDevices{ + {LVGName: "vg-a", FileDevices: []utils.FileDeviceStatus{{FilePath: "/data/sds-vg-a-cafebabec0de.img"}}}, + }) + if assert.Error(t, err) { + assert.Contains(t, err.Error(), "EIO") + } +} diff --git a/images/agent/internal/utils/commands.go b/images/agent/internal/utils/commands.go index e90a6bbab..2aa4b1125 100644 --- a/images/agent/internal/utils/commands.go +++ b/images/agent/internal/utils/commands.go @@ -28,6 +28,7 @@ import ( "os/exec" "path/filepath" "regexp" + "strconv" "strings" "time" @@ -70,6 +71,15 @@ type Commands interface { UdevadmTrigger(ctx context.Context, paths []string) (string, error) UnmarshalDevices(out []byte) ([]internal.Device, error) ReTag(ctx context.Context, log logger.Logger, metrics *monitoring.Metrics, ctrlName string, cmdTimeout time.Duration) error + + CreateFileDevice(ctx context.Context, path string, sizeBytes int64) (string, error) + SetupLoopDevice(ctx context.Context, filePath string) (string, string, error) + DetachLoopDevice(ctx context.Context, loopDev string) (string, error) + FindLoopDeviceByFile(ctx context.Context, filePath string) (string, string, error) + GetLoopBackingFile(ctx context.Context, loopDev string) (string, string, error) + RemoveFileDevice(ctx context.Context, path string) (string, error) + EnsureFileDeviceDirectory(ctx context.Context, directory string) (string, error) + GetAvailableBytes(ctx context.Context, directory string) (string, int64, error) } type commands struct { @@ -248,7 +258,17 @@ func (commands) CreatePV(path string) (string, error) { var stderr bytes.Buffer cmd.Stderr = &stderr - if err := cmd.Run(); err != nil { + // Route stderr through filterStdErr and treat the command as successful when + // the only output is a benign message (e.g. "File descriptor N leaked on lvm + // invocation"), mirroring ExtendLV. lvm.static run under nsenter routinely + // emits the fd-leak warning and exits non-zero even though pvcreate has + // already written the PV label; surfacing that as an error wrongly trips the + // create/extend rollback against a device that is in fact a healthy PV. A + // real pvcreate failure prints its own diagnostic line, which survives the + // filter, so genuine errors are still reported. + err := cmd.Run() + filteredStdErr := filterStdErr(cmd.String(), stderr) + if err != nil && filteredStdErr.Len() > 0 { return cmd.String(), fmt.Errorf("unable to run cmd: %s, err: %w, stderror = %s", cmd.String(), err, stderr.String()) } @@ -673,7 +693,7 @@ func (c *commands) ReTag(ctx context.Context, log logger.Logger, metrics *monito data []internal.LVData cmdStr string } - lvsRes, err := runWithTimeout(ctx, cmdTimeout, func(ctx context.Context) (lvsResult, error) { + lvsRes, err := RunWithTimeout(ctx, cmdTimeout, func(ctx context.Context) (lvsResult, error) { data, cmdStr, _, err := c.GetAllLVs(ctx) return lvsResult{data: data, cmdStr: cmdStr}, err }) @@ -695,7 +715,7 @@ func (c *commands) ReTag(ctx context.Context, log logger.Logger, metrics *monito if strings.Contains(tag, internal.LVMTags[1]) { start = time.Now() - cmdStr, err := runWithTimeout(ctx, cmdTimeout, func(ctx context.Context) (string, error) { + cmdStr, err := RunWithTimeout(ctx, cmdTimeout, func(ctx context.Context) (string, error) { return c.LVChangeDelTag(ctx, lv, tag) }) metrics.UtilsCommandsDuration(ctrlName, "lvchange").Observe(metrics.GetEstimatedTimeInSeconds(start)) @@ -708,7 +728,7 @@ func (c *commands) ReTag(ctx context.Context, log logger.Logger, metrics *monito } start = time.Now() - cmdStr, err = runWithTimeout(ctx, cmdTimeout, func(ctx context.Context) (string, error) { + cmdStr, err = RunWithTimeout(ctx, cmdTimeout, func(ctx context.Context) (string, error) { return c.VGChangeAddTag(ctx, lv.VGName, internal.LVMTags[0]) }) metrics.UtilsCommandsDuration(ctrlName, "vgchange").Observe(metrics.GetEstimatedTimeInSeconds(start)) @@ -730,7 +750,7 @@ func (c *commands) ReTag(ctx context.Context, log logger.Logger, metrics *monito data []internal.VGData cmdStr string } - vgsRes, err := runWithTimeout(ctx, cmdTimeout, func(ctx context.Context) (vgsResult, error) { + vgsRes, err := RunWithTimeout(ctx, cmdTimeout, func(ctx context.Context) (vgsResult, error) { data, cmdStr, _, err := c.GetAllVGs(ctx) return vgsResult{data: data, cmdStr: cmdStr}, err }) @@ -752,7 +772,7 @@ func (c *commands) ReTag(ctx context.Context, log logger.Logger, metrics *monito if strings.Contains(tag, internal.LVMTags[1]) { start = time.Now() - cmdStr, err := runWithTimeout(ctx, cmdTimeout, func(ctx context.Context) (string, error) { + cmdStr, err := RunWithTimeout(ctx, cmdTimeout, func(ctx context.Context) (string, error) { return c.VGChangeDelTag(ctx, vg.VGName, tag) }) metrics.UtilsCommandsDuration(ctrlName, "vgchange").Observe(metrics.GetEstimatedTimeInSeconds(start)) @@ -765,7 +785,7 @@ func (c *commands) ReTag(ctx context.Context, log logger.Logger, metrics *monito } start = time.Now() - cmdStr, err = runWithTimeout(ctx, cmdTimeout, func(ctx context.Context) (string, error) { + cmdStr, err = RunWithTimeout(ctx, cmdTimeout, func(ctx context.Context) (string, error) { return c.VGChangeAddTag(ctx, vg.VGName, internal.LVMTags[0]) }) metrics.UtilsCommandsDuration(ctrlName, "vgchange").Observe(metrics.GetEstimatedTimeInSeconds(start)) @@ -784,6 +804,206 @@ func (c *commands) ReTag(ctx context.Context, log logger.Logger, metrics *monito return nil } +func (commands) CreateFileDevice(ctx context.Context, path string, sizeBytes int64) (string, error) { + args := nsentrerExpendedArgs("/usr/bin/fallocate", "-l", strconv.FormatInt(sizeBytes, 10), path) + cmd := exec.CommandContext(ctx, internal.NSENTERCmd, args...) + + var stderr bytes.Buffer + cmd.Stderr = &stderr + + if err := cmd.Run(); err != nil { + return cmd.String(), fmt.Errorf("unable to create file device %s: %w, stderr: %s", path, err, stderr.String()) + } + return cmd.String(), nil +} + +func (commands) SetupLoopDevice(ctx context.Context, filePath string) (string, string, error) { + // --nooverlap makes losetup reuse an already-attached loop for this + // backing file (printing it via --show) instead of binding a second + // minor to the same file. Without it, a race between the startup + // reattach and the reconciler's provision step — or an existing + // attachment that FindLoopDeviceByFile reported only under an alias — + // would leak an extra loop device that nothing ever reaps. + // + // --direct-io=on tells the loop driver to open the backing file with + // O_DIRECT so reads/writes bypass the backing filesystem's page cache. + // Our stack layers a filesystem on top of LVM on top of the loop on top + // of the node's filesystem; without direct I/O every page is cached + // twice (once for the volume's filesystem, once for the backing file), + // doubling the RAM footprint and throttling throughput. The kernel + // silently falls back to buffered I/O when the backing file's offset or + // sector size is not aligned (drivers/block/loop.c), so requesting it is + // always safe — at worst it is ignored. + args := nsentrerExpendedArgs("/sbin/losetup", "--find", "--nooverlap", "--direct-io=on", "--show", filePath) + cmd := exec.CommandContext(ctx, internal.NSENTERCmd, args...) + + var stdout, stderr bytes.Buffer + cmd.Stdout = &stdout + cmd.Stderr = &stderr + + if err := cmd.Run(); err != nil { + return cmd.String(), "", fmt.Errorf("unable to setup loop device for %s: %w, stderr: %s", filePath, err, stderr.String()) + } + return cmd.String(), strings.TrimSpace(stdout.String()), nil +} + +func (commands) DetachLoopDevice(ctx context.Context, loopDev string) (string, error) { + args := nsentrerExpendedArgs("/sbin/losetup", "-d", loopDev) + cmd := exec.CommandContext(ctx, internal.NSENTERCmd, args...) + + var stderr bytes.Buffer + cmd.Stderr = &stderr + + if err := cmd.Run(); err != nil { + return cmd.String(), fmt.Errorf("unable to detach loop device %s: %w, stderr: %s", loopDev, err, stderr.String()) + } + return cmd.String(), nil +} + +func (commands) FindLoopDeviceByFile(ctx context.Context, filePath string) (string, string, error) { + args := nsentrerExpendedArgs("/sbin/losetup", "-j", filePath) + cmd := exec.CommandContext(ctx, internal.NSENTERCmd, args...) + + var stdout, stderr bytes.Buffer + cmd.Stdout = &stdout + cmd.Stderr = &stderr + + if err := cmd.Run(); err != nil { + return cmd.String(), "", fmt.Errorf("unable to find loop device for %s: %w, stderr: %s", filePath, err, stderr.String()) + } + + output := strings.TrimSpace(stdout.String()) + if output == "" { + return cmd.String(), "", nil + } + // `losetup -j ` prints one line per loop device bound to the + // file (`/dev/loopN: ... (backing-file)`). Parse the first line only + // — splitting the whole multi-line buffer on ":" would still yield the + // first device but silently hide the fact that several loops point at + // the same file. We return the first device and surface the extras in + // the command string so a leaked double-attach is at least visible. + lines := strings.Split(output, "\n") + first, _, _ := strings.Cut(lines[0], ":") + return cmd.String(), strings.TrimSpace(first), nil +} + +// GetLoopBackingFile returns the canonical backing-file path that loopDev +// is currently attached to. Empty string means the device is not attached. +// +// Implemented via `losetup --noheadings --output BACK-FILE ` so +// the agent does not have to spawn `cat /sys/block//loop/backing_file` +// in the host PID namespace just to read a single value. Goes through the +// same nsenter wrapper as every other host command in this package so the +// argv stays unit-testable. +func (commands) GetLoopBackingFile(ctx context.Context, loopDev string) (string, string, error) { + args := nsentrerExpendedArgs("/sbin/losetup", "--noheadings", "--output", "BACK-FILE", loopDev) + cmd := exec.CommandContext(ctx, internal.NSENTERCmd, args...) + + var stdout, stderr bytes.Buffer + cmd.Stdout = &stdout + cmd.Stderr = &stderr + + if err := cmd.Run(); err != nil { + return cmd.String(), "", fmt.Errorf("unable to read backing file for %s: %w, stderr: %s", loopDev, err, stderr.String()) + } + return cmd.String(), sanitizeBackingFilePath(stdout.String()), nil +} + +// sanitizeBackingFilePath normalises the backing-file path losetup reports. +// When the backing file has been unlinked while the loop is still attached, +// losetup (like /sys/block//loop/backing_file) appends a literal +// " (deleted)" marker, e.g. "/data/sds-vg-a-deadbeef0011.img (deleted)". +// Leaving the marker in place makes IsManagedFileDevicePath miss the basename +// and cleanup refuse to detach the loop, stranding the minor on the node +// forever. Strip the marker so ownership matching still recognises the file. +func sanitizeBackingFilePath(out string) string { + return strings.TrimSpace(strings.TrimSuffix(strings.TrimSpace(out), "(deleted)")) +} + +func (commands) RemoveFileDevice(ctx context.Context, path string) (string, error) { + args := nsentrerExpendedArgs("/bin/rm", "-f", path) + cmd := exec.CommandContext(ctx, internal.NSENTERCmd, args...) + + var stderr bytes.Buffer + cmd.Stderr = &stderr + + if err := cmd.Run(); err != nil { + return cmd.String(), fmt.Errorf("unable to remove file device %s: %w, stderr: %s", path, err, stderr.String()) + } + return cmd.String(), nil +} + +// EnsureFileDeviceDirectory creates directory (and any missing parents) on the +// host so the backing file can be allocated into it. The agent runs in PID 1's +// mount namespace, so this is `mkdir -p` against the node's root filesystem. +// `mkdir -p` is idempotent (no error if the directory already exists) and fails +// only when the path is genuinely unusable — a read-only filesystem, or a +// non-directory component along the way. +func (commands) EnsureFileDeviceDirectory(ctx context.Context, directory string) (string, error) { + args := nsentrerExpendedArgs("/bin/mkdir", "-p", directory) + cmd := exec.CommandContext(ctx, internal.NSENTERCmd, args...) + + var stderr bytes.Buffer + cmd.Stderr = &stderr + + if err := cmd.Run(); err != nil { + return cmd.String(), fmt.Errorf("unable to create directory %q on the node: %w, stderr: %s", directory, err, stderr.String()) + } + return cmd.String(), nil +} + +// GetAvailableBytes returns the number of bytes that can be allocated in +// directory's filesystem without dipping into the superuser-reserved +// blocks, so the caller can refuse an oversized backing file before +// `fallocate` either fails halfway or fills the node's root filesystem and +// trips kubelet's DiskPressure eviction. +// +// It reads the value with `stat -f` in PID 1's mount namespace (the agent +// itself does not share the host mount namespace, so a Go syscall.Statfs +// would measure the wrong filesystem): %S is the fundamental block size and +// %a is the count of blocks available to a non-superuser. Using %a rather +// than %f deliberately leaves the filesystem's reserve untouched, which +// doubles as built-in headroom for the node. +func (commands) GetAvailableBytes(ctx context.Context, directory string) (string, int64, error) { + args := nsentrerExpendedArgs("/usr/bin/stat", "-f", "-c", "%S %a", directory) + cmd := exec.CommandContext(ctx, internal.NSENTERCmd, args...) + + var stdout, stderr bytes.Buffer + cmd.Stdout = &stdout + cmd.Stderr = &stderr + + if err := cmd.Run(); err != nil { + return cmd.String(), 0, fmt.Errorf("unable to stat filesystem for %q: %w, stderr: %s", directory, err, stderr.String()) + } + + available, err := parseStatfsAvailableBytes(stdout.String()) + if err != nil { + return cmd.String(), 0, fmt.Errorf("unable to parse free space for %q: %w", directory, err) + } + return cmd.String(), available, nil +} + +// parseStatfsAvailableBytes parses the " " +// output of `stat -f -c "%S %a"` into the number of available bytes. +func parseStatfsAvailableBytes(out string) (int64, error) { + fields := strings.Fields(out) + if len(fields) != 2 { + return 0, fmt.Errorf("expected 2 fields, got %q", strings.TrimSpace(out)) + } + blockSize, err := strconv.ParseInt(fields[0], 10, 64) + if err != nil { + return 0, fmt.Errorf("invalid block size %q: %w", fields[0], err) + } + availBlocks, err := strconv.ParseInt(fields[1], 10, 64) + if err != nil { + return 0, fmt.Errorf("invalid available block count %q: %w", fields[1], err) + } + if blockSize < 0 || availBlocks < 0 { + return 0, fmt.Errorf("negative block size %d or available block count %d", blockSize, availBlocks) + } + return blockSize * availBlocks, nil +} + func unmarshalPVs(out []byte) ([]internal.PVData, error) { var pvR internal.PVReport @@ -891,7 +1111,13 @@ func filterStdErr(command string, stdErr bytes.Buffer) bytes.Buffer { // this needs as if the controller were restarted and found existing LVG thin-pools with size equals 100%VG space, // as the Thin-pool size on the node might be less than Spec one even with delta (because of metadata). So the controller // will try to resize the Thin-pool with 100%VG space and will get the error. + // + // LVM wording for a no-op resize differs across versions: older releases + // print "No size change.", newer ones (lvm 2.03.x) print + // "New size (N extents) matches existing size (M extents).". Both mean the + // requested size already matches the LV, so neither is a real failure. regexpNoSizeChangeError := ` No size change.+` + regexpSizeMatchesError := `New size \(.+\) matches existing size \(.+\)` regex1, err := regexp.Compile(regexpPattern) if err != nil { return stdErr @@ -904,12 +1130,17 @@ func filterStdErr(command string, stdErr bytes.Buffer) bytes.Buffer { if err != nil { return stdErr } + regex4, err := regexp.Compile(regexpSizeMatchesError) + if err != nil { + return stdErr + } for stdErrScanner.Scan() { line := stdErrScanner.Text() if regex1.MatchString(line) || regex2.MatchString(line) || - regex3.MatchString(line) { + regex3.MatchString(line) || + regex4.MatchString(line) { golog.Printf("WARNING: [filterStdErr] Line filtered from stderr due to matching exclusion pattern. Line: '%s'. Triggered by command: '%s'.", line, command) } else { filteredStdErr.WriteString(line + "\n") diff --git a/images/agent/internal/utils/commands_test.go b/images/agent/internal/utils/commands_test.go index 5123580ba..69bab0ff7 100644 --- a/images/agent/internal/utils/commands_test.go +++ b/images/agent/internal/utils/commands_test.go @@ -17,6 +17,7 @@ limitations under the License. package utils import ( + "bytes" "context" "testing" @@ -402,8 +403,8 @@ func TestLvmStaticExtendedArgs(t *testing.T) { } t.Run("config_value_contains_all_foreign_prefixes_and_retention", func(t *testing.T) { - // Defensive cross-check: --config must reject all four foreign - // device prefixes the post-filter knows about (rbd/drbd/nbd/loop) + // Defensive cross-check: --config must reject all foreign + // device prefixes the post-filter knows about (rbd/drbd/nbd) // and must cap /etc/lvm/archive growth. If either drops out due // to a refactor, this assertion catches it before the next scan // loop silently regresses. @@ -509,3 +510,106 @@ func TestUdevadmTriggerEmptyPathsIsNoop(t *testing.T) { assert.Empty(t, out) }) } + +// GetLoopBackingFile must strip the " (deleted)" marker losetup appends when +// the backing file was unlinked while the loop is still attached. Without +// stripping it, IsManagedFileDevicePath would not recognise the basename and +// cleanup would refuse to detach the loop, stranding the minor on the node. +func TestSanitizeBackingFilePath(t *testing.T) { + tests := []struct { + name string + in string + want string + }{ + {"plain", "/data/sds-vg-a-deadbeef0011.img", "/data/sds-vg-a-deadbeef0011.img"}, + {"trailing newline", "/data/sds-vg-a-deadbeef0011.img\n", "/data/sds-vg-a-deadbeef0011.img"}, + {"deleted marker", "/data/sds-vg-a-deadbeef0011.img (deleted)", "/data/sds-vg-a-deadbeef0011.img"}, + {"deleted marker with newline", "/data/sds-vg-a-deadbeef0011.img (deleted)\n", "/data/sds-vg-a-deadbeef0011.img"}, + {"empty", "", ""}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := sanitizeBackingFilePath(tt.in) + assert.Equal(t, tt.want, got) + if tt.want != "" { + assert.True(t, IsManagedFileDevicePath(got, "vg-a"), + "sanitized path must still be recognised as managed") + } + }) + } +} + +// parseStatfsAvailableBytes turns the " " +// output of `stat -f -c "%S %a"` into a byte count; GetAvailableBytes relies +// on it to refuse an oversized backing file before fallocate fills the node. +func TestParseStatfsAvailableBytes(t *testing.T) { + tests := []struct { + name string + in string + want int64 + wantErr bool + }{ + {"plain", "4096 1000", 4096 * 1000, false}, + {"trailing newline", "4096 1000\n", 4096 * 1000, false}, + {"extra whitespace", " 4096 1000 ", 4096 * 1000, false}, + {"zero available", "4096 0", 0, false}, + {"empty", "", 0, true}, + {"single field", "4096", 0, true}, + {"too many fields", "4096 1000 7", 0, true}, + {"non-numeric block size", "abc 1000", 0, true}, + {"non-numeric available", "4096 abc", 0, true}, + {"negative available", "4096 -1", 0, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := parseStatfsAvailableBytes(tt.in) + if tt.wantErr { + assert.Error(t, err) + return + } + assert.NoError(t, err) + assert.Equal(t, tt.want, got) + }) + } +} + +func TestFilterStdErr(t *testing.T) { + tests := []struct { + name string + stderr string + filtered bool // true => the line is recognised as benign and dropped + }{ + { + name: "old lvm no-op resize wording", + stderr: " No size change. (use --force to override)", + filtered: true, + }, + { + name: "new lvm no-op resize wording", + stderr: " New size (2553 extents) matches existing size (2553 extents).", + filtered: true, + }, + { + name: "regex version mismatch", + stderr: "Regex version mismatch, expected: 1.2.3 actual: 4.5.6", + filtered: true, + }, + { + name: "genuine error is kept", + stderr: " Volume group \"vg-thin-file\" not found.", + filtered: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var buf bytes.Buffer + buf.WriteString(tt.stderr) + got := filterStdErr("lvextend -l 100%VG /dev/vg/lv", buf) + if tt.filtered { + assert.Zero(t, got.Len(), "benign line must be filtered out so the no-op resize is not treated as an error") + } else { + assert.NotZero(t, got.Len(), "genuine error must be preserved") + } + }) + } +} diff --git a/images/agent/internal/utils/devicefilter.go b/images/agent/internal/utils/devicefilter.go index fc7a8e990..6a0a2ec05 100644 --- a/images/agent/internal/utils/devicefilter.go +++ b/images/agent/internal/utils/devicefilter.go @@ -64,8 +64,9 @@ func HostNsenterCanonicalResolver(ctx context.Context, devPath string) (string, // IsForeignDeviceBase reports whether the given canonical basename // belongs to a storage layer the agent must ignore. The check is a // strict prefix match against internal.ForeignDeviceBasePrefixes so it -// catches partitions of foreign devices too (e.g. "rbd14p1", "loop970", -// "drbd0"). +// catches partitions of foreign devices too (e.g. "rbd14p1", "nbd0p1", +// "drbd0"). Loop devices are deliberately absent from the prefix list — +// the agent manages file-backed loop devices as LVM PVs. func IsForeignDeviceBase(base string) bool { for _, prefix := range internal.ForeignDeviceBasePrefixes { if strings.HasPrefix(base, prefix) { @@ -77,7 +78,18 @@ func IsForeignDeviceBase(base string) bool { // FilterForeignPVs returns a copy of pvs with PVs whose underlying // canonical device belongs to a foreign storage layer (Ceph RBD, DRBD, -// NBD, loopback) removed. +// NBD) removed. +// +// Loop devices are intentionally NOT dropped here: the agent manages +// file-backed loop devices as LVM PVs (spec.fileDevices), so a blanket +// loop reject would hide its own managed PVs. Ownership of a loop PV is +// instead established later, in the discoverer, via the backing-file owner +// pattern (IsManagedFileDevicePath) gated on the VG's +// storage.deckhouse.io/lvmVolumeGroupName tag. Unmanaged loop PVs that +// form a whole VG (e.g. nested LVM inside a file-backed guest VM disk) are +// dropped separately by FilterForeignLoopPVs so they cannot collide by +// name with a managed VG; see that function for why the tag filter alone +// is not enough. // // lvm.static bundled with the agent has no udev integration and so it // enumerates devices via /dev/block/MAJOR:MINOR and /dev/disk/by-id/ @@ -98,7 +110,7 @@ func IsForeignDeviceBase(base string) bool { // assumption that a transient resolver failure must not silently hide // a legitimate PV. // -// Each resolver call runs under runWithTimeout(cmdTimeout) so a hung +// Each resolver call runs under RunWithTimeout(cmdTimeout) so a hung // nsenter-backed readlink cannot block the scan loop indefinitely. // This mirrors the per-command timeout protection introduced in // PR #290 for every other lvm.static / nsenter invocation in @@ -124,7 +136,7 @@ func FilterForeignPVs( out = append(out, pv) continue } - resolved, err := runWithTimeout(ctx, cmdTimeout, func(ctx context.Context) (string, error) { + resolved, err := RunWithTimeout(ctx, cmdTimeout, func(ctx context.Context) (string, error) { return resolver(ctx, pv.PVName) }) if err != nil { @@ -148,6 +160,81 @@ func FilterForeignPVs( return out } +// FilterForeignLoopPVs drops PVs that belong to an unmanaged, purely +// loop-backed Volume Group — e.g. nested LVM inside a guest VM's +// file-backed disk attached on the host via losetup. +// +// Loop devices are not rejected by FilterForeignPVs because the agent +// manages its own file-backed loop devices (spec.fileDevices) as PVs. +// But an unmanaged loop-backed VG that reaches the cache is dangerous: +// findDuplicateVGNames runs over every cached VG (before tag filtering), +// so a guest VG that happens to share a name with a managed VG +// (`data`, `vg0`, … are common defaults) is detected as a duplicate and +// takes the *managed* LVMVolumeGroup offline (VGReady=False), and the +// agent's name-keyed cache lookups (FindVG/FindLV) could mix the two. +// +// A VG is dropped here only when ALL of the following hold: +// - it is NOT tagged storage.deckhouse.io/enabled=true (managed VGs, +// including the agent's own file-backed ones, are always kept — the +// agent tags every VG it creates at vgcreate time); +// - it has at least one PV and every one of its PVs is a /dev/loop* +// device (a VG with any real block-device PV is a legitimate, +// potentially-adoptable local VG and must stay visible). +// +// This restores the pre-spec.fileDevices behaviour for foreign loop VGs +// (loop PVs used to be rejected wholesale) while keeping managed +// file-backed loop VGs. Bare loop PVs not part of any VG are kept; they +// carry no VG name and cannot poison name resolution. +// +// Detection is by the /dev/loop name prefix, matching how the discoverer +// itself classifies loop PVs (Discoverer.configureCandidateNodeDevices). +// A managed loop PV occasionally reported under a /dev/disk or /dev/block +// alias counts as "non-loop" here, which only makes the filter more +// conservative (the VG is kept), never dropping a managed VG. +func FilterForeignLoopPVs(log logger.Logger, vgs []internal.VGData, pvs []internal.PVData) []internal.PVData { + managed := make(map[string]struct{}, len(vgs)) + for _, vg := range vgs { + if strings.Contains(vg.VGTags, internal.LVMTags[0]) { + managed[vg.VGUUID] = struct{}{} + } + } + + hasAnyPV := make(map[string]bool, len(pvs)) + hasNonLoopPV := make(map[string]bool, len(pvs)) + for _, pv := range pvs { + if pv.VGUuid == "" { + continue + } + hasAnyPV[pv.VGUuid] = true + if !strings.HasPrefix(pv.PVName, "/dev/loop") { + hasNonLoopPV[pv.VGUuid] = true + } + } + + isForeignLoopVG := func(vgUUID string) bool { + if vgUUID == "" { + return false + } + if _, ok := managed[vgUUID]; ok { + return false + } + return hasAnyPV[vgUUID] && !hasNonLoopPV[vgUUID] + } + + out := make([]internal.PVData, 0, len(pvs)) + for _, pv := range pvs { + if isForeignLoopVG(pv.VGUuid) { + log.Info(fmt.Sprintf( + "[FilterForeignLoopPVs] dropping PV %q of unmanaged loop-backed VG %q (VG_UUID=%q)", + pv.PVName, pv.VGName, pv.VGUuid, + )) + continue + } + out = append(out, pv) + } + return out +} + // FilterVGsByPresentPVs returns a copy of vgs that keeps only VGs // referenced by at least one PV in pvs (matched by VGUuid). It is // meant to run right after FilterForeignPVs so that phantom VGs whose diff --git a/images/agent/internal/utils/devicefilter_test.go b/images/agent/internal/utils/devicefilter_test.go index b54167ff5..ef0076787 100644 --- a/images/agent/internal/utils/devicefilter_test.go +++ b/images/agent/internal/utils/devicefilter_test.go @@ -38,9 +38,9 @@ func TestIsForeignDeviceBase(t *testing.T) { {"rbd with partition", "rbd14p1", true}, {"drbd canonical", "drbd0", true}, {"nbd canonical", "nbd5", true}, - {"loop canonical", "loop970", true}, - {"loop with high index", "loop1234", true}, + {"loop canonical", "loop970", false}, + {"loop with high index", "loop1234", false}, {"nvme canonical", "nvme4n1p1", false}, {"sda canonical", "sda1", false}, {"md raid", "md1", false}, @@ -70,8 +70,8 @@ func TestFilterForeignPVs(t *testing.T) { {PVName: "/dev/md1", VGName: "vg0", VGUuid: "IZMRUl"}, {PVName: "/dev/block/251:144", VGName: "vg-1", VGUuid: "Czf0Sf"}, // -> rbd9 {PVName: "/dev/block/251:16", VGName: "vg-thin-data", VGUuid: "zR5ouf"}, // -> rbd1 - {PVName: "/dev/disk/by-id/lvm-pv-uuid-9Uuprg-IFQM-5c2y", VGName: "vg-1", VGUuid: "kHmPc6"}, // -> loop808 - {PVName: "/dev/disk/by-id/lvm-pv-uuid-xl1soD-zQZM-BRAt", VGName: "vg-thin-data", VGUuid: "He4J"}, // -> loop485 + {PVName: "/dev/disk/by-id/lvm-pv-uuid-9Uuprg-IFQM-5c2y", VGName: "vg-1", VGUuid: "kHmPc6"}, // -> loop808 (kept — loops are managed) + {PVName: "/dev/disk/by-id/lvm-pv-uuid-xl1soD-zQZM-BRAt", VGName: "vg-thin-data", VGUuid: "He4J"}, // -> loop485 (kept — loops are managed) } // Resolver mimics the readlink -f result observed on d8-virt-node-0 @@ -94,7 +94,11 @@ func TestFilterForeignPVs(t *testing.T) { } got := FilterForeignPVs(context.Background(), log, resolver, pvs, 0) - wantNames := []string{"/dev/nvme4n1p1", "/dev/nvme4n1p2", "/dev/md1"} + wantNames := []string{ + "/dev/nvme4n1p1", "/dev/nvme4n1p2", "/dev/md1", + "/dev/disk/by-id/lvm-pv-uuid-9Uuprg-IFQM-5c2y", + "/dev/disk/by-id/lvm-pv-uuid-xl1soD-zQZM-BRAt", + } gotNames := make([]string, 0, len(got)) for _, pv := range got { gotNames = append(gotNames, pv.PVName) @@ -190,6 +194,95 @@ func TestFilterForeignPVs_perCallTimeout(t *testing.T) { "(same contract as the transient-error branch)") } +func TestFilterForeignLoopPVs(t *testing.T) { + log, err := logger.NewLogger(logger.ErrorLevel) + if err != nil { + t.Fatalf("unable to create logger: %v", err) + } + + const managedTag = "storage.deckhouse.io/enabled=true,storage.deckhouse.io/lvmVolumeGroupName=vg-managed" + + tests := []struct { + name string + vgs []internal.VGData + pvs []internal.PVData + wantPVs []string + }{ + { + name: "drops an unmanaged loop-only VG", + vgs: []internal.VGData{ + {VGName: "data", VGUUID: "foreign", VGTags: ""}, + }, + pvs: []internal.PVData{ + {PVName: "/dev/loop7", VGUuid: "foreign", VGName: "data"}, + }, + wantPVs: nil, + }, + { + name: "keeps a managed loop-only VG (enabled tag present)", + vgs: []internal.VGData{ + {VGName: "vg-managed", VGUUID: "ours", VGTags: managedTag}, + }, + pvs: []internal.PVData{ + {PVName: "/dev/loop3", VGUuid: "ours", VGName: "vg-managed"}, + }, + wantPVs: []string{"/dev/loop3"}, + }, + { + name: "keeps an unmanaged VG with a non-loop PV (adoptable block VG)", + vgs: []internal.VGData{ + {VGName: "vg-block", VGUUID: "block", VGTags: ""}, + }, + pvs: []internal.PVData{ + {PVName: "/dev/sdb", VGUuid: "block", VGName: "vg-block"}, + }, + wantPVs: []string{"/dev/sdb"}, + }, + { + name: "keeps an unmanaged mixed VG (one loop, one block PV)", + vgs: []internal.VGData{ + {VGName: "vg-mixed", VGUUID: "mixed", VGTags: ""}, + }, + pvs: []internal.PVData{ + {PVName: "/dev/loop9", VGUuid: "mixed", VGName: "vg-mixed"}, + {PVName: "/dev/sdc", VGUuid: "mixed", VGName: "vg-mixed"}, + }, + wantPVs: []string{"/dev/loop9", "/dev/sdc"}, + }, + { + name: "keeps a bare loop PV not part of any VG", + vgs: nil, + pvs: []internal.PVData{ + {PVName: "/dev/loop1", VGUuid: ""}, + }, + wantPVs: []string{"/dev/loop1"}, + }, + { + name: "drops only the foreign loop VG, keeps the managed one with the same name", + vgs: []internal.VGData{ + {VGName: "data", VGUUID: "foreign", VGTags: ""}, + {VGName: "data", VGUUID: "ours", VGTags: managedTag}, + }, + pvs: []internal.PVData{ + {PVName: "/dev/loop7", VGUuid: "foreign", VGName: "data"}, + {PVName: "/dev/loop8", VGUuid: "ours", VGName: "data"}, + }, + wantPVs: []string{"/dev/loop8"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := FilterForeignLoopPVs(log, tt.vgs, tt.pvs) + gotNames := make([]string, 0, len(got)) + for _, pv := range got { + gotNames = append(gotNames, pv.PVName) + } + assert.ElementsMatch(t, tt.wantPVs, gotNames) + }) + } +} + func TestFilterVGsByPresentPVs(t *testing.T) { tests := []struct { name string diff --git a/images/agent/internal/utils/filedevices.go b/images/agent/internal/utils/filedevices.go new file mode 100644 index 000000000..af2411673 --- /dev/null +++ b/images/agent/internal/utils/filedevices.go @@ -0,0 +1,101 @@ +/* +Copyright 2026 Flant JSC + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package utils + +import ( + "crypto/sha256" + "encoding/hex" + "fmt" + "path/filepath" + "strings" + + "k8s.io/apimachinery/pkg/api/resource" + + "github.com/deckhouse/sds-node-configurator/images/agent/internal" +) + +// fileDeviceBasenameHashLen is the prefix length of the lower-case hex +// SHA-256 digest embedded in a managed backing-file basename. 12 hex +// chars carry 48 bits — collision-resistant enough to discriminate two +// fileDevices entries that share the same directory while staying short +// enough to fit comfortably into status fields. +const fileDeviceBasenameHashLen = 12 + +// BuildFileDevicePath returns the deterministic absolute path the agent +// uses for the backing file of a single spec.fileDevices entry. +// +// The basename is `-` +// where is a truncated SHA-256 of (directory, size). The name is +// fully determined by the spec entry's contents — not its position in +// the slice — so reordering, deleting, or inserting entries in +// `spec.fileDevices` does not silently rename an existing file. +// +// Together with IsManagedFileDevicePath this is the single owner marker +// the agent relies on: the discoverer never claims a loop PV as a +// managed file device unless its backing file's basename matches this +// pattern with the same lvgName. +func BuildFileDevicePath(directory, lvgName string, size resource.Quantity) string { + digest := sha256.Sum256(fmt.Appendf(nil, "%s|%d", directory, size.Value())) + hash := hex.EncodeToString(digest[:])[:fileDeviceBasenameHashLen] + basename := internal.FileDevicePrefix + lvgName + "-" + hash + internal.FileDeviceImageSuffix + return filepath.Join(directory, basename) +} + +// IsManagedFileDevicePath reports whether path looks like a backing file +// the agent itself created for the given lvgName. The check is purely +// structural (basename pattern) — it does not stat the file. It is used +// by the discoverer to gate "is this loop PV one of ours?" and by +// cleanup to refuse rm-ing unrelated paths even if status was somehow +// corrupted. +// +// An empty lvgName matches any LVG-owned file, which is the right +// behaviour during cluster-wide cleanup where the caller does not know +// the owning LVG name yet. +func IsManagedFileDevicePath(path, lvgName string) bool { + if path == "" { + return false + } + base := filepath.Base(path) + if !strings.HasPrefix(base, internal.FileDevicePrefix) { + return false + } + if !strings.HasSuffix(base, internal.FileDeviceImageSuffix) { + return false + } + middle := strings.TrimSuffix(strings.TrimPrefix(base, internal.FileDevicePrefix), internal.FileDeviceImageSuffix) + dashIdx := strings.LastIndex(middle, "-") + if dashIdx <= 0 || dashIdx >= len(middle)-1 { + return false + } + name := middle[:dashIdx] + hash := middle[dashIdx+1:] + if len(hash) != fileDeviceBasenameHashLen { + return false + } + for _, c := range hash { + switch { + case c >= '0' && c <= '9': + case c >= 'a' && c <= 'f': + default: + return false + } + } + if lvgName == "" { + return name != "" + } + return name == lvgName +} diff --git a/images/agent/internal/utils/filedevices_test.go b/images/agent/internal/utils/filedevices_test.go new file mode 100644 index 000000000..ff084b85f --- /dev/null +++ b/images/agent/internal/utils/filedevices_test.go @@ -0,0 +1,85 @@ +/* +Copyright 2026 Flant JSC + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package utils + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "k8s.io/apimachinery/pkg/api/resource" +) + +func TestBuildFileDevicePath_DeterministicAndDistinguishesEntries(t *testing.T) { + dir := "/data" + lvg := "vg-a" + size10 := resource.MustParse("10Gi") + size20 := resource.MustParse("20Gi") + + p1 := BuildFileDevicePath(dir, lvg, size10) + p2 := BuildFileDevicePath(dir, lvg, size10) + p3 := BuildFileDevicePath(dir, lvg, size20) + p4 := BuildFileDevicePath("/other", lvg, size10) + + assert.Equal(t, p1, p2, "same inputs must produce the same path") + assert.NotEqual(t, p1, p3, "different size must produce a different basename") + assert.NotEqual(t, p1, p4, "different directory must produce a different path") + + assert.True(t, IsManagedFileDevicePath(p1, lvg)) + assert.True(t, IsManagedFileDevicePath(p3, lvg)) + assert.True(t, IsManagedFileDevicePath(p4, lvg)) +} + +func TestIsManagedFileDevicePath(t *testing.T) { + tests := []struct { + name string + path string + lvg string + want bool + }{ + {"empty path", "", "vg-a", false}, + {"no prefix", "/data/random.img", "vg-a", false}, + {"no suffix", "/data/sds-vg-a-deadbeef0011", "vg-a", false}, + {"wrong lvg", "/data/sds-vg-b-deadbeef0011.img", "vg-a", false}, + {"matching lvg", "/data/sds-vg-a-deadbeef0011.img", "vg-a", true}, + {"hash too short", "/data/sds-vg-a-dead.img", "vg-a", false}, + {"non-hex hash", "/data/sds-vg-a-zzzzzzzzzzzz.img", "vg-a", false}, + {"any-lvg ok", "/data/sds-vg-a-deadbeef0011.img", "", true}, + {"any-lvg rejects bogus", "/etc/passwd", "", false}, + {"libvirt qcow2 looks nothing like ours", "/var/lib/libvirt/images/disk0.qcow2", "vg-a", false}, + {"snap loop image", "/var/lib/snapd/snaps/core22.snap", "vg-a", false}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.want, IsManagedFileDevicePath(tc.path, tc.lvg)) + }) + } +} + +func TestIsManagedFileDevicePath_RoundTripWithBuildFileDevicePath(t *testing.T) { + // Walk a representative grid of inputs to make sure Build/IsManaged + // stay in sync no matter what (directory, size) pair the user gives. + for _, dir := range []string{"/data", "/var/lib/sds", "/mnt/x"} { + for _, sz := range []string{"1Gi", "10Gi", "999Gi", "2Ti"} { + for _, lvg := range []string{"vg-a", "data-vg", "long-name-with-dashes"} { + q := resource.MustParse(sz) + p := BuildFileDevicePath(dir, lvg, q) + assert.True(t, IsManagedFileDevicePath(p, lvg), "%s should match lvg %s", p, lvg) + assert.False(t, IsManagedFileDevicePath(p, "other-lvg"), "%s must NOT match foreign lvg", p) + } + } + } +} diff --git a/images/controller/pkg/controller/lvm_volume_group_set_watcher.go b/images/controller/pkg/controller/lvm_volume_group_set_watcher.go index 5f44d543b..e934c5f69 100644 --- a/images/controller/pkg/controller/lvm_volume_group_set_watcher.go +++ b/images/controller/pkg/controller/lvm_volume_group_set_watcher.go @@ -250,6 +250,7 @@ func matchConfiguredLVGWithExistingOne(lvg *v1alpha1.LVMVolumeGroup, lvgs map[st func updateLVMVolumeGroupByConfiguredFromSet(ctx context.Context, cl client.Client, existing, configured *v1alpha1.LVMVolumeGroup) error { existing.Spec.ThinPools = configured.Spec.ThinPools existing.Spec.BlockDeviceSelector = configured.Spec.BlockDeviceSelector + existing.Spec.FileDevices = configured.Spec.FileDevices return cl.Update(ctx, existing) } @@ -293,6 +294,7 @@ func configureLVGBySet(lvgSet *v1alpha1.LVMVolumeGroupSet, node v1.Node) *v1alph BlockDeviceSelector: lvgSet.Spec.LVGTemplate.BlockDeviceSelector, ThinPools: lvgSet.Spec.LVGTemplate.ThinPools, Type: lvgSet.Spec.LVGTemplate.Type, + FileDevices: lvgSet.Spec.LVGTemplate.FileDevices, Local: v1alpha1.LVMVolumeGroupLocalSpec{ NodeName: node.Name, }, diff --git a/images/controller/pkg/controller/lvm_volume_group_set_watcher_test.go b/images/controller/pkg/controller/lvm_volume_group_set_watcher_test.go new file mode 100644 index 000000000..9d215d5a0 --- /dev/null +++ b/images/controller/pkg/controller/lvm_volume_group_set_watcher_test.go @@ -0,0 +1,84 @@ +/* +Copyright 2026 Flant JSC + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package controller + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + v1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "github.com/deckhouse/sds-node-configurator/api/v1alpha1" +) + +// A file-only LVMVolumeGroupSet template carries no blockDeviceSelector; +// the child LVMVolumeGroup is admitted only if configureLVGBySet copies +// fileDevices over. Without it the child has neither selector nor +// fileDevices and is rejected by its own CEL rule. +func TestConfigureLVGBySet_CopiesFileDevices(t *testing.T) { + fileDevices := []v1alpha1.LVMVolumeGroupFileDeviceSpec{ + {Directory: "/data/lvm-backing", Size: resource.MustParse("50Gi")}, + } + lvgSet := &v1alpha1.LVMVolumeGroupSet{ + ObjectMeta: metav1.ObjectMeta{Name: "set-a"}, + Spec: v1alpha1.LVMVolumeGroupSetSpec{ + LVGTemplate: v1alpha1.LVMVolumeGroupTemplate{ + ActualVGNameOnTheNode: "vg-file", + Type: "Local", + FileDevices: fileDevices, + }, + }, + } + + lvg := configureLVGBySet(lvgSet, v1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node-0"}}) + + assert.Equal(t, fileDevices, lvg.Spec.FileDevices) + assert.Nil(t, lvg.Spec.BlockDeviceSelector) + assert.Equal(t, "node-0", lvg.Spec.Local.NodeName) +} + +// Editing fileDevices on the set template (the documented "add an entry to +// grow capacity" flow) must propagate to already-created child LVGs. +func TestUpdateLVMVolumeGroupByConfiguredFromSet_PropagatesFileDevices(t *testing.T) { + configured := &v1alpha1.LVMVolumeGroup{ + Spec: v1alpha1.LVMVolumeGroupSpec{ + FileDevices: []v1alpha1.LVMVolumeGroupFileDeviceSpec{ + {Directory: "/data", Size: resource.MustParse("10Gi")}, + {Directory: "/data", Size: resource.MustParse("20Gi")}, + }, + }, + } + existing := &v1alpha1.LVMVolumeGroup{ + ObjectMeta: metav1.ObjectMeta{Name: "set-a-0"}, + Spec: v1alpha1.LVMVolumeGroupSpec{ + FileDevices: []v1alpha1.LVMVolumeGroupFileDeviceSpec{ + {Directory: "/data", Size: resource.MustParse("10Gi")}, + }, + }, + } + + ctx := context.Background() + cl := NewFakeClient() + assert.NoError(t, cl.Create(ctx, existing)) + + err := updateLVMVolumeGroupByConfiguredFromSet(ctx, cl, existing, configured) + assert.NoError(t, err) + assert.Equal(t, configured.Spec.FileDevices, existing.Spec.FileDevices) +} diff --git a/openapi/config-values.yaml b/openapi/config-values.yaml index 99cfa31ba..f31104798 100644 --- a/openapi/config-values.yaml +++ b/openapi/config-values.yaml @@ -19,3 +19,16 @@ properties: type: boolean default: true description: Enable support of thin LVM volumes. + fileDevicesDirectory: + type: string + default: /opt/deckhouse/sds/file-devices + pattern: '^/.*$' + description: | + Base directory on each node under which the agent is allowed to create + backing files for file-backed LVM devices (`spec.fileDevices`). + + Every `spec.fileDevices[].directory` must be this directory or a + subdirectory of it; the agent rejects backing files outside this subtree + to prevent an arbitrary host path (for example `/` or `/var/lib/kubelet`) + from being filled up. Override it to point at a dedicated data disk + mounted on the node when the default location does not have enough space. diff --git a/openapi/doc-ru-config-values.yaml b/openapi/doc-ru-config-values.yaml index d33875657..de24c5a1d 100644 --- a/openapi/doc-ru-config-values.yaml +++ b/openapi/doc-ru-config-values.yaml @@ -6,3 +6,14 @@ properties: description: Уровень логирования для приложений модуля. enableThinProvisioning: description: Включить поддержку thin-LVM-томов. + fileDevicesDirectory: + description: | + Базовый каталог на каждом узле, внутри которого агенту разрешено создавать + файлы-подложки для файловых LVM-устройств (`spec.fileDevices`). + + Каждый `spec.fileDevices[].directory` должен совпадать с этим каталогом + либо быть его подкаталогом; файлы-подложки вне этого поддерева агент + отклоняет, чтобы произвольный путь на узле (например, `/` или + `/var/lib/kubelet`) нельзя было переполнить. Переопределите параметр, + указав отдельный диск с данными, смонтированный на узле, если в каталоге + по умолчанию недостаточно места. diff --git a/templates/agent/daemonset.yaml b/templates/agent/daemonset.yaml index bcf76d99b..367a2095d 100644 --- a/templates/agent/daemonset.yaml +++ b/templates/agent/daemonset.yaml @@ -196,6 +196,8 @@ spec: valueFrom: fieldRef: fieldPath: spec.nodeName + - name: FILE_DEVICES_DIRECTORY + value: {{ .Values.sdsNodeConfigurator.fileDevicesDirectory | quote }} - name: LOG_LEVEL {{- if eq .Values.sdsNodeConfigurator.logLevel "ERROR" }} value: "0" diff --git a/templates/agent/nodegroupconfiguration-blacklist-loop-devices.yaml b/templates/agent/nodegroupconfiguration-blacklist-loop-devices.yaml index 77f785c29..6861c5f7a 100644 --- a/templates/agent/nodegroupconfiguration-blacklist-loop-devices.yaml +++ b/templates/agent/nodegroupconfiguration-blacklist-loop-devices.yaml @@ -22,9 +22,11 @@ spec: # See the License for the specific language governing permissions and # limitations under the License. - # Loop devices should not be queried by the LVM and multipath commands. - # So we add loop devices into blacklist for multipath and configure - # global_filter in lvm.conf for them + # Loop devices should not be queried by multipath — they are never + # multipath targets. Host-wide LVM also ignores loop devices via + # global_filter in lvm.conf; the agent re-attaches managed file-backed + # loop devices at startup and passes its own --config global_filter + # for every LVM command on managed Volume Groups. bb-event-on 'bb-sync-file-changed' '_on_multipath_config_changed' _on_multipath_config_changed() { @@ -60,4 +62,4 @@ spec: } configure_lvm - configure_multipath \ No newline at end of file + configure_multipath