YNU-805: Run multiple stress test pods & other improvements#584
YNU-805: Run multiple stress test pods & other improvements#584
Conversation
Summary of ChangesHello @dimast-x, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly upgrades the stress testing framework for the Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds per-pod stressTest configuration and five pod entries; replaces a Pod-based Helm stress-test with a ConfigMap-driven config + Job runner; and adds exponential-backoff retry logic to client creation and pool connection setup in the stress package. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant improvements to the stress testing capabilities, allowing multiple test pods to run in parallel via a Kubernetes Job. The configuration in values.yaml has been refactored to support this, which is a great enhancement for flexibility. Additionally, the Go stress test clients now include a resilient connection mechanism with retries and exponential backoff. While these are excellent changes, I've identified a critical security vulnerability and a medium-severity robustness issue in the Helm template that should be addressed.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clearnode/chart/templates/tests/stress-test.yaml`:
- Around line 29-32: The template currently writes STRESS_PRIVATE_KEY into a
ConfigMap using the $privateKey variable; move this into a Kubernetes Secret and
inject it via valueFrom: secretKeyRef instead of rendering the raw value. Change
the logic that reads $pod.privateKey | default $defaults.privateKey so you
render only a Secret reference (or accept secret name/key parameters) and update
the environment entry STRESS_PRIVATE_KEY to use env.valueFrom.secretKeyRef with
the configured secretName and key; ensure no raw privateKey value is rendered
into the ConfigMap template or chart outputs.
- Around line 23-28: The STRESS_SPECS env entry is generated with join " " which
creates raw spaces that get split when the env file is later exported; update
the pod-{{ $i }}.env template to emit a safely quoted value (e.g.
STRESS_SPECS='{{ join " " $pod.specs }}') or use a non-space delimiter (e.g.
join "," or "|") and ensure the consuming code parses that delimiter; apply the
same quoting/delimiter change to the other similar env lines (the second block
at lines referenced in the comment) so multi-entry specs are preserved during
export.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
clearnode/chart/config/v1-rc/clearnode.yamlclearnode/chart/templates/tests/stress-test.yamlclearnode/chart/values.yamlclearnode/stress/pool.goclearnode/stress/transfer.go
| pod-{{ $i }}.env: | | ||
| STRESS_SPECS={{ join " " $pod.specs }} | ||
| STRESS_WS_URL={{ $wsURL }} | ||
| STRESS_CONNECTIONS={{ $connections }} | ||
| STRESS_TIMEOUT={{ $timeout }} | ||
| STRESS_MAX_ERROR_RATE={{ $maxErrorRate }} |
There was a problem hiding this comment.
STRESS_SPECS with multiple entries will break env export.
join " " produces spaces, and export $(... | xargs) splits them, truncating STRESS_SPECS and potentially causing invalid exports. This breaks multi-spec pods.
🔧 Safer env loading + quoting
- STRESS_SPECS={{ join " " $pod.specs }}
+ STRESS_SPECS={{ join " " $pod.specs | quote }}
@@
- export $(cat /stress-config/pod-${JOB_COMPLETION_INDEX}.env | xargs)
+ set -a
+ . /stress-config/pod-${JOB_COMPLETION_INDEX}.env
+ set +aAlso applies to: 66-69
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clearnode/chart/templates/tests/stress-test.yaml` around lines 23 - 28, The
STRESS_SPECS env entry is generated with join " " which creates raw spaces that
get split when the env file is later exported; update the pod-{{ $i }}.env
template to emit a safely quoted value (e.g. STRESS_SPECS='{{ join " "
$pod.specs }}') or use a non-space delimiter (e.g. join "," or "|") and ensure
the consuming code parses that delimiter; apply the same quoting/delimiter
change to the other similar env lines (the second block at lines referenced in
the comment) so multi-entry specs are preserved during export.
dimast-x
force-pushed
the
feat/update-stress-tool
branch
from
6712ee9 to
83611d8
Compare
dimast-x
force-pushed
the
feat/update-stress-tool
branch
from
83611d8 to
8e7ff68
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
clearnode/chart/templates/tests/stress-test.yaml (2)
23-28:STRESS_SPECSvalue is unquoted —export $(... | xargs)will break with multi-entry specs.As previously noted,
join " "produces bare spaces in the env file, andexport $(cat ... | xargs)will word-split them. The current config happens to use single-entry specs, but this is a latent bug.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/chart/templates/tests/stress-test.yaml` around lines 23 - 28, The env file writes STRESS_SPECS unquoted which will be word-split by consumers like export $(cat ... | xargs); update the pod-{{ $i }}.env template in stress-test.yaml so STRESS_SPECS is produced as a single quoted/escaped value (e.g., wrap the result of join " " $pod.specs in quotes or use a join with a safe delimiter and escape) so that the join " " $pod.specs output cannot be split by shell word-splitting; ensure downstream consumers expect a quoted value and adjust parsing accordingly.
29-32:STRESS_PRIVATE_KEYrendered into a ConfigMap is a security concern.As previously noted, ConfigMaps are not secret-safe. This should be injected via a Kubernetes Secret reference.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/chart/templates/tests/stress-test.yaml` around lines 29 - 32, The template currently renders STRESS_PRIVATE_KEY into a ConfigMap using {{$pod.privateKey | default $defaults.privateKey}}, which exposes sensitive material; stop writing the private key into the ConfigMap and instead read it from a Kubernetes Secret. Modify the template to remove STRESS_PRIVATE_KEY from the ConfigMap and change the pod/container env entry to use valueFrom.secretKeyRef pointing to a Secret name and key (e.g., reference a secret that contains the private key under a known key); ensure the secret is created or documented as required and update any references to $pod.privateKey/$defaults.privateKey so they map to the Secret rather than being rendered directly.
🧹 Nitpick comments (2)
clearnode/stress/transfer.go (1)
54-68: Retry parameters and signer creation are duplicated acrosspool.go— consider extracting a shared helper.Both
createClient(transfer.go) andCreateClientPool(pool.go) share identical signer creation code (ethMsgSigner, stateSigner, txSigner from privateKey) yet maintain separate retry loops with conflicting backoff parameters:
- transfer.go uses 100ms base with no cap
- pool.go uses 500ms base with 10s cap
This duplication risks divergence during maintenance. Consider extracting a shared helper like
createSigners(privateKey string)for signer creation, and optionallyconnectWithRetry(wsURL string, stateSigner, txSigner interface{}, baseBackoff time.Duration, maxBackoff time.Duration)to parameterize backoff behavior if the difference is intentional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/stress/transfer.go` around lines 54 - 68, Both createClient (transfer.go) and CreateClientPool (pool.go) duplicate signer creation and retry/backoff logic; extract a shared helper createSigners(privateKey string) that returns ethMsgSigner, stateSigner, txSigner and replace both sites to call it, and replace the retry loop with a shared connectWithRetry(wsURL string, stateSigner, txSigner interface{}, baseBackoff time.Duration, maxBackoff time.Duration) (or a simpler connectWithRetry(wsURL, stateSigner, txSigner, baseBackoff) if no cap needed) so you can centralize the retry policy and parameterize baseBackoff and maxBackoff to reconcile the 100ms vs 500ms/10s behaviors used by createClient and CreateClientPool.clearnode/chart/templates/tests/stress-test.yaml (1)
72-76: Remove redundantJOB_COMPLETION_INDEXenvironment variable definition.Kubernetes automatically injects the
JOB_COMPLETION_INDEXenvironment variable into containers of Indexed-mode Jobs (GA since K8s 1.24). The manualfieldRefextraction from the pod annotation is redundant and can be removed.♻️ Suggested simplification
- env: - - name: JOB_COMPLETION_INDEX - valueFrom: - fieldRef: - fieldPath: metadata.annotations['batch.kubernetes.io/job-completion-index']🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/chart/templates/tests/stress-test.yaml` around lines 72 - 76, Remove the manual environment variable entry that sets JOB_COMPLETION_INDEX via a fieldRef to metadata.annotations['batch.kubernetes.io/job-completion-index'] because Kubernetes injects JOB_COMPLETION_INDEX for indexed Jobs automatically; locate the env block containing "name: JOB_COMPLETION_INDEX" and the valueFrom/fieldRef stanza and delete that entire env item so the container relies on the built-in injected variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clearnode/chart/templates/deployment.yaml`:
- Around line 14-17: You're adding a new label key app.kubernetes.io/component:
server to spec.selector.matchLabels in the Deployment template
(spec.selector.matchLabels), which is an immutable field and will break
upgrades; fix by either (a) revert this selector label change and keep selectors
identical to existing releases, (b) if the label is required, document a
migration step and implement a safe upgrade path such as adding a Helm
pre-upgrade hook to delete the old Deployment before creating the new one, or
(c) gate the change behind a new chart release name / new Deployment
metadata.name so Kubernetes treats it as a new object; apply the chosen approach
and update Chart NOTES.md to describe the migration if you choose
delete/migration or change the release name.
---
Duplicate comments:
In `@clearnode/chart/templates/tests/stress-test.yaml`:
- Around line 23-28: The env file writes STRESS_SPECS unquoted which will be
word-split by consumers like export $(cat ... | xargs); update the pod-{{ $i
}}.env template in stress-test.yaml so STRESS_SPECS is produced as a single
quoted/escaped value (e.g., wrap the result of join " " $pod.specs in quotes or
use a join with a safe delimiter and escape) so that the join " " $pod.specs
output cannot be split by shell word-splitting; ensure downstream consumers
expect a quoted value and adjust parsing accordingly.
- Around line 29-32: The template currently renders STRESS_PRIVATE_KEY into a
ConfigMap using {{$pod.privateKey | default $defaults.privateKey}}, which
exposes sensitive material; stop writing the private key into the ConfigMap and
instead read it from a Kubernetes Secret. Modify the template to remove
STRESS_PRIVATE_KEY from the ConfigMap and change the pod/container env entry to
use valueFrom.secretKeyRef pointing to a Secret name and key (e.g., reference a
secret that contains the private key under a known key); ensure the secret is
created or documented as required and update any references to
$pod.privateKey/$defaults.privateKey so they map to the Secret rather than being
rendered directly.
---
Nitpick comments:
In `@clearnode/chart/templates/tests/stress-test.yaml`:
- Around line 72-76: Remove the manual environment variable entry that sets
JOB_COMPLETION_INDEX via a fieldRef to
metadata.annotations['batch.kubernetes.io/job-completion-index'] because
Kubernetes injects JOB_COMPLETION_INDEX for indexed Jobs automatically; locate
the env block containing "name: JOB_COMPLETION_INDEX" and the valueFrom/fieldRef
stanza and delete that entire env item so the container relies on the built-in
injected variable.
In `@clearnode/stress/transfer.go`:
- Around line 54-68: Both createClient (transfer.go) and CreateClientPool
(pool.go) duplicate signer creation and retry/backoff logic; extract a shared
helper createSigners(privateKey string) that returns ethMsgSigner, stateSigner,
txSigner and replace both sites to call it, and replace the retry loop with a
shared connectWithRetry(wsURL string, stateSigner, txSigner interface{},
baseBackoff time.Duration, maxBackoff time.Duration) (or a simpler
connectWithRetry(wsURL, stateSigner, txSigner, baseBackoff) if no cap needed) so
you can centralize the retry policy and parameterize baseBackoff and maxBackoff
to reconcile the 100ms vs 500ms/10s behaviors used by createClient and
CreateClientPool.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
clearnode/chart/config/v1-rc/clearnode.yamlclearnode/chart/templates/deployment.yamlclearnode/chart/templates/service.yamlclearnode/chart/templates/tests/stress-test.yamlclearnode/chart/values.yamlclearnode/stress/pool.goclearnode/stress/transfer.go
🚧 Files skipped from review as they are similar to previous changes (1)
- clearnode/chart/values.yaml
| selector: | ||
| matchLabels: | ||
| {{- include "clearnode.common.selectorLabels" . | nindent 6 }} | ||
| app.kubernetes.io/component: server |
There was a problem hiding this comment.
Adding a label to spec.selector.matchLabels is a breaking change for existing Deployments.
Kubernetes does not allow updating the selector of an existing Deployment. If this chart is applied to a cluster where the Deployment already exists, the upgrade will fail with an error like field is immutable. The Deployment must be deleted and recreated, which causes downtime.
Consider whether a migration note or a helm.sh/hook pre-upgrade delete is needed, or whether this change should be gated behind a new Deployment name.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clearnode/chart/templates/deployment.yaml` around lines 14 - 17, You're
adding a new label key app.kubernetes.io/component: server to
spec.selector.matchLabels in the Deployment template
(spec.selector.matchLabels), which is an immutable field and will break
upgrades; fix by either (a) revert this selector label change and keep selectors
identical to existing releases, (b) if the label is required, document a
migration step and implement a safe upgrade path such as adding a Helm
pre-upgrade hook to delete the old Deployment before creating the new one, or
(c) gate the change behind a new chart release name / new Deployment
metadata.name so Kubernetes treats it as a new object; apply the chosen approach
and update Chart NOTES.md to describe the migration if you choose
delete/migration or change the release name.
2 participants
Summary by CodeRabbit
New Features
Refactor
Improvements