Conversation
There was a problem hiding this comment.
Pull request overview
Adds multi-region deployment documentation to the MkDocs site and aligns fleet-related terminology in existing docs.
Changes:
- Adds a new “Multi-Region Deployment” docs section (overview, setup, failover runbooks) to the preview documentation.
- Updates the AKS fleet playground README section header to “KubeFleet”.
- Updates the documentation agent tool list in its frontmatter.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| mkdocs.yml | Adds navigation entries for the new multi-region docs section. |
| documentdb-playground/aks-fleet-deployment/README.md | Renames a section header to “KubeFleet” for consistency with new docs. |
| docs/operator-public-documentation/preview/multi-region-deployment/setup.md | Introduces a multi-region setup guide including fleet and non-fleet workflows. |
| docs/operator-public-documentation/preview/multi-region-deployment/overview.md | Adds conceptual/architecture overview for multi-region deployments. |
| docs/operator-public-documentation/preview/multi-region-deployment/failover-procedures.md | Adds planned/unplanned failover runbooks. |
| .github/agents/documentation-agent.md | Updates agent tool configuration in the frontmatter. |
docs/operator-public-documentation/preview/multi-region-deployment/setup.md
Outdated
Show resolved
Hide resolved
docs/operator-public-documentation/preview/multi-region-deployment/setup.md
Outdated
Show resolved
Hide resolved
docs/operator-public-documentation/preview/multi-region-deployment/setup.md
Outdated
Show resolved
Hide resolved
docs/operator-public-documentation/preview/multi-region-deployment/setup.md
Outdated
Show resolved
Hide resolved
docs/operator-public-documentation/preview/multi-region-deployment/overview.md
Outdated
Show resolved
Hide resolved
xgerman
left a comment
There was a problem hiding this comment.
Documentation Agent Review — PR #314 Multi-Region Deployment Docs
Files: 6 changed (+1,134, −2)
New pages: overview.md, setup.md, failover-procedures.md
Overall Assessment
Strong first draft — well-structured with proper front matter, content tabs, admonitions, and code annotations. The three-page split (overview → setup → failover) is sensible. Several issues need attention before merge.
🔴 Critical
1. failover-procedures.md — Plaintext credentials in mongosh examples (lines 145, 274)
mongosh "mongodb://admin:password@localhost:10260/?tls=true&tlsAllowInvalidCertificates=true"
The quickstart uses k8s_secret_user:K8sSecret100 as clearly-fake placeholder credentials. Using admin:password looks like a real weak credential. Either use the same pattern as the quickstart or show how to extract the secret from Kubernetes:
PASSWORD=$(kubectl get secret documentdb-secret -n documentdb-preview-ns -o jsonpath='{.data.password}' | base64 -d)
mongosh "mongodb://documentdb-user:${PASSWORD}@localhost:10260/..."Also: admin isn't the default DocumentDB username — the quickstart uses k8s_secret_user. Ensure consistency.
2. failover-procedures.md — Missing directConnection=true and replicaSet=rs0 in mongosh URIs
The quickstart connection string includes directConnection=true&replicaSet=rs0. The failover doc omits both. Without directConnection=true, clients may attempt replica-set discovery through the gateway, which doesn't support it.
🟠 Major
3. setup.md — Per-cluster storage override uses environment field without explanation
The cloud-specific tab examples include:
- name: member-eastus2-cluster
storageClass: managed-csi
environment: aksThe CRD supports this (EnvironmentOverride string json:"environment,omitempty"), but the doc never explains what the environment field does. Add: "The environment field sets cloud-specific annotations on LoadBalancer services (e.g., NLB for EKS, internal LB for AKS)."
4. setup.md — documentdb-secret only contains password, not username
The example creates:
kubectl create secret generic documentdb-secret --from-literal=password='YourSecurePassword123!'But the mongosh examples in failover-procedures.md use admin as the user. What's the actual default username? The quickstart uses a different format. Document the expected secret keys.
5. failover-procedures.md — Broken link to fleet-add-region playground
see the [add region playground guide](...documentdb-playground/fleet-add-region/README.md)The directory exists locally but verify this file is merged to main on GitHub. If it's only on a feature branch, the link will 404 for readers.
6. overview.md — "cluster" used without qualifier in multiple places
The documentation agent requires: "Never use 'cluster' alone — always qualify as 'DocumentDB cluster' or 'Kubernetes cluster'." Several instances need qualification:
- "Each cluster must have a unique name across all regions" → which kind?
- "If the primary region fails, promote a replica in another region" → "replica" of what?
- Various other bare "cluster" references throughout
7. overview.md — "Self-name ConfigMap" naming mismatch
The cluster components list says "Self-name ConfigMap" but setup.md calls it cluster-identity ConfigMap. Use consistent naming across all pages.
8. setup.md — crossCloudNetworkingStrategy says "None" is default
The CRD field is omitempty with no explicit default annotation. If the operator treats empty string as "None", document that clearly.
🟡 Minor
9. failover-procedures.md — Checkbox list won't render
- [ ] **Replica health:** ...MkDocs needs pymdownx.tasklist for checkbox rendering, which isn't in mkdocs.yml. Convert to regular bullet list items.
10. failover-procedures.md — kubectl-plugin link text shows filename
[kubectl-plugin.md](../kubectl-plugin.md)Use descriptive text: [kubectl-documentdb plugin](../kubectl-plugin.md)
11. setup.md — Heading inconsistencies
#### Self Labelis vague — use#### Create the cluster identity ConfigMap#### Networking management— inconsistent casing vs other H4 headings
12. overview.md — Fleet Networking paragraph has grammar issues
"the Operator will create ServiceExports and MultiClusterServices on each Kubernetes cluster then, use those generated cross-regional services..."
Comma splice. Rewrite: "...the operator creates ServiceExport and MultiClusterService resources on each Kubernetes cluster and uses those cross-regional services to connect..."
13. .github/agents/documentation-agent.md — terminal → edit tool change
Why was terminal removed? If the documentation agent needs to run make api-docs or mkdocs build, it needs terminal access.
14. setup.md — documentdb-operator-crp.yaml only selects Namespace
The CRP selects kind: Namespace, name: documentdb-operator but doesn't include the Helm release/operator deployment. Clarify whether KubeFleet automatically installs the contents, or if this example is incomplete.
🟢 Nitpicks
15. failover-procedures.md Next Steps: "fail overs" → "failovers" (one word)
16. overview.md: Trailing space after "Reduce application response times..." sentence
17. mkdocs.yml placement: Multi-Region is added between Configuration and Advanced Configuration. Consider placing after Backup and Restore since it's more advanced.
18. overview.md: Add a link to KubeFleet on first mention in "Deployment Models".
Documentation Agent Checklist
| Check | Status | Notes |
|---|---|---|
| Front matter (title, description, tags) | ✅ Pass | All 3 new pages have proper front matter |
| Heading hierarchy | ✅ Pass | H1 → H2 → H3 properly nested |
| Content tabs for cloud providers | ✅ Pass | AKS/EKS/GKE tabs in setup.md |
| Code annotations | ✅ Pass | Used in main YAML example |
| Admonitions | ✅ Pass | !!! danger and !!! warning used |
| Links escape docs_dir | ✅ Pass | External playground links use absolute GitHub URLs |
| Internal cross-references | Verify fleet-add-region link on main | |
| "cluster" qualification | Several bare "cluster" instances | |
| MS Style Guide | Future tense, passive voice, parallel lists | |
pymdownx.tasklist |
❌ Missing | Checkbox syntax won't render |
Addendum: Microsoft Writing Style Guide Compliance
MS-🟠 Major Style Issues
MS-1. Excessive future tense ("will") — use present tense
MS Style Guide: "Use present tense. To the user, the action is present." The PR has 10+ instances of "will":
| File | Current | Suggested |
|---|---|---|
overview.md (DNS) |
"The operator will by default use..." | "The operator uses the DocumentDB cluster name..." |
overview.md (DNS) |
"it will be up to the user to coordinate" | "you must coordinate..." |
overview.md (Istio) |
"the operator will generate services" | "the operator generates services" |
overview.md (Fleet) |
"the Operator will create ServiceExports" | "the operator creates ServiceExports" |
setup.md (Self Label) |
"the DocumentDB CRD will be the same" | "the DocumentDB CRD is the same" |
setup.md (Fleet) |
"The fleet controller will install the operator" | "The fleet controller installs the operator" |
failover-procedures.md |
"there will be one fewer region" | "one fewer region remains" |
MS-2. Passive voice — convert to active
MS: "Use active voice. It's more direct."
| Current | Suggested |
|---|---|
| "Multi-region replication is configured in the resource" | "Configure multi-region replication in the resource" |
| "there are two integrations that can be used" | "you can use two integrations" |
| "the primary is safely demoted, writes are all flushed" | "you safely demote the primary and flush all writes" |
| "The old primary is removed from replication" | "The operator removes the old primary" |
MS-3. Non-parallel list construction
"When to perform failover" list mixes noun phrases and imperative verbs:
- Planned maintenance: Region maintenance, infrastructure upgrades... ← noun phrases
- Performance optimization: Move primary closer to write-heavy workload ← imperative
- Testing: Validate disaster recovery procedures ← imperative
Pick one pattern (recommend all noun phrases for consistency).
MS-4. Ambiguous pronouns ("This is…", "that connection")
MS: "Always follow 'this', 'that', 'it' with a noun."
| Current | Suggested |
|---|---|
| "This is a failover where the primary is safely demoted" | "A planned failover safely demotes the primary" |
| "This is a failover where the primary becomes unavailable" | "An unplanned failover occurs when the primary becomes unavailable" |
| "This is needed since the DocumentDB CRD..." | "This ConfigMap is required because the DocumentDB CRD..." |
MS-🟡 Minor Style Issues
MS-5. Inconsistent "operator" / "Operator" capitalization
overview.md Fleet section capitalizes "Operator" mid-sentence. All other instances use lowercase. Lowercase "the operator" is correct in running text.
MS-6. Third person slip in DNS section
"it will be up to the user to coordinate" → Change to second person: "you must coordinate..."
MS-7. Long sentences (MS recommends ≤26 words)
"If Fleet networking is installed on each of your clusters, then instead of populating the connections with default service names, the Operator will create ServiceExports and MultiClusterServices on each Kubernetes cluster then, use those generated cross-regional services to connect the CNPG instances to one another." (46 words)
Split into two sentences.
MS-8. More contractions would improve tone
MS: "Use contractions. They make writing less formal and more approachable." Only 2 contractions in the entire PR. Consider converting some "do not" → "don't", "is not" → "isn't" etc.
MS-9. Step headings mix nouns and verbs
Step 1: Pre-Failover Verification (noun) vs Step 2: Perform Failover (verb). Standardize to verb-first: Step 1: Verify pre-failover conditions.
MS-10. "Key points" list in setup.md mixes descriptions and instructions
First two items describe; last two items instruct. Use parallel construction throughout.
MS-🟢 Nitpicks
MS-11. #### Self Label — jargon heading. Use #### Create the cluster identity ConfigMap.
MS-12. failover-procedures.md "What happens:" numbered lists describe automatic behavior, not user steps. MS recommends bullets for non-sequential items.
MS-13. setup.md inline "Important:" callout should use !!! important admonition for consistency.
Style Guide Compliance Scorecard
| MS Style Rule | Status |
|---|---|
| Present tense | |
| Active voice | |
| Second person ("you") | ✅ Mostly good (1 slip) |
| Short sentences (≤26 words) | |
| Parallel list construction | |
| Contractions | 🟡 Could use more |
| Consistent terminology | |
| Pronouns followed by nouns |
Summary: Solid technical documentation. Priority fixes: (1) credentials pattern and missing directConnection=true in mongosh URIs, (2) "cluster" qualification pass, (3) present-tense conversion for the 10+ "will" instances, (4) split long sentences in DNS/Fleet section, (5) fix checkbox rendering.
Signed-off-by: Alexander Laye <alaye@microsoft.com>
760c581 to
277aa1c
Compare
Signed-off-by: Alexander Laye <alaye@microsoft.com>
f51e693 to
f47ba0c
Compare
|
|
||
| Choose which replica to promote based on: | ||
|
|
||
| - **Replication lag:** Prefer the replica with the lowest lag (most recent data) |
There was a problem hiding this comment.
Do we need to provide any command?
No description provided.