CNTRLPLANE-3724: adding agentic SDLC documentation for etcd - #389
CNTRLPLANE-3724: adding agentic SDLC documentation for etcd#389sandeepknd wants to merge 1 commit into
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
WalkthroughAdds AGENTS.md and ARCHITECTURE.md as repository documentation, updates CLAUDE.md to point to AGENTS.md, and revises CONTRIBUTING.md to surface the new fork-specific reading list and resource links. ChangesRepository documentation and guidance
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
sandeepknd
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@sandeepknd: This pull request references CNTRLPLANE-3724 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
/assign @dusk125 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@AGENTS.md`:
- Around line 117-131: The watch design notes overstate the guarantee by saying
“No events are lost”; update the wording in the AGENTS.md watch section to
describe ordered, resumable delivery instead, and keep the rest of the guidance
consistent with the watcher-related implementation symbols `watchable_store.go`,
`watcher.go`, and `pkg/v3/notify` so readers understand watches can be
compacted, disconnected, or backpressured.
- Around line 81-99: The fenced diagram example is missing a language tag,
causing markdownlint warnings and inconsistent rendering. Update the affected
fenced blocks in AGENTS.md to use an explicit tag such as text, and scan the
rest of the file for the same bare-fence pattern so all examples are tagged
consistently.
In `@ARCHITECTURE.md`:
- Around line 150-173: Update the EtcdServer sketch so it mirrors the live
struct in server.go: replace the non-existent types and fields shown here with
the actual EtcdServer members, and add the missing real ones such as
snapshotter, authStore, alarmStore, and AccessController. Use the current
server/etcdserver/server.go EtcdServer definition as the source of truth, and
keep the field grouping aligned with the real layout so this documentation stays
authoritative.
- Around line 788-805: Qualify the Watch Guarantees section in ARCHITECTURE.md
so it does not read as an absolute promise; update the text under the “Watch
Guarantees” and “Slow Consumer Handling” headings to reflect that ordering,
uniqueness, and resumability only hold within the available history window and
that watches may be canceled on compaction or slow consumption. Keep the wording
aligned with the existing watcher/victim behavior described in the
watch-handling docs and make sure the guarantees are presented as conditional
rather than unconditional.
- Around line 1059-1070: Update the recovery section to make the restore flow
the default path: in the snapshot recovery steps shown near the “Restore from
snapshot on one member” and “Start restored member” examples, keep the `etcdutl
snapshot restore` flow as the primary guidance and replace the `etcd
--force-new-cluster` start step with a stronger note that `--force-new-cluster`
is only a discouraged fallback. Reference the existing recovery examples and
wording around `snapshot restore` and `force-new-cluster` so the new text
clearly steers users to the restore-based path first.
- Around line 56-1428: The fenced code blocks in ARCHITECTURE.md are missing
language annotations, which triggers markdownlint MD040. Update each bare fence
in the document to use the appropriate language tag based on the snippet
content, such as go, bash, protobuf, or text, and ensure the named sections
around the examples (for example, the gRPC service definitions, client/v3
samples, and shell commands) are tagged consistently. If any fence is
intentionally language-agnostic, handle it via an explicit lint exemption
instead of leaving it bare.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6d4c4813-b06a-4b41-9c66-342b7476e084
📥 Commits
Reviewing files that changed from the base of the PR and between bf6c009 and 70889660acd6d0e762d06cb5b5b414c50eeb8e88.
📒 Files selected for processing (4)
AGENTS.mdARCHITECTURE.mdCLAUDE.mdCONTRIBUTING.md
| ``` | ||
| ┌─────────────────────────────────────────┐ | ||
| │ KV Store (Interface) │ | ||
| └─────────────────┬───────────────────────┘ | ||
| │ | ||
| ┌─────────────────▼───────────────────────┐ | ||
| │ MVCC Layer │ | ||
| │ - Revision management │ | ||
| │ - Transaction coordination │ | ||
| │ - Watch event generation │ | ||
| └─────────────────┬───────────────────────┘ | ||
| │ | ||
| ┌─────────────────▼───────────────────────┐ | ||
| │ BoltDB Backend (bbolt) │ | ||
| │ - B+tree storage │ | ||
| │ - ACID transactions │ | ||
| │ - Snapshot support │ | ||
| └─────────────────────────────────────────┘ | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Tag the fenced examples.
These bare fences are already tripping markdownlint, and the same pattern repeats throughout the file. Please add explicit language tags (text, go, protobuf, etc.) so the doc renders consistently and the lint warnings go away.
🛠️ Example fix
- ```
+ ```text📝 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.
| ``` | |
| ┌─────────────────────────────────────────┐ | |
| │ KV Store (Interface) │ | |
| └─────────────────┬───────────────────────┘ | |
| │ | |
| ┌─────────────────▼───────────────────────┐ | |
| │ MVCC Layer │ | |
| │ - Revision management │ | |
| │ - Transaction coordination │ | |
| │ - Watch event generation │ | |
| └─────────────────┬───────────────────────┘ | |
| │ | |
| ┌─────────────────▼───────────────────────┐ | |
| │ BoltDB Backend (bbolt) │ | |
| │ - B+tree storage │ | |
| │ - ACID transactions │ | |
| │ - Snapshot support │ | |
| └─────────────────────────────────────────┘ | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 81-81: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@AGENTS.md` around lines 81 - 99, The fenced diagram example is missing a
language tag, causing markdownlint warnings and inconsistent rendering. Update
the affected fenced blocks in AGENTS.md to use an explicit tag such as text, and
scan the rest of the file for the same bare-fence pattern so all examples are
tagged consistently.
Source: Linters/SAST tools
| │ (Single) │ | ||
| └──────────────┘ | ||
| ``` | ||
|
|
||
| **Use**: Local development, testing | ||
| **Fault Tolerance**: None | ||
| **Performance**: Full read/write speed | ||
|
|
||
| ### Production (3 Nodes) | ||
|
|
||
| ``` | ||
| ┌──────────────┐ ┌──────────────┐ ┌──────────────┐ | ||
| │ etcd-1 │◄────►│ etcd-2 │◄────►│ etcd-3 │ | ||
| │ (Leader) │ │ (Follower) │ │ (Follower) │ | ||
| └──────────────┘ └──────────────┘ └──────────────┘ | ||
| ``` | ||
|
|
||
| **Use**: Small production clusters | ||
| **Fault Tolerance**: 1 node failure | ||
| **Quorum**: 2 nodes | ||
|
|
||
| ### High Availability (5 Nodes) | ||
|
|
||
| ``` | ||
| ┌──────────────┐ ┌──────────────┐ ┌──────────────┐ | ||
| │ etcd-1 │◄────►│ etcd-2 │◄────►│ etcd-3 │ | ||
| │ (Leader) │ │ (Follower) │ │ (Follower) │ | ||
| └──────────────┘ └──────────────┘ └──────────────┘ | ||
| ▲ ▲ | ||
| │ │ | ||
| ▼ ▼ | ||
| ┌──────────────┐ ┌──────────────┐ | ||
| │ etcd-4 │◄──────────────────────────►│ etcd-5 │ | ||
| │ (Follower) │ │ (Follower) │ | ||
| └──────────────┘ └──────────────┘ | ||
| ``` | ||
|
|
||
| **Use**: Large production clusters | ||
| **Fault Tolerance**: 2 node failures | ||
| **Quorum**: 3 nodes | ||
|
|
||
| ### Multi-Region (5 Nodes) | ||
|
|
||
| ``` | ||
| Region 1 Region 2 Region 3 | ||
| ┌──────────┐ ┌──────────┐ ┌──────────┐ | ||
| │ etcd-1 │◄───────►│ etcd-2 │◄───────►│ etcd-3 │ | ||
| │(Follower)│ │ (Leader) │ │(Follower)│ | ||
| └──────────┘ └──────────┘ └──────────┘ | ||
| ▲ | ||
| │ | ||
| ▼ | ||
| Region 1 ┌──────────┐ Region 3 | ||
| ┌──────────┐ │ etcd-4 │ ┌──────────┐ | ||
| │ etcd-5 │◄───────►│(Follower)│◄───────►│ (etc) │ | ||
| │(Follower)│ └──────────┘ │ │ | ||
| └──────────┘ Region 2 └──────────┘ | ||
| ``` | ||
|
|
||
| **Use**: Global availability | ||
| **Considerations**: | ||
| - Higher latency (cross-region) | ||
| - Place majority in low-latency region | ||
| - Consider network costs | ||
|
|
||
| ### Kubernetes/OpenShift | ||
|
|
||
| ``` | ||
| ┌─────────────────────────────────────────────┐ | ||
| │ Kubernetes/OpenShift Cluster │ | ||
| │ │ | ||
| │ ┌────────────────────────────────────────┐ │ | ||
| │ │ Control Plane Nodes │ │ | ||
| │ │ │ │ | ||
| │ │ ┌──────────┐ ┌──────────┐ ┌───────┐│ │ | ||
| │ │ │ etcd-1 │ │ etcd-2 │ │ etcd-3││ │ | ||
| │ │ │(Static │ │(Static │ │(Static││ │ | ||
| │ │ │ Pod) │ │ Pod) │ │ Pod) ││ │ | ||
| │ │ └──────────┘ └──────────┘ └───────┘│ │ | ||
| │ │ ▲ ▲ ▲ │ │ | ||
| │ └───────┼──────────────┼──────────────┼──┘ │ | ||
| │ │ │ │ │ | ||
| │ ┌───────▼──────────────▼──────────────▼──┐ │ | ||
| │ │ kube-apiserver instances │ │ | ||
| │ │ (read/write cluster state to etcd) │ │ | ||
| │ └────────────────────────────────────────┘ │ | ||
| └─────────────────────────────────────────────┘ | ||
| ``` | ||
|
|
||
| **Characteristics**: | ||
| - etcd runs as static pods | ||
| - Co-located with kube-apiserver | ||
| - Dedicated data directory (hostPath) | ||
| - Separate network for peer communication | ||
|
|
||
| --- | ||
|
|
||
| **Document Version**: 1.0 | ||
| **Last Updated**: 2026-06-25 | ||
| **Maintained By**: OpenShift etcd Team |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add language tags to the fenced blocks.
markdownlint is already flagging every bare fence here (MD040). Please mark the snippets with the right language (go, bash, protobuf, text, etc.) or exempt the file if bare fences are intentional.
🧰 Tools
🪛 LanguageTool
[style] ~1293-~1293: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...res defragmentation - Not optimized for very large datasets (>100GB) ### Why gRPC? **Rea...
(EN_WEAK_ADJECTIVE)
🪛 markdownlint-cli2 (0.22.1)
[warning] 56-56: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 107-107: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 120-120: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 129-129: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 211-211: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 243-243: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 252-252: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 331-331: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 345-345: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 384-384: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 396-396: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 471-471: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 477-477: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 484-484: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 506-506: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 570-570: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 730-730: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 811-811: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 1326-1326: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 1339-1339: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 1352-1352: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 1372-1372: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 1396-1396: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@ARCHITECTURE.md` around lines 56 - 1428, The fenced code blocks in
ARCHITECTURE.md are missing language annotations, which triggers markdownlint
MD040. Update each bare fence in the document to use the appropriate language
tag based on the snippet content, such as go, bash, protobuf, or text, and
ensure the named sections around the examples (for example, the gRPC service
definitions, client/v3 samples, and shell commands) are tagged consistently. If
any fence is intentionally language-agnostic, handle it via an explicit lint
exemption instead of leaving it bare.
Source: Linters/SAST tools
| **Key Structures**: | ||
| ```go | ||
| type EtcdServer struct { | ||
| // Raft consensus | ||
| r raftNode | ||
| raftStorage *raft.MemoryStorage | ||
|
|
||
| // Storage | ||
| kv mvcc.ConsistentWatchableKV | ||
| be backend.Backend | ||
|
|
||
| // Cluster state | ||
| cluster api.Cluster | ||
| id types.ID | ||
|
|
||
| // Configuration | ||
| Cfg config.ServerConfig | ||
|
|
||
| // Lease management | ||
| lessor lease.Lessor | ||
|
|
||
| // Apply layer | ||
| applyV3 apply.ApplyV3 | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Make this EtcdServer sketch match server.go.
The current example names types that do not exist in the live struct (mvcc.ConsistentWatchableKV, api.Cluster, apply.ApplyV3) and omits several real fields (snapshotter, authStore, alarmStore, AccessController, etc.). Readers will treat this as authoritative, so it should mirror the actual struct layout. Based on the current server/etcdserver/server.go struct.
🤖 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 `@ARCHITECTURE.md` around lines 150 - 173, Update the EtcdServer sketch so it
mirrors the live struct in server.go: replace the non-existent types and fields
shown here with the actual EtcdServer members, and add the missing real ones
such as snapshotter, authStore, alarmStore, and AccessController. Use the
current server/etcdserver/server.go EtcdServer definition as the source of
truth, and keep the field grouping aligned with the real layout so this
documentation stays authoritative.
| ### Watch Guarantees | ||
|
|
||
| 1. **Ordered**: Events delivered in revision order | ||
| 2. **Reliable**: No events are lost or duplicated | ||
| 3. **Resumable**: Can resume from any revision | ||
| 4. **Atomic**: Transactional puts generate single event | ||
|
|
||
| ### Slow Consumer Handling | ||
|
|
||
| **Problem**: Slow consumer can't keep up with event rate. | ||
|
|
||
| **Solution**: Event buffering with overflow detection. | ||
|
|
||
| **Behavior**: | ||
| - Events buffered in channel (default 1024) | ||
| - If buffer fills, watcher marked as "victim" | ||
| - Victim watchers receive all queued events in one batch | ||
| - Client must process or risk watch cancellation |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Qualify the watch guarantees.
This reads as an absolute guarantee, but etcd only promises ordered/unique/reliable delivery within the available history window, and a slow or compacted watch can still be canceled and must be re-established. (etcd.io)
🤖 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 `@ARCHITECTURE.md` around lines 788 - 805, Qualify the Watch Guarantees section
in ARCHITECTURE.md so it does not read as an absolute promise; update the text
under the “Watch Guarantees” and “Slow Consumer Handling” headings to reflect
that ordering, uniqueness, and resumability only hold within the available
history window and that watches may be canceled on compaction or slow
consumption. Keep the wording aligned with the existing watcher/victim behavior
described in the watch-handling docs and make sure the guarantees are presented
as conditional rather than unconditional.
| **2. Restore from snapshot on one member**: | ||
| ```bash | ||
| etcdutl snapshot restore snapshot.db \ | ||
| --name=member1 \ | ||
| --initial-cluster=member1=http://host1:2380 \ | ||
| --initial-advertise-peer-urls=http://host1:2380 | ||
| ``` | ||
|
|
||
| **3. Start restored member**: | ||
| ```bash | ||
| etcd --force-new-cluster | ||
| ``` |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Use the restore flow as the default recovery path.
The normal recovery path is etcdutl snapshot restore, which creates new data dirs and rewrites member/cluster IDs; --force-new-cluster is only a discouraged fallback and can panic if old members are still alive. (etcd.io)
🤖 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 `@ARCHITECTURE.md` around lines 1059 - 1070, Update the recovery section to
make the restore flow the default path: in the snapshot recovery steps shown
near the “Restore from snapshot on one member” and “Start restored member”
examples, keep the `etcdutl snapshot restore` flow as the primary guidance and
replace the `etcd --force-new-cluster` start step with a stronger note that
`--force-new-cluster` is only a discouraged fallback. Reference the existing
recovery examples and wording around `snapshot restore` and `force-new-cluster`
so the new text clearly steers users to the restore-based path first.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sandeepknd The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
♻️ Duplicate comments (5)
ARCHITECTURE.md (4)
1059-1070: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDon't make
--force-new-clusterthe normal restart step.After
snapshot restore, the restored member should be started normally.--force-new-clusteris only a fallback and makes this recovery path look unsafe by default.🤖 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 `@ARCHITECTURE.md` around lines 1059 - 1070, The recovery steps in the snapshot restore section should not present `--force-new-cluster` as the standard way to start the restored member. Update the instructions around the `snapshot restore` and `etcd` startup steps so the restored member is started normally by default, and mention `--force-new-cluster` only as an exceptional fallback if needed.
150-173: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftMirror the live
EtcdServerlayout.This sketch still doesn't match
server.goand will read as authoritative. Please replace the placeholder types/fields with the real members from the struct definition.🤖 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 `@ARCHITECTURE.md` around lines 150 - 173, The EtcdServer sketch in Architecture documentation is still using placeholder types and fields instead of the real struct layout from EtcdServer in server.go. Update the documented members to match the actual EtcdServer definition exactly, including the correct field names and types for raft, storage, cluster state, configuration, lease, and apply components, so the reference stays authoritative and in sync with the code.
788-805: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winQualify the watch guarantees.
No events are lost or duplicatedis still too strong; watches can be canceled by compaction or slow consumers and must be resumed from a revision.🤖 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 `@ARCHITECTURE.md` around lines 788 - 805, Qualify the Watch Guarantees section in ARCHITECTURE.md so it no longer states unconditional reliability. Update the guarantee text to reflect that watches preserve order and resumability, but may be canceled by compaction or slow consumer handling, requiring clients to resume from a revision; keep the wording aligned with the Watch Guarantees and Slow Consumer Handling sections.
56-102: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winTag the remaining bare fences.
The document still has many unlabeled code/diagram fences, so MD040 will keep firing and the examples will render inconsistently. Please mark them with explicit languages such as
go,bash,protobuf, ortext.Also applies to: 107-134, 211-261, 331-350, 384-402, 471-488, 506-517, 730-754, 780-786, 811-831, 1326-1416
🤖 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 `@ARCHITECTURE.md` around lines 56 - 102, The architecture document still contains bare fenced blocks that trigger MD040 and inconsistent rendering. Update the affected fences around the etcd cluster diagram and the other listed sections by adding an explicit language tag that matches the content, using text for diagrams and the appropriate code language for any actual snippets. Keep the existing content intact and make sure each fence is clearly labeled so the markdown linter stops flagging them.Source: Linters/SAST tools
AGENTS.md (1)
21-30: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winTag the remaining bare fences.
These blocks still trip MD040. Please add explicit language tags (
text,bash, etc.) so the examples render consistently.Also applies to: 69-74, 178-183
🤖 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 `@AGENTS.md` around lines 21 - 30, The documentation examples still contain bare fenced code blocks that trigger MD040. Update the fenced blocks in AGENTS.md under the listed section blocks so each fence has an explicit language tag such as text or bash, and apply the same fix to the other remaining bare fences noted in the comment. Keep the surrounding example content unchanged and ensure the fenced blocks under the storage/client/CLI sections are consistently tagged.Source: Linters/SAST tools
🤖 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.
Duplicate comments:
In `@AGENTS.md`:
- Around line 21-30: The documentation examples still contain bare fenced code
blocks that trigger MD040. Update the fenced blocks in AGENTS.md under the
listed section blocks so each fence has an explicit language tag such as text or
bash, and apply the same fix to the other remaining bare fences noted in the
comment. Keep the surrounding example content unchanged and ensure the fenced
blocks under the storage/client/CLI sections are consistently tagged.
In `@ARCHITECTURE.md`:
- Around line 1059-1070: The recovery steps in the snapshot restore section
should not present `--force-new-cluster` as the standard way to start the
restored member. Update the instructions around the `snapshot restore` and
`etcd` startup steps so the restored member is started normally by default, and
mention `--force-new-cluster` only as an exceptional fallback if needed.
- Around line 150-173: The EtcdServer sketch in Architecture documentation is
still using placeholder types and fields instead of the real struct layout from
EtcdServer in server.go. Update the documented members to match the actual
EtcdServer definition exactly, including the correct field names and types for
raft, storage, cluster state, configuration, lease, and apply components, so the
reference stays authoritative and in sync with the code.
- Around line 788-805: Qualify the Watch Guarantees section in ARCHITECTURE.md
so it no longer states unconditional reliability. Update the guarantee text to
reflect that watches preserve order and resumability, but may be canceled by
compaction or slow consumer handling, requiring clients to resume from a
revision; keep the wording aligned with the Watch Guarantees and Slow Consumer
Handling sections.
- Around line 56-102: The architecture document still contains bare fenced
blocks that trigger MD040 and inconsistent rendering. Update the affected fences
around the etcd cluster diagram and the other listed sections by adding an
explicit language tag that matches the content, using text for diagrams and the
appropriate code language for any actual snippets. Keep the existing content
intact and make sure each fence is clearly labeled so the markdown linter stops
flagging them.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 70734fd5-c535-4179-b634-a516c6b59d78
📥 Commits
Reviewing files that changed from the base of the PR and between 70889660acd6d0e762d06cb5b5b414c50eeb8e88 and 1bcee55.
📒 Files selected for processing (4)
AGENTS.mdARCHITECTURE.mdCLAUDE.mdCONTRIBUTING.md
✅ Files skipped from review due to trivial changes (2)
- CLAUDE.md
- CONTRIBUTING.md
|
@sandeepknd: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
@sandeepknd Sorry just getting around to this but I think a very useful piece that's missing in these context docs is an overview of the more permanent downstream patches that make openshift/etcd different from upstream. E.g the whole workflow around And there's other ones around learner flags/env vars that we've set. The current architecture is largely just an overview of how upstream etcd works. Let me know if you want to add that or I can take over this PR as well if not. |
|
Hi @hasbro17 , |
|
Replaced by #411 |
4 participants
Hi Team,
This PR about adding agentic SDLC documentation for etcd. Kindly find the below details.
AGENTS.md - AI Agent Development Guide
ARCHITECTURE.md - Detailed System Documentation
CONTRIBUTING.md - Updated with Links
Summary by CodeRabbit