memprofile: add http debug test #7
Conversation
eve-kernel-amd64-v6.1.38-generic
8859f43ee8c9: eve_defconfig: Enable PSI (Pressure Stall Information).
eve-kernel-amd64-v6.1.38-rt
37bc37278441: eve_defconfig: Enable PSI (Pressure Stall Information).
eve-kernel-arm64-v5.10.104-nvidia
7b9a55e0b659: defconfig: Enable PSI in default configuration.
eve-kernel-arm64-v6.1.38-generic
acbfadebeb76: eve_defconfig: Enable PSI in default configuration.
bb8ed98284d6: eve_defconfig: Remove ST33HTPM support.
The second commit in the 5.10 kernel update is not logically a part of this
patch set, but a leftover from a previous one.
Signed-off-by: Nikolay Martyanov <nikolay@zededa.com>
This commit introduces a Pressure Stall Information (PSI) collector to the AgentLog component in Pillar. The collector periodically gathers memory pressure data from the system, logging it in a managed file. The code also includes functionality for handling the file size to ensure that only recent data is kept. This collector is designed to be shared between the AgentLog component and a future standalone tool for enhanced memory pressure monitoring. Signed-off-by: Nikolay Martyanov <nikolay@zededa.com>
This commit introduces new constants for the memory-monitor directories within the Pillar codebase. Specifically, it adds paths for the memory-monitor output directory and the PSI log file. This change ensures that Pillar components are aware of and can correctly reference the locations used by the memory-monitor. Signed-off-by: Nikolay Martyanov <nikolay@zededa.com>
This commit adds functionality to the Pillar internal debug API, allowing the start and stop of the Memory PSI collector via HTTP POST requests. New endpoints `/memory-monitor/psi-collector/start` and `/memory-monitor/psi-collector/stop` have been introduced, enabling control over the PSI collector process. Signed-off-by: Nikolay Martyanov <nikolay@zededa.com>
…ript. This commit extends the `eve` script with new commands `psi-collector start|stop`. These commands interact with the internal debug API to start and stop the Memory PSI collector, respectively. This addition makes it convenient to control the PSI collector directly from the terminal, with the results stored in `/persist/memory-monitor/output/psi.txt`. Signed-off-by: Nikolay Martyanov <nikolay@zededa.com>
This commit adds a standalone version of the PSI Collector, allowing it to be built and used independently on older versions of EVE where the collector is not integrated into the root filesystem. The new component includes a Makefile for building the binary, a README with instructions, and a basic main.go implementation that handles the PSI collection process. This ensures compatibility and provides memory pressure monitoring capabilities for legacy systems. Signed-off-by: Nikolay Martyanov <nikolay@zededa.com>
This commit adds a new tool, `psi-visualizer`, designed to visualize PSI data, which is essential for understanding memory pressure patterns over time. The tool uses Python with Plotly to create interactive plots from PSI logs generated by the PSI Collector. The visualizer includes a Makefile for environment setup, a `requirements.txt` for dependencies, and a `visualize.py` script that reads PSI data and generates plots. A README is also included to guide users through setup and usage, making it easier to analyze memory pressure visually. Signed-off-by: Nikolay Martyanov <nikolay@zededa.com>
WalkthroughThe changes enhance the Changes
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
|
@coderabbitai review |
Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (2)
pkg/pillar/agentlog/agentlog_test.go (1)
248-249: Consider making the HTTP server address configurable.Hardcoding the server address can lead to conflicts in shared environments.
- go agentlog.ListenDebug(log, "stackdump", "memdump") + serverAddress := "localhost:6543" + go agentlog.ListenDebug(log, serverAddress, "stackdump", "memdump")pkg/pillar/agentlog/memprofile.go (1)
44-44: Document the intended use cases for modifyingPressureMemoryFile.Changing this from a constant to a variable allows runtime modifications, which should be controlled and documented to prevent unintended side effects.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- pkg/pillar/agentlog/agentlog.go (4 hunks)
- pkg/pillar/agentlog/agentlog_test.go (2 hunks)
- pkg/pillar/agentlog/memprofile.go (2 hunks)
Additional comments not posted (2)
pkg/pillar/agentlog/agentlog.go (2)
8-8: Good use of theerrorspackage for improved error handling.The addition of the
errorspackage allows for more precise error checking usingerrors.Is.
Line range hint
237-342:
ExportingListenDebugand refining error handling enhances functionality.The changes improve accessibility and robustness. Ensure that the function is used appropriately in the codebase.
Verification successful
Exporting
ListenDebugis currently contained within the package.The function
ListenDebugis used internally within the package and is tested inagentlog_test.go. There are no external usages impacting other parts of the codebase.
agentlog_test.go: TestsListenDebugby starting and stopping the HTTP service.agentlog.go: Internal usage ofListenDebug.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify new usages of `ListenDebug` in the codebase. # Test: Search for usages of `ListenDebug`. Expect: Occurrences of the new function name. rg --type go -A 5 $'ListenDebug'Length of output: 1169
| mpfh, err := os.CreateTemp("", "memory-pressure") | ||
| if err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
Use t.Fatalf instead of panic for error handling in tests.
Using t.Fatalf provides better test output and integration with testing tools.
- if err != nil {
- panic(err)
- }
+ if err != nil {
+ t.Fatalf("unexpected error: %v", err)
+ }Also applies to: 239-241, 255-259, 261-264, 268-271, 273-276, 293-296
There was a problem hiding this comment.
Do we really need these "better test output and integration with testing tools" in the scope of this test?
There was a problem hiding this comment.
@OhmSpectator, using t.Fatalf instead of panic in tests is generally recommended because it provides better integration with the Go testing framework. It allows for more informative test output and ensures that the test is properly marked as failed, which can be particularly beneficial in automated testing environments or CI/CD pipelines. However, if these benefits are not crucial for your current testing scope, you might choose to keep the existing implementation. Ultimately, the decision depends on your project's specific needs and constraints.
There was a problem hiding this comment.
I think it makes sense to use both panic and t.Fatalf as this way, we can distinguish between the errors caused by generic system failure (testing env is broken - use panic) and real test failure (use t.Fatalf for that). Or are there better approaches?
There was a problem hiding this comment.
@OhmSpectator, your approach to using both panic and t.Fatalf to differentiate between system failures and test failures is understandable. However, using panic in tests can lead to abrupt exits, which might skip other tests and make it harder to gather comprehensive results.
A more structured approach could involve using custom error handling or logging to categorize errors. You could log system-related errors with a specific tag or message, making it clear they are not test failures. Additionally, using t.Skip or t.Skipf for known environmental issues can help manage test execution without resorting to panic. This way, you maintain the clarity of test results while avoiding the potential downsides of using panic.
There was a problem hiding this comment.
Give me an example of such handling...
There was a problem hiding this comment.
@OhmSpectator, here's an example of how you can handle errors in tests using structured logging and custom error handling instead of panic. This approach helps distinguish between system-related issues and actual test failures:
package mypackage
import (
"log"
"os"
"testing"
)
func TestExample(t *testing.T) {
// Simulate an environmental check
if !isEnvironmentReady() {
log.Println("Environment not ready, skipping test")
t.Skip("Skipping test due to environmental issues")
}
// Simulate a test operation
file, err := os.Open("testfile.txt")
if err != nil {
log.Printf("System error: %v", err)
t.Fatalf("Failed to open test file: %v", err)
}
defer file.Close()
// Further test logic...
}
// isEnvironmentReady simulates an environmental check
func isEnvironmentReady() bool {
// Add logic to verify if the environment is ready
// Return false if not ready
return true
}In this example, log.Println and log.Printf are used to log messages with specific information about the error type. t.Skip is used to skip the test if the environment is not ready, while t.Fatalf handles actual test failures. This way, you maintain clear and structured test results without using panic.
| @@ -42,6 +40,9 @@ const ( | |||
| StatLineSize = 75 | |||
| ) | |||
|
|
|||
| // PressureMemoryFile is the memory pressure file | |||
| var PressureMemoryFile = "/proc/pressure/memory" | |||
There was a problem hiding this comment.
@christoph-zededa, I'm not too fond of making such a value a variable. It's a constant. Is there a way to mock a constant in Go? Or maybe we can introduce conditional builds with different values for test env and prod env?
There was a problem hiding this comment.
We could f.e. make the path a parameter to listenDebug and when we invoke it from the tests, we can give it a different path than in the real system.
There was a problem hiding this comment.
Yeah... It looks like a proper solution...
There was a problem hiding this comment.
On the other hand, as I consider adding parameters to all the functions, I start thinking about the possibility of collecting PSI stats for not just memory but also CPU and IO. In that case, we could make the PSI collector more generic, move it to another file, or even a different component. However, I then realize that we don't actually need all of that right now, and I hesitate to redo all the code now.
So, can we mock a constant at this point?
| panic(err) | ||
| } | ||
|
|
||
| err = os.MkdirAll(types.MemoryMonitorOutputDir, 0755) |
There was a problem hiding this comment.
It will fail to create a directory at /persist on a host system...
There was a problem hiding this comment.
Or is it supposed to run only in a container? How do you start the tests? Not with go test?
There was a problem hiding this comment.
os.MkdirAll is like mkdir -p it won't fail if the directory already exists.
I start the test either from an ubuntu container lf-edge/eve@master...christoph-zededa:eve:rr or from the eve-build container or the pillar container itself.
make test runs all tests in a container - so it takes a while ...
There was a problem hiding this comment.
Aha... In the pillar dir, I can run make build-docker-env enter-docker-env, and the tests work there. It's not very convenient, but it works.
|
I find the approach useful, but we still have to integrate it better... |
|
@christoph-zededa, I wantd to push an extra commit on top of the PR, but got this |
I guess you either have to create a PR into my branch which is a PR into your branch. |
I wanted to draft the approach in this PR. |
OhmSpectator
force-pushed
the
feature/add-mem-pressure-statistics-collector
branch
7 times, most recently
from
71c2091
to
19f2306
Compare
OhmSpectator
force-pushed
the
feature/add-mem-pressure-statistics-collector
branch
3 times, most recently
from
0969b2b
to
122406e
Compare
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
listenDebugtoListenDebugto increase its accessibility.PressureMemoryFilefrom a constant to a variable for runtime modifications.