OCPBUGS-80952: perf: latenyc: compute memory resources dynamically#1517
OCPBUGS-80952: perf: latenyc: compute memory resources dynamically#1517shajmakh wants to merge 1 commit into
Conversation
WalkthroughThis PR adds configurable memory allocation for latency test pods via the ChangesLatency Test Memory Configuration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 9 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (9 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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: shajmakh The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![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
🤖 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 `@test/e2e/performanceprofile/functests/4_latency/latency.go`:
- Around line 287-289: The check in getLatencyTestMemory that returns
defaultTestMemory when cpus == defaultTestCpus is unreachable given the current
call-site logic (latencyTestCpus is normalized before calling), so either remove
that branch to simplify getLatencyTestMemory (delete the if cpus ==
defaultTestCpus { return defaultTestMemory, nil } case) or keep it but add a
short comment on the cpus parameter explaining this is defensive for future
callers (mentioning defaultTestCpus and why it might still be passed) so readers
know the branch is intentional; locate getLatencyTestMemory and update
accordingly.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c80f1a1c-34b6-48e4-95b1-f963938d2729
📒 Files selected for processing (1)
test/e2e/performanceprofile/functests/4_latency/latency.go
shajmakh
changed the title
openshift-ci-robot
added
jira/severity-critical
|
@shajmakh: This pull request references Jira Issue OCPBUGS-80952, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
When CPUs are very high the pod's fixed memory resources may become too low to run the latency checks. Add an environment variable to allow more flexibility while preserving the old behavior for backward compatibility. The new behavior goes like this: 1. If no env var is set, keep the default old behavior (1Gi) 2. else, if it was set to a specific memory value and it's valid quantity then use that in the latency pod, otherwise throw an error. If the env var value was set to `dynamic`, the test will compute the memory by (number of computed CPUs * 32Mi). `32Mi` was picked based on input from consumers of the application; If happened that the memory is still not enough, the user has the flexibility to override the total memory with an explicit value. Signed-off-by: Shereen Haj <shajmakh@redhat.com>
openshift-ci-robot
added
jira/valid-bug
|
@shajmakh: This pull request references Jira Issue OCPBUGS-80952, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/retest |
|
/retest-required |
|
@shajmakh: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/retest-required |
4 participants
When CPUs are very high the pod's fixed memory resources may become too
low to run the latency checks. Add an environment variable to allow more
flexibility while preserving the old behavior for backward
compatibility.
The new behavior goes like this:
quantity then use that in the latency pod, otherwise throw an error.
If the env var value was set to
dynamic, the test will compute thememory by (number of computed CPUs * 32Mi).
32Miwas picked based on input from consumers of the application; Ifhappened that the memory is still not enough, the user has the
flexibility to override the total memory with an explicit value.
Summary by CodeRabbit
New Features
LATENCY_TEST_MEMORYenvironment variable, enabling fine-tuned resource allocation.Documentation