SBR docs: update sbdconfig-user-guide with Quick Start, Known Issues, etc#2
Conversation
|
@mshitrit - FYI |
|
I think this PR captures and documents well some of SBR critial issues, which requires workarounds or additional attention. I think it would be a good idea either to have a separate PR (in case there are here issues which aren't pending fix for GA), or to wait until GA with this PR - in which case it might be closed or significantly modified. |
razo7
left a comment
razo7
left a comment
There was a problem hiding this comment.
I crawled the SBR codebase against this PR and found significant drift. mshitrit's comment was prescient as most workarounds this PR documents have been fixed since January.
What's correct and valuable
- Changing "cluster-scoped" to "namespaced" ✅
- Moving namespace from
spec.namespacetometadata.namespace✅ - Updating status from
daemonSetReadyboolean to Conditions pattern ✅ - Adding
sharedStorageClassandnodeSelectordocumentation ✅ - OpenShift-specific
oc debugcommands ✅ - Namespace placeholders in kubectl commands ✅
- Conditions status detail (types, reasons, messages) ✅
Critical mismatches with current codebase
spec.imageremoved from CRD — all references, examples, workarounds are wrong- SCC is now auto-managed — controller creates CRB to built-in
privilegedSCC - 9 documented fields don't exist in CRD:
imagePullPolicy,staleNodeTimeout,watchdogTimeout,petIntervalMultiple,logLevel,iotimeout,rebootMethod,sbdUpdateInterval,peerCheckInterval - Field name errors:
sbdWatchdogPath→watchdogPath,sbdTimeoutSeconds→sbrTimeoutSeconds - Missing real fields:
maxConsecutiveFailures(2-32, default 7),detectOnlyMode(Disabled/Enabled) - Mount path:
/dev/sbd→/dev/sbr - Metric prefix:
sbd_*→sbr_*(context lines, not changed by this PR) - Metrics port: 8080 → 8082 (context line, not changed by this PR)
- Repo URL:
storage-base-remediation→storage-based-remediation
Given the scope of drift, I would recommend that this PR should either be substantially rewritten against the current codebase (keeping the good structural changes), or closed in favor of a fresh docs PR built from the actual CRD types @maximunited
See inline comments for specifics.
|
Consider adding
|
a818d18 to
2e49324
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughUpdated StorageBasedRemediationConfig documentation with namespaced resource guidance, revised configuration and status references, expanded installation and management procedures, storage and metrics details, troubleshooting commands, and refreshed examples. ChangesStorageBasedRemediationConfig documentation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@docs/sbr-config-quick-reference.md`:
- Around line 151-158: Update the maxConsecutiveFailures description in the
configuration reference table to state that the threshold applies to watchdog,
SBR-device, local-heartbeat, and peer-heartbeat failures, matching the CRD API
contract and user guide.
- Around line 59-61: Update the log commands in the quick-reference section to
target a single SBR configuration instead of all resources sharing the
app=sbr-agent label. Use the configuration-specific selector or resource name
established by the deployment manifests, and document the exact label contract
if selecting by label; apply the same scoping to both standard and follow-mode
commands.
- Around line 84-85: Update the “Check metrics” example in the SBR configuration
quick reference to use the documented Service/ServiceMonitor access flow with
kubectl port-forward instead of directly curling the node IP. Keep the example
consistent with the existing metrics documentation.
In `@docs/sbr-config-user-guide.md`:
- Around line 174-179: Update the maxConsecutiveFailures documentation in the
detailed configuration section and quick reference to state that the threshold
applies to SBR-device, watchdog, local-heartbeat, and peer-heartbeat failures,
not just storage I/O failures. Preserve the existing default, range, and
time-to-detection details.
- Around line 297-315: Clarify the “Multi-Cluster Configuration” example that
the two StorageBasedRemediationConfig YAML documents must be applied separately
from their corresponding clusters, or present them as distinct per-cluster
files. Preserve the existing cluster-specific names and namespaces while making
the application context explicit.
- Line 649: Update the ServiceMonitor example in the configuration guide to use
the `metrics` port name instead of `runtime-metrics`, matching the port exposed
by `config/prometheus/sbr-agent-metrics.yaml`.
- Around line 430-435: Update the “Supported StorageClass Examples” section to
clarify that Azure Files is supported only when backed by NFS v4.1 with POSIX
file-locking semantics, or remove the Azure Files entry if SMB-backed shares are
the intended configuration. Do not leave the current generic azurefile-csi
example implying all Azure Files variants are supported.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 333f9fe5-5880-42de-b723-395788ea5dc4
📒 Files selected for processing (2)
docs/sbr-config-quick-reference.mddocs/sbr-config-user-guide.md
|
Following up on this — RHWA-694 (Productize SBR Phase III) is now closed/done, and the workarounds this PR originally documented (invalid default Per the option you raised — rewrite vs. close — I went with significantly modifying the PR rather than closing it: rebased onto current |
|
Added a TODO to the README, but with a correction — I verified this against current
Added a TODO in the README's Installation section documenting this with the verified evidence, and pointed the primary install path at OLM instead of |
|
/test ci/prow/4.23-ci-bundle-my-bundle |
|
/test 4.23-ci-bundle-my-bundle |
|
/retest |
1 similar comment
|
/retest |
The docs previously targeted the pre-January SBDConfig API and workarounds that no longer apply. Update sbr-config-user-guide.md and sbr-config-quick-reference.md to match the current StorageBasedRemediationConfig CRD: - Remove spec.image (removed in PR medik8s#44); image is now injected via the RELATED_IMAGE_AGENT env var - Remove the "Known Issues" workarounds (invalid default image, manual SCC) since both were resolved upstream - Remove manual SCC YAML; the controller auto-creates the ClusterRoleBinding - Rename sbdWatchdogPath -> watchdogPath, drop 9 fields that no longer exist in the spec (imagePullPolicy, staleNodeTimeout, watchdogTimeout, etc.) - Add missing real fields: nodeSelector, sbrTimeoutSeconds, maxConsecutiveFailures, detectOnlyMode - Fix mount path (/dev/sbd -> /dev/sbr), metrics prefix (sbd_ -> sbr_) and port (8080 -> 8082), and a repo URL typo - Replace daemonSetReady status field with the current conditions array
- Scope log commands to a single config via the storagebasedremediationconfig label instead of the shared app=sbr-agent label - Fix metrics access example: agent has no hostNetwork/hostPort, so use a Service + port-forward instead of curling the node IP directly - Clarify maxConsecutiveFailures covers watchdog, SBR-device, and local/peer-heartbeat failures, not just storage I/O - Split the multi-cluster example into separate per-cluster manifests with a note to apply each against its own cluster context - Narrow the Azure Files storage example to NFS v4.1-backed shares, since SMB-backed shares lack the required POSIX file-locking semantics - Fix ServiceMonitor example port name (metrics, matching the Service) since it does not need to match the container's port name (runtime-metrics)
Verified with kustomize build config/default that leader_election_role_binding.yaml, metrics_auth_role_binding.yaml, and sbr_operator_role_binding.yaml still render subjects with the stale `namespace: system` instead of `sbr-operator-system`, because their ServiceAccount subject name is already prefixed and no longer matches kustomize's nameReference transformer. This breaks leader election and metrics-auth RBAC under `make deploy`, matching the diagnosis in PR medik8s#23 (closed, not merged). Point users at OLM installation instead until fixed.
fd22a10 to
8cf875e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@README.md`:
- Around line 71-89: Update the numbered Quick Start installation step in
README.md so it no longer presents make deploy as the default supported path.
Direct users to the documented OLM/manifest installation procedure, or clearly
label make deploy as a development-only workaround until the RBAC issues are
fixed; keep the existing OLM guidance and installation reference consistent.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ef5de7d2-9a2c-4806-84f6-d000ad094f69
📒 Files selected for processing (3)
README.mddocs/sbr-config-quick-reference.mddocs/sbr-config-user-guide.md
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/sbr-config-quick-reference.md
- docs/sbr-config-user-guide.md
|
CI failure analysis: transient OOM - not related to this PR The
Root cause: The CI resource config requests 1000Mi memory with no limit and no CPU limit. Without a CPU limit, Go 1.26 defaults GOMAXPROCS to the node's full CPU count (~16 cores), spawning too many parallel compiler processes for the available memory. This is random, not deterministic. The same job passes on PRs #69, #70, #63, and #74 - it depends on CI cluster load and node placement. PR #2 had bad luck: the first 4 runs hit merge conflicts (now fixed by rebase), and the remaining runs landed on memory-pressured nodes. Other medik8s repos ( |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
razo7
left a comment
razo7
left a comment
There was a problem hiding this comment.
Great work here @maximunited
left one NIT
| kind: StorageBasedRemediationConfig | ||
| metadata: | ||
| name: default | ||
| namespace: openshift-operators |
There was a problem hiding this comment.
NIT: Quick Start uses namespace: openshift-operators but every other example in both the user guide and quick reference uses sbr-operator-system (or scenario-specific namespaces like sbr-dev). The kustomize default is also sbr-operator-system.
Either use sbr-operator-system here for consistency, or add a note explaining why Quick Start targets openshift-operators (e.g. OLM default install namespace).
There was a problem hiding this comment.
Fixed. Changed namespace: openshift-operators → namespace: sbr-operator-system in the Quick Start example to match every other example in both docs files and the kustomize default.
|
|
||
| # Check metrics (agent does not use hostNetwork; use a Service + port-forward, | ||
| # not a direct node-IP curl — see Monitoring and Observability) | ||
| kubectl port-forward -n <namespace> svc/sbr-agent-metrics 8082:8082 |
There was a problem hiding this comment.
This port-forward command will fail: the Service at config/prometheus/sbr-agent-metrics.yaml exposes port 8080 (targetPort 8080), not 8082. The agent binary listens on 8082 (DefaultMetricsPort in internal/agent/flags.go:110), so the Service manifest is the bug — but until it's fixed, this command hits a port the Service doesn't expose.
Workaround: target the DaemonSet directly instead of the Service:
kubectl port-forward -n <namespace> ds/sbr-agent-<config-name> 8082:8082Also: docs/sbr-agent-prometheus-metrics.md (linked from this PR at the user guide) still says port 8080 in 9 places — contradicts the 8082 used here.
Tracked as RHWA-1372.
There was a problem hiding this comment.
Fixed. Changed the port-forward target from svc/sbr-agent-metrics 8082:8082 (fails since the Service exposes port 8080) to ds/sbr-agent-<config-name> 8082:8082, targeting the DaemonSet directly on the actual agent port. Updated the comment to reference RHWA-1372.
Will leave docs/sbr-agent-prometheus-metrics.md for the RHWA-1372 fix since the Service manifest port is the root cause.
- Use sbr-operator-system namespace in Quick Start for consistency with all other examples and the kustomize default - Target DaemonSet directly for port-forward (ds/sbr-agent-<config-name>) since the Service manifest exposes 8080 not 8082 (RHWA-1372) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: maximunited, razo7 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
3 participants
Update SBDConfig User Guide Documentation
This PR updates the SBDConfig user guide with new sections and important improvements:
Key Changes
spec.image)imagefield is now required (not optional)metadata.namespace, notspec.namespace)sbdconfig-quick-reference.mdto match user guide changesSummary by CodeRabbit
make deploylimitations and the intended fix.