Skip to content

Conversation

@eslutsky
Copy link
Contributor

@eslutsky eslutsky commented Dec 4, 2025

Resolves #150
Depends on #153

This tests can be executed with:

make env "CMD=bash ./src/topolvm/tests/createWorkloads.sh"

Summary by CodeRabbit

Release Notes

  • New Features

    • Enhanced TopoLVM storage configuration management with improved device class settings.
  • Tests

    • Added automated verification of TopoLVM functionality to the CI/CD pipeline, including workload provisioning and storage I/O validation to ensure reliable storage operations.

✏️ Tip: You can customize this high-level summary in your review settings.

@eslutsky eslutsky requested a review from a team as a code owner December 4, 2025 12:23
@coderabbitai
Copy link

coderabbitai bot commented Dec 4, 2025

Walkthrough

This PR adds end-to-end testing infrastructure for TopoLVM. It introduces a new test script that validates TopoLVM functionality by creating a test workload in Kubernetes, then integrates the test into the CI/CD pipeline alongside a new configuration patch for lvmd settings.

Changes

Cohort / File(s) Summary
TopoLVM Configuration Patch
src/topolvm/assets/topolvm_configmap-lvmd_patch.yaml, src/topolvm/assets/kustomization.yaml
New ConfigMap patch for lvmd with socket configuration and device classes; patch entry added to kustomization.yaml
E2E Test Script
src/topolvm/tests/createWorkloads.sh
New Bash test script that creates test namespace, PVC, and Deployment, then validates data persistence via read/write operations inside a pod
CI/CD Integration
.github/actions/build/action.yaml
Added test step to GitHub Actions workflow that executes the new TopoLVM test script
Manifest Generation
src/topolvm/generate_manifests.sh
Integrated new lvmd ConfigMap patch into kustomize workflow

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Minor attention areas:
    • Test script timeout and error handling logic (pod readiness polling, phase validation)
    • ConfigMap schema correctness for lvmd configuration (socket-name, device-classes structure)
    • CI integration: verify CMD parameter path and test artifact expectations

Suggested reviewers

  • ggiguash

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title 'Add topolvm e2e test' directly matches the main objective of adding end-to-end tests for TopoLVM functionality.
Linked Issues check ✅ Passed The PR implements e2e tests for TopoLVM configuration by adding test scripts, manifest patches, and CI integration, fulfilling issue #150's requirement to test previously untested TopoLVM functionality.
Out of Scope Changes check ✅ Passed All changes are directly scoped to adding TopoLVM e2e tests: new test scripts, ConfigMap patch manifests, kustomization updates, and CI integration—no extraneous modifications present.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e0bb2db and 3ae6fab.

📒 Files selected for processing (5)
  • .github/actions/build/action.yaml (1 hunks)
  • src/topolvm/assets/kustomization.yaml (1 hunks)
  • src/topolvm/assets/topolvm_configmap-lvmd_patch.yaml (1 hunks)
  • src/topolvm/generate_manifests.sh (2 hunks)
  • src/topolvm/tests/createWorkloads.sh (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • src/topolvm/assets/kustomization.yaml
  • src/topolvm/tests/createWorkloads.sh
  • .github/actions/build/action.yaml
🔇 Additional comments (3)
src/topolvm/generate_manifests.sh (2)

95-104: Config patch generation looks good.

The lvmd ConfigMap patch correctly uses escaped newlines within the YAML data field, which will render as actual newlines when parsed. This follows the same pattern as the existing service and webhook patches.


114-118: Patch properly integrated into kustomization.

The lvmd patch is correctly added to the kustomization patches list. This resolves the earlier review feedback about integrating the new patch.

src/topolvm/assets/topolvm_configmap-lvmd_patch.yaml (1)

1-8: ConfigMap structure is sound.

The manifest correctly defines the lvmd configuration patch with proper YAML formatting and namespace specification. The spare-gb: 0 setting is appropriate for test workloads.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (3)
src/topolvm/tests/createWorkloads.sh (1)

62-66: Simplify the pod-name check syntax.

The parameter expansion ${podName=} is non-idiomatic; use "${podName}" for clarity since the variable is already initialized.

-while [[ "${podName=}" == "" && ${iter} -gt 0 ]]; do
+while [[ "${podName}" == "" && ${iter} -gt 0 ]]; do
src/cluster_manager.sh (2)

389-390: Add defensive check for /etc/hosts.

Line 389 assumes /etc/hosts exists. While standard on most systems, verify it exists before copying to avoid failures in minimal/container environments.

+if [ ! -f /etc/hosts ]; then
+    echo "ERROR: /etc/hosts not found" >&2
+    exit 1
+fi
 cp /etc/hosts "${workdir}/hosts"

419-419: Remove redundant cleanup.

Line 419 is unreachable; trap at line 378 already removes ${workdir} on exit.

        fi
    fi
-    rm -rf "${workdir}"
 }
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 05a80b9 and cf92b88.

📒 Files selected for processing (5)
  • Makefile (5 hunks)
  • src/cluster_manager.sh (5 hunks)
  • src/topolvm/assets/kustomization.yaml (1 hunks)
  • src/topolvm/assets/topolvm_configmap-lvmd_patch.yaml (1 hunks)
  • src/topolvm/tests/createWorkloads.sh (1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-29T07:57:10.203Z
Learnt from: pacevedom
Repo: microshift-io/microshift PR: 103
File: src/cluster_manager.sh:327-327
Timestamp: 2025-10-29T07:57:10.203Z
Learning: In src/cluster_manager.sh, the file `/var/lib/microshift/resources/kubeadmin/kubeconfig` (without a node name) is guaranteed to exist if MicroShift has started and is ready, as it is used internally. This generic kubeconfig path is appropriate for cluster-wide operations like querying cluster status, while node-specific paths like `/var/lib/microshift/resources/kubeadmin/${node_name}/kubeconfig` are used for node-specific operations like joining nodes.

Applied to files:

  • src/topolvm/tests/createWorkloads.sh
  • src/cluster_manager.sh
📚 Learning: 2025-10-29T07:41:49.737Z
Learnt from: pacevedom
Repo: microshift-io/microshift PR: 103
File: src/cluster_manager.sh:112-128
Timestamp: 2025-10-29T07:41:49.737Z
Learning: In src/cluster_manager.sh, the file `/var/lib/microshift/resources/kubeadmin/$node_name/kubeconfig` is guaranteed to exist as part of the MicroShift startup process. If MicroShift is ready, this kubeconfig file will be present, so validation checks before copying are not required in the `_join_node` function.

Applied to files:

  • src/topolvm/tests/createWorkloads.sh
  • Makefile
  • src/cluster_manager.sh
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: centos10-bootc
  • GitHub Check: quick-rpm-and-clean (ubuntu-24.04-arm)
  • GitHub Check: quick-start-and-clean (ubuntu-24.04)
  • GitHub Check: quick-start-and-clean (ubuntu-24.04-arm)
🔇 Additional comments (3)
Makefile (1)

156-159: LGTM.

The env target correctly delegates to cluster_env() with sensible defaults: empty pod_name triggers default to NODE_BASE_NAME}1, and empty CMD triggers interactive shell. Documentation is clear.

src/topolvm/assets/topolvm_configmap-lvmd_patch.yaml (1)

1-8: LGTM.

ConfigMap properly aligns with cluster configuration (volume-group: myvg1 matches VG_NAME). YAML formatting and Kustomize integration are correct.

src/topolvm/assets/kustomization.yaml (1)

9-9: LGTM.

Patch path is correctly formatted and appended. No ordering conflicts with existing patches.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
.github/actions/build/action.yaml (1)

120-121: Remove unnecessary blank lines for consistency.

Lines 120–121 introduce extra whitespace that may be unintentional. Consider removing excess blank lines to maintain consistent spacing with the rest of the action file.

         make env "CMD=bash ./src/topolvm/tests/createWorkloads.sh"
-
-
         for i in $(seq 2 ${{ inputs.node-count }}); do
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cf92b88 and 562bf88.

📒 Files selected for processing (4)
  • .github/actions/build/action.yaml (2 hunks)
  • src/topolvm/assets/kustomization.yaml (1 hunks)
  • src/topolvm/assets/topolvm_configmap-lvmd_patch.yaml (1 hunks)
  • src/topolvm/tests/createWorkloads.sh (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/topolvm/assets/topolvm_configmap-lvmd_patch.yaml
  • src/topolvm/tests/createWorkloads.sh
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
  • GitHub Check: quick-rpm-and-clean (ubuntu-24.04)
  • GitHub Check: quick-rpm-and-clean (ubuntu-24.04-arm)
  • GitHub Check: quick-start-and-clean (ubuntu-24.04)
  • GitHub Check: quick-start-and-clean (ubuntu-24.04-arm)
  • GitHub Check: isolated-network (ovnk, ubuntu-24.04, 1)
  • GitHub Check: fedora-bootc (ubuntu-24.04-arm)
  • GitHub Check: fedora-bootc (ubuntu-24.04)
  • GitHub Check: ubuntu-rpm2deb
  • GitHub Check: isolated-network (kindnet, ubuntu-24.04, 0)
  • GitHub Check: centos10-bootc
  • GitHub Check: centos9-bootc
🔇 Additional comments (2)
src/topolvm/assets/kustomization.yaml (1)

9-9: Verify the patch file exists at src/topolvm/assets/topolvm_configmap-lvmd_patch.yaml and contains valid YAML.

The patch reference syntax is correct and follows the existing pattern in the kustomization file, but confirm the referenced patch file exists with valid Kustomize patch content.

.github/actions/build/action.yaml (1)

107-108: Verify that the test script and make env target exist.

The TopoLVM test is properly positioned after basic MicroShift validation but before node scaling. However, confirm that:

  1. The script src/topolvm/tests/createWorkloads.sh exists and is executable
  2. The make env target supports the CMD parameter as expected

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
.github/actions/build/action.yaml (1)

120-121: Remove extra blank lines.

Lines 120–121 contain only whitespace and should be removed to maintain clean formatting.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1e37e13 and 20ccb40.

📒 Files selected for processing (2)
  • .github/actions/build/action.yaml (3 hunks)
  • .github/actions/prebuild/action.yaml (1 hunks)
🔇 Additional comments (3)
.github/actions/prebuild/action.yaml (1)

36-36: Verify awk prerequisite is actually used.

The addition of awk aligns with the PR's new test and env functionality, but confirm it's actively used in the scripts or Makefile targets added in this PR (especially in the new cluster_env implementation or test scripts).

.github/actions/build/action.yaml (2)

90-90: Verify EXPOSE_KUBEAPI_PORT is properly handled in Makefile and cluster_manager.sh.

The variable is set but ensure the Makefile run target actually consumes it and correctly propagates it downstream, as indicated in the PR objectives.


107-108: Verify the make env target and test script exist and work as expected.

This step depends on:

  1. A make env target in the Makefile that accepts a CMD parameter
  2. The script at ./src/topolvm/tests/createWorkloads.sh exists and is executable
  3. The script will appropriately fail if TopoLVM issues occur

Confirm these prerequisites are in place and functional.

@eslutsky eslutsky force-pushed the add_topolvm_test branch 2 times, most recently from a6e6303 to aeb5770 Compare December 5, 2025 18:41
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (2)
src/cluster_manager.sh (2)

381-392: Simplify pod name / command disambiguation.

Lines 381–387 implicitly treat the first argument as a pod name if a container with that name exists; otherwise, it's treated as part of the command. This works but is non-obvious. Consider an explicit check or clearer logic:

-    if [ -z "${pod_name}" ]; then
-        pod_name="${NODE_BASE_NAME}1"
-    elif ! _is_container_created "${pod_name}" 2>/dev/null; then
-        # First argument is not a container, treat it as part of the command
-        command="${pod_name} ${command}"
-        pod_name="${NODE_BASE_NAME}1"
-    fi
+    # If no pod_name provided or it's not a container, use default and treat first arg as command
+    if [ -z "${pod_name}" ] || ! _is_container_created "${pod_name}" 2>/dev/null; then
+        if [ -z "${pod_name}" ]; then
+            pod_name="${NODE_BASE_NAME}1"
+        else
+            command="${pod_name} ${command}"
+            pod_name="${NODE_BASE_NAME}1"
+        fi
+    fi

Or document this behavior with an inline comment to aid future readers.


409-419: Dead code path after exec bash.

Line 418 (rm -rf "${workdir}") is unreachable when the interactive shell is invoked (line 416: exec bash -i). The trap at line 397 already handles cleanup, so this redundancy can be removed. For the command-execution path (line 412), the trap still fires on exit.

     if [ -n "${command}" ]; then
         # Execute the command and exit
         echo "Executing command in environment with kubeconfig..."
         sh -c "${command}"
     else
         # Start interactive shell
         echo "Starting shell environment with kubeconfig..."
         exec bash -i
     fi
-    rm -rf "${workdir}"
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a6e6303 and aeb5770.

📒 Files selected for processing (6)
  • .github/actions/build/action.yaml (3 hunks)
  • .github/actions/prebuild/action.yaml (1 hunks)
  • src/cluster_manager.sh (5 hunks)
  • src/topolvm/assets/kustomization.yaml (1 hunks)
  • src/topolvm/assets/topolvm_configmap-lvmd_patch.yaml (1 hunks)
  • src/topolvm/tests/createWorkloads.sh (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • .github/actions/build/action.yaml
  • src/topolvm/assets/topolvm_configmap-lvmd_patch.yaml
  • src/topolvm/tests/createWorkloads.sh
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-23T11:55:38.018Z
Learnt from: pacevedom
Repo: microshift-io/microshift PR: 103
File: src/image/postinstall.sh:52-54
Timestamp: 2025-10-23T11:55:38.018Z
Learning: In MicroShift multinode deployments at src/image/postinstall.sh, etcd ports 2379 and 2380 are exposed in the public firewall zone because nodes may be deployed across different networks and need to communicate with each other for cluster formation and operation.

Applied to files:

  • src/cluster_manager.sh
📚 Learning: 2025-10-29T07:57:10.203Z
Learnt from: pacevedom
Repo: microshift-io/microshift PR: 103
File: src/cluster_manager.sh:327-327
Timestamp: 2025-10-29T07:57:10.203Z
Learning: In src/cluster_manager.sh, the file `/var/lib/microshift/resources/kubeadmin/kubeconfig` (without a node name) is guaranteed to exist if MicroShift has started and is ready, as it is used internally. This generic kubeconfig path is appropriate for cluster-wide operations like querying cluster status, while node-specific paths like `/var/lib/microshift/resources/kubeadmin/${node_name}/kubeconfig` are used for node-specific operations like joining nodes.

Applied to files:

  • src/cluster_manager.sh
📚 Learning: 2025-10-29T07:41:49.737Z
Learnt from: pacevedom
Repo: microshift-io/microshift PR: 103
File: src/cluster_manager.sh:112-128
Timestamp: 2025-10-29T07:41:49.737Z
Learning: In src/cluster_manager.sh, the file `/var/lib/microshift/resources/kubeadmin/$node_name/kubeconfig` is guaranteed to exist as part of the MicroShift startup process. If MicroShift is ready, this kubeconfig file will be present, so validation checks before copying are not required in the `_join_node` function.

Applied to files:

  • src/cluster_manager.sh
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
  • GitHub Check: isolated-network (kindnet, ubuntu-24.04, 0)
  • GitHub Check: fedora-bootc (ubuntu-24.04-arm)
  • GitHub Check: fedora-bootc (ubuntu-24.04)
  • GitHub Check: centos9-bootc
  • GitHub Check: quick-start-and-clean (ubuntu-24.04)
  • GitHub Check: quick-rpm-and-clean (ubuntu-24.04)
  • GitHub Check: quick-start-and-clean (ubuntu-24.04-arm)
  • GitHub Check: quick-rpm-and-clean (ubuntu-24.04-arm)
  • GitHub Check: centos10-bootc
🔇 Additional comments (7)
src/topolvm/assets/kustomization.yaml (1)

9-9: Verify the patch file exists and is properly formatted.

The patch entry addition looks syntactically correct and maintains consistency with existing entries. However, ensure that topolvm_configmap-lvmd_patch.yaml is present in the same directory and contains a valid Kubernetes manifest.

.github/actions/prebuild/action.yaml (1)

36-36: Dependency addition looks good.

The gawk package aligns with the _get_ip_address() function in cluster_manager.sh (line 81) that uses awk for subnet calculations.

src/cluster_manager.sh (5)

135-142: _get_hostname() implementation looks solid.

Error handling and early exit on failure are appropriate. The function follows the established pattern in the script.


122-130: Config injection flow is clear.

The conditional logic around EXPOSE_KUBEAPI_PORT, file creation, copy, restart, and cleanup is well-structured. The use of _get_hostname ensures consistency when subjectAltNames are injected.


106-109: Port mapping logic is correct.

The conditional injection of -p 6443:6443 based on EXPOSE_KUBEAPI_PORT follows the established pattern for optional flags (e.g., network_opts, vol_opts).


455-468: CLI routing and usage update are consistent.

The env command is properly added to the main dispatch and reflected in the usage message. The pass-through of arguments via "$@" is correct.


16-16: EXPOSE_KUBEAPI_PORT configuration is well-placed.

New variable follows the existing pattern (default value, environment override). No concerns.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

♻️ Duplicate comments (1)
src/cluster_manager.sh (1)

373-406: Fix kubeconfig path: use node name instead of hostname. Line 393 attempts to copy kubeconfig using the host's FQDN from _get_hostname(), but kubeconfigs inside the container are organized by node name (e.g., microshift-okd-1), not the host's FQDN. This will cause the podman cp command to fail. Additionally, add validation that the copy succeeds before attempting to use the kubeconfig.

Apply this diff to use the correct kubeconfig path:

 cluster_env() {
     local command="${1:-}"
    
     # Set first_container from the first value of containers array
     local -r first_container=$(_get_running_containers | head -n1)
     if [ -z "${first_container}" ]; then
         echo "ERROR: No running cluster containers found." >&2
         exit 1
     fi
     # Verify that ${API_SERVER_PORT} is open for connections on the host
     if ! sudo ss -ltn "( sport = :${API_SERVER_PORT} )" | grep -q ":${API_SERVER_PORT}"; then
         echo "ERROR: API server port ${API_SERVER_PORT} is closed, make sure EXPOSE_KUBEAPI_PORT is set to 1." >&2
         exit 1
     fi

     local -r workdir=$(mktemp -d /tmp/kubeconfig-XXXXXX)
     # shellcheck disable=SC2064
     trap "rm -rf '${workdir}'" RETURN

     echo "Copying kubeconfig from ${first_container}..."
-    sudo podman cp "${first_container}:/var/lib/microshift/resources/kubeadmin/$(_get_hostname)/kubeconfig" "${workdir}/kubeconfig"
+    sudo podman cp "${first_container}:/var/lib/microshift/resources/kubeadmin/kubeconfig" "${workdir}/kubeconfig"
     sudo chown "$(whoami):$(whoami)" "${workdir}/kubeconfig"
     export KUBECONFIG="${workdir}/kubeconfig"
     
     if [ -n "${command}" ]; then
         # Execute the command and exit
         echo "Executing command in environment with kubeconfig..."
         sh -c "${command}"
     else
         # Start interactive shell
         echo "Starting shell environment with kubeconfig..."
         bash -li 
     fi
 }

This uses the generic kubeconfig path that is guaranteed to exist when MicroShift is running, as documented in learnings about cluster_manager.sh.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between aeb5770 and e0bb2db.

📒 Files selected for processing (7)
  • .github/actions/build/action.yaml (2 hunks)
  • Makefile (5 hunks)
  • src/cluster_manager.sh (6 hunks)
  • src/topolvm/assets/kustomization.yaml (1 hunks)
  • src/topolvm/assets/topolvm_configmap-lvmd_patch.yaml (1 hunks)
  • src/topolvm/generate_manifests.sh (1 hunks)
  • src/topolvm/tests/createWorkloads.sh (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/topolvm/assets/topolvm_configmap-lvmd_patch.yaml
  • src/topolvm/tests/createWorkloads.sh
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: pacevedom
Repo: microshift-io/microshift PR: 103
File: src/image/postinstall.sh:52-54
Timestamp: 2025-10-23T11:55:38.018Z
Learning: In MicroShift multinode deployments at src/image/postinstall.sh, etcd ports 2379 and 2380 are exposed in the public firewall zone because nodes may be deployed across different networks and need to communicate with each other for cluster formation and operation.
📚 Learning: 2025-10-29T07:57:10.203Z
Learnt from: pacevedom
Repo: microshift-io/microshift PR: 103
File: src/cluster_manager.sh:327-327
Timestamp: 2025-10-29T07:57:10.203Z
Learning: In src/cluster_manager.sh, the file `/var/lib/microshift/resources/kubeadmin/kubeconfig` (without a node name) is guaranteed to exist if MicroShift has started and is ready, as it is used internally. This generic kubeconfig path is appropriate for cluster-wide operations like querying cluster status, while node-specific paths like `/var/lib/microshift/resources/kubeadmin/${node_name}/kubeconfig` are used for node-specific operations like joining nodes.

Applied to files:

  • src/cluster_manager.sh
📚 Learning: 2025-10-29T07:41:49.737Z
Learnt from: pacevedom
Repo: microshift-io/microshift PR: 103
File: src/cluster_manager.sh:112-128
Timestamp: 2025-10-29T07:41:49.737Z
Learning: In src/cluster_manager.sh, the file `/var/lib/microshift/resources/kubeadmin/$node_name/kubeconfig` is guaranteed to exist as part of the MicroShift startup process. If MicroShift is ready, this kubeconfig file will be present, so validation checks before copying are not required in the `_join_node` function.

Applied to files:

  • src/cluster_manager.sh
  • Makefile
📚 Learning: 2025-10-29T07:44:21.893Z
Learnt from: pacevedom
Repo: microshift-io/microshift PR: 103
File: src/cluster_manager.sh:77-81
Timestamp: 2025-10-29T07:44:21.893Z
Learning: MicroShift is not designed to support clusters with 245+ nodes running as containers on the same system. IP address allocation in cluster_manager.sh does not need overflow validation for this reason.

Applied to files:

  • src/cluster_manager.sh
📚 Learning: 2025-10-17T10:31:57.408Z
Learnt from: ggiguash
Repo: microshift-io/microshift PR: 57
File: src/debian/install.sh:12-0
Timestamp: 2025-10-17T10:31:57.408Z
Learning: In the MicroShift project, the Ubuntu version in src/debian/install.sh is intentionally hardcoded to "xUbuntu_20.04" because CRI-O builds are not available for all the latest Ubuntu versions. This should not be changed to dynamic detection.

Applied to files:

  • src/cluster_manager.sh
📚 Learning: 2025-10-17T10:53:08.461Z
Learnt from: ggiguash
Repo: microshift-io/microshift PR: 57
File: .github/workflows/builders.yaml:75-84
Timestamp: 2025-10-17T10:53:08.461Z
Learning: In the microshift repository, the RPM-to-Debian conversion workflow follows this pattern: `convert.sh` receives an RPM directory as input and outputs .deb files to a `deb/` subdirectory within that directory. The `install.sh` script expects to receive the parent RPM directory (not the deb subdirectory) and uses recursive `find` commands to locate the .deb files in subdirectories.

Applied to files:

  • Makefile
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
  • GitHub Check: quick-rpm-and-clean (ubuntu-24.04-arm)
  • GitHub Check: quick-rpm-and-clean (ubuntu-24.04)
  • GitHub Check: quick-start-and-clean (ubuntu-24.04)
  • GitHub Check: quick-start-and-clean (ubuntu-24.04-arm)
  • GitHub Check: fedora-bootc (ubuntu-24.04-arm)
  • GitHub Check: ubuntu-rpm2deb
  • GitHub Check: fedora-bootc (ubuntu-24.04)
  • GitHub Check: isolated-network (ovnk, ubuntu-24.04, 1)
  • GitHub Check: centos10-bootc
  • GitHub Check: centos9-bootc
  • GitHub Check: isolated-network (kindnet, ubuntu-24.04, 0)
🔇 Additional comments (4)
src/topolvm/generate_manifests.sh (1)

95-104: Verify the lvmd.yaml ConfigMap data format.

The ConfigMap patch uses escaped newlines (\n) in a heredoc with single quotes (line 96), which means they'll be literal \n strings rather than actual newlines. Confirm this is the intended format for lvmd configuration consumption.

Check how the lvmd service consumes this ConfigMap data and whether it expects literal \n or actual newlines in the YAML structure.

Makefile (1)

180-182: LGTM. The env target correctly depends on run-ready to ensure the cluster is ready before exposing the environment.

src/cluster_manager.sh (1)

86-93: Good error handling for hostname resolution. The _get_hostname() function validates the FQDN and exits gracefully if unavailable.

.github/actions/build/action.yaml (1)

107-108: The TopoLVM test script at src/topolvm/tests/createWorkloads.sh exists and is properly referenced in the workflow.

Comment on lines 117 to 123
local port_opts=""
local mount_opts=""
if [ "${EXPOSE_KUBEAPI_PORT}" = "1" ]; then
port_opts="-p ${API_SERVER_PORT}:${API_SERVER_PORT}"
echo -e "apiServer:\n subjectAltNames:\n - $(_get_hostname)" | sudo tee "${EXTRA_CONFIG}" >/dev/null
mount_opts="--volume ${EXTRA_CONFIG}:/etc/microshift/config.d/api_server.yaml:ro"
fi
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Add validation for API config file creation. When EXPOSE_KUBEAPI_PORT=1, the code writes to ${EXTRA_CONFIG} without validating the directory exists or the write succeeds.

Apply this diff to ensure the directory exists and handle failures:

     local port_opts=""
     local mount_opts=""
     if [ "${EXPOSE_KUBEAPI_PORT}" = "1" ]; then
         port_opts="-p ${API_SERVER_PORT}:${API_SERVER_PORT}"
+        sudo mkdir -p "$(dirname "${EXTRA_CONFIG}")"
+        if ! echo -e "apiServer:\n  subjectAltNames:\n    - $(_get_hostname)" | sudo tee "${EXTRA_CONFIG}" >/dev/null; then
+            echo "ERROR: Failed to write API server configuration to ${EXTRA_CONFIG}" >&2
+            exit 1
+        fi
-        echo -e "apiServer:\n  subjectAltNames:\n    - $(_get_hostname)" | sudo tee "${EXTRA_CONFIG}" >/dev/null
         mount_opts="--volume ${EXTRA_CONFIG}:/etc/microshift/config.d/api_server.yaml:ro"
     fi
🤖 Prompt for AI Agents
In src/cluster_manager.sh around lines 117 to 123, when EXPOSE_KUBEAPI_PORT=1
the script writes an apiServer YAML to ${EXTRA_CONFIG} without ensuring the
parent directory exists or that the write succeeded; update the code to create
the parent directory (mkdir -p) for ${EXTRA_CONFIG} before writing, check the
tee/write command exit status and on failure emit an error message to stderr (or
the script logger) and exit non‑zero, and only set mount_opts after verifying
the write was successful.

Signed-off-by: Evgeny Slutsky <eslutsky@redhat.com>
Signed-off-by: Evgeny Slutsky <eslutsky@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add TopoLVM e2e tests

2 participants