fix(firehose-evm): fixes and clean-up #364
Conversation
WalkthroughThe pull request introduces updates to the Firehose Ethereum application configuration files, including version increments, new parameters, and modifications to existing ones. Key changes involve the restructuring of environment variable references, updates to initialization containers, and adjustments to service dependencies, enhancing the overall configurability of the application. Changes
Possibly related PRs
Suggested reviewers
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (2)
Additional context usedLanguageTool
Markdownlint
Additional comments not posted (8)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
charts/firehose-ethereum/templates/resources/workload.yaml (1)
170-177: Approve with suggestions: Command specification enhancementThe implementation for specifying custom commands in the Pod is a valuable addition. It enhances the flexibility of the chart by allowing users to override default commands when necessary.
However, there are a few suggestions for improvement:
- Fix indentation:
Thecommand:line should be indented to match the surrounding code. Apply this diff:{{- with .Pod.command }} {{- if not (empty .) }} - command: + command: {{- range $element := . }} - {{ $element }} {{- end }} {{- end }} {{- end }}
- Add a comment:
Consider adding a brief comment explaining the purpose of this block, e.g.:{{- with .Pod.command }} {{- if not (empty .) }} # Override default command if specified command: {{- range $element := . }} - {{ $element }} {{- end }} {{- end }} {{- end }}
- Input validation:
Consider adding input validation for.Pod.commandto ensure it contains valid command strings. This could be done in the chart's values validation or by adding a helper template.charts/firehose-ethereum/values.yaml (1)
462-463: Approve memory-backed volume, but monitor usageThe change to use a memory-backed emptyDir for the
config-processedvolume is a good optimization for performance. This can significantly speed up operations that frequently access this volume.However, be aware that this will increase the memory usage of the pod. Ensure that the nodes have sufficient memory to handle this, and consider setting appropriate resource limits. It's also recommended to monitor memory usage closely after deploying this change to ensure it doesn't lead to unexpected resource pressure.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- charts/firehose-ethereum/README.md (3 hunks)
- charts/firehose-ethereum/templates/_lib.metadata.tpl (1 hunks)
- charts/firehose-ethereum/templates/resources/workload.yaml (1 hunks)
- charts/firehose-ethereum/values.yaml (6 hunks)
Additional context used
LanguageTool
charts/firehose-ethereum/README.md
[uncategorized] ~191-~191: Loose punctuation mark.
Context: ...ap | ConfigMap customization | object | `{"enabled":true,"metadata":{"annotations":{},"labels":{}},"options":{"template":"{{- $nodeArgs := false }}\nstart:\n args:\n {{- range .Pod.fireeth.services }}\n - {{ . }}\n {{- if (contains "node" .) }}\n...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~194-~194: Loose punctuation mark.
Context: ...| ConfigMap specific options | object | `{"template":"{{- $nodeArgs := false }}\nstart:\n args:\n {{- range .Pod.fireeth.services }}\n - {{ . }}\n {{- if (contains "node" .) }}\n...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~195-~195: Loose punctuation mark.
Context: ...plate for the fireeth config | string | `"{{- $nodeArgs := false }}\nstart:\n args:\n {{- range .Pod.fireeth.services }}\n - {{ . }}\n {{- if (contains "node" .) }}\n...(UNLIKELY_OPENING_PUNCTUATION)
[style] ~277-~277: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...faults.volumes | Pod volumes | object | `{"config":{"configMap":{"defaultMode":420,"name":"{{ include "metadata.fullname" $ }}-{{ .componentName }}-config"},"enabled":"{{ .Pod.configMap.enabled }}"},"config-processed":{"emptyDir":{"medium":"Memory"},"enabled":"{{ and .Pod.configMap.options.useEnvSubst .Pod.conf...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~279-~279: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...rehoseDefaults> is available | object | `{"firehose":{"fireeth":{"config":{"firehose-grpc-listen-addr":"0.0.0.0:10015"}},"ports":{"fh-grpc":{"containerPort":"{{ with .Pod.fireeth.config }}{{ if (index . "firehose-grpc-listen-addr") }}{{ splitList ":" (index . "firehose-grpc-listen-addr") | last | int }}{{ else }}{{ nil }}{{ end }}{{ end }}","protocol":"TCP"}},"service":{"enabled":true,"spec":{"ports":{"fh-grpc":{"port":"{{ with .Pod.fireeth.config }}{{ if (index . "firehose-grpc-listen-addr") }}{{ splitList ":" (index . "firehose-grpc-listen-addr") | last | int }}{{ else }}{{ nil }}{{ end }}{{ end }}","protocol":"TCP"}}}}},"index-builder":{"fireeth":{"config":{"index-builder-grpc-listen-addr":"0.0.0.0:10009","index-builder-index-size":"1000"}},"ports":{"index-grpc":{"containerPort":"{{ with .Pod.fireeth.config }}{{ if (index . "index-builder-grpc-listen-addr") }}{{ splitList ":" (index . "index-builder-grpc-listen-addr") | last | int }}{{ else }}{{ nil }}{{ end }}{{ end }}","protocol":"TCP"}},"service":{"enabled":true,"spec":{"ports":{"index-grpc":{"port":"{{ with .Pod.fireeth.config }}{{ if (index . "index-builder-grpc-listen-addr") }}{{ splitList ":" (index . "index-builder-grpc-listen-addr") | last | int }}{{ else }}{{ nil }}{{ end }}{{ end }}","protocol":"TCP"}}}}},"merger":{"fireeth":{"config":{"merger-grpc-listen-addr":":10012"}},"ports":{"merger-grpc":{"containerPort":"{{ with .Pod.fireeth.config }}{{ if (index . "merger-grpc-listen-addr") }}{{ splitList ":" (index . "merger-grpc-listen-addr") | last | int }}{{ else }}{{ nil }}{{ end }}{{ end }}","protocol":"TCP"}},"service":{"enabled":true,"spec":{"ports":{"merger-grpc":{"port":"{{ with .Pod.fireeth.config }}{{ if (index . "merger-grpc-listen-addr") }}{{ splitList ":" (index . "merger-grpc-listen-addr") | last | int }}{{ else }}{{ nil }}{{ end }}{{ end }}","protocol":"TCP"}}}}},"reader-node":{"clusterRbac":{"enabled":"{{ .Pod.fireeth.p2p.enabled }}","roleSpec":{"rules":[{"apiGroups":[""],"resources":["nodes"],"verbs":["get","list","watch"]}]}},"configMap":{"data":"{{ with .Pod.fireeth.genesisJson }}{{ .enabled | ternary ( .data | toYaml | nindent 8 ) nil }}{{ end }}","options":{"useEnvSubst":true}},"env":{"MANAGER_API_PORT":"{{ with .Pod.fireeth.config }}{{ hasKey . "reader-n...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~283-~283: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... for the reader-node service | object | `{"clusterRbac":{"enabled":"{{ .Pod.fireeth.p2p.enabled }}","roleSpec":{"rules":[{"apiGroups":[""],"resources":["nodes"],"verbs":["get","list","watch"]}]}},"configMap":{"data":"{{ with .Pod.fireeth.genesisJson }}{{ .enabled | ternary ( .data | toYaml | nindent 8 ) nil }}{{ end }}","options":{"useEnvSubst":true}},"env":{"MANAGER_API_PORT":"{{ with .Pod.fireeth.config }}{{ hasKey . "reader-n...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~286-~286: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...node.ports | Container ports | object | `{"node-authrpc":{"containerPort":"{{ with .Pod.fireeth.nodeArgs }}{{ hasKey . "authrpc.port" | ternary (index . "authrpc.port" | int) nil }}{{ end }}","protocol":"TCP"},"node-metrics":{"containerport":"{{ with .Pod.fireeth.nodeMetrics }}{{ if .port }}{{ .port | int }}{{ else }}null{{ end }}{{ end }}","protocol":"TCP"},"node-mgr":{"containerPort":"{{ splitList ":" ( index .Pod.fireeth.config "reader-node-manager-api-addr" ) | last | int }}","protocol":"TCP"},"node-rpc":{"containerPort":"{{ with .Pod.fireeth.nodeArgs }}{{ hasKey . "http.p...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
Markdownlint
charts/firehose-ethereum/README.md
207-207: null
Spaces inside emphasis markers(MD037, no-space-in-emphasis)
Additional comments not posted (13)
charts/firehose-ethereum/templates/_lib.metadata.tpl (1)
68-68: LGTM! Consider verifying the.componentNamevariable.The addition of the
app.kubernetes.io/componentlabel is a good practice and aligns with Kubernetes recommended label set. It will enhance the granularity of component identification within your Kubernetes resources.To ensure the
.componentNamevariable is correctly set and used, you may want to run the following verification:This will help identify where
.componentNameis defined and used, ensuring it's properly set for all components.Verification successful
LGTM! The
.componentNamevariable is correctly defined and used across the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage and setting of .componentName variable # Test: Search for places where .componentName is set or used rg --type yaml -g '*.yaml' -g '*.yml' -g '*.tpl' '.componentName' # Test: Check if there are any cases where the component label might be empty rg --type yaml -g '*.yaml' -g '*.yml' -g '*.tpl' "app.kubernetes.io/component: ''"Length of output: 30985
charts/firehose-ethereum/values.yaml (6)
333-338: LGTM: Improved init container configurationThe changes to the
enabledfields for the init containers are good improvements. They now dynamically determine whether to enable the containers based on specific pod configurations (fireeth.p2p.enabledandconfigMap.options.useEnvSubst). This allows for more flexible and configuration-driven deployments.
373-375: Clarify the purpose of the empty command arrayAn empty command array has been added to the configuration. This could potentially override the default command of the container, which might lead to unexpected behavior.
Could you please clarify the intention behind adding this empty command array? If it's meant to be populated later or in certain conditions, consider adding a comment explaining its purpose. If it's not needed, it might be better to remove it to avoid confusion.
Line range hint
1-824: Summary of changes and recommendationsOverall, the changes in this file appear to be positive improvements to the Firehose Ethereum configuration. Key points include:
- More dynamic init container configuration
- Updated naming conventions for Kubernetes secrets
- Performance optimization with memory-backed volumes
- More specific relayer source configuration
- Updated transaction history configuration
While most changes are approved, please ensure to:
- Verify the removal of
FIREETH_COMMON_INDEX_STORE_URLdoesn't impact the system negatively- Clarify the purpose of the empty command array
- Monitor memory usage after implementing the memory-backed volume
- Verify consistency of the new relayer source name across the system
- Confirm compatibility of the new transaction history configuration with your Ethereum node software
After addressing these points, the changes should significantly improve the flexibility and clarity of your Firehose Ethereum deployment.
524-524: Approve updated relayer source, verify consistencyThe change to the
relayer-sourceconfiguration, updating it from "reader:10010" to "firehose-ethereum-1-reader:10010", appears to be a more specific and descriptive naming convention. This is generally a good practice as it reduces ambiguity in a potentially complex system.Please ensure that this new source name "firehose-ethereum-1-reader" is consistently used across the entire system, including in any documentation, other configuration files, or code that might reference the relayer source. Run the following command to check for any inconsistencies:
#!/bin/bash # Check for occurrences of both old and new relayer source names rg "reader:10010|firehose-ethereum-1-reader:10010" --type yaml --type go
608-608: Approve updated transaction history configuration, verify compatibilityThe change from
txlookuplimit: 1000tohistory.transactions: 1000appears to be an update to the configuration format for specifying transaction history limits. The value remains the same, suggesting this is primarily a syntactic change.Please verify that this new configuration key (
history.transactions) is compatible with the current version of the Ethereum node software being used. It's important to ensure that this change doesn't inadvertently alter the behavior of the node.You may want to check the Ethereum node's documentation or run a test deployment to confirm that the transaction history is still limited as expected. Additionally, search the codebase for any other occurrences of
txlookuplimitthat might need to be updated:#!/bin/bash # Check for occurrences of both old and new transaction history configuration rg "txlookuplimit|history.transactions" --type yaml --type go
348-348: Approve naming convention change and verify removed env varThe change from
SecretKeyReftosecretKeyRefaligns with Kubernetes naming conventions, which is good.However, the removal of the
FIREETH_COMMON_INDEX_STORE_URLenvironment variable configuration needs verification.Please confirm that the removal of
FIREETH_COMMON_INDEX_STORE_URLis intentional and doesn't negatively impact the system's ability to locate the index store. If this configuration has been moved elsewhere or is no longer needed, please update the documentation accordingly.Verification successful
Removal of
FIREETH_COMMON_INDEX_STORE_URLConfirmed SafeThe
FIREETH_COMMON_INDEX_STORE_URLenvironment variable is not referenced elsewhere in the codebase. Its removal does not impact the system's ability to locate the index store.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if FIREETH_COMMON_INDEX_STORE_URL is used elsewhere in the codebase rg "FIREETH_COMMON_INDEX_STORE_URL" --type yaml --type goLength of output: 243
Script:
#!/bin/bash # Check if FIREETH_COMMON_INDEX_STORE_URL is used elsewhere in the codebase rg "FIREETH_COMMON_INDEX_STORE_URL" --type yaml --type goLength of output: 57
charts/firehose-ethereum/README.md (6)
190-190: Approval: New parameter for custom container entrypointThe addition of
firehoseComponentDefaults.commandas a list type parameter is a good improvement. It allows users to specify a custom container entrypoint, providing more flexibility in how the Firehose-Ethereum containers are initialized and run.
198-204: Approval: Enhanced S3 storage configurationThe updates to
firehoseComponentDefaults.envFromare beneficial:
- New environment variables for different S3 bucket URLs (forked, merged, and one blocks) provide more granular control over storage locations.
- The change from
SecretKeyReftosecretKeyRefaligns with Kubernetes naming conventions.These modifications offer better flexibility in configuring S3 storage for different block types and improve adherence to Kubernetes standards.
234-234: Approval: Improved flexibility in init container configurationThe update to
firehoseComponentDefaults.initContainersis a positive change:
- It introduces dynamic enabling of init containers based on
Pod.fireeth.p2p.enabledandPod.configMap.options.useEnvSubst.- This change allows for more flexible and conditional initialization of P2P and environment substitution features.
This modification enhances the adaptability of the deployment to different configuration scenarios, potentially reducing resource usage when certain features are not needed.
277-277: Approval: Enhanced volume configurationsThe updates to
firehoseComponentDefaults.volumesbring valuable improvements:
- A new
config-processedvolume has been added, allowing for dynamic processing of configurations.- An
env-dirvolume has been introduced, which can be used for storing environment variables.These additions provide more flexibility in managing configurations and environment variables, potentially improving the adaptability and maintainability of the Firehose-Ethereum deployment.
Tools
LanguageTool
[style] ~277-~277: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...faults.volumes | Pod volumes | object | `{"config":{"configMap":{"defaultMode":420,"name":"{{ include "metadata.fullname" $ }}-{{ .componentName }}-config"},"enabled":"{{ .Pod.configMap.enabled }}"},"config-processed":{"emptyDir":{"medium":"Memory"},"enabled":"{{ and .Pod.configMap.options.useEnvSubst .Pod.conf...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
206-209: Verify the impact of configuration changesSeveral important changes have been made to
firehoseComponentDefaults.fireeth.config:
- The
common-index-block-sizesparameter has been removed, which may affect indexing behavior.- New rate limiting parameters (
firehose-rate-limit-bucket-fill-rateandfirehose-rate-limit-bucket-size) have been added.- Other configuration parameters have been modified.
These changes could significantly impact the performance and behavior of the Firehose-Ethereum application.
Please ensure that:
- The removal of
common-index-block-sizesdoesn't negatively affect the indexing process.- The new rate limiting parameters are properly configured for your use case.
- Other modified parameters are set to appropriate values.
Run the following script to check for any references to the removed parameter and to verify the new configuration:
#!/bin/bash # Check for any remaining references to common-index-block-sizes echo "Checking for references to common-index-block-sizes:" rg --type yaml "common-index-block-sizes" # Display the new configuration echo "New fireeth configuration:" rg --type yaml "firehose-rate-limit-bucket-fill-rate|firehose-rate-limit-bucket-size"Tools
Markdownlint
207-207: null
Spaces inside emphasis markers(MD037, no-space-in-emphasis)
279-287: Verify the updated relayer source configurationAn important change has been made to the
firehoseServiceDefaults.relayerconfiguration:
- The
relayer-sourcehas been updated fromreader:10010tofirehose-ethereum-1-reader:10010.This modification suggests a change in the service architecture or naming convention for the reader service.
Please ensure that:
- The new relayer source address (
firehose-ethereum-1-reader:10010) is correct and accessible within your cluster.- Any dependent services or configurations that may reference the old reader address are updated accordingly.
Run the following script to check for any references to the old and new reader addresses:
Verification successful
Relayer source configuration successfully updated
The
firehoseServiceDefaults.relayerconfiguration has been correctly updated:
- The
relayer-sourceis now set tofirehose-ethereum-1-reader:10010.- No references to the old reader address
reader:10010were found in the codebase.This confirms that the old address has been removed and the new configuration is properly applied.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash echo "Checking for references to the old reader address:" rg --type yaml "reader:10010" echo "Checking for references to the new reader address:" rg --type yaml "firehose-ethereum-1-reader:10010"Length of output: 472
Script:
#!/bin/bash echo "Verifying references to the old reader address 'reader:10010':" rg --type yaml "reader:10010" charts/firehose-ethereum/ echo "Verifying references to the new reader address 'firehose-ethereum-1-reader:10010':" rg --type yaml "firehose-ethereum-1-reader:10010" charts/firehose-ethereum/Length of output: 645
Tools
LanguageTool
[style] ~279-~279: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...rehoseDefaults> is available | object | `{"firehose":{"fireeth":{"config":{"firehose-grpc-listen-addr":"0.0.0.0:10015"}},"ports":{"fh-grpc":{"containerPort":"{{ with .Pod.fireeth.config }}{{ if (index . "firehose-grpc-listen-addr") }}{{ splitList ":" (index . "firehose-grpc-listen-addr") | last | int }}{{ else }}{{ nil }}{{ end }}{{ end }}","protocol":"TCP"}},"service":{"enabled":true,"spec":{"ports":{"fh-grpc":{"port":"{{ with .Pod.fireeth.config }}{{ if (index . "firehose-grpc-listen-addr") }}{{ splitList ":" (index . "firehose-grpc-listen-addr") | last | int }}{{ else }}{{ nil }}{{ end }}{{ end }}","protocol":"TCP"}}}}},"index-builder":{"fireeth":{"config":{"index-builder-grpc-listen-addr":"0.0.0.0:10009","index-builder-index-size":"1000"}},"ports":{"index-grpc":{"containerPort":"{{ with .Pod.fireeth.config }}{{ if (index . "index-builder-grpc-listen-addr") }}{{ splitList ":" (index . "index-builder-grpc-listen-addr") | last | int }}{{ else }}{{ nil }}{{ end }}{{ end }}","protocol":"TCP"}},"service":{"enabled":true,"spec":{"ports":{"index-grpc":{"port":"{{ with .Pod.fireeth.config }}{{ if (index . "index-builder-grpc-listen-addr") }}{{ splitList ":" (index . "index-builder-grpc-listen-addr") | last | int }}{{ else }}{{ nil }}{{ end }}{{ end }}","protocol":"TCP"}}}}},"merger":{"fireeth":{"config":{"merger-grpc-listen-addr":":10012"}},"ports":{"merger-grpc":{"containerPort":"{{ with .Pod.fireeth.config }}{{ if (index . "merger-grpc-listen-addr") }}{{ splitList ":" (index . "merger-grpc-listen-addr") | last | int }}{{ else }}{{ nil }}{{ end }}{{ end }}","protocol":"TCP"}},"service":{"enabled":true,"spec":{"ports":{"merger-grpc":{"port":"{{ with .Pod.fireeth.config }}{{ if (index . "merger-grpc-listen-addr") }}{{ splitList ":" (index . "merger-grpc-listen-addr") | last | int }}{{ else }}{{ nil }}{{ end }}{{ end }}","protocol":"TCP"}}}}},"reader-node":{"clusterRbac":{"enabled":"{{ .Pod.fireeth.p2p.enabled }}","roleSpec":{"rules":[{"apiGroups":[""],"resources":["nodes"],"verbs":["get","list","watch"]}]}},"configMap":{"data":"{{ with .Pod.fireeth.genesisJson }}{{ .enabled | ternary ( .data | toYaml | nindent 8 ) nil }}{{ end }}","options":{"useEnvSubst":true}},"env":{"MANAGER_API_PORT":"{{ with .Pod.fireeth.config }}{{ hasKey . "reader-n...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~283-~283: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... for the reader-node service | object | `{"clusterRbac":{"enabled":"{{ .Pod.fireeth.p2p.enabled }}","roleSpec":{"rules":[{"apiGroups":[""],"resources":["nodes"],"verbs":["get","list","watch"]}]}},"configMap":{"data":"{{ with .Pod.fireeth.genesisJson }}{{ .enabled | ternary ( .data | toYaml | nindent 8 ) nil }}{{ end }}","options":{"useEnvSubst":true}},"env":{"MANAGER_API_PORT":"{{ with .Pod.fireeth.config }}{{ hasKey . "reader-n...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~286-~286: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...node.ports | Container ports | object | `{"node-authrpc":{"containerPort":"{{ with .Pod.fireeth.nodeArgs }}{{ hasKey . "authrpc.port" | ternary (index . "authrpc.port" | int) nil }}{{ end }}","protocol":"TCP"},"node-metrics":{"containerport":"{{ with .Pod.fireeth.nodeMetrics }}{{ if .port }}{{ .port | int }}{{ else }}null{{ end }}{{ end }}","protocol":"TCP"},"node-mgr":{"containerPort":"{{ splitList ":" ( index .Pod.fireeth.config "reader-node-manager-api-addr" ) | last | int }}","protocol":"TCP"},"node-rpc":{"containerPort":"{{ with .Pod.fireeth.nodeArgs }}{{ hasKey . "http.p...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
add componentName to selector labels
add command override to container
fixes and clean-up
Summary by CodeRabbit
New Features
Bug Fixes
Chores