docs: restore and update storage documentation - #63
senolcolak wants to merge 18 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved issues include unsafe storage defaults, missing CephBlockPool setup, and documented commands or paths that fail as written.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Restores and updates storage documentation for Rook, Ceph, and related components, aligned with current versions.
Changes:
- Updates Rook and Rook-Ceph deployment, CSI, CephFS, verification, and cleanup guidance.
- Expands Ceph architecture and storage-interface documentation.
- Refreshes Arbiter, Chorus, Liquid-Ceph, and storage navigation documentation.
File summaries
| File | Description |
|---|---|
docs/storage/rook.md |
Updates Rook installation and storage workflows. |
docs/storage/rook-ceph.md |
Adds comprehensive Rook-Ceph deployment guidance. |
docs/storage/liquid-ceph.md |
Documents Liquid-Ceph integration details. |
docs/storage/index.md |
Refreshes storage architecture and component relationships. |
docs/storage/chorus.md |
Updates Chorus capabilities and access notes. |
docs/storage/ceph.md |
Expands Ceph architecture and interface documentation. |
docs/storage/arbiter.md |
Adds Arbiter setup and development instructions. |
Review details
Suppressed comments (3)
docs/storage/ceph.md:389
- CephFS provides POSIX semantics once mounted, but it is not a drop-in NFS/SMB protocol replacement: clients still need a CephFS kernel, FUSE, or libcephfs client and Ceph credentials. This wording can lead readers to assume existing NFS/SMB clients can mount it directly.
POSIX compliance ensures that existing applications can use CephFS without
modification, making it a drop-in replacement for traditional network filesystems
like NFS or SMB.
docs/storage/index.md:30
- The component table now introduces an
Observabilitylayer, but the preceding architecture text still says the stack has only three layers and omits observability from that model. This conflicts with the repository's cross-cutting treatment of observability (for example,docs/overview/architecture.md:45); label this rowCross-cuttingor update the layer description consistently.
| [Observability & Audit](/observability/) | Observability | Metrics, dashboards, alerting, and audit - Prometheus, Perses, Prysm |
docs/storage/rook-ceph.md:318
- This command is marked as conditional, but without
--ignore-not-foundit returns an error whenever the reader did not create the optional object store. Add the same not-found handling used for the other optional resources so the documented cleanup path remains valid.
# Delete object storage (if created)
kubectl delete -f object.yaml
- Files reviewed: 7/7 changed files
- Comments generated: 6
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved setup and configuration issues remain in the Rook and Arbiter documentation.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (6)
docs/storage/arbiter.md:104
- After cloning only
external-arbiter-operator, this walkthrough never creates therookdirectory, so the firstkubectl applyfails with a missing path and the subsequent cluster/toolbox steps cannot run. Clone the pinned Rook checkout (as the Rook-Ceph guide does) before these commands, or reference an existing checkout.
kubectl apply -f ./rook/deploy/examples/crds.yaml
kubectl apply -f ./rook/deploy/examples/common.yaml
kubectl apply -f ./rook/deploy/examples/operator.yaml
kubectl apply -f ./rook/deploy/examples/csi-operator.yaml
docs/storage/arbiter.md:85
- The quick start provisions only one Lima VM/Kubernetes cluster, but Arbiter's purpose is to deploy a monitor on a separate remote tiebreaker cluster. The later user/secret setup does not create that second cluster, so this walkthrough cannot exercise the topology it documents and may produce a false high-availability result. Provision a distinct target cluster and kubeconfig, or clearly label this as a same-cluster development-only test.
# Create VM instance
limactl create --name=k8s ./contrib/vm.yaml
docs/storage/observability/index.md:2
- The frontmatter title now says
Observability & Audit, while the page heading remains# Observability; this makes the generated navigation and visible page heading disagree. Keep the frontmatter title and H1 aligned.
docs/storage/rook-ceph.md:28 - This prerequisite advertises a one-node test, but the commands below clone and apply the standard
cluster.yamlworkflow, whose default three monitors cannot be scheduled on one node whenallowMultiplePerNodeis false. Either require three worker nodes for this workflow or switch the single-node path tocluster-test.yamland label it test-only.
- At least 3 worker nodes for a production cluster (1 node minimum for testing)
docs/storage/rook.md:95
- Only the Rook chart is pinned here;
ceph-csi-operator/ceph-csi-driversis installed without--version. A future CSI chart can therefore be combined with the values from the v1.20.7 Rook tag, making this setup non-reproducible and potentially incompatible with that Rook release. Pin the CSI chart to the version tested with Rook v1.20.7, or use versioned release-managed manifests.
helm install --namespace rook-ceph \
--wait \
-f https://raw.githubusercontent.com/rook/rook/v1.20.7/deploy/charts/ceph-csi-drivers/values.yaml \
ceph-csi-drivers ceph-csi-operator/ceph-csi-drivers
docs/storage/rook.md:121
- This manifest is linked from
getting-started/minimal.mdas the single-node Ceph setup, butmon.count: 3together withallowMultiplePerNode: falsecannot schedule on a one-node cluster. Following that documented path leaves the CephCluster unable to become Ready; provide a separate single-node manifest/settings or update the minimal guide.
mon:
count: 3
allowMultiplePerNode: false
- Files reviewed: 13/15 changed files
- Comments generated: 4
- Review effort level: Lite
85e0c35 to
489cab0
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The explicit-device example, observability target, readiness waits, RGW workflow, and single-node setup still need correction.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
docs/getting-started/minimal.md:99
- This paragraph describes a required single-node
CephClusterconfiguration, but the guide provides no manifest orkubectlcommand to create it. After the Helm install, the workflow jumps directly to OpenStack, so no Ceph cluster exists for the stated storage backend. Add an inline test-only manifest/command, or link to a concrete checked-in example containing these monitor and replica settings.
Then deploy a minimal single-node `CephCluster`. This test-only configuration
requires one monitor, permits that monitor on the single node, and uses
single-replica pools. Do not apply the three-node example unchanged. See
[Storage - Rook](/storage/rook) for the production cluster configuration.
docs/storage/arbiter.md:119
- These commands only submit the CephCluster and toolbox resources; they do not wait for Rook to reconcile the cluster or for
rook-ceph-toolsto become Ready. The laterkubectl exec ... ceph mon dumpcan therefore run before the toolbox exists or before quorum is established and fail intermittently. Add readiness waits before deploying and verifying Arbiter.
# Create Ceph cluster
kubectl apply -f ./rook/deploy/examples/cluster-test.yaml
# Install the Ceph toolbox used by the verification step
kubectl apply -f ./rook/deploy/examples/toolbox.yaml
docs/storage/rook.md:32
- This describes RGW as something that can be dynamically provisioned through a Kubernetes StorageClass, but StorageClasses provision RBD/CephFS volumes; RGW is exposed through a
CephObjectStoreand object-bucket/S3 APIs instead. Please separate the block/file StorageClass workflow from the RGW object-storage workflow so readers do not look for a nonexistent RGW StorageClass.
define storage classes that map to Ceph pools, allowing applications to
dynamically provision persistent volumes for block storage (RBD), shared file
systems (CephFS), or object storage (RGW) through standard Kubernetes
mechanisms.
- Files reviewed: 14/16 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
Moderate documentation workflow gaps must be addressed before approval.
Review details
Suppressed comments (5)
Previously missed (3) — in code that hasn't changed since the last review.
docs/getting-started/minimal.md:99
- This now warns that a single-node manifest needs one monitor and single-replica pools, but the guide still provides no manifest or command to create that
CephCluster; it jumps from the operator install to Step 5. A reader cannot complete the advertised minimal storage setup, and the only linked Rook guide is the three-node production example that this text says not to apply unchanged. Add the single-node resource (or link to a version-pinned test manifest) and itskubectl applycommand here.
docs/storage/arbiter.md:119 - Applying the toolbox deployment is asynchronous, but the later verification step immediately executes into
rook-ceph-tools. Without waiting for the deployment, the command can fail while the pod is still starting; add a rollout wait here (and keep the existing readiness checks before running Ceph commands).
docs/storage/rook.md:32 - This describes RGW object storage as something applications dynamically provision through a Kubernetes StorageClass/PV, but RBD and CephFS are the CSI-backed StorageClass interfaces here. RGW is exposed by a
CephObjectStoreresource and consumed through S3/Swift (or object-bucket) APIs, so this wording sends readers toward a nonexistent RGW StorageClass.
docs/storage/arbiter.md:119
- These
kubectl applycalls are asynchronous, but the next sections immediately build and install Arbiter and later exec intorook-ceph-tools. Without waiting for the CephCluster and toolbox to become ready, the documented sequence can race and fail before those resources exist; add readiness checks after creating the cluster and toolbox.
# Create Ceph cluster
kubectl apply -f ./rook/deploy/examples/cluster-test.yaml
# Install the Ceph toolbox used by the verification step
kubectl apply -f ./rook/deploy/examples/toolbox.yaml
docs/storage/index.md:30
- The page says the stack is organized into three layers, but this new row introduces
Observabilityas a fourth layer. Since the observability section is cross-cutting across compute, storage, and OpenStack, classify this row as cross-cutting or update the architecture statement so the page does not contradict itself.
| [Observability & Audit](./observability/) | Observability | Metrics, dashboards, alerting, and audit - Prometheus, Perses, Prysm |
- Files reviewed: 14/16 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Five unresolved review findings remain in deployment guidance and documentation consistency.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (4)
Previously missed (3) — in code that hasn't changed since the last review.
docs/getting-started/minimal.md:99
- This revised getting-started step warns users not to apply the three-node example, but it still provides no single-node
CephClustermanifest orkubectl applycommand. A new user following the minimal path is left without a deployable storage configuration; add the concrete test-only YAML/apply step or link to an existing minimal manifest.
docs/storage/observability/prometheus.md:16 rook-ceph-mgris the Ceph Manager daemon name, not a Rook operator/exporter. Labeling it as a Rook manager that exposes operator status and daemon lifecycle events misidentifies the metric source; describe the Ceph Manager metrics here or point to the actual Rook operator exporter.
docs/storage/rook-ceph.md:107- The explicit-device example assigns OSDs to only
node1andnode2, while the guide'scluster.yamlstill requires three monitors withallowMultiplePerNode: false. Following this replacement config leaves the third monitor unschedulable, so the cluster cannot become Ready (and the default replicated pools also lack a third host). Add a third node/device or clearly mark this as an incomplete fragment.
docs/storage/arbiter.md:58
- This prerequisite offers Minikube or another Kubernetes provider, but the walkthrough below is Lima-specific: it calls
limactl, uses./contrib/vm.yaml, and builds through the Lima guest'snerdctl. A user choosing the advertised alternative cannot follow these commands; either make Lima a hard prerequisite or provide provider-specific setup commands.
- `lima` (or another method to provision Kubernetes locally, such as Minikube)
- Files reviewed: 20/20 changed files
- Comments generated: 1
- Review effort level: Lite
Signed-off-by: senol.colak <senol.colak@sap.com>
Address the storage documentation review findings: - replace non-breaking spaces so copied YAML and shell commands remain valid - add the missing Rook clone and installation sequence - clone the correct Arbiter repository before running project commands - delete dependent Ceph resources before removing the cluster - point storage readers to the canonical observability section and remove duplicate pages Signed-off-by: senol.colak <senol.colak@sap.com>
Apply findings from the final comprehensive review: - deploy the Rook cluster only after readers customize its manifest - describe Arbiter as proactively provisioning and reconciling its monitor - align cert-manager setup with the current Arbiter project guide - correct RBD resize, QEMU/KVM, RGW caching, and OSD replacement claims - describe Liquid-Ceph as Limes quota and usage metering for RGW - remove organization-restricted links and retain only publicly valid references - make Rook teardown remove workloads and claims before cluster resources - add complete OSD signature cleanup commands Signed-off-by: senol.colak <senol.colak@sap.com>
Resolve the final compatibility and safety review findings: - pin the Helm chart, example manifests, and clone to Rook v1.17.9 - use the Ceph v19.2.3 image recommended by that Rook release - document Rook 1.17 supported Kubernetes versions - warn that useAllDevices consumes every eligible raw device - replace misleading production hardware minimums with sizing guidance - clarify RBD snapshot consistency, current RGW serving, and MDS failover behavior Signed-off-by: senol.colak <senol.colak@sap.com>
Fix issues found during the additional comprehensive review: - install and remove the Ceph toolbox used by troubleshooting commands - move host-device inspection out of the toolbox container - remove Arbiter custom resources before uninstalling their operator - delete the Lima OSD disk created by the Arbiter quick start - make the Arbiter verification wait instruction actionable - replace the misleading serial storage diagram with component relationships - verify every referenced Rook and Arbiter file against its upstream repository Signed-off-by: senol.colak <senol.colak@sap.com>
Address findings from the additional documentation-quality review: - give repeated Ceph sections unique anchors - add required spacing around headings and code fences - convert resource URLs into labeled Markdown links - remove the extra blank line in the Rook-Ceph guide - normalize em dashes reintroduced from the snapshot to current main style Signed-off-by: senol.colak <senol.colak@sap.com>
Resolve findings from the latest comprehensive review: - use Rook v1.20.7, Ceph v20.2.4, and Kubernetes 1.31 through 1.37 - install and remove the CSI operator required by the manifest workflow - remove the duplicate rbd-pvc creation that caused AlreadyExists - wait for RGW and CSI test pods so verification detects mount failures - remove duplicate Rook and Arbiter summary paragraphs - verify every referenced v1.20.7 manifest and the published Helm chart Signed-off-by: senol.colak <senol.colak@sap.com>
Signed-off-by: senol.colak <senol.colak@sap.com>
Signed-off-by: senol.colak <senol.colak@sap.com>
Move Prysm, Prometheus, Perses, and their overview back under Storage as they appeared in the pre-restructure documentation. Preserve the expanded page content and update navigation and cross-references to the canonical storage paths. Signed-off-by: senol.colak <senol.colak@sap.com>
Clarify safe Rook deployment requirements, make optional cleanup idempotent, restore missing Arbiter setup, and correct CephFS and observability wording. Signed-off-by: senol.colak <senol.colak@sap.com>
Signed-off-by: senol.colak <senol.colak@sap.com>
Keep compatibility pages at the former top-level observability paths while the canonical content remains under storage/observability. Signed-off-by: senol.colak <senol.colak@sap.com>
Add a pinned single-node Ceph and CSI workflow for the minimal environment, wait for Arbiter prerequisites, correct storage architecture wording, and complete the three-node device example. Signed-off-by: senol.colak <senol.colak@sap.com>
c9cc7ff to
96f040a
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate documentation workflow issues remain.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
docs/storage/ceph.md:67
- This adds a second
## Architectureheading; the page already has one at line 26. The generated outline therefore contains two indistinguishable sections, even though the content here is specifically about storage interfaces. Rename this heading to make the section unambiguous.
## Architecture
docs/storage/rook-ceph.md:335
kubectl deletereturns before the CephCluster finalizer has completed teardown, but the next commands immediately remove the operator and CRDs. That can strand the cluster in Terminating because Rook is no longer running to clear its finalizer; wait for the CephCluster deletion before uninstalling the operator.
kubectl delete -f cluster.yaml
- Files reviewed: 20/20 changed files
- Comments generated: 2
- Review effort level: Lite
Disambiguate the Ceph architecture headings and wait for the CephCluster finalizer before removing Rook resources. Signed-off-by: senol.colak <senol.colak@sap.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Three moderate documentation workflow issues remain unresolved.
Review details
Suppressed comments (3)
docs/getting-started/minimal.md:153
- The pinned
csi/rbd/storageclass.yamlis a multi-document manifest that also definesCephBlockPool/replicapoolwith the standard replicated pool settings. This guide has already createdreplicapoolas a single-copy pool for the one-node cluster, so applying the upstream file here overwrites that compatible pool and can leave RBD provisioning unable to place replicas. Apply a StorageClass-only/customized manifest that preserves the single-replica pool instead.
kubectl apply -f https://raw.githubusercontent.com/rook/rook/v1.20.7/deploy/examples/csi/rbd/storageclass.yaml
docs/storage/arbiter.md:117
cluster-test.yamlin the pinned Rook examples creates the standardrook-cephCephCluster, so waiting oncephcluster/my-clusterfails with a NotFound error and stops this quick start before the toolbox step. Wait on the cluster name defined by that manifest.
kubectl -n rook-ceph wait --for=condition=Ready cephcluster/my-cluster --timeout=15m
docs/storage/rook.md:111
- This safety alternative is incomplete: the sample still has
useAllNodes: trueat line 130, so turning off onlyuseAllDevicesdoes not constrain node selection and can conflict with an explicitstorage.nodeslist. Instruct readers to set bothuseAllNodesanduseAllDevicestofalsebefore selecting devices, as in the detailed guide.
The example sets `useAllDevices: true`. Rook will consume every eligible raw
device it discovers on the selected nodes. Use dedicated storage nodes, or set
this option to `false` and select devices explicitly before applying the
manifest.
- Files reviewed: 20/20 changed files
- Comments generated: 0 new
- Review effort level: Lite
Keep the minimal single-replica pool intact, make Arbiter readiness independent of the test manifest name, and require both Rook device-selection safeguards. Signed-off-by: senol.colak <senol.colak@sap.com>
There was a problem hiding this comment.
🟡 Changes recommended
Four unresolved findings remain, including a critical CSI installation gap.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
docs/getting-started/minimal.md:94
- This standalone guide returns as soon as Helm submits the Rook chart, then immediately installs the CSI driver chart. That can race the creation/readiness of the CSI operator CRDs and make the second Helm install fail with an unknown resource; use the same
--waitused instorage/rook.mdbefore starting the CSI install.
docs/storage/arbiter.md:130
- The prerequisites list Docker/Podman, but the build step invokes
nerdctlinside the Lima VM. Neithernerdctlnor how it is made available is documented, so a reader following the stated prerequisites cannot reproduce this build; document/install the actual tool or change the command to use the listed engine.
limactl shell k8s sudo nerdctl --namespace k8s.io build \
-t localhost:5000/cobaltcore-dev/external-arbiter-operator:latest \
-f ./Dockerfile .
docs/storage/index.md:66
- This relationship is reversed and omits the actual consumer: Liquid-Ceph reads RGW quota/capacity/usage data and reports it to Limes, as described in
storage/liquid-ceph.md, rather than metering data into RGW. Update the diagram to show the RGW-to-Liquid-Ceph flow and a Liquid-Ceph-to-Limes relationship.
Chorus -. replicates objects .-> RGW
LiquidCeph[Liquid-Ceph] -. meters quota and usage .-> RGW
Observability[Prometheus, Perses, and Prysm] -. monitors .-> RADOS
- Files reviewed: 20/20 changed files
- Comments generated: 1
- Review effort level: Lite
| kubectl create -f csi-operator.yaml | ||
| kubectl create -f operator.yaml |
Remove empty top-level observability compatibility pages, wait for the Rook operator before CSI installation, document the Lima nerdctl dependency, and correct Liquid-Ceph data flow. Signed-off-by: senol.colak <senol.colak@sap.com>
There was a problem hiding this comment.
🔵 Needs a closer look
One newly introduced kubectl command in the new Rook-Ceph install guide uses an invalid JSONPath expression, which will prevent readers from retrieving the dashboard password as documented.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
docs/storage/rook-ceph.md:175
- The JSONPath expression used to print the dashboard password is not valid for kubectl; it should reference the secret's data field (e.g.
.data.password). As written, this typically returns an empty string and the decoded password step fails silently.
docs/storage/rook.md:25 - The sentence lists tasks as "..., the management of the monitor quorum", which is grammatically incorrect and reads like two separate clauses without a conjunction. Reword to keep the list parallel (e.g., "...and managing the monitor quorum").
- Files reviewed: 15/16 changed files
- Comments generated: 0 new
- Review effort level: Lite
Remove the stale standalone observability section reference from the overview navigation guidance. Signed-off-by: senol.colak <senol.colak@sap.com>
There was a problem hiding this comment.
🔵 Needs a closer look
The new Rook-Ceph guide contains an invalid kubectl JSONPath example for retrieving the dashboard password, which will fail when users follow the instructions.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
docs/storage/rook-ceph.md:175
- The kubectl JSONPath expression is missing the root selector (
.). As written,jsonpath="{['data']['password']}"is not valid kubectl JSONPath and will fail to extract the dashboard password; use the standard{.data.password}form instead.
- Files reviewed: 16/17 changed files
- Comments generated: 0 new
- Review effort level: Lite
Summary
Validation