PoC: replace LINSTOR with Blockstor (external-mode LinstorCluster + blockstor controller/apiserver/satellite) - #2942
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a proof-of-concept replacement of the Java-based LINSTOR controller with Blockstor, a from-scratch reimplementation. The changes involve deploying Blockstor's controller, apiserver, and satellite components while maintaining compatibility with the existing linstor-csi and storage classes. This architecture shift utilizes external mode for the LinstorCluster and includes updated e2e bootstrap scripts to accommodate the new storage backend. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces in-tree LINSTOR stack with external Blockstor in EXTERNAL mode. Adds eight ChangesLINSTOR to Blockstor backend migration
Sequence Diagram(s)sequenceDiagram
participant piraeusOp as piraeus-operator
participant blockstorAPI as blockstor-apiserver<br>(mTLS :3371)
participant blockstorCtrl as blockstor-controller
participant blockstorSat as blockstor-satellite<br>(DaemonSet)
participant k8s as Kubernetes API
rect rgba(70, 130, 180, 0.5)
note over piraeusOp,blockstorAPI: EXTERNAL mode wiring
piraeusOp->>blockstorAPI: LinstorCluster externalController HTTPS mTLS
blockstorAPI->>k8s: CRUD blockstor.cozystack.io CRs
end
rect rgba(100, 160, 100, 0.5)
note over blockstorCtrl,blockstorSat: Reconciliation plane
blockstorCtrl->>k8s: Watch/reconcile blockstor CRs
blockstorSat->>k8s: Update Node/StoragePool status
blockstorSat->>blockstorSat: Manage DRBD via /dev /lib/modules /run mounts
end
rect rgba(200, 130, 60, 0.5)
note over piraeusOp,blockstorSat: e2e setup flow
piraeusOp->>blockstorSat: kubectl exec satellite pod
blockstorSat->>blockstorSat: zpool create data on /dev/vdc
piraeusOp->>k8s: kubectl apply StoragePool CR (ZFS_THIN)
piraeusOp->>k8s: kubectl apply Node CR (InternalIP netInterface)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
/hold PoC — do not merge. Opened to run the e2e suite against the Blockstor storage backend. |
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Nitpick comments (3)
hack/e2e-post-install-prep.sh (1)
40-41: ⚡ Quick winSatellite readiness gate is hardcoded to 3 pods.
At Line 41,
-eq 3makes this step topology-specific and can fail in clusters with a different worker count, even though the rest of the script discovers nodes/pods dynamically.Suggested fix
+EXPECTED_SATELLITES=$(kubectl get nodes -l '!node-role.kubernetes.io/control-plane' --no-headers 2>/dev/null | wc -l | tr -d ' ') echo "[post-install-prep] waiting for 3 blockstor-satellite pods Ready" -timeout 300 sh -ec 'until [ $(kubectl -n '"$NS"' get pods -l app=blockstor-satellite --no-headers 2>/dev/null | awk "{print \$2}" | grep -c "^1/1$") -eq 3 ]; do sleep 5; done' +echo "[post-install-prep] waiting for ${EXPECTED_SATELLITES} blockstor-satellite pods Ready" +timeout 300 sh -ec 'until [ $(kubectl -n '"$NS"' get pods -l app=blockstor-satellite --no-headers 2>/dev/null | awk "{print \$2}" | grep -c "^1/1$") -eq '"$EXPECTED_SATELLITES"' ]; do sleep 5; done'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hack/e2e-post-install-prep.sh` around lines 40 - 41, The blockstor-satellite readiness check at line 41 has a hardcoded pod count of 3 in the condition `-eq 3`, which makes it topology-specific and will fail in clusters with different worker counts. Replace this hardcoded value with a dynamically calculated variable that determines the expected number of blockstor-satellite pods based on the actual cluster topology (similar to how other parts of the script discover resources dynamically). Store the count of blockstor-satellite pods or worker nodes in a variable before the timeout command, then use that variable instead of the hardcoded 3 in the until condition check.packages/system/linstor/templates/blockstor-controller.yaml (2)
84-89: ⚡ Quick winMake the controller root filesystem read-only.
The controller already runs non-root and has no declared writable filesystem paths; set
readOnlyRootFilesystem: trueand add an explicitemptyDironly if the binary needs/tmp.🔒 Proposed hardening
securityContext: runAsNonRoot: true runAsUser: 65532 allowPrivilegeEscalation: false + readOnlyRootFilesystem: true capabilities: {drop: [ALL]} seccompProfile: {type: RuntimeDefault}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/system/linstor/templates/blockstor-controller.yaml` around lines 84 - 89, Add `readOnlyRootFilesystem: true` to the securityContext block in the blockstor-controller.yaml template to enforce a read-only root filesystem. Additionally, if the controller binary requires a writable `/tmp` directory, add an explicit `emptyDir` volume mount to the pod specification to provide temporary storage without compromising the read-only root constraint.Source: Linters/SAST tools
1-93: 🏗️ Heavy liftKeep Blockstor as a vendored subchart before merging.
This adds a new system component directly under
templates/; for the mergeable version, package Blockstor underpackages/system/linstor/charts/and keep this chart as the umbrella/wiring layer. As per coding guidelines,packages/system/**: “Follow Helm Charts umbrella pattern with vendored upstream charts incharts/subdirectory for system components charts inpackages/system/”.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/system/linstor/templates/blockstor-controller.yaml` around lines 1 - 93, The blockstor-controller resources including the ServiceAccount, ClusterRole, ClusterRoleBinding, and Deployment are currently defined as direct templates but should be reorganized as a vendored subchart following the umbrella pattern. Move these resource definitions into a separate Helm chart located in the charts subdirectory (creating a new chart structure with its own Chart.yaml, values.yaml, and templates directory for the blockstor controller), then update the parent chart to reference this vendored subchart as a dependency so the umbrella chart acts as the wiring layer while the actual resource definitions live in the vendored subchart.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@hack/e2e-post-install-prep.sh`:
- Around line 37-38: The kubectl get linstorcluster command is missing the
namespace flag that should be applied consistently throughout the script. Locate
the kubectl command that retrieves the linstorcluster availability status and
add the `-n "$NS"` namespace parameter to ensure it queries the correct
namespace (cozy-linstor) instead of defaulting to the current context's
namespace, which prevents timeout failures due to checking the wrong namespace.
In
`@packages/system/linstor/definitions/blockstor.cozystack.io_controllerconfigs.yaml`:
- Around line 20-23: The ControllerConfig CRD description states that only
instances with name=default are recognized, but the schema currently accepts any
metadata.name value, allowing typos to pass validation silently and be ignored
by the controller. Add validation rules to the ControllerConfig CRD schema to
enforce that only metadata.name=default is permitted, rejecting any other names
at admission time. This validation should be applied to both the main
ControllerConfig definition and the location also mentioned at lines 331-333.
- Around line 1-7: The Blockstor CRDs including the
blockstor.cozystack.io_controllerconfigs.yaml CustomResourceDefinition are
currently embedded directly in the linstor chart definitions directory, which
violates the umbrella chart pattern. Move all Blockstor CRD files from
packages/system/linstor/definitions to a vendored subchart structure under a
charts/ subdirectory (such as packages/system/linstor/charts/blockstor or
similar), following the Helm Charts umbrella pattern for system components.
Ensure the linstor chart properly references and includes these CRDs as a
vendored dependency subchart rather than owning them directly, and update any
Chart.yaml dependencies accordingly.
In `@packages/system/linstor/definitions/blockstor.cozystack.io_nodes.yaml`:
- Around line 42-83: The drbdMinorRange and drbdPortRange structures in the
schema lack validation rules to enforce sensible ranges. Add cross-field
validation constraints to both structures to ensure that the max value is
greater than or equal to the min value. Additionally, add validation to
drbdPortRange to enforce that the max value does not exceed 65535, which is the
maximum valid TCP port number. This prevents invalid Node CRs from passing
admission validation and failing later during DRBD allocation. Use appropriate
OpenAPI validation rules (such as x-kubernetes-validations) to implement these
constraints.
In
`@packages/system/linstor/definitions/blockstor.cozystack.io_resourcedefinitions.yaml`:
- Around line 216-232: Add a validation rule to the x-kubernetes-validations
section of the drbdPort field to ensure the port value, when set, falls within
valid TCP port range (1-65535). Since drbdPort is optional, the validation
should allow nil values but reject any integer that is negative or exceeds
65535. Add this as a second validation rule after the existing set-once
constraint rule to prevent invalid allocator seeds from being persisted.
- Around line 348-405: The volumeDefinitions array in the
ResourceDefinitionVolume schema needs validation to ensure data integrity. Add
validation rules to the volumeDefinitions array to enforce that volumeNumber
values are unique across the array (making it keyed by volumeNumber), that
volumeNumber values are positive (greater than 0), and that sizeKib values are
positive (greater than 0). Use Kubernetes CEL validation rules or list map keys
to implement these constraints, preventing ambiguous restore and adoption
scenarios by rejecting duplicate/negative volume numbers and non-positive sizes
at admission time.
In
`@packages/system/linstor/definitions/blockstor.cozystack.io_resourcegroups.yaml`:
- Around line 284-305: The volumeGroups array currently allows duplicate
volumeNumber entries, which can lead to ambiguous props and flags for the same
volume. Add uniqueness validation by including x-kubernetes-list-type set to
"map" and x-kubernetes-list-map-keys set to the volumeNumber field in the
volumeGroups array definition. This will ensure that each volumeNumber appears
only once within the volumeGroups array, preventing duplicate entries and
eliminating ambiguity when spawned resources inherit properties and flags.
In `@packages/system/linstor/definitions/blockstor.cozystack.io_resources.yaml`:
- Around line 58-75: Add range validation constraints to both drbdNodeID and
drbdPort fields in the x-kubernetes-validations section to enforce that
drbdNodeID accepts only values between 0 and 15 (as documented) and drbdPort
accepts only valid TCP port numbers. These validations should reject invalid
values at admission time since these are authoritative set-once values. Add
these constraints to the drbdNodeID field around line 73 and to the drbdPort
field around line 249-274, using additional validation rules that check if self
is within the required ranges.
- Around line 753-758: The current validation rule for the Resource identity
uses `oldSelf.hasValue() || ...` which skips validation on updates, allowing
spec.resourceDefinitionName or spec.nodeName to be changed while metadata.name
remains unchanged. To fix this, remove the `oldSelf.hasValue() || ` condition
from the rule in the x-kubernetes-validations block so that the metadata.name
validation against the spec.resourceDefinitionName and spec.nodeName combination
is enforced on both create and update operations, preventing inconsistencies
where the immutable metadata.name no longer matches the current spec values.
In `@packages/system/linstor/definitions/blockstor.cozystack.io_snapshots.yaml`:
- Around line 306-311: The current validation rule only ensures metadata.name
matches the concatenation of spec.resourceDefinitionName and spec.snapshotName
during creation, but does not prevent these spec fields from being modified
after creation. Add x-kubernetes-validations rules for both
spec.resourceDefinitionName and spec.snapshotName fields to make them immutable.
Each rule should check if this is an update operation (oldSelf.hasValue()
returns true) and if so, verify that the field value has not changed from its
original value. This will ensure the identity fields remain consistent with the
immutable metadata.name throughout the snapshot's lifecycle.
- Around line 263-301: The nodeStatus field currently lacks map semantics,
causing Kubernetes status patches to treat the entire array atomically and
potentially replace sibling node entries instead of updating individual nodes.
Add the x-kubernetes-list-map-keys extension to the nodeStatus array definition
to enable per-node status updates by specifying nodeName as the map key, since
satellites create entries keyed by node name. This will ensure that individual
node status updates do not affect sibling entries.
In
`@packages/system/linstor/definitions/blockstor.cozystack.io_storagepools.yaml`:
- Around line 202-206: The validation rule for metadata.name in the
x-kubernetes-validations section currently uses `oldSelf.hasValue() ||` which
causes the validation to only enforce the naming convention on Create operations
and allows it to be bypassed on Updates. This permits someone to patch
spec.poolName or spec.nodeName to values that don't match the immutable
metadata.name. Remove the `oldSelf.hasValue() ||` portion from the rule
condition so that the metadata.name validation against spec.poolName and
spec.nodeName is enforced consistently on both Create and Update operations.
In `@packages/system/linstor/templates/blockstor-apiserver-tls.yaml`:
- Around line 71-75: The dnsNames field in the certificate resource hardcodes
the namespace as cozy-linstor instead of using the templated namespace value,
which breaks TLS certificate verification when deployed to a different
namespace. Replace all occurrences of the hardcoded cozy-linstor namespace in
the dnsNames entries (specifically in blockstor-apiserver.cozy-linstor,
blockstor-apiserver.cozy-linstor.svc, and
blockstor-apiserver.cozy-linstor.svc.cluster.local) with {{ .Release.Namespace
}}. Additionally, apply the same fix to the externalController.url field in
cluster.yaml where cozy-linstor is similarly hardcoded to ensure consistent
templating across both files.
In `@packages/system/linstor/templates/blockstor-apiserver.yaml`:
- Around line 97-102: The securityContext in the blockstor-apiserver.yaml
template is missing the readOnlyRootFilesystem setting for improved defense in
depth. Add `readOnlyRootFilesystem: true` to the securityContext block alongside
the existing runAsNonRoot, runAsUser, allowPrivilegeEscalation, capabilities,
and seccompProfile settings. This will restrict the container from writing to
the root filesystem, which is safe since the apiserver only needs to read TLS
files from mounted secrets.
In `@packages/system/linstor/templates/blockstor-controller.yaml`:
- Around line 27-32: The RBAC rules in the blockstor-controller.yaml and
blockstor-satellite.yaml templates use wildcard resources which grant access to
both current and future Blockstor resources. Replace the wildcard `["*"]` in the
resources field (line 28 in controller template) with an explicit list of all
eight Blockstor CRD names. Similarly, replace the wildcard `["*/status",
"*/finalizers"]` in the resources field (line 31 in controller template) with
explicit subresource paths for each CRD following the pattern "crdname/status"
and "crdname/finalizers". Apply the same changes to the corresponding lines in
blockstor-satellite.yaml (lines 64 and 67) to ensure consistent and restrictive
RBAC rules across both templates.
In `@packages/system/linstor/templates/blockstor-satellite.yaml`:
- Around line 54-56: The RBAC rule grants overly broad cluster-wide access to
all Secrets with get, list, and watch verbs. Replace the ClusterRole entry that
grants permissions on the secrets resource with a namespaced Role scoped to the
release namespace that uses the resourceNames field to restrict access to only
the specific Secret names required by the blockstor-satellite pods, and reduce
the verbs to only what is necessary (typically just get). Then create a
corresponding RoleBinding in the release namespace that binds this Role to the
blockstor-satellite service account.
In `@packages/system/linstor/templates/cluster.yaml`:
- Around line 18-19: The externalController URL in the cluster.yaml template has
a hard-coded namespace reference to cozy-linstor, which conflicts with the use
of templated Release.Namespace elsewhere in the Blockstor workloads. Replace the
hard-coded namespace string in the blockstor-apiserver service URL with the Helm
template variable for Release.Namespace to ensure the URL dynamically points to
the correct service regardless of deployment namespace. Additionally, update the
Subject Alternative Names (SANs) in the blockstor-apiserver-tls.yaml template
file to use the same templated namespace variable so the TLS certificate matches
the actual service FQDN.
In `@packages/system/linstor/templates/crds/crds.yaml`:
- Around line 1-4: The current template structure in
packages/system/linstor/templates/ does not follow the required umbrella
vendoring pattern for system packages. Restructure the linstor package to vendor
upstream charts under a charts/ subdirectory (packages/system/linstor/charts/)
and move the crds.yaml template and its related definitions to align with the
umbrella pattern. Update the file reference paths in the crds.yaml template to
correctly point to the definitions within the vendored chart structure rather
than using direct file globbing from the templates directory.
---
Nitpick comments:
In `@hack/e2e-post-install-prep.sh`:
- Around line 40-41: The blockstor-satellite readiness check at line 41 has a
hardcoded pod count of 3 in the condition `-eq 3`, which makes it
topology-specific and will fail in clusters with different worker counts.
Replace this hardcoded value with a dynamically calculated variable that
determines the expected number of blockstor-satellite pods based on the actual
cluster topology (similar to how other parts of the script discover resources
dynamically). Store the count of blockstor-satellite pods or worker nodes in a
variable before the timeout command, then use that variable instead of the
hardcoded 3 in the until condition check.
In `@packages/system/linstor/templates/blockstor-controller.yaml`:
- Around line 84-89: Add `readOnlyRootFilesystem: true` to the securityContext
block in the blockstor-controller.yaml template to enforce a read-only root
filesystem. Additionally, if the controller binary requires a writable `/tmp`
directory, add an explicit `emptyDir` volume mount to the pod specification to
provide temporary storage without compromising the read-only root constraint.
- Around line 1-93: The blockstor-controller resources including the
ServiceAccount, ClusterRole, ClusterRoleBinding, and Deployment are currently
defined as direct templates but should be reorganized as a vendored subchart
following the umbrella pattern. Move these resource definitions into a separate
Helm chart located in the charts subdirectory (creating a new chart structure
with its own Chart.yaml, values.yaml, and templates directory for the blockstor
controller), then update the parent chart to reference this vendored subchart as
a dependency so the umbrella chart acts as the wiring layer while the actual
resource definitions live in the vendored subchart.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 64b1003b-7ebd-42fe-930c-7c6aac2775ba
📒 Files selected for processing (28)
hack/e2e-post-install-prep.shpackages/system/linstor/Makefilepackages/system/linstor/definitions/blockstor.cozystack.io_controllerconfigs.yamlpackages/system/linstor/definitions/blockstor.cozystack.io_nodes.yamlpackages/system/linstor/definitions/blockstor.cozystack.io_physicaldevices.yamlpackages/system/linstor/definitions/blockstor.cozystack.io_resourcedefinitions.yamlpackages/system/linstor/definitions/blockstor.cozystack.io_resourcegroups.yamlpackages/system/linstor/definitions/blockstor.cozystack.io_resources.yamlpackages/system/linstor/definitions/blockstor.cozystack.io_snapshots.yamlpackages/system/linstor/definitions/blockstor.cozystack.io_storagepools.yamlpackages/system/linstor/hack/plunger/plunger-controller.shpackages/system/linstor/hack/plunger/plunger-drbd-logger.shpackages/system/linstor/hack/plunger/plunger-satellite.shpackages/system/linstor/templates/blockstor-apiserver-tls.yamlpackages/system/linstor/templates/blockstor-apiserver.yamlpackages/system/linstor/templates/blockstor-controller.yamlpackages/system/linstor/templates/blockstor-satellite.yamlpackages/system/linstor/templates/cluster.yamlpackages/system/linstor/templates/crds/crds.yamlpackages/system/linstor/templates/linstor-api-tls.yamlpackages/system/linstor/templates/linstor-internal-tls.yamlpackages/system/linstor/templates/plunger/configmap-scripts.yamlpackages/system/linstor/templates/podscrape.yamlpackages/system/linstor/templates/satellites-cozy.yamlpackages/system/linstor/templates/satellites-plunger.yamlpackages/system/linstor/templates/satellites-reloader.yamlpackages/system/linstor/templates/satellites-talos.yamlpackages/system/linstor/values.yaml
💤 Files with no reviewable changes (11)
- packages/system/linstor/hack/plunger/plunger-drbd-logger.sh
- packages/system/linstor/templates/linstor-internal-tls.yaml
- packages/system/linstor/templates/satellites-reloader.yaml
- packages/system/linstor/templates/plunger/configmap-scripts.yaml
- packages/system/linstor/templates/satellites-talos.yaml
- packages/system/linstor/templates/podscrape.yaml
- packages/system/linstor/hack/plunger/plunger-satellite.sh
- packages/system/linstor/templates/satellites-cozy.yaml
- packages/system/linstor/hack/plunger/plunger-controller.sh
- packages/system/linstor/templates/satellites-plunger.yaml
- packages/system/linstor/templates/linstor-api-tls.yaml
| echo "[post-install-prep] waiting for LinstorCluster Available (external mode)" | ||
| timeout 300 sh -ec 'until kubectl get linstorcluster linstorcluster -o jsonpath="{.status.conditions[?(@.type==\"Available\")].status}" 2>/dev/null | grep -q True; do sleep 5; done' |
There was a problem hiding this comment.
Namespace is missing in the LinstorCluster availability check.
At Line 38, kubectl get linstorcluster ... is not scoped with -n "$NS" while the rest of this script consistently targets cozy-linstor. This can make the readiness gate fail by timeout even when the resource is healthy.
Suggested fix
-timeout 300 sh -ec 'until kubectl get linstorcluster linstorcluster -o jsonpath="{.status.conditions[?(@.type==\"Available\")].status}" 2>/dev/null | grep -q True; do sleep 5; done'
+timeout 300 sh -ec 'until kubectl -n '"$NS"' get linstorcluster linstorcluster -o jsonpath="{.status.conditions[?(@.type==\"Available\")].status}" 2>/dev/null | grep -q True; do sleep 5; done'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| echo "[post-install-prep] waiting for LinstorCluster Available (external mode)" | |
| timeout 300 sh -ec 'until kubectl get linstorcluster linstorcluster -o jsonpath="{.status.conditions[?(@.type==\"Available\")].status}" 2>/dev/null | grep -q True; do sleep 5; done' | |
| echo "[post-install-prep] waiting for LinstorCluster Available (external mode)" | |
| timeout 300 sh -ec 'until kubectl -n '"$NS"' get linstorcluster linstorcluster -o jsonpath="{.status.conditions[?(@.type==\"Available\")].status}" 2>/dev/null | grep -q True; do sleep 5; done' |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@hack/e2e-post-install-prep.sh` around lines 37 - 38, The kubectl get
linstorcluster command is missing the namespace flag that should be applied
consistently throughout the script. Locate the kubectl command that retrieves
the linstorcluster availability status and add the `-n "$NS"` namespace
parameter to ensure it queries the correct namespace (cozy-linstor) instead of
defaulting to the current context's namespace, which prevents timeout failures
due to checking the wrong namespace.
| --- | ||
| apiVersion: apiextensions.k8s.io/v1 | ||
| kind: CustomResourceDefinition | ||
| metadata: | ||
| annotations: | ||
| controller-gen.kubebuilder.io/version: v0.20.1 | ||
| name: controllerconfigs.blockstor.cozystack.io |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Keep Blockstor CRDs behind an umbrella subchart.
These generated Blockstor CRDs are embedded directly under packages/system/linstor/definitions, which makes the linstor chart own the component API surface instead of vendoring it through charts/. Please move the Blockstor chart/CRDs under a vendored subchart path or document an approved exception before merging. As per coding guidelines, packages/system/**: Follow Helm Charts umbrella pattern with vendored upstream charts in charts/ subdirectory for system components charts in packages/system/.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/system/linstor/definitions/blockstor.cozystack.io_controllerconfigs.yaml`
around lines 1 - 7, The Blockstor CRDs including the
blockstor.cozystack.io_controllerconfigs.yaml CustomResourceDefinition are
currently embedded directly in the linstor chart definitions directory, which
violates the umbrella chart pattern. Move all Blockstor CRD files from
packages/system/linstor/definitions to a vendored subchart structure under a
charts/ subdirectory (such as packages/system/linstor/charts/blockstor or
similar), following the Helm Charts umbrella pattern for system components.
Ensure the linstor chart properly references and includes these CRDs as a
vendored dependency subchart rather than owning them directly, and update any
Chart.yaml dependencies accordingly.
Source: Coding guidelines
| description: |- | ||
| ControllerConfig is the cluster-wide singleton config for the | ||
| blockstor controller. Exactly one instance with name=`default` | ||
| is recognised; any other instance is ignored. Phase 10.4. |
There was a problem hiding this comment.
Reject non-default ControllerConfig names at admission.
Line 22 says only default is recognized, but the CRD accepts any metadata.name; a typoed singleton applies cleanly and is then silently ignored by the controller.
Proposed fix
required:
- spec
type: object
+ x-kubernetes-validations:
+ - message: ControllerConfig metadata.name must be default
+ rule: self.metadata.name == 'default'Also applies to: 331-333
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/system/linstor/definitions/blockstor.cozystack.io_controllerconfigs.yaml`
around lines 20 - 23, The ControllerConfig CRD description states that only
instances with name=default are recognized, but the schema currently accepts any
metadata.name value, allowing typos to pass validation silently and be ignored
by the controller. Add validation rules to the ControllerConfig CRD schema to
enforce that only metadata.name=default is permitted, rejecting any other names
at admission time. This validation should be applied to both the main
ControllerConfig definition and the location also mentioned at lines 331-333.
| drbdMinorRange: | ||
| description: |- | ||
| drbdMinorRange is the inclusive [min, max] /dev/drbd<N> minor | ||
| range the allocator picks from. Replaces | ||
| `Props["DrbdOptions/MinorNrRange"]`. nil inherits the | ||
| controller-wide default (20000–65535). Phase 10.3. | ||
| properties: | ||
| max: | ||
| description: max is the upper bound (inclusive). Must be ≥ Min. | ||
| format: int32 | ||
| minimum: 0 | ||
| type: integer | ||
| min: | ||
| description: min is the lower bound (inclusive). | ||
| format: int32 | ||
| minimum: 0 | ||
| type: integer | ||
| required: | ||
| - max | ||
| - min | ||
| type: object | ||
| drbdPortRange: | ||
| description: |- | ||
| drbdPortRange is the inclusive [min, max] TCP port range the | ||
| allocator picks DRBD listen ports from for replicas placed on | ||
| this node. Replaces `Props["DrbdOptions/TcpPortRange"]`. nil | ||
| inherits the controller-wide default (20000–20999). Phase 10.3. | ||
| properties: | ||
| max: | ||
| description: max is the upper bound (inclusive). Must be ≥ Min. | ||
| format: int32 | ||
| minimum: 0 | ||
| type: integer | ||
| min: | ||
| description: min is the lower bound (inclusive). | ||
| format: int32 | ||
| minimum: 0 | ||
| type: integer | ||
| required: | ||
| - max | ||
| - min | ||
| type: object |
There was a problem hiding this comment.
Enforce sane allocator ranges.
Both range structs require min/max but accept max < min; drbdPortRange also accepts invalid TCP ports. Bad Node CRs can pass admission and fail later during DRBD allocation.
Proposed fix
drbdMinorRange:
description: |-
drbdMinorRange is the inclusive [min, max] /dev/drbd<N> minor
@@
required:
- max
- min
type: object
+ x-kubernetes-validations:
+ - message: drbdMinorRange.max must be greater than or equal to drbdMinorRange.min
+ rule: self.max >= self.min
drbdPortRange:
@@
max:
description: max is the upper bound (inclusive). Must be ≥ Min.
format: int32
+ maximum: 65535
- minimum: 0
+ minimum: 1
type: integer
min:
description: min is the lower bound (inclusive).
format: int32
- minimum: 0
+ maximum: 65535
+ minimum: 1
type: integer
required:
- max
- min
type: object
+ x-kubernetes-validations:
+ - message: drbdPortRange.max must be greater than or equal to drbdPortRange.min
+ rule: self.max >= self.min📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| drbdMinorRange: | |
| description: |- | |
| drbdMinorRange is the inclusive [min, max] /dev/drbd<N> minor | |
| range the allocator picks from. Replaces | |
| `Props["DrbdOptions/MinorNrRange"]`. nil inherits the | |
| controller-wide default (20000–65535). Phase 10.3. | |
| properties: | |
| max: | |
| description: max is the upper bound (inclusive). Must be ≥ Min. | |
| format: int32 | |
| minimum: 0 | |
| type: integer | |
| min: | |
| description: min is the lower bound (inclusive). | |
| format: int32 | |
| minimum: 0 | |
| type: integer | |
| required: | |
| - max | |
| - min | |
| type: object | |
| drbdPortRange: | |
| description: |- | |
| drbdPortRange is the inclusive [min, max] TCP port range the | |
| allocator picks DRBD listen ports from for replicas placed on | |
| this node. Replaces `Props["DrbdOptions/TcpPortRange"]`. nil | |
| inherits the controller-wide default (20000–20999). Phase 10.3. | |
| properties: | |
| max: | |
| description: max is the upper bound (inclusive). Must be ≥ Min. | |
| format: int32 | |
| minimum: 0 | |
| type: integer | |
| min: | |
| description: min is the lower bound (inclusive). | |
| format: int32 | |
| minimum: 0 | |
| type: integer | |
| required: | |
| - max | |
| - min | |
| type: object | |
| drbdMinorRange: | |
| description: |- | |
| drbdMinorRange is the inclusive [min, max] /dev/drbd<N> minor | |
| range the allocator picks from. Replaces | |
| `Props["DrbdOptions/MinorNrRange"]`. nil inherits the | |
| controller-wide default (20000–65535). Phase 10.3. | |
| properties: | |
| max: | |
| description: max is the upper bound (inclusive). Must be ≥ Min. | |
| format: int32 | |
| minimum: 0 | |
| type: integer | |
| min: | |
| description: min is the lower bound (inclusive). | |
| format: int32 | |
| minimum: 0 | |
| type: integer | |
| required: | |
| - max | |
| - min | |
| type: object | |
| x-kubernetes-validations: | |
| - message: drbdMinorRange.max must be greater than or equal to drbdMinorRange.min | |
| rule: self.max >= self.min | |
| drbdPortRange: | |
| description: |- | |
| drbdPortRange is the inclusive [min, max] TCP port range the | |
| allocator picks DRBD listen ports from for replicas placed on | |
| this node. Replaces `Props["DrbdOptions/TcpPortRange"]`. nil | |
| inherits the controller-wide default (20000–20999). Phase 10.3. | |
| properties: | |
| max: | |
| description: max is the upper bound (inclusive). Must be ≥ Min. | |
| format: int32 | |
| maximum: 65535 | |
| minimum: 1 | |
| type: integer | |
| min: | |
| description: min is the lower bound (inclusive). | |
| format: int32 | |
| maximum: 65535 | |
| minimum: 1 | |
| type: integer | |
| required: | |
| - max | |
| - min | |
| type: object | |
| x-kubernetes-validations: | |
| - message: drbdPortRange.max must be greater than or equal to drbdPortRange.min | |
| rule: self.max >= self.min |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/system/linstor/definitions/blockstor.cozystack.io_nodes.yaml` around
lines 42 - 83, The drbdMinorRange and drbdPortRange structures in the schema
lack validation rules to enforce sensible ranges. Add cross-field validation
constraints to both structures to ensure that the max value is greater than or
equal to the min value. Additionally, add validation to drbdPortRange to enforce
that the max value does not exceed 65535, which is the maximum valid TCP port
number. This prevents invalid Node CRs from passing admission validation and
failing later during DRBD allocation. Use appropriate OpenAPI validation rules
(such as x-kubernetes-validations) to implement these constraints.
| drbdPort: | ||
| description: |- | ||
| drbdPort is an OPTIONAL preferred TCP port seed for this RD, | ||
| mirroring upstream LINSTOR's `RscDfn.port` preferred value. The | ||
| per-node port allocator (Resource.Spec.DRBDPort) tries this | ||
| value first on each hosting node and falls back to a per-node | ||
| free port on collision. nil means "no preference — allocate | ||
| per-node freely". This is a seed, NOT the authoritative | ||
| listen-port: the authoritative per-replica value lives on | ||
| Resource.Spec.DRBDPort. clusterIP-style settable-once so an | ||
| accidental edit can't perturb the per-node allocation. | ||
| format: int32 | ||
| type: integer | ||
| x-kubernetes-validations: | ||
| - message: drbdPort is settable-once | ||
| optionalOldSelf: true | ||
| rule: '!oldSelf.hasValue() || self == oldSelf.value()' |
There was a problem hiding this comment.
Clamp spec.drbdPort to valid TCP ports.
This set-once field currently accepts negative values and ports above 65535; once persisted, invalid allocator seeds are hard to repair because later edits are rejected.
Proposed fix
drbdPort:
@@
format: int32
+ maximum: 65535
+ minimum: 1
type: integer
x-kubernetes-validations:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| drbdPort: | |
| description: |- | |
| drbdPort is an OPTIONAL preferred TCP port seed for this RD, | |
| mirroring upstream LINSTOR's `RscDfn.port` preferred value. The | |
| per-node port allocator (Resource.Spec.DRBDPort) tries this | |
| value first on each hosting node and falls back to a per-node | |
| free port on collision. nil means "no preference — allocate | |
| per-node freely". This is a seed, NOT the authoritative | |
| listen-port: the authoritative per-replica value lives on | |
| Resource.Spec.DRBDPort. clusterIP-style settable-once so an | |
| accidental edit can't perturb the per-node allocation. | |
| format: int32 | |
| type: integer | |
| x-kubernetes-validations: | |
| - message: drbdPort is settable-once | |
| optionalOldSelf: true | |
| rule: '!oldSelf.hasValue() || self == oldSelf.value()' | |
| drbdPort: | |
| description: |- | |
| drbdPort is an OPTIONAL preferred TCP port seed for this RD, | |
| mirroring upstream LINSTOR's `RscDfn.port` preferred value. The | |
| per-node port allocator (Resource.Spec.DRBDPort) tries this | |
| value first on each hosting node and falls back to a per-node | |
| free port on collision. nil means "no preference — allocate | |
| per-node freely". This is a seed, NOT the authoritative | |
| listen-port: the authoritative per-replica value lives on | |
| Resource.Spec.DRBDPort. clusterIP-style settable-once so an | |
| accidental edit can't perturb the per-node allocation. | |
| format: int32 | |
| maximum: 65535 | |
| minimum: 1 | |
| type: integer | |
| x-kubernetes-validations: | |
| - message: drbdPort is settable-once | |
| optionalOldSelf: true | |
| rule: '!oldSelf.hasValue() || self == oldSelf.value()' |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/system/linstor/definitions/blockstor.cozystack.io_resourcedefinitions.yaml`
around lines 216 - 232, Add a validation rule to the x-kubernetes-validations
section of the drbdPort field to ensure the port value, when set, falls within
valid TCP port range (1-65535). Since drbdPort is optional, the validation
should allow nil values but reject any integer that is negative or exceeds
65535. Add this as a second validation rule after the existing set-once
constraint rule to prevent invalid allocator seeds from being persisted.
| securityContext: | ||
| runAsNonRoot: true | ||
| runAsUser: 65532 | ||
| allowPrivilegeEscalation: false | ||
| capabilities: {drop: [ALL]} | ||
| seccompProfile: {type: RuntimeDefault} |
There was a problem hiding this comment.
Add readOnlyRootFilesystem: true for defense in depth.
The container securityContext is well-configured but missing readOnlyRootFilesystem. Since the apiserver only needs to read TLS files from a mounted secret and doesn't appear to require write access to the root filesystem, this should be safe to enable.
🛡️ Proposed fix
securityContext:
runAsNonRoot: true
runAsUser: 65532
allowPrivilegeEscalation: false
capabilities: {drop: [ALL]}
seccompProfile: {type: RuntimeDefault}
+ readOnlyRootFilesystem: true📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| securityContext: | |
| runAsNonRoot: true | |
| runAsUser: 65532 | |
| allowPrivilegeEscalation: false | |
| capabilities: {drop: [ALL]} | |
| seccompProfile: {type: RuntimeDefault} | |
| securityContext: | |
| runAsNonRoot: true | |
| runAsUser: 65532 | |
| allowPrivilegeEscalation: false | |
| capabilities: {drop: [ALL]} | |
| seccompProfile: {type: RuntimeDefault} | |
| readOnlyRootFilesystem: true |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/system/linstor/templates/blockstor-apiserver.yaml` around lines 97 -
102, The securityContext in the blockstor-apiserver.yaml template is missing the
readOnlyRootFilesystem setting for improved defense in depth. Add
`readOnlyRootFilesystem: true` to the securityContext block alongside the
existing runAsNonRoot, runAsUser, allowPrivilegeEscalation, capabilities, and
seccompProfile settings. This will restrict the container from writing to the
root filesystem, which is safe since the apiserver only needs to read TLS files
from mounted secrets.
Source: Linters/SAST tools
| - apiGroups: [blockstor.cozystack.io] | ||
| resources: ["*"] | ||
| verbs: [get, list, watch, create, update, patch, delete] | ||
| - apiGroups: [blockstor.cozystack.io] | ||
| resources: ["*/status", "*/finalizers"] | ||
| verbs: [get, update, patch] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Expect no wildcard Blockstor resource/subresource grants in these templates.
rg -n 'resources:\s*\["\*"\]|"\*/(status|finalizers)"' \
packages/system/linstor/templates/blockstor-controller.yaml \
packages/system/linstor/templates/blockstor-satellite.yamlRepository: cozystack/cozystack
Length of output: 443
Replace wildcard Blockstor RBAC with explicit CRD resources in both controller and satellite templates.
Wildcard rules in packages/system/linstor/templates/blockstor-controller.yaml (lines 28, 31) and packages/system/linstor/templates/blockstor-satellite.yaml (lines 64, 67) grant access to current and future Blockstor resources/subresources. Enumerate the eight CRDs and their /status//finalizers subresources explicitly instead. Kubernetes RBAC documentation explicitly warns that wildcard rules include both present and future resources.
🔒 Proposed RBAC tightening
- apiGroups: [blockstor.cozystack.io]
- resources: ["*"]
+ resources:
+ - controllerconfigs
+ - nodes
+ - physicaldevices
+ - resourcedefinitions
+ - resourcegroups
+ - resources
+ - snapshots
+ - storagepools
verbs: [get, list, watch, create, update, patch, delete]
- apiGroups: [blockstor.cozystack.io]
- resources: ["*/status", "*/finalizers"]
+ resources:
+ - controllerconfigs/status
+ - nodes/status
+ - physicaldevices/status
+ - resourcedefinitions/status
+ - resourcegroups/status
+ - resources/status
+ - snapshots/status
+ - storagepools/status
+ - controllerconfigs/finalizers
+ - nodes/finalizers
+ - physicaldevices/finalizers
+ - resourcedefinitions/finalizers
+ - resourcegroups/finalizers
+ - resources/finalizers
+ - snapshots/finalizers
+ - storagepools/finalizers
verbs: [get, update, patch]Apply this same fix to both blockstor-controller.yaml and blockstor-satellite.yaml.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - apiGroups: [blockstor.cozystack.io] | |
| resources: ["*"] | |
| verbs: [get, list, watch, create, update, patch, delete] | |
| - apiGroups: [blockstor.cozystack.io] | |
| resources: ["*/status", "*/finalizers"] | |
| verbs: [get, update, patch] | |
| - apiGroups: [blockstor.cozystack.io] | |
| resources: | |
| - controllerconfigs | |
| - nodes | |
| - physicaldevices | |
| - resourcedefinitions | |
| - resourcegroups | |
| - resources | |
| - snapshots | |
| - storagepools | |
| verbs: [get, list, watch, create, update, patch, delete] | |
| - apiGroups: [blockstor.cozystack.io] | |
| resources: | |
| - controllerconfigs/status | |
| - nodes/status | |
| - physicaldevices/status | |
| - resourcedefinitions/status | |
| - resourcegroups/status | |
| - resources/status | |
| - snapshots/status | |
| - storagepools/status | |
| - controllerconfigs/finalizers | |
| - nodes/finalizers | |
| - physicaldevices/finalizers | |
| - resourcedefinitions/finalizers | |
| - resourcegroups/finalizers | |
| - resources/finalizers | |
| - snapshots/finalizers | |
| - storagepools/finalizers | |
| verbs: [get, update, patch] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/system/linstor/templates/blockstor-controller.yaml` around lines 27
- 32, The RBAC rules in the blockstor-controller.yaml and
blockstor-satellite.yaml templates use wildcard resources which grant access to
both current and future Blockstor resources. Replace the wildcard `["*"]` in the
resources field (line 28 in controller template) with an explicit list of all
eight Blockstor CRD names. Similarly, replace the wildcard `["*/status",
"*/finalizers"]` in the resources field (line 31 in controller template) with
explicit subresource paths for each CRD following the pattern "crdname/status"
and "crdname/finalizers". Apply the same changes to the corresponding lines in
blockstor-satellite.yaml (lines 64 and 67) to ensure consistent and restrictive
RBAC rules across both templates.
Source: Linters/SAST tools
| - apiGroups: [""] | ||
| resources: [secrets] | ||
| verbs: [get, list, watch] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Expect "no" after scoping Secret access.
NAMESPACE="${NAMESPACE:-cozy-linstor}"
kubectl auth can-i list secrets --all-namespaces \
--as="system:serviceaccount:${NAMESPACE}:blockstor-satellite"
kubectl auth can-i get secrets --all-namespaces \
--as="system:serviceaccount:${NAMESPACE}:blockstor-satellite"Repository: cozystack/cozystack
Length of output: 157
🏁 Script executed:
# Read the file to see the full RBAC configuration
wc -l packages/system/linstor/templates/blockstor-satellite.yamlRepository: cozystack/cozystack
Length of output: 126
🏁 Script executed:
# Read the entire file to understand the context
cat -n packages/system/linstor/templates/blockstor-satellite.yamlRepository: cozystack/cozystack
Length of output: 8994
Scope Secret access to fixed names in the release namespace.
The ClusterRole grants get, list, watch on all Secrets across all namespaces. Every privileged blockstor-satellite DaemonSet pod inherits these permissions; a compromised satellite would expose cluster-wide credentials beyond LUKS passphrases. Convert the rule to a namespaced Role scoping access to specific Secret names (or use get only if direct references suffice), then create a RoleBinding in the release namespace.
Current RBAC configuration
- apiGroups: [""]
resources: [secrets]
verbs: [get, list, watch]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/system/linstor/templates/blockstor-satellite.yaml` around lines 54 -
56, The RBAC rule grants overly broad cluster-wide access to all Secrets with
get, list, and watch verbs. Replace the ClusterRole entry that grants
permissions on the secrets resource with a namespaced Role scoped to the release
namespace that uses the resourceNames field to restrict access to only the
specific Secret names required by the blockstor-satellite pods, and reduce the
verbs to only what is necessary (typically just get). Then create a
corresponding RoleBinding in the release namespace that binds this Role to the
blockstor-satellite service account.
Source: Linters/SAST tools
| externalController: | ||
| url: https://blockstor-apiserver.cozy-linstor.svc:3371 |
There was a problem hiding this comment.
Template the external controller namespace.
The Blockstor workloads are namespaced with {{ .Release.Namespace }}, but this URL is hard-coded to cozy-linstor; any non-default release namespace points CSI at the wrong Service and can also break the serving-cert SAN match. Template this value and keep blockstor-apiserver-tls.yaml SANs in sync.
🐛 Proposed fix
externalController:
- url: https://blockstor-apiserver.cozy-linstor.svc:3371
+ url: "https://blockstor-apiserver.{{ .Release.Namespace }}.svc:3371"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| externalController: | |
| url: https://blockstor-apiserver.cozy-linstor.svc:3371 | |
| externalController: | |
| url: "https://blockstor-apiserver.{{ .Release.Namespace }}.svc:3371" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/system/linstor/templates/cluster.yaml` around lines 18 - 19, The
externalController URL in the cluster.yaml template has a hard-coded namespace
reference to cozy-linstor, which conflicts with the use of templated
Release.Namespace elsewhere in the Blockstor workloads. Replace the hard-coded
namespace string in the blockstor-apiserver service URL with the Helm template
variable for Release.Namespace to ensure the URL dynamically points to the
correct service regardless of deployment namespace. Additionally, update the
Subject Alternative Names (SANs) in the blockstor-apiserver-tls.yaml template
file to use the same templated namespace variable so the TLS certificate matches
the actual service FQDN.
| {{- range $path, $_ := .Files.Glob "definitions/*" }} | ||
| --- | ||
| {{ $.Files.Get $path }} | ||
| {{- end }} |
There was a problem hiding this comment.
System chart structure breaks the required umbrella vendoring pattern.
This adds system-component manifest logic under packages/system/linstor/templates/ rather than following the required vendored-upstream charts/ umbrella layout for packages/system/**. Please restructure this layer to comply with the package contract before merge.
As per coding guidelines, packages/system/**: “Follow Helm Charts umbrella pattern with vendored upstream charts in charts/ subdirectory for system components charts in packages/system/.”
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/system/linstor/templates/crds/crds.yaml` around lines 1 - 4, The
current template structure in packages/system/linstor/templates/ does not follow
the required umbrella vendoring pattern for system packages. Restructure the
linstor package to vendor upstream charts under a charts/ subdirectory
(packages/system/linstor/charts/) and move the crds.yaml template and its
related definitions to align with the umbrella pattern. Update the file
reference paths in the crds.yaml template to correctly point to the definitions
within the vendored chart structure rather than using direct file globbing from
the templates directory.
Source: Coding guidelines
440edb1 to
fe42418
Compare
af5db5e to
84ba857
Compare
fe42418 to
c2c6413
Compare
b86298f to
0842364
Compare
IvanHunters
left a comment
There was a problem hiding this comment.
Reviewed as a PoC (marked do-not-merge). Architecture is sound; a few things to capture before it becomes mergeable:
templates/satellites-talos.yaml: the{{- if .Values.talos.enabled }}guard was removed, so theLinstorSatelliteConfigurationpatching piraeus's ownlinstor-satelliteDaemonSet renders unconditionally. In external mode piraeus still deployslinstor-satellitenext to the newblockstor-satellite— two privileged DaemonSets both driving DRBD//devon every node (and the piraeus one CrashLoops on Talos, per your e2e diag). External mode should explicitly suppress the piraeus satellite.- No storage migration path: on existing clusters, flipping
LinstorClusterto external mode removes the linstor-controller Deployment and blockstor starts with an empty CRD store — existing PVs would likely be stranded. This was validated only as a fresh install. A migration/import story is a hard requirement for a real merge. - Minor: the apiserver's cluster-wide Secrets write (create/update/patch across all namespaces) is broader than its stated need (the passphrase Secret in its own namespace) — consider a namespace-scoped Role.
- Base branch is behind main (migration 44 vs 50) and carries borrowed VPA/test-retry changes to drop on rebase.
Swap the in-cluster Java LINSTOR controller for Blockstor, a Kubernetes-native, LINSTOR-API-compatible control plane. piraeus-operator now runs in EXTERNAL mode: it deploys no linstor-controller and only drives linstor-csi, pointed at the blockstor apiserver (mTLS :3371). The reused linstor-csi keeps the linstor.csi.linbit.com provisioner, so existing StorageClasses and PVCs are unchanged. - blockstor controller/apiserver/satellite are the published ghcr.io/cozystack/blockstor-* v0.1.17 images, pinned by digest. - LinstorCluster gains externalController.url + blockstor-api-ca apiTLS and drops the in-cluster controller/plunger podTemplate; controller-level DRBD properties (auto-diskful + Net tuning) are applied to the external controller over the LINSTOR REST API. - linstor-compat.yaml adds a linstor-controller Service alias (selecting the apiserver pods) + linstor-client-tls so linstor-gui/-scheduler keep resolving https://linstor-controller.cozy-linstor.svc:3371. - Removes the obsolete plunger / satellites-* / internal-TLS templates and vendors the blockstor CRDs. - helm-unittest specs are rewritten for the external-mode surface. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io>
- e2e-post-install-prep.sh creates the backing zpool inside each blockstor-satellite pod, registers per-node Node + StoragePool CRs, and waits on the blockstor Deployments / LinstorCluster instead of an in-cluster linstor-controller. Readiness gates run off one shared deadline (wait_for) so the ~70s operator-emits-HR latency no longer races a per-step timeout. - run-kubernetes.sh reads pool free capacity from the blockstor StoragePool CRs (status.freeCapacity) instead of exec'ing 'linstor sp l' in a controller pod that no longer exists in external mode. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io>
8c5e5c0 to
6db8c68
Compare
VerdictNOT LGTM This is a fresh-install-only PoC (correctly labelled do-not-merge/hold); the existing-customer upgrade path from classic LINSTOR is unhandled, a removed Findings[CRITICAL] This PR flips [MAJOR] The diff deletes the enclosing [MAJOR] The Deployment has no [MAJOR] Each ClusterRole grants [MAJOR] The apiserver ClusterRole grants [MAJOR]
[MAJOR]
[MINOR]
Claim mismatches[PARTIAL] PR body: images "pinned to Caveats
Recommended follow-ups
|
What changes
LinstorCluster→ external mode. piraeus-operator no longer runs its own controller; it only drives linstor-csi, which now talks to the Blockstor apiserver over the LINSTOR-compatible REST API (mTLS:3371).spec.externalController.url+spec.apiTLS.certManager(chained toblockstor-api-ca) so linstor-csi gets client certs.cozy-linstor: controller Deployment, apiserver Deployment + mTLS Service (:3371), satellite DaemonSet (privileged/hostNetwork; DRBD module from the Talossiderolabs/drbdextension via/lib/modules), the apiserver TLS PKI (chained from the existingca-bootstrapperIssuer), and theblockstor.cozystack.ioCRDs.controllerfromghcr.io/cozystack/blockstor-controller;apiserver/satellitepinned tottl.shbuilds carrying two fixes surfaced by this PoC — now released in blockstor v0.1.15: the satellite waits for the backing zvol device before mkfs/attach (zfs/udev race), and the apiserver resolves a cache-missed ResourceDefinition via the direct API reader (multi-replica RD-404). linstor-csi unchanged.hack/e2e-post-install-prep.sh): pre-creates the cluster-scopedNodeCRs (InternalIP → DRBD peer resolution), the backingdatazpool on each worker's/dev/vdc, and a per-nodeStoragePool data. Thelocal/replicatedStorageClasses (provisionerlinstor.csi.linbit.com,storagePool: data) are untouched.What it took to go green (beyond the storage swap)
main.test-apps-%— absorbs transient readiness flakes that rotate per app; each app bats self-cleans at the start, so a re-invocation is a clean fresh attempt. Proof it's load-bearing:mariadbpassed on attempt 3.Why external mode
Blockstor reimplements the LINSTOR controller/REST/satellite; the CSI wire-shape is identical, so the cleanest swap is to disable piraeus's bundled controller and point linstor-csi at Blockstor's apiserver.
What e2e proved
The
vminstance/ RWX app tests provision PVCs on thereplicatedStorageClass (3-way DRBD). The green run = Blockstor served real replicated DRBD volumes through the unchanged CSI + StorageClass path.Known gaps / PoC caveats
imagebuild is stubbed) — fine for a PoC branch, not for merge (a mergeable version needs in-tree Dockerfiles).Summary by CodeRabbit
Release Notes