[e2e] new test cases for guest memory overhead#2640
Conversation
Signed-off-by: Lanfon Fan <lanfon.fan@suse.com>
Signed-off-by: Lanfon Fan <lanfon.fan@suse.com>
There was a problem hiding this comment.
Pull request overview
This PR adds automated coverage for Harvester’s additional guest memory overhead ratio setting, including new API-level validation tests and an integration test that checks the setting’s effect on the VM launcher pod’s memory limit. It also updates the “expected settings” list to include newly supported settings.
Changes:
- Added API tests for
additional-guest-memory-overhead-ratio(get/update with valid/invalid values). - Added VM integration tests validating memory overhead behavior at default and doubled ratios.
- Updated the expected settings set to include settings introduced in newer Harvester versions.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
harvester_e2e_tests/integrations/test_3_vm_functions.py |
Adds a fixture and integration tests that measure VM pod memory overhead under different ratio settings. |
harvester_e2e_tests/fixtures/api_client.py |
Updates the expected_settings fixture to include additional settings (including version-introduced ones). |
harvester_e2e_tests/apis/test_settings.py |
Adds API tests for the new overhead ratio setting and adjusts imports accordingly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| code, data = api_client.settings.get() | ||
| assert 200 == code, (code, data) | ||
|
|
||
| origin_val = data.get('value', "") |
There was a problem hiding this comment.
code, data = api_client.settings.get(setting_name)
| assert overhead - overhead_ratio['_overhead'] * 2 < 1000, ( | ||
| "additional-guest-memory-overhead-ratio not working as expected after set to double\n" | ||
| f"expected overhead: {overhead_ratio['_overhead']*2}, actual overhead: {overhead}" |
There was a problem hiding this comment.
Could you help explain why overhead - overhead_ratio['_overhead'] * 2 < 1000?
Is 1000 the approximate value of twice the default overhead (240 * 1.5 * 2) ?
There was a problem hiding this comment.
# from previous test case
N = memory + 1.5 ratio
`_overhead` = N - memory
# in this test case
N2 = memory + 3 ratio
`overhead` = N2 - memorythus, overhead should be double value of _overhead, but consider the floating bytes, I use 1000 (~1KiB) to be the tolerance value.
Coplit's suggestion looks acceptable and more readable, I will run some tests to check whether we need 1MiB or lower.
| for i in vmi['metadata']['relationships']: | ||
| if i.get('toType') == "pod": | ||
| ns, name = i['toId'].split("/") | ||
| break | ||
| else: | ||
| raise AssertionError( | ||
| f"Unable to find the pod name of VM {vm_name}\n" | ||
| f"VMI info: {vmi}" | ||
| ) | ||
|
|
||
| code, data = api_client.get_pods(name, ns) | ||
| assert 200 == code, (code, data) | ||
|
|
||
| pod_mem = int(data['spec']['containers'][0]['resources']['limits']['memory']) | ||
|
|
||
| overhead = pod_mem - self._mem | ||
| assert overhead - overhead_ratio['_overhead'] * 2 < 1000, ( | ||
| "additional-guest-memory-overhead-ratio not working as expected after set to double\n" | ||
| f"expected overhead: {overhead_ratio['_overhead']*2}, actual overhead: {overhead}" | ||
| ) |
| f"Setting {setting_name} not be updated to default value " | ||
| f"when update empty value to it, the updated value: {data['value']}" |
There was a problem hiding this comment.
Maybe we could update the message to When set empty value to {setting_name}, this setting should be set to the default value.
| f"Setting {setting_name} not be updated to default value " | ||
| f"when update empty value to it, the updated value: {data['value']}" |
There was a problem hiding this comment.
Maybe we could update the message to When set empty value to {setting_name}, this setting should be set to the default value.
| assert 200 == code, ( | ||
| f"Failed to update {setting_name} to value {val} with error: {code}, {data}" | ||
| ) | ||
| sleep(0.1) |
There was a problem hiding this comment.
After update, should check value by get.
| code, data = api_client.settings.update(setting_name, {"value": val}) | ||
| assert 422 == code, ( | ||
| f"Setting {setting_name} be updated to the invalid value {val}" | ||
| ) |
There was a problem hiding this comment.
Check value should still original_value.
| code, data = api_client.settings.get() | ||
| assert 200 == code, (code, data) | ||
|
|
||
| origin_val = data.get('value', "") |
There was a problem hiding this comment.
code, data = api_client.settings.get(setting_name)
|
btw, please attach the test report for reference. |
Which issue(s) this PR fixes:
Issue #1497
What this PR does / why we need it:
Special notes for your reviewer:
We documented the overhead will be
240Mi, but as I tested, it's not fixed as our documentation.so test cases implemented to check:
240 * 1.5 MiAdditional documentation or context
ref to https://docs.harvesterhci.io/v1.6/advanced/index/#additional-guest-memory-overhead-ratio