Skip to content

STOR-2868: Implement device relinking #598

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-bot merged 17 commits into from
Apr 10, 2026
Merged

STOR-2868: Implement device relinking #598

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
a272ceb
Start creating LocalVolumeDeviceLink objects
gnufied Mar 11, 2026
d12b18d
Relax validations for preferredSymLink and validLinks
gnufied Mar 13, 2026
5a77a03
Handle reconcilation of rejected devices
gnufied Mar 13, 2026
480677a
Add tests for localvolume and localvolumeset objects
gnufied Mar 13, 2026
70b6b91
Refactor diskutil to get uncached device-ids
gnufied Mar 18, 2026
8f67520
Use utilexec from k8s.io for command execution
gnufied Mar 20, 2026
724dff8
Recreate symlink
gnufied Mar 23, 2026
ded965d
Implement metrics emission for mismatching symlinks
gnufied Mar 20, 2026
83337f7
Address review comments
gnufied Mar 26, 2026
7414aa1
Improve test coverage for reconciler and pv creation
gnufied Mar 26, 2026
284e31d
Handle the case of missing symlinks in /dev/disk/by-id
gnufied Apr 1, 2026
36927b1
Fix code in pvLinkCache
gnufied Apr 2, 2026
eb9c565
Remove unnecessary tests
gnufied Apr 2, 2026
b2982f7
Add symlink fallback test for persistent volumes
gnufied Apr 2, 2026
c47fa7f
Remove some of unused code
gnufied Apr 2, 2026
4f8d3c3
Add a new function for finding stale symlinks
gnufied Apr 8, 2026
e2b8b2b
Implement cache write through for pvLinkCache
gnufied Apr 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions CLAUDE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,15 @@ Test framework in `test/framework/` provides utilities for:
- Resource creation and cleanup
- Volume management operations

## Code Formatting

After editing any Go file, always run formatting and import fixes before finishing:
```bash
gopls format -w <file1> <file2> ...
# gopls imports only accepts one file at a time
gopls imports -w <file>
```

## Common Patterns

- Controllers use controller-runtime framework with reconciliation loops
Expand Down
21 changes: 21 additions & 0 deletions HACKING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,27 @@

2. Create images as documented below

## Overriding diskmaker image or operator image directly after deployment

> ⚠️ This is for temporary development/testing only. Deleting the Subscription disables OLM-driven upgrades/reconciliation.
> Recreate the Subscription after validation to return to managed state.

1. Record current Subscription/CSV:
```bash
oc get subscription -n openshift-local-storage
oc get csv -n openshift-local-storage | grep local-storage-operator
```
2. Delete the Subscription (temporary):
```bash
oc delete subscription <subscription-name> -n openshift-local-storage
```
3. Edit the installed CSV and override image(s) in `spec.install.spec.deployments[].spec.template.spec.containers[]`
(e.g., `DISKMAKER_IMAGE` and/or operator container image):
```bash
oc edit csv <csv-name> -n openshift-local-storage
```
4. After testing, recreate/apply the Subscription manifest to re-enable OLM management.

Comment thread
gnufied marked this conversation as resolved.
## Running the operator locally

1. Install LSO via OLM/OperatorHub in GUI
Expand Down
2 changes: 2 additions & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ ifeq ($(VERSION),)
VERSION = latest
endif

.DEFAULT_GOAL := build

Comment on lines +13 to +14
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 8, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major

Default goal now points to a path that can fail on clean checkouts

With Line 13 setting default goal to build, plain make immediately runs targets that output to $(TARGET_DIR) without ensuring that directory exists. On environments where _output/bin is missing, this can fail right away. Please make build resilient by creating $(TARGET_DIR) before go build.

Proposed fix 
 build-diskmaker:
+	mkdir -p $(TARGET_DIR)
 	env GOOS=$(TARGET_GOOS) GOARCH=$(TARGET_GOARCH) go build -mod=vendor -ldflags '-X main.version=$(REV)' -o $(TARGET_DIR)/diskmaker $(CURPATH)/cmd/diskmaker-manager
 .PHONY: build-diskmaker

 build-operator:
+	mkdir -p $(TARGET_DIR)
 	env GOOS=$(TARGET_GOOS) GOARCH=$(TARGET_GOARCH) go build -mod=vendor -ldflags '-X main.version=$(REV)' -o $(TARGET_DIR)/local-storage-operator $(CURPATH)/cmd/local-storage-operator
 .PHONY: build-operator

As per coding guidelines, "-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@Makefile` around lines 13 - 14, The build target fails on clean checkouts
because it assumes $(TARGET_DIR) exists; update the Makefile's build target (the
"build" target that invokes "go build") to ensure $(TARGET_DIR) is created first
(e.g., run mkdir -p $(TARGET_DIR) or an equivalent) before running go build so
outputs can be written safely; reference the TARGET_DIR variable and the build
target rule when making the change.

Copy link
Copy Markdown
Member Author

@gnufied gnufied Apr 10, 2026

Choose a reason for hiding this comment

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

go build -o**will** create the output file's parent directories automatically — this is actually a feature ofgo build(since Go 1.13+). From thego builddocs: the-o` flag creates any necessary parent directories.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 10, 2026

Choose a reason for hiding this comment

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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check Go version in go.mod and verify go build mkdir behavior from release notes or help
head -5 go.mod

# Check if there's any mention of go build creating directories
go help build 2>/dev/null | grep -i "director\|mkdir\|parent\|-o" || echo "go not available"

# Check Go release notes for this behavior
curl -s "https://go.dev/doc/go1.13" | grep -i "build.*director\|mkdir\|parent" | head -10 || true

Length of output: 1536

🏁 Script executed:

#!/bin/bash
# Check Go's own source/changelog for mkdir behavior with go build -o
curl -s "https://go.dev/doc/go1.16" | grep -i "build\|mkdir\|director" | grep -i "creat\|parent\|mkdir" | head -20 || true

# Also try to find any Go changelog entry specifically about this
curl -s "https://go.dev/doc/go1.13" 2>/dev/null | grep -i "build" | head -20 || true

# Check if there is a Go issue about this
curl -s "https://github.com/golang/go/search?q=go+build+-o+create+directory&type=issues" 2>/dev/null | grep -i "title\|result" | head -10 || true

Length of output: 1787

🏁 Script executed:

#!/bin/bash
# Check Go source code to see if it creates parent directories when using -o flag
curl -s "https://raw.githubusercontent.com/golang/go/master/src/cmd/go/internal/work/build.go" 2>/dev/null | grep -i "mkdir\|MkdirAll" | head -20 || true

# Check the exec.go or action.go in the go build toolchain
curl -s "https://raw.githubusercontent.com/golang/go/master/src/cmd/go/internal/work/exec.go" 2>/dev/null | grep -n -i "mkdir\|MkdirAll\|output.*dir\|OutFile" | head -30 || true

Length of output: 1750

@gnufied, you're correct — I was wrong. Looking at Go's own source in cmd/go/internal/work/exec.go (line 1848): sh.Mkdir(filepath.Dir(a.Target)) is called before writing the output, which confirms the toolchain creates the parent directory of the -o target automatically. No mkdir -p is needed in the Makefile. I'll retract this suggestion.

📝 Learnings were identified and not saved (knowledge base disabled). Enable

# Use podman or docker to build containers. Can bet set explicitly.
# make bundle REGISTRY=quay.io/username TOOL_BIN=`which docker`
ifeq ($(TOOL_BIN),)
Expand Down
9 changes: 6 additions & 3 deletions api/v1/localvolumedevicelink_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,13 +79,12 @@ type LocalVolumeDeviceLinkStatus struct {
// preferredLinkTarget is the /dev/disk/by-id symlink for the device that the local storage
// operator evaluated as the most stable and the least error prone. The local storage operator
// recommends using this symlink, after a careful review by the cluster admin.
// +required
// +kubebuilder:validation:MinLength=1
// +optional
// +kubebuilder:validation:MaxLength=4096
PreferredLinkTarget string `json:"preferredLinkTarget,omitempty"`
// validLinkTargets is the list of /dev/disk/by-id symlinks for the device that the local
// storage operator considers as valid. The list may contain at most 256 entries.
// +required
// +optional
// +listType=set
// +kubebuilder:validation:MaxItems=256
ValidLinkTargets []string `json:"validLinkTargets,omitempty"`
Expand Down Expand Up @@ -113,6 +112,10 @@ type LocalVolumeDeviceLinkList struct {
Items []LocalVolumeDeviceLink `json:"items"`
}

func init() {
SchemeBuilder.Register(&LocalVolumeDeviceLink{}, &LocalVolumeDeviceLinkList{})
}

// DeviceLinkPolicy defines how symlinks for given volumes should be treated
// by the LSO. Valid values are - None, CurrentLinkTarget and PreferredLinkTarget
// +kubebuilder:validation:Enum=None;CurrentLinkTarget;PreferredLinkTarget
Expand Down
33 changes: 33 additions & 0 deletions assets/templates/localmetrics/prometheus-rule.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,3 +37,36 @@ spec:
annotations:
summary: "LocalVolumeSet has had a deletion timestamp older than 72 hours"
description: "LocalVolumeSet {{ $labels.lvSetName }} has been marked for deletion for more than 72 hours."
- name: lso_no_stable_volume_path
rules:
- alert: LSONoStableLocalVolumePath
expr: min_over_time(lso_device_link_without_stable_path{policy="None"}[5m]) == 1
Copy link
Copy Markdown
Contributor

@jsafrane jsafrane Apr 8, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure if it's a bug or not: min_over_time does not count "missing metric" as "zero". I.e, when I get the alert and set the policy to "CurrentLinkTarget", it takes 5 minutes for the alert to disappear, because all the values the min_over_time evaluates are either "1" (slowly fading away) or the metric does not report any value at all. There is no "0" value ever.

So, should it be just expr: lso_device_link_without_stable_path{policy="None"} == 1 with for: 10m?

Copy link
Copy Markdown
Member Author

@gnufied gnufied Apr 8, 2026

Choose a reason for hiding this comment

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

I am not sure. Fix it as a follow up I guess?

for: 10m
labels:
severity: warning
annotations:
summary: |
Local Storage operator has observed that pv {{ $labels.persistent_volume }} does not
have a stable device-path in /dev/disk/by-id/ that it can use and hence it may be using
device paths which may change on reboot. Such device paths should be used carefully.
message: "Local volume represented by PersistentVolume {{ $labels.persistent_volume }} does not have any valid stable path"
description: |
Local Storage operator has observed that pv {{ $labels.persistent_volume }} does not
have a stable device-path in /dev/disk/by-id/ that it can use and hence it may be using
device paths which may change on reboot. Such device paths should be used carefully.
- name: lso_device_link_mismatch
rules:
- alert: LSODeviceLinkMismatch
expr: min_over_time(lso_device_link_mismatch{policy="None"}[5m]) == 1
for: 10m
labels:
severity: warning
annotations:
summary: |
Local Storage operator has observed that pv {{ $labels.persistent_volume }} does not
have matching symlink it should be using vs symlink LSO prefers it to use for stability between reboots and OS upgrade.
message: "Local volume represented by PersistentVolume {{ $labels.persistent_volume }} uses a sub-optimal device symlink"
description: |
Local Storage operator has observed that pv {{ $labels.persistent_volume }} is using a symlink
that is not optimum for stability between reboots and OS upgrades. Consider updating
LocalVolumeDeviceLink policy to preferredSymlink to allow LSO to manage the symlink.
19 changes: 18 additions & 1 deletion cmd/diskmaker-manager/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,31 +100,48 @@ func startManager(cmd *cobra.Command, args []string) error {
return err
}

// Create the shared LVDL cache and register it as a manager.Runnable.
// The manager will call Start(ctx) after its own caches are started,
// and the cache waits for LVDL informer sync before processing events.
pvLinkCache := common.NewLocalVolumeDeviceLinkCache(mgr.GetClient(), mgr)
if err := mgr.Add(pvLinkCache); err != nil {
klog.ErrorS(err, "failed to add pvLinkCache runnable")
return err
}

if err = diskmakerControllerLv.NewLocalVolumeReconciler(
mgr.GetClient(),
mgr.GetAPIReader(),
mgr.GetScheme(),
common.GetLocalDiskLocationPath(),
&provDeleter.CleanupStatusTracker{ProcTable: provDeleter.NewProcTable()},
getRuntimeConfig(diskmakerControllerLv.ComponentName, mgr),
pvLinkCache,
).WithManager(mgr); err != nil {
klog.ErrorS(err, "unable to create LocalVolume diskmaker controller")
return err
}

if err = diskmakerControllerLvSet.NewLocalVolumeSetReconciler(
mgr.GetClient(),
mgr.GetAPIReader(),
mgr.GetScheme(),
&diskmakerControllerLvSet.WallTime{},
&provDeleter.CleanupStatusTracker{ProcTable: provDeleter.NewProcTable()},
getRuntimeConfig(diskmakerControllerLvSet.ComponentName, mgr),
pvLinkCache,
).WithManager(mgr); err != nil {
klog.ErrorS(err, "unable to create LocalVolumeSet diskmaker controller")
return err
}

// Create the LVDL custom collector. mgr.GetClient() is already backed by
// the controller-runtime informer cache and does not hit the API server.
deviceLinkCollector := localmetrics.NewDeviceLinkCollector(mgr.GetClient(), namespace)

// start local server to emit custom metrics
err = localmetrics.NewConfigBuilder().
WithCollectors(localmetrics.LVMetricsList).
WithCollectors(append(localmetrics.LVMetricsList, deviceLinkCollector)).
Build()
if err != nil {
return errors.Wrap(err, "failed to configure local metrics")
Expand Down
4 changes: 4 additions & 0 deletions config/manifests/stable/local-storage-operator.clusterserviceversion.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,8 @@ spec:
- localvolumediscoveries/status
- localvolumediscoveryresults
- localvolumediscoveryresults/status
- localvolumedevicelinks
- localvolumedevicelinks/status
verbs:
- get
- list
Expand Down Expand Up @@ -313,6 +315,8 @@ spec:
- localvolumediscoveries/status
- localvolumediscoveryresults
- localvolumediscoveryresults/status
- localvolumedevicelinks
- localvolumedevicelinks/status
verbs:
- get
- list
Expand Down
3 changes: 0 additions & 3 deletions config/manifests/stable/local.storage.openshift.io_localvolumedevicelinks.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,6 @@ spec:
operator evaluated as the most stable and the least error prone. The local storage operator
recommends using this symlink, after a careful review by the cluster admin.
maxLength: 4096
minLength: 1
type: string
validLinkTargets:
description: |-
Expand All @@ -144,8 +143,6 @@ spec:
x-kubernetes-list-type: set
required:
- currentLinkTarget
- preferredLinkTarget
- validLinkTargets
type: object
required:
- spec
Expand Down
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ require (
github.com/modern-go/reflect2 v1.0.3-0.20250322232337-35a7c28c31ee // indirect
github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect
github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 // indirect
github.com/prometheus/client_model v0.6.2 // indirect
github.com/prometheus/client_model v0.6.2
github.com/prometheus/common v0.67.5 // indirect
github.com/prometheus/procfs v0.20.1 // indirect
github.com/spf13/pflag v1.0.10 // indirect
Expand Down
73 changes: 71 additions & 2 deletions hack/test-e2e.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,77 @@ export TEST_WATCH_NAMESPACE=${TEST_OPERATOR_NAMESPACE:-openshift-local-storage}
export TEST_OPERATOR_NAMESPACE=${TEST_OPERATOR_NAMESPACE:-openshift-local-storage}
export TEST_LOCAL_DISK=${TEST_LOCAL_DISK:-""}

usage() {
cat <<'EOF'
Usage: hack/test-e2e.sh [--suite <name>] [go test args...]

Run all e2e suites (default), or select one suite:
--suite LocalVolumeDiscovery
--suite LocalVolumeSet
--suite LocalVolume

Examples:
hack/test-e2e.sh --suite LocalVolumeSet
hack/test-e2e.sh --suite localvolumeset -count=1
hack/test-e2e.sh -run 'TestLocalStorage/LocalVolumeSet$'
EOF
}

suite=""
extra_args=()

while [[ $# -gt 0 ]]; do
case "$1" in
--suite)
if [[ -z "${2:-}" ]]; then
echo "error: --suite requires a value" >&2
usage
exit 2
fi
suite="$2"
shift 2
;;
--suite=*)
suite="${1#*=}"
shift
;;
Comment thread
gnufied marked this conversation as resolved.
-h|--help)
usage
exit 0
;;
*)
extra_args+=("$1")
shift
;;
esac
done

suite_run_regex=""
if [[ -n "$suite" ]]; then
suite_lower=$(echo "$suite" | tr '[:upper:]' '[:lower:]')
case "$suite_lower" in
localvolumediscovery)
suite_name="LocalVolumeDiscovery"
;;
localvolumeset)
suite_name="LocalVolumeSet"
;;
localvolume)
suite_name="LocalVolume"
;;
*)
echo "error: unsupported suite '$suite'" >&2
usage
exit 2
;;
esac
suite_run_regex="TestLocalStorage/${suite_name}$"
fi

go test -timeout 0 ./test/e2e/... \
-root=$(pwd) \
-kubeconfig=${KUBECONFIG} \
-root="$(pwd)" \
-kubeconfig="${KUBECONFIG}" \
-v \
-parallel=1 \
${suite_run_regex:+-run "$suite_run_regex"} \
"${extra_args[@]}"
Loading