Add hotreload.sh #2110
Add hotreload.sh #2110
Conversation
This script is inspired by the hotreload mode in the main repo. When using a k8s cluster with access to the local filesystem (like kind, k3s, minikube, etc.), it configures the collector container in the collector daemonset to execute a locally built binary, which is useful for iterating fast or running with sanitizers enabled. The script also provides a --revert option for dropping the configuration changes once they are no longer needed.
This new option will compile the collector binary with thread sanitizer support. libtsan is added to the image-dev in order for collector to be able to use it at runtime. libubsan is also added since it was missing.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @Molter73 - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider simplifying the revert logic in
hotreload.shwhich currently relies on multiple index-based patches. - Consider making the in-container collector binary path (
/host/src/cmake-build/collector/collector) configurable inhotreload.sh.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
utilities/hotreload.sh
Outdated
| exit 0 | ||
| ;; | ||
|
|
||
| '-r' | '--hotreload') |
There was a problem hiding this comment.
suggestion: Inconsistent flag naming for revert/hotreload option.
The usage and getopt list use --revert, but the case handles --hotreload—please align them (e.g., use --revert consistently).
| '-r' | '--hotreload') | |
| '-r' | '--revert') |
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #2110 +/- ##
=======================================
Coverage 28.40% 28.40%
=======================================
Files 94 94
Lines 5717 5717
Branches 2531 2531
=======================================
Hits 1624 1624
Misses 3383 3383
Partials 710 710
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -16,7 +16,13 @@ set(CMAKE_CXX_FLAGS_RELEASE "-O3 -fno-strict-aliasing -DNDEBUG") | |||
| if(ADDRESS_SANITIZER) | |||
| set(DISABLE_PROFILING ON) | |||
| set(USE_VALGRIND OFF) | |||
| set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fsanitize=address -fsanitize=undefined -DGRPC_ASAN_SUPRESSED") | |||
| set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fsanitize=address -fsanitize=undefined -DGRPC_ASAN_SUPPRESSED") | |||
There was a problem hiding this comment.
Did you mean to add this change?
There was a problem hiding this comment.
Yes, I did, it has been wrong since we first added it.
Description
This script is inspired by the hotreload mode in the main repo.
When using a k8s cluster with access to the local filesystem (like kind,
k3s, minikube, etc.), it configures the collector container in the
collector daemonset to execute a locally built binary, which is useful
for iterating fast or running with sanitizers enabled.
The script also provides a --revert option for dropping the
configuration changes once they are no longer needed.
Checklist
Automated testing
If any of these don't apply, please comment below.
Testing Performed
Tested locally on my KinD cluster.