Make TopoLVM VG_NAME and SPARE_GB configurable #197
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add environment variable support for configuring TopoLVM: - VG_NAME: LVM volume group name (default: myvg1) - SPARE_GB: Spare GB to reserve in volume group (default: 10) This allows users to customize TopoLVM configuration without modifying source code, useful for testing environments where 10GB spare is too large. Usage: make run VG_NAME=testvg SPARE_GB=2 LVM_VOLSIZE=5G VG_NAME=customvg SPARE_GB=5 sudo -E ./src/quickstart.sh Closes microshift-io#24 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds VG_NAME and SPARE_GB configuration across packaging, orchestration, Makefile and Ansible defaults; replaces hard-coded VG references with variables; adds a runtime TopoLVM patch script and systemd ExecStartPre drop-in to generate an lvmd ConfigMap; propagates SPARE_GB into container and cleanup flows. Changes
Sequence Diagram(s)sequenceDiagram
participant Host as Host OS
participant Systemd as systemd
participant Patch as patch_lvmd_config.sh
participant MicroShift as MicroShift Service/Container
participant Manifests as /etc/microshift/manifests.d
Host->>Host: Export VG_NAME, SPARE_GB
Host->>Systemd: systemd start microshift.service
Systemd->>Patch: ExecStartPre runs patch script
Patch->>Patch: Read /proc/1/environ -> VG_NAME, SPARE_GB
Patch->>Patch: Validate SPARE_GB
Patch->>Manifests: Write topolvm-lvmd-configmap.yaml + kustomization (if non-default)
Patch-->>Systemd: Exit success
Systemd->>MicroShift: Start microshift container/service
MicroShift->>Manifests: MicroShift loads generated manifests
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@ansible/roles/microshift-okd-bootc/vars/main.yaml`:
- Around line 15-17: The variables lvm_vg_name and lvm_spare_gb are
self-referential and cause Ansible recursion; replace their template expressions
with plain literal defaults (e.g., set lvm_vg_name to "myvg1" and lvm_spare_gb
to 10) or move those literal defaults into defaults/main.yaml so they become
lowest-precedence defaults and can still be overridden by inventory or
--extra-vars; update any references to lvm_vg_name and lvm_spare_gb accordingly
and remove the "{{ ... | default(...) }}" constructs that reference the same
names.
In `@Makefile`:
- Line 27: VG_NAME is currently defined with := in the "Internal variables"
section so it can't be overridden from the environment; move the VG_NAME
definition into the "Options used in the 'run' target" section alongside
SPARE_GB and change its assignment to use ?= (e.g., VG_NAME ?= ...) so that
environment overrides like VG_NAME=testvg make run work; ensure you remove the
old := definition so there are no duplicate VG_NAME declarations and leave
SPARE_GB as-is.
In `@src/topolvm/patch_lvmd_config.sh`:
- Around line 23-26: The early-exit when VG_NAME=myvg1 and SPARE_GB=10 leaves
stale files in PATCH_DIR; modify the conditional branch in patch_lvmd_config.sh
so that before exiting it cleans PATCH_DIR (e.g., remove any existing patch
files or empty the directory) and handles the case where PATCH_DIR may not exist
or be empty; update the block that checks VG_NAME and SPARE_GB to perform the
cleanup of "${PATCH_DIR}" (or specific patch filenames) and then exit 0.
🧹 Nitpick comments (1)
src/topolvm/patch_lvmd_config.sh (1)
43-49: Consider validatingSPARE_GBis numeric.A non-numeric
SPARE_GBvalue would produce an invalidlvmd.yaml, which could cause TopoLVM to fail at runtime with a confusing error.Proposed validation (after line 19)
SPARE_GB="${SPARE_GB:-10}" + +if ! [[ "${SPARE_GB}" =~ ^[0-9]+$ ]]; then + echo "ERROR: SPARE_GB must be a positive integer, got '${SPARE_GB}'" >&2 + exit 1 +fi
- Fix Ansible self-referential variable recursion by moving lvm_vg_name and lvm_spare_gb defaults to defaults/main.yaml - Move VG_NAME from internal variables to run options with ?= so environment overrides work correctly - Clean up stale patch files when using default values Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/topolvm/patch_lvmd_config.sh`:
- Around line 18-19: The script currently sets SPARE_GB but doesn't validate it;
add a guard after the SPARE_GB assignment that checks SPARE_GB matches a
non-negative integer (e.g. regex ^[0-9]+$ or numeric parse) and that it is >= 0,
and if not print a clear error referencing SPARE_GB (and exit with non-zero
status) so we fail fast before generating lvmd.yaml; keep the existing default
assignment (VG_NAME and SPARE_GB) but ensure the validation runs immediately
after the SPARE_GB variable is set.
🧹 Nitpick comments (2)
src/topolvm/patch_lvmd_config.sh (1)
33-52: YAML values should be quoted to be safe.While LVM VG names are typically alphanumeric, quoting
${SPARE_GB}and${VG_NAME}in the generated YAML guards against edge cases and makes the intent explicit.Proposed fix
- spare-gb: ${SPARE_GB} - volume-group: ${VG_NAME} + spare-gb: "${SPARE_GB}" + volume-group: "${VG_NAME}"Makefile (1)
137-137: The env-var preamble is getting unwieldy.Each target repeats 7+ variables inline. Consider defining a shared variable to reduce duplication and improve readability.
Example
+# Common environment for cluster_manager.sh invocations +CLUSTER_ENV = USHIFT_IMAGE=${USHIFT_IMAGE} ISOLATED_NETWORK=${ISOLATED_NETWORK} \ + LVM_DISK=${LVM_DISK} LVM_VOLSIZE=${LVM_VOLSIZE} \ + VG_NAME=${VG_NAME} SPARE_GB=${SPARE_GB} + .PHONY: run run: - `@USHIFT_IMAGE`=${USHIFT_IMAGE} ISOLATED_NETWORK=${ISOLATED_NETWORK} LVM_DISK=${LVM_DISK} LVM_VOLSIZE=${LVM_VOLSIZE} VG_NAME=${VG_NAME} SPARE_GB=${SPARE_GB} EXPOSE_KUBEAPI_PORT=${EXPOSE_KUBEAPI_PORT} ./src/cluster_manager.sh create + @${CLUSTER_ENV} EXPOSE_KUBEAPI_PORT=${EXPOSE_KUBEAPI_PORT} ./src/cluster_manager.sh createThen reuse
${CLUSTER_ENV}inadd-node,start,stop,run-ready,run-healthy,run-status,env, andclean.
Validate that SPARE_GB is a non-negative integer before generating the lvmd.yaml ConfigMap to fail fast with a clear error message. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The patch_lvmd_config.sh doesn't exist on main branch until the PR is merged. Update quickrpm.sh to skip download if script already exists, and update CI to pre-install the script from the PR branch. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/actions/quick-rpm-clean/action.yaml:
- Around line 69-71: The run invoking quickrpm.sh forwards only TAG so VG_NAME
and SPARE_GB from the workflow environment are not available inside the
container; update the podman exec invocation that runs quickrpm.sh (the sudo
podman exec -i microshift-okd ... bash -xeuo pipefail < ./src/quickrpm.sh line)
to also pass VG_NAME and SPARE_GB into the container environment (e.g., add env
VG_NAME=... and env SPARE_GB=... tied to the workflow/workflow inputs) so
quickrpm.sh and the later verify step both see the same VG_NAME/SPARE_GB values.
🧹 Nitpick comments (3)
src/quickrpm.sh (2)
10-11: SPARE_GB is not validated before use.If a user passes
SPARE_GB="banana", the systemd drop-in is written and the error only surfaces at service start time (whenpatch_lvmd_config.shvalidates). Consider failing fast here for a better user experience.Suggested validation
VG_NAME="${VG_NAME:-myvg1}" SPARE_GB="${SPARE_GB:-10}" + +if ! [[ "${SPARE_GB}" =~ ^[0-9]+$ ]]; then + echo "ERROR: SPARE_GB must be a non-negative integer, got '${SPARE_GB}'" + exit 1 +fi
121-125: Stale script on re-run with a different BRANCH.The
-xguard skips re-download, so switchingBRANCHon a subsequent run won't update the patch script. Probably fine for a one-shot installer, but worth a comment if intentional..github/actions/quick-rpm-clean/action.yaml (1)
83-85:VG_NAMEis read from the environment but not declared as an action input.Other configurable values (
bootc-image-url,install-tag, etc.) are declared ininputs:. Consider addingvg-name(andspare-gb) as optional inputs with defaults so callers can use the standardwith:syntax and the interface is self-documenting.
Forward VG_NAME and SPARE_GB environment variables to the container so quickrpm.sh and verify steps see consistent values. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add environment variable support for configuring TopoLVM:
This allows users to customize TopoLVM configuration without modifying source code, useful for testing environments where 10GB spare is too large.
Usage:
make run VG_NAME=testvg SPARE_GB=2 LVM_VOLSIZE=5G
VG_NAME=customvg SPARE_GB=5 sudo -E ./src/quickstart.sh
Closes #24
Summary by CodeRabbit
New Features
Documentation
Bug Fixes / Cleanup