Add architecture and deployment flow documentation - #28
Open
senolcolak wants to merge 5 commits into
Open
senolcolak wants to merge 5 commits into
senolcolak wants to merge 5 commits into
Conversation
|
This PR is stale because it has been open for 45 days with no activity. |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new documentation contains a few concrete correctness issues (namespace in secret creation, API call used for reachability, and spec field spelling/permission-check details) that could mislead deployments.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a new documentation set for the external-arbiter-operator, aiming to explain the operator’s architecture and provide an end-to-end deployment walkthrough.
Changes:
- Add architecture documentation with ASCII diagrams and reconciliation/data-flow explanations (
docs/ARCHITECTURE.md). - Add a detailed phased deployment walkthrough including example manifests/commands and troubleshooting (
docs/DEPLOYMENT-FLOW.md). - Add a documentation index/entry point (
docs/README.md) and update REUSE scope to include docs and contrib paths (REUSE.toml).
File summaries
| File | Description |
|---|---|
| REUSE.toml | Includes contrib/** and docs/** in REUSE annotations path list. |
| docs/README.md | Adds a docs landing page linking to architecture and deployment docs. |
| docs/DEPLOYMENT-FLOW.md | Adds a detailed, diagram-driven deployment timeline, quick reference commands, and troubleshooting flow. |
| docs/ARCHITECTURE.md | Adds architecture diagrams and detailed reconciliation/data-flow documentation. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+267
to
+271
| │ Check: Can create deployments? │ | ||
| │ API: POST /apis/authorization.k8s.io/v1/selfsubjectaccessreviews │ | ||
| │ ├─ Resource: deployments │ | ||
| │ ├─ Verb: create │ | ||
| │ ├─ Namespace: external-arbiter │ |
| │ │ │ ┌──────────────────┐ │ │ ┌──────────────────────────────┐ │ │ │ | ||
| │ │ │ │ Spec: │ │ │ │ Spec: │ │ │ │ | ||
| │ │ │ │ - namespace │ │ │ │ - cephCluster (ref) │ │ │ │ | ||
| │ │ │ │ - accessKeyRef │ │ │ │ - remoteCluster (ref/inline) │ │ │ │ |
| │ source: │ | ||
| │ - mon secrets │ | ||
| │ - mon configmap│ | ||
| │ - mon deploymnt│ |
Comment on lines
+63
to
+66
| │ kubectl create secret generic │ | ||
| │ external-arbiter \ │ | ||
| │ --from-file=kubeconfig.yaml │ | ||
| └────────────────────────────────────────┘ |
Comment on lines
+236
to
+240
| │ Test Remote Cluster Connection │ | ||
| │ │ | ||
| │ API Call: GET /api/v1/namespaces/external-arbiter │ | ||
| │ → https://remote-cluster-api:6443 │ | ||
| └───────────────────────────────────────────────────────────────────┘ |
- Add ARCHITECTURE.md with detailed component diagrams - High-level architecture showing source and remote clusters - RemoteCluster and RemoteArbiter reconciliation flows - Component interaction sequences - Ceph configuration data flow - Permission model and RBAC requirements - State machines for both controllers - Key design patterns - Add DEPLOYMENT-FLOW.md with step-by-step deployment guide - 8 deployment phases from prerequisites to steady state - Detailed operator installation steps - RemoteCluster and RemoteArbiter creation flows - Arbiter resource deployment with complete manifests - Pod startup logs and Ceph quorum formation - Quick deployment reference commands - Troubleshooting decision trees Both documents use ASCII diagrams for maximum compatibility. Signed-off-by: senol.colak <senol.colak@sap.com>
Signed-off-by: senol.colak <senol.colak@sap.com>
- Add missing commas after .helmignore and *.tpl entries - Add docs/** path to license annotations for new documentation files - Fixes REUSE compliance check failure Signed-off-by: senol.colak <senol.colak@sap.com>
This fixes REUSE compliance check by covering all files in the contrib directory including: - contrib/charts/external-arbiter-operator/.helmignore - contrib/charts/external-arbiter-operator/templates/_helpers.tpl - contrib/k8s/examples/*.yaml - contrib/k8s/test/*.yaml All these files will now be covered by the Apache-2.0 license annotation. Signed-off-by: senol.colak <senol.colak@sap.com>
The architecture and deployment-flow diagrams described fabricated arbiter resources that do not match the operator's actual behavior. Corrected to reflect the real mechanism: - Mon ID uses the ext- prefix with a suffix allocated from the ExternalMonIDs domain (ext-a), not a source-quorum letter (ext-c). - Arbiter Deployment is a DeepCopy of the source Rook mon Deployment, patched (monmap init container, volume rewrites, --id patch) rather than authored from scratch with a hand-written ceph-mon command. - Keyring Secret, override ConfigMap and env-var Secret are copied verbatim from the source mon; the arbiter joins the source cluster's existing Paxos quorum (same fsid/keyring/monmap). - Resources use generateName with a random suffix and are located by the ceph.cobaltcore.sap.com/lookup label; only keyring/envvar Secrets carry the role label (Service/Deployment/override do not). - Service is created only when spec.service is set and exposes both msgr ports (6789 v1, 3300 v2); selector is lookup-only. - ServiceAccount name is <release>-controller-manager. - Reconcile flow: generate MonID before reading mon config; add MonitorDeploymentExists/Ready conditions. - Fixed fabricated startup logs (mon.ext-a, no 0.0.0.0:3300 public addr). Signed-off-by: senol.colak <senol.colak@sap.com>
senolcolak
force-pushed
the
docs/architecture-diagrams
branch
from
September 4, 2026 11:38
755e041 to
7d37e41
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Several key deployment/RBAC details in the new docs conflict with the repository’s existing setup script and Helm chart naming, which would lead users to follow incorrect commands.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (8)
docs/DEPLOYMENT-FLOW.md:33
- The role name in the prerequisites diagram is inconsistent with the actual role created by hack/configure-k8s-user.sh ("external-arbiter-role").
│ Role: arbiter-role │
docs/DEPLOYMENT-FLOW.md:46
- The RoleBinding line shows binding a ServiceAccount (arbiter-sa) to the role, but the setup script binds the "external-arbiter" user to the role.
│ arbiter-sa → role │
└────────────────────────┘
docs/DEPLOYMENT-FLOW.md:55
- The kubeconfig box calls out an "SA Token", but the current setup script generates a kubeconfig using a client certificate/key for the "external-arbiter" user.
│ kubeconfig.yaml │
│ - API Server URL │
│ - SA Token │
│ - CA Certificate │
└────────────────────────┘
docs/DEPLOYMENT-FLOW.md:807
- This comment says the script creates a ServiceAccount, but ./hack/configure-k8s-user.sh actually creates a user (client cert), Role/RoleBinding, and writes contrib/k8s/examples/secret.yaml.
./hack/configure-k8s-user.sh # Creates SA, Role, RoleBinding, kubeconfig
docs/DEPLOYMENT-FLOW.md:850
- The arbiter Deployment on the remote cluster is created with generateName (arbiter-deployment-...) and selected via the lookup label; using a fixed deployment name "external-arbiter" in the docs will fail.
kubectl get pods -n external-arbiter
kubectl logs -n external-arbiter deployment/external-arbiter
docs/ARCHITECTURE.md:312
- Typo in the RemoteArbiter reconciliation flow: "deploymnt" should be "deployment".
│ - mon deploymnt│
docs/ARCHITECTURE.md:42
- The diagram uses "accessKeyRef", but the CRD field name is "accesskeyRef" (lowercase "k") per the JSON tag in RemoteClusterSpec; using the wrong field name will confuse users applying YAML.
│ │ │ │ - accessKeyRef │ │ │ │ - remoteCluster (ref/inline) │ │ │ │
docs/ARCHITECTURE.md:589
- This permission model section describes a ServiceAccount (arbiter-sa) on the remote cluster, but the documented setup in README.md and hack/configure-k8s-user.sh uses a user (client certificate) named "external-arbiter".
REMOTE CLUSTER (Target Namespace)
┌───────────────────────────────────────────────────────────────┐
│ Service Account: arbiter-sa (created by user) │
│ │
- Files reviewed: 4/4 changed files
- Comments generated: 5
- Review effort level: Lite
Comment on lines
+125
to
+127
| │ │ │ ServiceAccount: arbiter-sa │ │ │ | ||
| │ │ │ Role: arbiter-role │ │ │ | ||
| │ │ │ RoleBinding: arbiter-rolebinding │ │ │ |
Comment on lines
+23
to
+28
| │ - 2 OSDs │ │ 2. Create ServiceAccount | ||
| └────────────────────────┘ ▼ | ||
| ┌────────────────────────┐ | ||
| │ ServiceAccount: │ | ||
| │ arbiter-sa │ | ||
| └────────────────────────┘ |
Comment on lines
+696
to
+699
| │ Step 16: Check Arbiter Deployment Status (via remote client) │ | ||
| │ ├─ GET /apis/apps/v1/namespaces/external-arbiter/deployments/ │ | ||
| │ │ external-arbiter │ | ||
| │ │ │ |
Comment on lines
+826
to
+828
| # 4. Wait for RemoteCluster ready | ||
| kubectl wait --for=condition=Ready remotecluster/external-arbiter \ | ||
| -n arbiter-operator --timeout=60s |
| # VERIFICATION | ||
| # ──────────── | ||
| # Check operator logs | ||
| kubectl logs -n arbiter-operator deployment/external-arbiter-operator-manager -f |
jrse
self-requested a review
September 7, 2026 13:38
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR adds comprehensive documentation for the external-arbiter-operator project.
Changes
docs/ARCHITECTURE.md- Detailed architecture diagrams and technical designdocs/DEPLOYMENT-FLOW.md- Step-by-step deployment guide with visual timelinedocs/README.md- Documentation index and navigation guideDocumentation Contents
ARCHITECTURE.md (668 lines)
DEPLOYMENT-FLOW.md (915 lines)
README.md (52 lines)
All diagrams use ASCII art for maximum compatibility and can be viewed in any text editor, terminal, or GitHub web interface.
Total Changes