Added etcd health and performance analysis commands to openshift plugin #147
Conversation
WalkthroughAdds a new Etcd plugin (manifest, marketplace entry, README, and two command docs: /etcd:health-check and /etcd:analyze-performance), expands and normalizes plugin metadata in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant CLI as Plugin CLI
participant Plugin as Etcd Plugin
participant Etcd as Etcd Cluster
rect rgba(220,235,255,0.6)
Note over Plugin,Etcd: Health check flow
end
User->>CLI: /etcd:health-check [--verbose]
CLI->>Plugin: dispatch health-check
Plugin->>Etcd: run etcdctl health/member list/status
Etcd-->>Plugin: health & member data
Plugin-->>CLI: formatted health report
CLI-->>User: display report
rect rgba(230,245,230,0.6)
Note over Plugin,Etcd: Performance analysis flow
end
User->>CLI: /etcd:analyze-performance [--duration N]
CLI->>Plugin: dispatch analyze-performance
Plugin->>Etcd: collect metrics, logs, leader info (over duration)
Etcd-->>Plugin: metrics & logs
Plugin-->>CLI: analysis + remediation suggestions
CLI-->>User: display performance summary
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (6 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
openshift-ci
bot
added
the
needs-ok-to-test
|
Hi @sandeepknd. Thanks for your PR. I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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 kubernetes-sigs/prow repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
plugins/openshift/commands/etcd-check-health.md (1)
33-33: Wrap bare URLs in markdown link syntax.Lines 33, 450, and 451 contain bare URLs that should be wrapped in markdown link syntax for proper formatting.
Example fix for line 33:
- - Install from: https://mirror.openshift.com/pub/openshift-v4/clients/ocp/ + - Install from: [https://mirror.openshift.com/pub/openshift-v4/clients/ocp/](https://mirror.openshift.com/pub/openshift-v4/clients/ocp/)Also applies to: 450-450, 451-451
plugins/openshift/commands/etcd-analyze-performance.md (2)
36-36: Wrap bare URLs in markdown link syntax.Lines 36, 512, and 513 contain bare URLs that should be wrapped in markdown link syntax for proper formatting and accessibility.
Also applies to: 512-512, 513-513
363-376: Initialize ISSUES and WARNINGS variables earlier in the script.The
ISSUESandWARNINGSvariables are initialized on lines 363–364, but they're only set if the performance report section is reached. For robustness, initialize these at the start of the script (line 82, after prerequisite checks) so they're always defined:echo "Analyzing etcd performance (last $DURATION minutes)..." +ISSUES=0 +WARNINGS=0
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (4)
PLUGINS.md(1 hunks)docs/data.json(1 hunks)plugins/openshift/commands/etcd-analyze-performance.md(1 hunks)plugins/openshift/commands/etcd-check-health.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
plugins/openshift/commands/etcd-check-health.md
10-10: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
33-33: Bare URL used
(MD034, no-bare-urls)
330-330: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
335-335: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
377-377: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
450-450: Bare URL used
(MD034, no-bare-urls)
451-451: Bare URL used
(MD034, no-bare-urls)
plugins/openshift/commands/etcd-analyze-performance.md
21-21: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🔇 Additional comments (2)
docs/data.json (1)
497-508: Registry entries for new etcd commands are properly formatted.The two new command entries follow the existing JSON structure and naming conventions. No issues detected.
PLUGINS.md (1)
148-149: New OpenShift plugin commands are properly registered.Both
etcd-analyze-performanceandetcd-check-healthare correctly positioned in the OpenShift plugin section with descriptions and proper formatting that matches existing entries.
|
/ok-to-test |
openshift-ci
bot
added
ok-to-test
|
Should there be an "etcd"-specific plugin? |
This is related to health check with respect to etcd (which is under Openshift only) |
|
Hi @stbenjam , |
sandeepknd
force-pushed
the
etcd-health-perf
branch
from
fdc25c2 to
667d950
Compare
|
[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.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@stbenjam , |
sandeepknd
force-pushed
the
etcd-health-perf
branch
from
667d950 to
1856045
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
plugins/etcd/commands/health-check.md (2)
10-10: Fix Markdown linting issues: language specifiers and bare URLs.Code blocks lack language specifiers and URLs are bare:
- Lines 10, 330, 335, 377: Code blocks should use
```bashinstead of```- Lines 33, 450-451: Bare URLs should be wrapped in Markdown link syntax
Example fixes:
-``` +```bash -https://mirror.openshift.com/pub/openshift-v4/clients/ocp/ +[OpenShift CLI](https://mirror.openshift.com/pub/openshift-v4/clients/ocp/)Also applies to: 33-33, 330-330, 335-335, 377-377, 450-451
56-312: Fix critical issues: uninitialized variables and incorrect exit code logic.The implementation script has multiple correctness issues that will cause failures:
Issue 1 (Line 305): Uninitialized
$WARNINGSvariableLine 305 references
$WARNINGSbut it is never initialized or incremented. The script checks for warnings at lines 155, 161, 236 but only prints messages without updating a counter.Issue 2 (Line 265): Uninitialized
$VERBOSEvariableLine 265 checks
if [ "$VERBOSE" = "true" ]but there is no code to parse the--verboseflag from command arguments.Issue 3 (Lines 307, 310): Exit codes always zero
Both warning and error paths exit with code 0, preventing proper status reporting. Critical issues should exit with non-zero code.
Proposed fix:
+# Initialize variables +WARNINGS=0 +VERBOSE=false + +# Parse arguments +for arg in "$@"; do + if [ "$arg" = "--verbose" ]; then + VERBOSE=true + fi +done # Later, increment counter when warnings detected: -if [ "$UNSTARTED" -gt 0 ]; then - echo "WARNING: $UNSTARTED member(s) in unstarted state" -fi +if [ "$UNSTARTED" -gt 0 ]; then + echo "WARNING: $UNSTARTED member(s) in unstarted state" + WARNINGS=$((WARNINGS + 1)) +fi # And fix exit codes: -if [ "$WARNINGS" -gt 0 ]; then - echo "Status: WARNING - Found $WARNINGS warnings requiring attention" - exit 0 -else - echo "Status: HEALTHY - All checks passed" - exit 0 -fi +if [ "$WARNINGS" -gt 0 ]; then + echo "Status: WARNING - Found $WARNINGS warnings requiring attention" + exit 2 +else + echo "Status: HEALTHY - All checks passed" + exit 0 +fiplugins/etcd/commands/analyze-performance.md (1)
10-10: Fix Markdown linting issues: language specifiers and bare URLs.Code blocks lack language specifiers and URLs are bare throughout the file:
- Lines 10, 375, 380, 477: Code blocks should use
```bashinstead of```- Lines 36, 579-580: Bare URLs should be wrapped in Markdown links
Example fixes:
-``` +```bash -https://mirror.openshift.com/pub/openshift-v4/clients/ocp/ +[OpenShift CLI](https://mirror.openshift.com/pub/openshift-v4/clients/ocp/)Also applies to: 36-36, 375-375, 380-380, 477-477, 579-580
🧹 Nitpick comments (1)
plugins/etcd/README.md (1)
23-23: Fix Markdown linting issues: language specifiers and bare URLs.Several code blocks lack language specifiers and bare URLs are used without Markdown link wrapping:
- Code blocks at lines 23, 28, 44, 49 should use
```bashinstead of```- Bare URLs at lines 58, 157-159, 164 should be wrapped in Markdown link syntax:
[text](url)Example fix for line 23:
-``` +```bashExample fix for line 58:
-Install from https://mirror.openshift.com/pub/openshift-v4/clients/ocp/ +Install from [OpenShift CLI](https://mirror.openshift.com/pub/openshift-v4/clients/ocp/)Also applies to: 28-28, 44-44, 49-49, 58-58, 157-159, 164-164
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (7)
.claude-plugin/marketplace.json(1 hunks)PLUGINS.md(2 hunks)docs/data.json(1 hunks)plugins/etcd/.claude-plugin/plugin.json(1 hunks)plugins/etcd/README.md(1 hunks)plugins/etcd/commands/analyze-performance.md(1 hunks)plugins/etcd/commands/health-check.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- plugins/etcd/.claude-plugin/plugin.json
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
plugins/etcd/commands/analyze-performance.md
10-10: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
36-36: Bare URL used
(MD034, no-bare-urls)
375-375: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
380-380: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
477-477: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
579-579: Bare URL used
(MD034, no-bare-urls)
580-580: Bare URL used
(MD034, no-bare-urls)
plugins/etcd/README.md
23-23: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
28-28: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
44-44: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
49-49: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
58-58: Bare URL used
(MD034, no-bare-urls)
157-157: Bare URL used
(MD034, no-bare-urls)
158-158: Bare URL used
(MD034, no-bare-urls)
159-159: Bare URL used
(MD034, no-bare-urls)
164-164: Bare URL used
(MD034, no-bare-urls)
plugins/etcd/commands/health-check.md
10-10: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
33-33: Bare URL used
(MD034, no-bare-urls)
330-330: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
335-335: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
377-377: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
450-450: Bare URL used
(MD034, no-bare-urls)
451-451: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (5)
PLUGINS.md (1)
9-9: Etcd plugin properly integrated into documentation index.The plugin entry is correctly ordered alphabetically in the table of contents, and the plugin section follows the established format with clear command documentation. Links to the detailed README are in place.
Also applies to: 65-73
.claude-plugin/marketplace.json (1)
67-71: Etcd plugin correctly registered in marketplace.The plugin entry is properly positioned between openshift and yaml entries with correct JSON structure, appropriate source path, and description that matches the registry entries.
plugins/etcd/README.md (1)
1-167: Comprehensive plugin documentation with excellent structure and guidance.The README provides clear prerequisites, installation steps, use cases, and troubleshooting guidance. The common issues section is particularly useful with actionable remediation steps. Security considerations and performance benchmarks are appropriately included.
docs/data.json (1)
525-545: Etcd plugin entries correctly registered in data catalog.The two command entries (analyze-performance and health-check) are properly structured with correct synopsis, argument hints, and metadata. The format follows the established pattern used by the openshift plugin for complex argument specifications. The version, skills array, and has_readme flag are all appropriate.
plugins/etcd/commands/analyze-performance.md (1)
293-293: Portability concern is valid but less critical than severity indicates; code already includes fallback handling.The code at lines 293, 298, and 331 does use
bcfor arithmetic. However, the analysis shows:
Fallback already present: Line 293 already includes
2>/dev/null || echo 0, which gracefully handles missingbc.Alpine includes bc by default: Alpine's official Docker image includes a bc applet via BusyBox by default. Since etcd typically runs on Alpine-based containers,
bcavailability is less of a concern than the review suggests.Awk alternatives are valid: The suggested awk replacements work correctly in testing, but are optional improvements rather than mandatory fixes given the existing error handling.
Recommendation: The code is currently resilient to missing
bc. If maximum portability is desired, consider replacing thebccalls with awk as suggested, or document thebcdependency. However, verify your specific container environment first—if you're using standard Alpine-based etcd images, no changes are required.
| # Parse duration argument (default: 5 minutes) | ||
| DURATION=5 | ||
| if [[ "$1" == "--duration" ]] && [[ -n "$2" ]]; then | ||
| DURATION=$2 | ||
| fi |
There was a problem hiding this comment.
Fix argument parsing to properly handle --duration flag.
The current implementation expects positional arguments ($1 and $2) but the command signature in documentation specifies the --duration <minutes> flag syntax. The parsing logic should handle both forms or consistently use one approach.
Current code at lines 75-79:
DURATION=5
if [[ "$1" == "--duration" ]] && [[ -n "$2" ]]; then
DURATION=$2
fiThis assumes --duration and the value are separate positional args (e.g., --duration 15), which works but is fragile. Consider using a more robust argument parser or documenting the exact syntax users must follow.
🤖 Prompt for AI Agents
In plugins/etcd/commands/analyze-performance.md around lines 75 to 79, the
duration flag parsing assumes positional args ($1/$2) and fails for combined
forms or different arg orders; update parsing to robustly handle both
“--duration <minutes>” and “--duration=<minutes>” (or document that only one
form is supported). Implement a simple loop over "$@" to detect --duration and
--duration=<value>, validate the value is numeric, and set DURATION accordingly
(falling back to default 5 if absent); ensure other flags are ignored and that
parsing supports args in any order.
1856045 to
9da6fe7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
plugins/etcd/commands/analyze-performance.md (1)
10-10: Add language specifier to code blocks for syntax highlighting.Markdown code blocks throughout the file lack language specifiers. Update all bash/shell code blocks to use
```bashinstead of plain backticks. Affected lines include: 10, 64, 102, 148, 161, 230, 263, 284, 384, 389, 425, and 430.Also applies to: 64-64, 102-102, 148-148, 161-161, 230-230, 263-263, 284-284, 384-384, 389-389, 425-425, 430-430
🧹 Nitpick comments (3)
plugins/etcd/commands/health-check.md (1)
10-10: Add language specifier to code blocks for syntax highlighting.Multiple code blocks throughout the file use plain backticks instead of ```bash. This affects lines 10, 64, 80, 110, 123, 139, 169, 194, 224, 246, 264, 290, 330, 335, 384, and 389 (and similar locations). Update all bash/shell code blocks to use the bash language specifier for proper syntax highlighting.
Also applies to: 64-64, 80-80, 110-110, 123-123, 139-139, 169-169, 194-194, 224-224, 246-246, 264-264, 290-290, 330-330, 335-335, 384-384, 389-389
plugins/etcd/commands/analyze-performance.md (2)
75-79: Improve argument parsing to handle multiple--durationformats.The current implementation assumes
--durationand its value are consecutive positional arguments ($1and$2), which is fragile. It doesn't handle:
- Combined form:
--duration=15- Value validation (no check that
$2is numeric)- Flags in different positions
Consider a more robust approach using a loop over
$@:DURATION=5 while [[ $# -gt 0 ]]; do case $1 in --duration) if [[ -n "$2" ]] && [[ "$2" =~ ^[0-9]+$ ]]; then DURATION="$2" shift 2 else echo "Error: --duration requires a numeric value" exit 1 fi ;; --duration=*) DURATION="${1#*=}" if ! [[ "$DURATION" =~ ^[0-9]+$ ]]; then echo "Error: --duration requires a numeric value" exit 1 fi shift ;; *) shift ;; esac done
358-367: Clarify exit code behavior for WARNING status.Both "HEALTHY" and "WARNING" states return exit code 0. This may be intentional (warnings are non-fatal), but clarify in documentation whether callers should treat exit 0 as "all clear" when warnings are present. If warnings should trigger failure for scripting purposes, consider returning exit 2 for the WARNING branch:
else echo "Status: ⚠ WARNING - Found $WARNINGS performance warnings" - exit 0 + exit 2 fiDocument the exit code semantics clearly so users understand that exit 0 includes both healthy and warning states.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (7)
.claude-plugin/marketplace.json(1 hunks)PLUGINS.md(2 hunks)docs/data.json(1 hunks)plugins/etcd/.claude-plugin/plugin.json(1 hunks)plugins/etcd/README.md(1 hunks)plugins/etcd/commands/analyze-performance.md(1 hunks)plugins/etcd/commands/health-check.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- plugins/etcd/.claude-plugin/plugin.json
🚧 Files skipped from review as they are similar to previous changes (1)
- PLUGINS.md
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
plugins/etcd/commands/health-check.md
10-10: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
36-36: Bare URL used
(MD034, no-bare-urls)
384-384: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
389-389: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (3)
.claude-plugin/marketplace.json (1)
67-71: Well-structured plugin registry entry.The marketplace entry is properly formatted and follows the established plugin structure. The entry is well-positioned and the description accurately reflects the plugin's functionality.
plugins/etcd/README.md (1)
1-170: Clear, comprehensive plugin documentation.The README provides excellent coverage of prerequisites, use cases, common issues with solutions, and security considerations. The performance benchmarks and resource links are particularly valuable for users troubleshooting etcd problems.
docs/data.json (1)
525-545: No duplicates found—etcd entry is properly registered.Verification confirms exactly one etcd plugin entry exists in docs/data.json with correct structure and complete metadata. The AI summary's claim of duplicate entries was incorrect.
| echo "Total members: $MEMBER_COUNT" | ||
|
|
||
| if [ "$MEMBER_COUNT" -lt 3 ]; then | ||
| echo "WARNING: Etcd cluster has less than 3 members (quorum at risk)" |
There was a problem hiding this comment.
Fix uninitialized WARNINGS variable and inconsistent exit codes.
The script references an undefined $WARNINGS variable in the summary section (line 305) that is never initialized or incremented throughout the script. Additionally, both the "WARNING" branch (line 307) and "HEALTHY" branch (line 310) return exit code 0, so the exit status doesn't distinguish between warnings and a truly healthy state.
Initialize WARNINGS=0 at the script start and increment it at each warning condition (lines 155, 161, 236, and others). Then update the exit logic to return a non-zero code when warnings are present:
+WARNINGS=0
+
# Check for unstarted members
UNSTARTED=$(echo "$MEMBER_LIST" | grep "unstarted" | wc -l)
if [ "$UNSTARTED" -gt 0 ]; then
echo "WARNING: $UNSTARTED member(s) in unstarted state"
+ WARNINGS=$((WARNINGS + 1))
fiAnd update the summary logic to properly exit with a non-zero code for warnings:
if [ "$WARNINGS" -gt 0 ]; then
echo "Status: WARNING - Found $WARNINGS warnings requiring attention"
- exit 0
+ exit 2
else
echo "Status: HEALTHY - All checks passed"
exit 0
fiAlso applies to: 161-161, 236-236, 305-311
🤖 Prompt for AI Agents
In plugins/etcd/commands/health-check.md around lines 155, 161, 236 and the
summary block at 305-311, the script uses an uninitialized $WARNINGS variable
and returns exit code 0 for both warning and healthy branches; initialize
WARNINGS=0 near the script start, increment WARNINGS at each warning condition
(including the echo at line 155, similar places at 161 and 236), and modify the
summary/exit logic so that if WARNINGS>0 the script prints the warning summary
and exits non‑zero (e.g., exit 1), otherwise print healthy and exit 0, ensuring
the printed summary includes the warnings count.
ddfeb16 to
029f25b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
plugins/etcd/commands/analyze-performance.md (1)
75-79: Fix argument parsing to robustly handle the--durationflag.The current implementation only handles the positional form (
--duration 15) and doesn't account for combined syntax (--duration=15), other flags, or validation that the duration is numeric. This makes the parser fragile and error-prone.Implement a loop-based argument parser that:
- Handles both
--duration <minutes>and--duration=<minutes>forms- Validates the duration value is a positive integer
- Ignores other flags gracefully
- Preserves the default value if absent
+ # Parse duration argument (default: 5 minutes) + DURATION=5 + while [[ $# -gt 0 ]]; do + case "$1" in + --duration) + if [[ -n "$2" && "$2" =~ ^[0-9]+$ ]]; then + DURATION="$2" + shift 2 + else + echo "Error: --duration requires a positive integer argument" + exit 1 + fi + ;; + --duration=*) + DURATION="${1#--duration=}" + if ! [[ "$DURATION" =~ ^[0-9]+$ ]]; then + echo "Error: --duration requires a positive integer argument" + exit 1 + fi + shift + ;; + *) + shift + ;; + esac + doneThis replaces lines 75–79 with a more robust parser that validates input and handles both flag formats.
🧹 Nitpick comments (4)
plugins/etcd/README.md (1)
23-25: Add language specifiers to code blocks and wrap bare URLs.Markdown linting flagged multiple code blocks missing language specifiers and bare URLs that should be wrapped in angle brackets or converted to markdown links.
For code blocks, change
```to```bashor other appropriate language (lines 23–25, 28–31, 44–46, 49–51, 67–73, 77–82). For bare URLs (lines 58, 157–159, 164), wrap in angle brackets or convert to markdown links for proper rendering.Also applies to: 28-31, 44-46, 49-51, 67-73, 77-82, 157-165
plugins/etcd/commands/analyze-performance.md (1)
10-10: Add language specifiers to code blocks and wrap bare URLs.Markdown linting flagged code blocks without language specifiers and bare URLs. Change
```to```bashfor shell snippets (lines 10, 384, 389, 486). Wrap bare URLs in angle brackets or convert to markdown links (lines 36, 588–589).Also applies to: 36-36, 384-384, 389-389, 486-486, 588-589
plugins/etcd/commands/health-check.md (2)
22-22: Fix heading level hierarchy.Line 22 uses heading level h3 (
### 5. Check Etcd Member List) but should increment from h2. Ensure all section headings follow proper nesting (start with h2 and increment by one level at a time).
10-10: Add language specifiers to code blocks and wrap bare URLs.Markdown linting flagged code blocks without language specifiers and bare URLs. Change
```to```bashfor shell snippets (lines 10, 330, 335, 377). Wrap bare URLs in angle brackets or convert to markdown links (lines 450–451).Also applies to: 330-335, 377-377, 450-451
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (7)
.claude-plugin/marketplace.json(1 hunks)PLUGINS.md(2 hunks)docs/data.json(1 hunks)plugins/etcd/.claude-plugin/plugin.json(1 hunks)plugins/etcd/README.md(1 hunks)plugins/etcd/commands/analyze-performance.md(1 hunks)plugins/etcd/commands/health-check.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- PLUGINS.md
- plugins/etcd/.claude-plugin/plugin.json
- .claude-plugin/marketplace.json
- docs/data.json
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
plugins/etcd/commands/analyze-performance.md
10-10: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
36-36: Bare URL used
(MD034, no-bare-urls)
384-384: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
389-389: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
486-486: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
588-588: Bare URL used
(MD034, no-bare-urls)
589-589: Bare URL used
(MD034, no-bare-urls)
plugins/etcd/README.md
23-23: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
28-28: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
44-44: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
49-49: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
58-58: Bare URL used
(MD034, no-bare-urls)
157-157: Bare URL used
(MD034, no-bare-urls)
158-158: Bare URL used
(MD034, no-bare-urls)
159-159: Bare URL used
(MD034, no-bare-urls)
164-164: Bare URL used
(MD034, no-bare-urls)
plugins/etcd/commands/health-check.md
22-22: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
sandeepknd
force-pushed
the
etcd-health-perf
branch
from
029f25b to
181ba12
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
plugins/etcd/commands/analyze-performance.md (1)
75-79: Argument parsing should handle both--duration <value>and--duration=<value>forms.The current implementation only supports positional arguments and silently ignores combined forms like
--duration=15, leading to the default 5-minute duration being used unexpectedly.Consider using a more robust parsing loop:
DURATION=5 -if [[ "$1" == "--duration" ]] && [[ -n "$2" ]]; then - DURATION=$2 -fi + +while [[ $# -gt 0 ]]; do + case "$1" in + --duration) + DURATION="${2:-5}" + shift 2 + ;; + --duration=*) + DURATION="${1#*=}" + shift + ;; + *) + shift + ;; + esac +done + +# Validate DURATION is numeric +if ! [[ "$DURATION" =~ ^[0-9]+$ ]]; then + echo "Error: --duration must be a positive integer" + exit 1 +fiThis handles both
--duration 15and--duration=15, validates the input, and supports flags in any order.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (7)
.claude-plugin/marketplace.json(1 hunks)PLUGINS.md(2 hunks)docs/data.json(1 hunks)plugins/etcd/.claude-plugin/plugin.json(1 hunks)plugins/etcd/README.md(1 hunks)plugins/etcd/commands/analyze-performance.md(1 hunks)plugins/etcd/commands/health-check.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/data.json
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
plugins/etcd/commands/health-check.md
22-22: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
plugins/etcd/README.md
23-23: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
28-28: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
44-44: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
49-49: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
58-58: Bare URL used
(MD034, no-bare-urls)
157-157: Bare URL used
(MD034, no-bare-urls)
158-158: Bare URL used
(MD034, no-bare-urls)
159-159: Bare URL used
(MD034, no-bare-urls)
164-164: Bare URL used
(MD034, no-bare-urls)
plugins/etcd/commands/analyze-performance.md
10-10: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
36-36: Bare URL used
(MD034, no-bare-urls)
384-384: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
389-389: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
486-486: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
588-588: Bare URL used
(MD034, no-bare-urls)
589-589: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (4)
plugins/etcd/.claude-plugin/plugin.json (1)
1-8: Plugin metadata looks good.Simple, clean plugin descriptor. No issues.
PLUGINS.md (1)
65-73: Documentation entry is clear and consistent.The etcd plugin entry follows the established format, clearly describes the two commands, and properly links to detailed documentation.
.claude-plugin/marketplace.json (1)
67-71: Plugin registry entry is well-formed.Consistent with existing marketplace entries, properly configured source path, and descriptive name.
plugins/etcd/README.md (1)
1-170: Comprehensive and well-structured README.Excellent user-facing documentation that clearly explains the plugin's purpose, documents both commands with examples, and provides troubleshooting guidance. The performance benchmarks are helpful for understanding etcd health thresholds.
sandeepknd
force-pushed
the
etcd-health-perf
branch
from
181ba12 to
8a183de
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
plugins/etcd/commands/health-check.md (2)
10-10: Add language specifiers to bash code blocks for syntax highlighting.Markdown linting flags missing language specifiers. Change
```to```bashat lines 10, 330, 335, and 377.Also applies to: 330-330, 335-335, 377-377
60-74: Initialize and parse WARNINGS and VERBOSE variables to enable --verbose flag.The script references
$WARNINGS(line 305) and$VERBOSE(line 265) variables that are never initialized or parsed from arguments. The documented--verboseflag is non-functional, and warning tracking does not work.Apply this fix to initialize variables and parse arguments:
if ! oc whoami &> /dev/null; then echo "Error: Not connected to an OpenShift cluster." exit 1 fi + +# Initialize variables and parse arguments +WARNINGS=0 +VERBOSE=false + +while [[ $# -gt 0 ]]; do + case "$1" in + --verbose|-v) + VERBOSE=true + shift + ;; + *) + shift + ;; + esac +doneThen increment
WARNINGSat each warning condition:if [ "$MEMBER_COUNT" -lt 3 ]; then echo "WARNING: Etcd cluster has less than 3 members (quorum at risk)" + WARNINGS=$((WARNINGS + 1)) fiif [ "$UNSTARTED" -gt 0 ]; then echo "WARNING: $UNSTARTED member(s) in unstarted state" + WARNINGS=$((WARNINGS + 1)) fiif [ "$DISK_USAGE" -gt 80 ]; then echo "WARNING: Disk usage on $pod is ${DISK_USAGE}% (threshold: 80%)" + WARNINGS=$((WARNINGS + 1)) fiFinally, fix the exit logic to properly signal status:
if [ "$WARNINGS" -gt 0 ]; then echo "Status: WARNING - Found $WARNINGS warnings requiring attention" - exit 0 + exit 1 else echo "Status: ✓ HEALTHY - All checks passed" exit 0 fiBased on learnings from past review comments, these were flagged as critical in previous iterations.
plugins/etcd/commands/analyze-performance.md (1)
10-10: Add language specifiers to code blocks and wrap bare URLs.Markdown linting flags:
- Missing language specifiers: change
```to```bashat lines 10, 384, 389, 486- Bare URLs: wrap in angle brackets
<https://...>at lines 588–589Also applies to: 384-384, 389-389, 486-486, 588-589
🧹 Nitpick comments (2)
plugins/etcd/README.md (1)
23-23: Address markdown linting issues: language specifiers and bare URLs.The markdownlint tool flags missing language specifiers on code blocks and bare URLs. While not breaking, these should be fixed for consistency.
Suggested fixes:
- Add language specifiers to code blocks: change
```to```bashfor shell examples (lines 23, 28, 44, 49)- Wrap bare URLs in angle brackets:
<https://...>for lines 58, 157–159, 164Also applies to: 28-28, 44-44, 49-49, 58-58, 157-159, 164-164
plugins/etcd/commands/analyze-performance.md (1)
75-79: Improve argument parsing to handle both--duration <minutes>and--duration=<minutes>formats.The current parsing only handles positional arguments and is fragile. Users might naturally try
--duration=15or pass arguments in different orders. A more robust parser would improve usability.Apply this more robust argument parsing:
# Parse duration argument (default: 5 minutes) DURATION=5 -if [[ "$1" == "--duration" ]] && [[ -n "$2" ]]; then - DURATION=$2 -fi + +while [[ $# -gt 0 ]]; do + case "$1" in + --duration) + if [[ -n "$2" && "$2" =~ ^[0-9]+$ ]]; then + DURATION="$2" + shift 2 + else + echo "Error: --duration requires a numeric value" + exit 1 + fi + ;; + --duration=*) + DURATION="${1#*=}" + if ! [[ "$DURATION" =~ ^[0-9]+$ ]]; then + echo "Error: --duration requires a numeric value" + exit 1 + fi + shift + ;; + *) + shift + ;; + esac +doneThis handles
--duration 15,--duration=15, and validates numeric input.Based on learnings from past review comments, this was flagged as a major concern in previous iterations.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (8)
.claude-plugin/marketplace.json(1 hunks).github/workflows/lint-plugins.yml(1 hunks)PLUGINS.md(2 hunks)docs/data.json(1 hunks)plugins/etcd/.claude-plugin/plugin.json(1 hunks)plugins/etcd/README.md(1 hunks)plugins/etcd/commands/analyze-performance.md(1 hunks)plugins/etcd/commands/health-check.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- PLUGINS.md
- plugins/etcd/.claude-plugin/plugin.json
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
plugins/etcd/README.md
23-23: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
28-28: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
44-44: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
49-49: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
58-58: Bare URL used
(MD034, no-bare-urls)
157-157: Bare URL used
(MD034, no-bare-urls)
158-158: Bare URL used
(MD034, no-bare-urls)
159-159: Bare URL used
(MD034, no-bare-urls)
164-164: Bare URL used
(MD034, no-bare-urls)
plugins/etcd/commands/analyze-performance.md
10-10: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
36-36: Bare URL used
(MD034, no-bare-urls)
384-384: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
389-389: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
486-486: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
588-588: Bare URL used
(MD034, no-bare-urls)
589-589: Bare URL used
(MD034, no-bare-urls)
plugins/etcd/commands/health-check.md
10-10: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
33-33: Bare URL used
(MD034, no-bare-urls)
330-330: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
335-335: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
377-377: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
450-450: Bare URL used
(MD034, no-bare-urls)
451-451: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (6)
.github/workflows/lint-plugins.yml (2)
20-24: Debug step improves troubleshooting without blocking workflow.The debug step conditionally displays workspace contents for troubleshooting. The
|| trueclauses ensure the workflow doesn't fail if directories are missing.
29-29: Correct quoting protects against paths with spaces.Quoting the volume mount path ensures proper handling of workspace paths containing spaces or special characters.
.claude-plugin/marketplace.json (1)
67-71: Plugin marketplace entry is well-formed.The etcd plugin entry follows the marketplace structure with correct namespace, source path, and description.
plugins/etcd/README.md (1)
1-171: README documentation is comprehensive and well-organized.The plugin documentation covers prerequisites, commands, installation, use cases, troubleshooting, security, and resources. Content is clear and actionable.
docs/data.json (2)
525-545: Plugin registry entry is correctly structured.The etcd plugin entry follows the registry format with valid metadata, command definitions, and alignment with command documentation files.
1-634: Ignore this review comment—the file contains exactly one etcd entry, not duplicates.The grep verification confirms there is only one
"name": "etcd"entry indocs/data.json. The premise of the review comment (that duplicate etcd entries exist) is unsupported by evidence.Likely an incorrect or invalid review comment.
| if [ "$VERBOSE" = "true" ]; then | ||
| echo "" | ||
| echo "Checking etcd performance metrics..." | ||
|
|
||
| # Get metrics from etcd pod | ||
| METRICS=$(oc exec -n openshift-etcd "$ETCD_POD" -c etcd -- curl -s http://localhost:2379/metrics 2>/dev/null) | ||
|
|
||
| # Parse key metrics | ||
| echo "Backend Commit Duration (p99):" | ||
| echo "$METRICS" | grep "etcd_disk_backend_commit_duration_seconds" | grep "quantile=\"0.99\"" | head -1 | ||
|
|
||
| echo "" | ||
| echo "WAL Fsync Duration (p99):" | ||
| echo "$METRICS" | grep "etcd_disk_wal_fsync_duration_seconds" | grep "quantile=\"0.99\"" | head -1 | ||
|
|
||
| echo "" | ||
| echo "Leader Changes:" | ||
| echo "$METRICS" | grep "etcd_server_leader_changes_seen_total" | head -1 | ||
| fi |
There was a problem hiding this comment.
Verbose metrics block will never execute without VERBOSE initialization fix.
The conditional block at line 265 checks if [ "$VERBOSE" = "true" ], but the variable is never initialized. Apply the initialization fix above to enable this functionality.
🤖 Prompt for AI Agents
In plugins/etcd/commands/health-check.md around lines 265 to 283, the VERBOSE
conditional will never run because VERBOSE is never initialized; initialize
VERBOSE earlier (above this block) by setting a default and/or parsing a verbose
flag (e.g., set VERBOSE from an environment variable or CLI option and default
it to "false" when unset), or add simple initialization like
VERBOSE=${VERBOSE:-false} so the conditional can evaluate to "true" when
intended; ensure any flag parsing sets VERBOSE="true" when the user requests
verbose output.
| if [ "$WARNINGS" -gt 0 ]; then | ||
| echo "Status: WARNING - Found $WARNINGS warnings requiring attention" | ||
| exit 0 | ||
| else | ||
| echo "Status: HEALTHY - All checks passed" | ||
| exit 0 | ||
| fi |
There was a problem hiding this comment.
Fix exit code logic to distinguish warnings from healthy status.
Both the WARNING and HEALTHY branches return exit 0, preventing proper status signaling. Update the exit logic (as shown in the above diff) to return exit 1 for warnings so downstream automation can properly detect and act on warnings.
🤖 Prompt for AI Agents
In plugins/etcd/commands/health-check.md around lines 305 to 311, the script
currently exits with 0 for both WARNING and HEALTHY branches; change the WARNING
branch to exit 1 so warnings produce a non-zero exit code for downstream
automation to detect, leaving the HEALTHY branch as exit 0 and preserving the
echo messages.
8a183de to
7281978
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
plugins/etcd/commands/analyze-performance.md (1)
10-10: Add language specifiers to code blocks for syntax highlighting.Multiple Markdown fenced code blocks lack language specifiers. Update plain
```to```bashfor all shell/bash code blocks throughout the file.Also applies to: 64-82, 384-389, 486-488
plugins/etcd/commands/health-check.md (1)
10-10: Add language specifiers to code blocks.Multiple Markdown fenced code blocks lack language specifiers. Update plain
```to```bashfor shell code blocks.Also applies to: 330-330, 335-335, 377-377
🧹 Nitpick comments (3)
plugins/etcd/commands/analyze-performance.md (2)
36-36: Wrap bare URLs in Markdown links.Bare URLs should be wrapped in Markdown link syntax for proper rendering. Example:
[etcd tuning guide](https://etcd.io/docs/latest/tuning/)Also applies to: 588-589
75-79: Improve--durationargument parsing to handle both flag formats.The current implementation assumes positional arguments (
--durationat$1and value at$2) but does not handle the combined form--duration=<minutes>or args in different order. The documentation specifies[--duration <minutes>], which implies flexibility.Implement robust argument parsing using a loop over
"$@"to detect both--duration <value>and--duration=<value>, validate the value is numeric, and maintain backward compatibility:DURATION=5 while [[ $# -gt 0 ]]; do case "$1" in --duration) DURATION="$2" shift 2 ;; --duration=*) DURATION="${1#*=}" shift ;; *) shift ;; esac done # Validate DURATION is numeric if ! [[ "$DURATION" =~ ^[0-9]+$ ]]; then echo "Error: Duration must be a positive integer (in minutes)" exit 1 fiplugins/etcd/commands/health-check.md (1)
33-33: Wrap bare URLs in Markdown links.Bare URLs should be wrapped in Markdown link syntax. Example:
[etcd documentation](https://etcd.io/docs/)Also applies to: 450-451
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (8)
.claude-plugin/marketplace.json(1 hunks).github/workflows/lint-plugins.yml(1 hunks)PLUGINS.md(2 hunks)docs/data.json(1 hunks)plugins/etcd/.claude-plugin/plugin.json(1 hunks)plugins/etcd/README.md(1 hunks)plugins/etcd/commands/analyze-performance.md(1 hunks)plugins/etcd/commands/health-check.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/data.json
- .github/workflows/lint-plugins.yml
- plugins/etcd/.claude-plugin/plugin.json
- plugins/etcd/README.md
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
plugins/etcd/commands/health-check.md
10-10: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
33-33: Bare URL used
(MD034, no-bare-urls)
330-330: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
335-335: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
377-377: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
450-450: Bare URL used
(MD034, no-bare-urls)
451-451: Bare URL used
(MD034, no-bare-urls)
plugins/etcd/commands/analyze-performance.md
10-10: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
36-36: Bare URL used
(MD034, no-bare-urls)
384-384: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
389-389: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
486-486: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
588-588: Bare URL used
(MD034, no-bare-urls)
589-589: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (2)
PLUGINS.md (1)
9-9: Clean addition to plugin index and documentation.The Etcd plugin section is properly integrated with consistent formatting, clear command descriptions, and correct cross-references to the README.
Also applies to: 65-73
.claude-plugin/marketplace.json (1)
67-71: Etcd plugin marketplace entry looks good.Properly formatted and consistent with other plugin entries.
| - **--verbose** (optional): Enable detailed output | ||
| - Shows etcd member details | ||
| - Displays performance metrics | ||
| - Includes log snippets for errors | ||
| - Provides additional diagnostic information |
There was a problem hiding this comment.
Initialize VERBOSE variable and implement --verbose flag parsing; fix exit code logic.
The script references $VERBOSE at line 265 but never initializes it or parses the --verbose flag documented at lines 50-54. Additionally, both the WARNING and HEALTHY branches return exit 0, preventing downstream automation from detecting warnings.
Apply this comprehensive fix:
# Add after the oc connection check (after line 72)
WARNINGS=0
VERBOSE=false
# Parse --verbose argument
while [[ $# -gt 0 ]]; do
case "$1" in
--verbose|-v)
VERBOSE=true
shift
;;
*)
shift
;;
esac
doneThen update the exit logic at lines 305-311 to distinguish warnings from healthy:
if [ "$WARNINGS" -gt 0 ]; then
echo "Status: WARNING - Found $WARNINGS warnings requiring attention"
- exit 0
+ exit 1
else
echo "Status: ✓ HEALTHY - All checks passed"
exit 0
fiAlso applies to: 60-74, 265-283, 305-311
🤖 Prompt for AI Agents
In plugins/etcd/commands/health-check.md around lines 60-74, 265-283 and
305-311, the script never initializes VERBOSE nor parses the documented
--verbose/-v flag and the current exit logic returns 0 for both WARNING and
HEALTHY; initialize WARNINGS=0 and VERBOSE=false immediately after the oc
connection check (after line 72), add a simple while/case loop to consume
arguments and set VERBOSE=true when --verbose or -v is passed, then change the
final exit behavior so a WARNING state exits non‑zero (e.g., exit 1) while
HEALTHY exits 0, and ensure any checks that reference $VERBOSE use the
initialized variable to avoid unbound variable errors.
| ### 1. Verify Prerequisites | ||
|
|
||
| Check if oc CLI is available and cluster is accessible: | ||
|
|
||
| ```bash | ||
| if ! command -v oc &> /dev/null; then | ||
| echo "Error: oc CLI not found. Please install OpenShift CLI." | ||
| exit 1 | ||
| fi | ||
|
|
||
| if ! oc whoami &> /dev/null; then | ||
| echo "Error: Not connected to an OpenShift cluster." | ||
| exit 1 | ||
| fi | ||
| ``` |
There was a problem hiding this comment.
Initialize WARNINGS variable and increment at each warning condition.
The script references $WARNINGS at line 305 for the summary but never initializes or increments it. This causes undefined variable behavior. Initialize WARNINGS=0 early in the script and increment it at each warning condition:
# Add after the oc connection check (after line 72)
WARNINGS=0Then increment WARNINGS at each warning condition:
if [ "$MEMBER_COUNT" -lt 3 ]; then
echo "WARNING: Etcd cluster has less than 3 members (quorum at risk)"
+ WARNINGS=$((WARNINGS + 1))
fi
if [ "$UNSTARTED" -gt 0 ]; then
echo "WARNING: $UNSTARTED member(s) in unstarted state"
+ WARNINGS=$((WARNINGS + 1))
fi
if [ "$DISK_USAGE" -gt 80 ]; then
echo "WARNING: Disk usage on $pod is ${DISK_USAGE}% (threshold: 80%)"
+ WARNINGS=$((WARNINGS + 1))
fi
if [ "$CURRENT_SIZE" -gt "$MAX_DB_SIZE" ]; then
echo "WARNING: Etcd database size ($CURRENT_SIZE bytes) exceeds recommended maximum (8GB)"
+ WARNINGS=$((WARNINGS + 1))
fiAlso applies to: 155-155, 161-161, 236-236, 305-311
🤖 Prompt for AI Agents
In plugins/etcd/commands/health-check.md around lines 60 to 74 and affecting
lines 155, 161, 236, and 305-311, the script uses the WARNINGS variable in the
summary but never initializes or increments it; initialize WARNINGS=0
immediately after the oc CLI/cluster checks (around line 72) and update each
warning-producing conditional at the referenced lines to increment WARNINGS
(e.g., WARNINGS=$((WARNINGS+1))) whenever a warning is emitted so the final
summary reflects the correct count.
|
CodeRabbit's comments look legitimate and worth fixing |
sandeepknd
force-pushed
the
etcd-health-perf
branch
from
7281978 to
89c6e0e
Compare
openshift-merge-robot
added
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
plugins/etcd/commands/analyze-performance.md (2)
9-12: Add language specifiers to fenced code blocks and clean up linksSeveral code fences are bare triple backticks (no language), and markdownlint flags them (MD040). For CLI examples and outputs:
- Use
bashfor invocations:-## Synopsis -``` -/etcd:analyze-performance [--duration <minutes>] -``` +## Synopsis +```bash +/etcd:analyze-performance [--duration <minutes>] +```
- Use
bashfor shell snippets,text(orconsole) for long sample outputs.Also, the “See Also” section uses bare URLs; consider converting them to Markdown links, e.g.:
-- Etcd performance guide: https://etcd.io/docs/latest/tuning/ +- Etcd performance guide: [https://etcd.io/docs/latest/tuning/](https://etcd.io/docs/latest/tuning/)Repeat for the OpenShift docs URL.
Also applies to: 384-387, 388-483, 486-488, 588-589
1-4: Fixargument-hintquoting and harden--durationparsing with validationThe frontmatter and duration argument parsing have multiple issues:
- Line 3:
argument-hint: "[--duration <minutes>]"uses quotes, which renders as escaped quotes in downstream data structures.- Lines 75-79: Duration parsing only works when
--durationis the first argument and fails to:
- Accept
--duration=15form- Validate numeric input
- Handle flags in any order
Fix line 3:
-argument-hint: "[--duration <minutes>]" +argument-hint: [--duration <minutes>]Fix lines 75-79:
-# Parse duration argument (default: 5 minutes) -DURATION=5 -if [[ "$1" == "--duration" ]] && [[ -n "$2" ]]; then - DURATION=$2 -fi +# Parse duration argument (default: 5 minutes) +DURATION=5 +while [[ $# -gt 0 ]]; do + case "$1" in + --duration) + shift + if [[ -z "$1" ]] || ! [[ "$1" =~ ^[0-9]+$ ]]; then + echo "Error: --duration requires an integer value (minutes)" >&2 + exit 1 + fi + DURATION="$1" + ;; + --duration=*) + VALUE="${1#--duration=}" + if ! [[ "$VALUE" =~ ^[0-9]+$ ]]; then + echo "Error: --duration must be an integer (minutes)" >&2 + exit 1 + fi + DURATION="$VALUE" + ;; + esac + shift +doneThis supports both
--duration 15and--duration=15, validates input as integer, and allows flags in any order.plugins/etcd/commands/health-check.md (3)
305-311: Consider using a non-zero exit code when warnings are presentCurrently:
if [ "$WARNINGS" -gt 0 ]; then echo "Status: WARNING - Found $WARNINGS warnings requiring attention" exit 0 else echo "Status: HEALTHY - All checks passed" exit 0 fiand the docs say exit code 0 “may have warnings”. If this command is used in automation where warnings should be surfaced distinctly from a clean bill of health, you may want to return a non-zero code for warnings (e.g.,
exit 2) and update the “Return Value” section accordingly.If you intentionally prefer “warnings but still exit 0”, consider clarifying that in the docs as an explicit design choice.
Also applies to: 316-320
9-12: Align code fences with markdownlint and add link formattingmarkdownlint is flagging:
- Fenced blocks without a language (MD040) around the synopsis and examples.
- Bare URLs in the prereqs and “See Also” section (MD034).
Suggested tweaks:
- Use language hints for CLI examples:
-## Synopsis -``` -/etcd:health-check [--verbose] -``` +## Synopsis +```bash +/etcd:health-check [--verbose] +``` -### Example 1: Basic health check -``` -/etcd:health-check -``` +### Example 1: Basic health check +```bash +/etcd:health-check +```(and similarly for the verbose example and sample output blocks, using
bashortext).
- Convert bare URLs to Markdown links, e.g.:
-- Etcd documentation: https://etcd.io/docs/ +- Etcd documentation: [https://etcd.io/docs/](https://etcd.io/docs/)and likewise for the OpenShift docs URL.
Also applies to: 329-333, 334-336, 377-379, 450-451
2-4: Initialize and trackWARNINGS, parse--verbose, and defineVERBOSEThere are a few interrelated issues in the implementation:
WARNINGSis read in the summary (if [ "$WARNINGS" -gt 0 ]; then) but never initialized or incremented in any of the warning branches, so:
- Bash prints
integer expression expectedand the condition always falls through to the “HEALTHY” branch.- The summary will always report 0 warnings regardless of earlier
WARNING:messages.VERBOSEis used inif [ "$VERBOSE" = "true" ]; thenbut is never set, and the documented--verboseflag is never parsed, so verbose mode can’t be enabled.argument-hintin the frontmatter is quoted, which propagates as extra quotes indocs/data.json.Suggested fixes:
- Unquote the argument hint in frontmatter:
-argument-hint: "[--verbose]" +argument-hint: [--verbose]
- After the cluster connection check, initialize and parse flags:
-if ! oc whoami &> /dev/null; then - echo "Error: Not connected to an OpenShift cluster." - exit 1 -fi +if ! oc whoami &> /dev/null; then + echo "Error: Not connected to an OpenShift cluster." + exit 1 +fi + +# Initialize counters and flags +WARNINGS=0 +VERBOSE=false + +# Parse optional flags (currently only --verbose / -v) +while [[ $# -gt 0 ]]; do + case "$1" in + --verbose|-v) + VERBOSE=true + ;; + *) + # Ignore unknown args for now + ;; + esac + shift +done
- Increment
WARNINGSwherever you emit aWARNING::if [ "$MEMBER_COUNT" -lt 3 ]; then echo "WARNING: Etcd cluster has less than 3 members (quorum at risk)" + WARNINGS=$((WARNINGS + 1)) fi if [ "$UNSTARTED" -gt 0 ]; then echo "WARNING: $UNSTARTED member(s) in unstarted state" + WARNINGS=$((WARNINGS + 1)) fi if [ "$DISK_USAGE" -gt 80 ]; then echo "WARNING: Disk usage on $pod is ${DISK_USAGE}% (threshold: 80%)" + WARNINGS=$((WARNINGS + 1)) fi if [ "$CURRENT_SIZE" -gt "$MAX_DB_SIZE" ]; then echo "WARNING: Etcd database size ($CURRENT_SIZE bytes) exceeds recommended maximum (8GB)" echo "Consider defragmentation or checking for excessive key growth" + WARNINGS=$((WARNINGS + 1)) fiWith these changes, the summary’s warning count and the
--verbosebehavior will match the documentation.#!/bin/bash # Verify that WARNINGS and VERBOSE are not referenced elsewhere without initialization. rg -n "WARNINGS|VERBOSE" plugins/etcd/commands/health-check.md -n -C3Also applies to: 148-162, 210-217, 225-237, 260-283, 291-311
🧹 Nitpick comments (3)
plugins/etcd/README.md (1)
22-31: Fix markdownlint issues: add code block languages and avoid bare URLsTo keep docs passing
markdownlintand improve readability:
- Add language specifiers to CLI/example code fences, e.g.:
-**Usage:** -``` -/etcd:health-check [--verbose] -``` +**Usage:** +```bash +/etcd:health-check [--verbose] +``` -**Example:** -``` -/etcd:health-check -/etcd:health-check --verbose -``` +**Example:** +```bash +/etcd:health-check +/etcd:health-check --verbose +```
Do similarly for the
/etcd:analyze-performanceusage/examples.Replace bare URLs with proper links to satisfy MD034, e.g.:
-1. **OpenShift CLI (oc)** - Install from https://mirror.openshift.com/pub/openshift-v4/clients/ocp/ +1. **OpenShift CLI (oc)** - Install from [OpenShift CLI downloads](https://mirror.openshift.com/pub/openshift-v4/clients/ocp/)and likewise in the Resources and Contributing sections.
Also applies to: 43-52, 56-60, 157-165
plugins/etcd/commands/analyze-performance.md (1)
31-49: Document additional CLI prerequisites used by the script (jq,bc)The implementation relies on tools beyond
oc, for example:
jqfor JSON parsing (jq -rin the DB status and fragmentation sections).bcfor numeric comparisons (bc -lin the fragmentation checks, computing MB sizes).These are not mentioned in the Prerequisites section. To avoid runtime surprises on systems where they’re not installed, consider explicitly documenting them, e.g.:
-1. **OpenShift CLI (oc)** +1. **OpenShift CLI (oc)** - Install from: … - Verify with: `oc version` + +2. **jq and bc utilities** + - Required for JSON parsing and numeric calculations + - Verify with: `jq --version` and `bc --version`(and renumber the remaining items).
Also applies to: 100-142, 291-349
plugins/etcd/commands/health-check.md (1)
32-43: Documentjqandcurlas prerequisitesThe health-check implementation assumes
jqandcurlare available:
jqis used multiple times to parse etcdctl JSON.curlis used to fetch metrics fromhttp://localhost:2379/metrics.They’re not mentioned in the “Prerequisites” section, which may surprise users running on minimal environments. Consider explicitly listing them, e.g.:
-3. **Cluster admin permissions** +3. **Cluster admin permissions** - Required to access etcd pods and execute commands - Verify with: `oc auth can-i get pods -n openshift-etcd` + +4. **jq and curl utilities** + - Required for JSON parsing (`jq`) and metrics collection (`curl`) + - Verify with: `jq --version` and `curl --version` + -4. **Healthy etcd namespace** - - The openshift-etcd namespace must exist - - At least one etcd pod must be running +5. **Healthy etcd namespace** + - The openshift-etcd namespace must exist + - At least one etcd pod must be runningThis makes operational requirements explicit.
Also applies to: 189-213, 247-251, 269-283
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (9)
.claude-plugin/marketplace.json(1 hunks).github/workflows/lint-plugins.yml(1 hunks)PLUGINS.md(2 hunks)docs/data.json(1 hunks)plugins/etcd/.claude-plugin/plugin.json(1 hunks)plugins/etcd/README.md(1 hunks)plugins/etcd/commands/analyze-performance.md(1 hunks)plugins/etcd/commands/health-check.md(1 hunks)scripts/build-website.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- plugins/etcd/.claude-plugin/plugin.json
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/lint-plugins.yml
- PLUGINS.md
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
plugins/etcd/README.md
23-23: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
28-28: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
44-44: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
49-49: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
58-58: Bare URL used
(MD034, no-bare-urls)
157-157: Bare URL used
(MD034, no-bare-urls)
158-158: Bare URL used
(MD034, no-bare-urls)
159-159: Bare URL used
(MD034, no-bare-urls)
164-164: Bare URL used
(MD034, no-bare-urls)
plugins/etcd/commands/analyze-performance.md
10-10: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
36-36: Bare URL used
(MD034, no-bare-urls)
384-384: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
389-389: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
486-486: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
588-588: Bare URL used
(MD034, no-bare-urls)
589-589: Bare URL used
(MD034, no-bare-urls)
plugins/etcd/commands/health-check.md
10-10: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
33-33: Bare URL used
(MD034, no-bare-urls)
330-330: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
335-335: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
377-377: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
450-450: Bare URL used
(MD034, no-bare-urls)
451-451: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (3)
scripts/build-website.py (1)
145-146: Deterministic JSON output looks goodEnabling
sort_keys=Trueimproves reproducibility ofdocs/data.jsonwithout changing its structure or values; this is a safe, beneficial change for diffs and tests..claude-plugin/marketplace.json (1)
67-71: Etcd marketplace entry is consistent and well-scopedThe new
etcdplugin entry matches the repository layout (./plugins/etcd) and description aligns with the README; structure ofmarketplace.jsonremains consistent.docs/data.json (1)
6-452: Expanded plugin metadata looks consistent with generator scriptThe additions of
argument_hint,has_readme,name, andversionacross existing plugins (git, jira, ci, openshift, etc.) match the structure produced bybuild_website_data()and make the catalog more informative without changing semantics. Command names and synopses align with their respective plugins’ docs.
| "argument_hint": "\"[--duration <minutes>]\"", | ||
| "description": "Analyze etcd performance metrics, latency, and identify bottlenecks", | ||
| "name": "analyze-performance", | ||
| "synopsis": "/etcd:analyze-performance [--duration <minutes>]" | ||
| }, | ||
| { | ||
| "argument_hint": "\"[--verbose]\"", | ||
| "description": "Check etcd cluster health, member status, and identify issues", | ||
| "name": "health-check", | ||
| "synopsis": "/etcd:health-check [--verbose]" | ||
| } |
There was a problem hiding this comment.
Remove extra quoting from etcd argument_hint values
For the new etcd plugin commands, the argument_hint fields include literal quotes:
"argument_hint": "\"[--duration <minutes>]\"",
...
"argument_hint": "\"[--verbose]\"",This likely comes from quoted argument-hint values in the markdown frontmatter and will render with stray quotes in UIs.
Once you unquote the frontmatter (e.g., argument-hint: [--duration <minutes>] and argument-hint: [--verbose]), regenerate docs/data.json so these become:
-"argument_hint": "\"[--duration <minutes>]\"",
+"argument_hint": "[--duration <minutes>]",
...
-"argument_hint": "\"[--verbose]\"",
+"argument_hint": "[--verbose]",which matches the style used by other plugins.
🤖 Prompt for AI Agents
In docs/data.json around lines 528-538 the etcd plugin entries have
argument_hint values that include literal quotes (e.g. "\"[--duration
<minutes>]\""), which causes stray quotes in UIs; update the source markdown
frontmatter to remove the quotes (use argument-hint: [--duration <minutes>] and
argument-hint: [--verbose] without surrounding quotes), then regenerate
docs/data.json so the argument_hint fields become unquoted strings (e.g.
"[--duration <minutes>"] matching the style used by other plugins.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
docs/data.json (1)
645-645: Remove extra quoting from etcdargument_hintvalues.As noted in a previous review, the etcd plugin has
argument_hintfields with literal escaped quotes. This issue also affects the openshift and node-tuning plugins, suggesting a systematic problem with how quoted frontmatter values are parsed.Update the source markdown frontmatter to remove the surrounding quotes, then regenerate
docs/data.json.Also applies to: 651-651
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (4)
.claude-plugin/marketplace.json(1 hunks)PLUGINS.md(2 hunks)docs/data.json(1 hunks)scripts/build-website.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- PLUGINS.md
- .claude-plugin/marketplace.json
🔇 Additional comments (1)
scripts/build-website.py (1)
175-175: Good addition for reproducibility.Adding
sort_keys=Trueensures deterministic JSON output, making diffs cleaner and version control more predictable.
| }, | ||
| { | ||
| "name": "cluster-health-check", | ||
| "argument_hint": "\"[--verbose] [--output-format]\"", |
There was a problem hiding this comment.
Remove extra quoting from openshift argument_hint values.
Similar to the etcd plugin, the openshift plugin has argument_hint fields with literal escaped quotes that will render incorrectly in UIs:
"argument_hint": "\"[--verbose] [--output-format]\"",
"argument_hint": "\"[release-image] [platform] [options]\"",
"argument_hint": "\"[install-dir]\"",Update the source markdown frontmatter to remove the surrounding quotes (e.g., argument-hint: [--verbose] [--output-format]), then regenerate docs/data.json.
Also applies to: 599-599, 605-605
🤖 Prompt for AI Agents
In docs/data.json around lines 587, 599 and 605, the openshift plugin
argument_hint values contain literal escaped surrounding quotes (e.g.
"\"[--verbose] [--output-format]\"") which render incorrectly; edit the original
plugin markdown frontmatter to replace argument-hint: "\"[...]" entries with
unquoted argument-hint: [--verbose] [--output-format] (i.e., remove the
surrounding quotes and any escaping), then regenerate docs/data.json so the JSON
contains plain unquoted argument_hint values without escaped quotes.
| "commands": [ | ||
| { | ||
| "name": "analyze-node-tuning", | ||
| "argument_hint": "\"[--sosreport PATH] [--format json|markdown] [--max-irq-samples N]\"", |
There was a problem hiding this comment.
Remove extra quoting from node-tuning argument_hint values.
The node-tuning plugin also has argument_hint fields with literal escaped quotes:
"argument_hint": "\"[--sosreport PATH] [--format json|markdown] [--max-irq-samples N]\"",
"argument_hint": "\"[profile-name] [--summary ...] [--sysctl ...] [options]\"",This is part of the systematic issue affecting multiple plugins. Update the source markdown frontmatter to remove the surrounding quotes, then regenerate docs/data.json.
Also applies to: 811-811
🤖 Prompt for AI Agents
In docs/data.json around lines 805 and 811, the node-tuning entries for
"argument_hint" contain literal escaped surrounding quotes (e.g. "\"[--sosreport
PATH] ...]\""); edit the source markdown frontmatter for the node-tuning
plugin(s) to remove the extra surrounding quotes so argument_hint is a plain
string without embedded quotes, then regenerate docs/data.json from the sources
so the JSON reflects the corrected values.
3 participants
Summary
Added comprehensive etcd health monitoring and performance analysis commands for OpenShift clusters. This new plugin provides essential diagnostic capabilities for troubleshooting
etcd-related cluster issues and proactive performance monitoring.
Changes
New Plugin: etcd
Added a complete etcd monitoring plugin with two key commands:
1. /etcd:health-check - Comprehensive health diagnostics
2. /etcd:analyze-performance - Performance analysis and bottleneck detection
Key Features
Files Added
Test Plan
Use Cases
Generated with https://claude.com/claude-code
Test Results
● Etcd Health Check Summary
===============================================
Etcd Health Check Report
Check Time: 2025-11-19
Cluster: api.cam-src-69220.qe.devcluster.openshift.com:6443
User: system:admin
ETCD POD STATUS
Total Pods: 3/3 Running
All pods ready with 0 restarts
Pod Details:
- etcd-ip-10-0-1-63.us-east-2.compute.internal (Node: ip-10-0-1-63.us-east-2.compute.internal)
- etcd-ip-10-0-61-180.us-east-2.compute.internal (Node: ip-10-0-61-180.us-east-2.compute.internal)
- etcd-ip-10-0-86-112.us-east-2.compute.internal (Node: ip-10-0-86-112.us-east-2.compute.internal)
CLUSTER HEALTH
All endpoints healthy:
✓ https://10.0.1.63:2379 (6.7ms)
✓ https://10.0.61.180:2379 (12.2ms)
✓ https://10.0.86.112:2379 (13.2ms)
CLUSTER MEMBERSHIP
Total Members: 3
Quorum: Healthy (3/3 members started)
All members in "started" state
No unstarted or learner members
LEADERSHIP
Leader: https://10.0.1.63:2379 (ip-10-0-1-63.us-east-2.compute.internal)
Raft Term: 6
Raft Index: 103431
All members synchronized (same raft index)
DATABASE STATUS
Etcd Version: 3.5.24
Database Sizes:
- 10.0.1.63: 86 MB (79 MB in use, 7% fragmentation)
- 10.0.61.180: 86 MB (79 MB in use, 7% fragmentation)
- 10.0.86.112: 86 MB (79 MB in use, 7% fragmentation)
Status: Database size well within 8GB limit
Fragmentation: Normal (7% is healthy)
DISK SPACE
Node: ip-10-0-1-63.us-east-2.compute.internal
/var/lib/etcd: 120G total, 21G used (17%)
Node: ip-10-0-61-180.us-east-2.compute.internal
/var/lib/etcd: 120G total, 22G used (19%)
Node: ip-10-0-86-112.us-east-2.compute.internal
/var/lib/etcd: 120G total, 22G used (19%)
Status: All nodes well below 80% threshold
RECENT ERRORS
No errors, warnings, or fatal messages in recent logs (last 100 lines)
===============================================
OVERALL STATUS: HEALTHY ✓
Summary:
✓ All 3 etcd pods running with no restarts
✓ Cluster health excellent (all endpoints healthy)
✓ Quorum maintained (3/3 members)
✓ Leader elected and stable
✓ Low latency response times (<15ms)
✓ Database size optimal (86MB << 8GB limit)
✓ Low fragmentation (7%)
✓ Ample disk space (17-19% usage)
✓ No recent errors in logs
✓ All members synchronized
Recommendations:
- No immediate action required
- Continue monitoring cluster health
- Current configuration is optimal
The etcd cluster is in excellent health with all checks passing. All three members are healthy, synchronized, and operating normally with good performance metrics.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.