-
Notifications
You must be signed in to change notification settings - Fork 6
Verify VIF limit can be reached #338
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,93 @@ | ||
from pkgfixtures import host_with_saved_yum_state | ||
import ipaddress | ||
import logging | ||
import os | ||
import pytest | ||
import tempfile | ||
|
||
# Requirements: | ||
# - one XCP-ng host (--host) >= 8.2 | ||
# - a VM (--vm) | ||
# - the first network on the host can be used to reach the host | ||
|
||
vif_limit = 16 | ||
interface_name = "enX" | ||
vcpus = '8' | ||
|
||
# There is a ResourceWarning due to background=True on an ssh call | ||
# We do ensure the processes are killed | ||
@pytest.mark.filterwarnings("ignore::ResourceWarning") | ||
@pytest.mark.debian_uefi_vm | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This would need the mark to be declared in this commit already. @stormi I'm not yet 100% uptospeed with how VM selection work, but that pattern reminds me something - is it even going to work as expected? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure, i'll reorder the commits There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, regarding VM selection, that's the first time we'd have a marker that asks for a specific distro. But that's not the first time that we have a test that wants a Unix VM with requirements not met by alpine (see vtpm tests). IMO:
Another related topic: should the fixture checking the requirements make the test fail, or skip? In most cases, we'll want the test to fail. It indicates that we called the test with a VM that is not appropriate. However, if we want to include the test in a "multi" job, one that runs on many different VMs, and if the test can run with most unix VMs (for one version of the test) and windows VMs (for another version of the test) then skipping just for the few VMs that are not suitable (alpine...) could be a way. There's also the possibility to reuse existing VM collections that already exclude alpine in It's all a question of compromise between being specific and limiting the amount of jobs and different VM collections in CI. (Another side note: in the future I'd like skipped tests to be considered as failures in Jenkins, unless they belong to a list of expected skips). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm a bit lost on the PR's context. Is the per-image limitation coming from the guest kernel or something else? If it's something not statically-defined by the image itself, I think runtime detection + There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The question for you was mostly whether you'd find useful to test Windows VMs with up to 16 VIFs. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it'd be useful. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why specifically UEFI? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the only thing this really needs to express is "non-alpine", preferably debian because some other distros might have different naming schemes for interfaces. uefi is not necessary |
||
class TestVIFLimit: | ||
def test_vif_limit(self, host_with_saved_yum_state, imported_vm): | ||
host = host_with_saved_yum_state | ||
vm = imported_vm | ||
if (vm.is_running()): | ||
logging.info("VM already running, shutting it down first") | ||
vm.shutdown(verify=True) | ||
|
||
network_uuid = vm.vifs()[0].param_get('network-uuid') | ||
existing_vifs = len(vm.vifs()) | ||
|
||
logging.info(f'Get {vcpus} vCPUs for the VM') | ||
vm.param_set('VCPUs-max', vcpus) | ||
vm.param_set('VCPUs-at-startup', vcpus) | ||
Comment on lines
+33
to
+34
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a permanent change done to the VM, we likely don't want that when it is not a throwaway VM. I'd think this is a good candidate for a fixture. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. but the vm is destroyed at the end |
||
|
||
logging.info('Create VIFs before starting the VM') | ||
for i in range(existing_vifs, vif_limit): | ||
vm.create_vif(i, network_uuid=network_uuid) | ||
Comment on lines
+36
to
+38
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. similarly those should be disposed in cleanup phase There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. when the vm is destroyed, these are cleaned up as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Though we currently give module scope to VM fixtures (so they are indeed destroyed between tests of different modules), we strive to always leave resources in the state where we found them. There are exceptions:
|
||
|
||
vm.start() | ||
vm.wait_for_os_booted() | ||
|
||
logging.info('Verify the interfaces exist in the guest') | ||
for i in range(0, vif_limit): | ||
if vm.ssh_with_result([f'test -d /sys/class/net/{interface_name}{i}']).returncode != 0: | ||
guest_error = vm.ssh_with_result(['dmesg | grep -B1 -A3 xen_netfront']).stdout | ||
logging.error("dmesg:\n%s", guest_error) | ||
assert False, "The interface does not exist in the guest, check dmesg output above for errors" | ||
|
||
logging.info('Configure interfaces') | ||
config = '\n'.join([f'iface {interface_name}{i} inet dhcp\n' | ||
f'auto {interface_name}{i}' | ||
for i in range(existing_vifs, vif_limit)]) | ||
vm.ssh([f'echo "{config}" >> /etc/network/interfaces']) | ||
|
||
logging.info('Install iperf3 on VM and host') | ||
if vm.ssh_with_result(['apt install iperf3 --assume-yes']).returncode != 0: | ||
assert False, "Failed to install iperf3 on the VM" | ||
host.yum_install(['iperf3']) | ||
last-genius marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
logging.info('Reconfigure VM networking') | ||
if vm.ssh_with_result(['systemctl restart networking']).returncode != 0: | ||
assert False, "Failed to configure networking" | ||
|
||
# Test iperf on all interfaces in parallel | ||
# Clean up on exceptions | ||
try: | ||
logging.info('Create separate iperf servers on the host') | ||
with tempfile.NamedTemporaryFile('w') as host_script: | ||
iperf_configs = [f'iperf3 -s -p {5100+i} &' | ||
for i in range(0, vif_limit)] | ||
host_script.write('\n'.join(iperf_configs)) | ||
host_script.flush() | ||
host.scp(host_script.name, host_script.name) | ||
host.ssh([f'nohup bash -c "bash {host_script.name}" < /dev/null &>/dev/null &'], | ||
background=True) | ||
|
||
logging.info('Start multiple iperfs on separate interfaces on the VM') | ||
with tempfile.NamedTemporaryFile('w') as vm_script: | ||
iperf_configs = [f'iperf3 --no-delay -c {host.hostname_or_ip} ' | ||
f'-p {5100+i} --bind-dev {interface_name}{i} ' | ||
f'--interval 0 --parallel 1 --time 30 &' | ||
for i in range(0, vif_limit)] | ||
vm_script.write('\n'.join(iperf_configs)) | ||
vm_script.flush() | ||
vm.scp(vm_script.name, vm_script.name) | ||
stdout = vm.ssh([f'bash {vm_script.name}']) | ||
|
||
# TODO: log this into some performance time series DB | ||
logging.info(stdout) | ||
finally: | ||
vm.ssh(['pkill iperf3 || true']) | ||
host.ssh('killall iperf3') |
Uh oh!
There was an error while loading. Please reload this page.