Skip to content
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

Obtaing the ip address of the guest and update address cache #3935

Merged
merged 1 commit into from
Aug 11, 2024

Conversation

TasmiyaNalatwad
Copy link
Contributor

@TasmiyaNalatwad TasmiyaNalatwad commented Jun 24, 2024

Few of the times it is seen that the ip address is not being fetch and the avocado runs were failed with error "ERROR: Failures occurred while postprocess:\n\n: Guest virt-tests-vm1 dmesg verification failed: Login timeout expired (output: 'exceeded 240 s timeout, last failure: No ipv4 DHCP lease for MAC aa:bb:cc:dd:ee:ff') "

To handle this error the patch has been sent. The patch helps in obtaining ip address of the guest using "virsh-net-dhcp-leases default" command. If the guest mac address is found in the command output, the mac ipv4 address is obatined and updated in the address.cache

The patch helps in obtaining ip address of the guest using "virsh-net-dhcp-leases default" command.
If the guest mac address is found in the command output, the mac ipv4 address is obatined and updated in the address.cache

Signed-off-by: Tasmiya Nalatwad <tasmiya@linux.vnet.ibm.com>
@TasmiyaNalatwad
Copy link
Contributor Author

Please find the avocado test run results executed on the patch

12:24:12 WARNING : Overriding user setting and enabling kvm bootstrap as guest tests are requested
12:24:13 INFO : Check for environment
12:24:18 INFO : Creating temporary mux dir
12:24:18 INFO :
12:24:18 INFO : Running Guest Tests Suite lifecycle_bck
12:24:18 INFO : Running: /usr/local/bin/avocado run --vt-type libvirt --vt-config /home/tests/data/avocado-vt/backends/libvirt/cfg/lifecycle_bck.cfg --force-job-id cab81490c1993d1add354f23d2c3e9698a106fdf --job-results-dir /home/tests/results --vt-only-filter "virtio_scsi virtio_net qcow2 Ubuntu.24.04.ppc64le"
JOB ID : cab81490c1993d1add354f23d2c3e9698a106fdf
JOB LOG : /home/tests/results/job-2024-06-24T12.24-cab8149/job.log
(1/1) type_specific.io-github-autotest-libvirt.virsh.destroy.error_test.extra_option: STARTED
(1/1) type_specific.io-github-autotest-libvirt.virsh.destroy.error_test.extra_option: PASS (256.51 s)
RESULTS : PASS 1 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0
JOB HTML : /home/tests/results/job-2024-06-24T12.24-cab8149/results.html
JOB TIME : 263.65 s
12:28:49 INFO :
12:28:49 INFO : Summary of test results can be found below:
TestSuite

# Read guest address via serial console and update VM address
# cache to avoid get out-dated address.
utils_net.update_mac_ip_address(self, timeout)
ipaddr = self.get_address(nic_index, ip_version)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function "utils_net.update_mac_ip_address" is trying to do serial login (create session and login) and then update the mac_ip_address in address.cache but as there is no ip address, the connection to serial console fails and following function self.get_address also fails.
Hence removing this approach and trying to get the ip address from virsh-net-dhcp-leases command

Copy link
Contributor

@harihare harihare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link

@misanjumn misanjumn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@bskjois bskjois left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link

@PraveenPenguin PraveenPenguin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@lmr lmr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lmr lmr merged commit 8ed80f8 into avocado-framework:master Aug 11, 2024
41 checks passed
@chloerh
Copy link
Contributor

chloerh commented Aug 13, 2024

Hi @TasmiyaNalatwad
About the code check of this pr, I found a very weird thing: only 41 checks were run for this pr which did not include code style check while other prs have 50 checks run for ci-check. I have checked one pr created on June 19 and one on June 25, they both had 50 checks run.
I'm totally confused. Do you happen to have any idea what happened here? Thanks in advance.

@PraveenPenguin
Copy link

@chloerh it is like GitHub action (which trigger CI) only calls for first time contributor when maintainer's ACK it , that's why it missed via automation engine , for regular contributor it triggers automatically (as restrict random request to trigger CI (which is not authentic) thus to save resources )

@chloerh
Copy link
Contributor

chloerh commented Aug 14, 2024

@chloerh it is like GitHub action (which trigger CI) only calls for first time contributor when maintainer's ACK it , that's why it missed via automation engine , for regular contributor it triggers automatically (as restrict random request to trigger CI (which is not authentic) thus to save resources )

@PraveenPenguin I see. Thanks for explaining. I was worried about something might be wrong with our ci-check (randomly)

@luckyh
Copy link
Contributor

luckyh commented Aug 19, 2024

Hello @TasmiyaNalatwad @lmr, this introduced a regression against obtaining the guest's ip addresses properly by the changed routine. That's because:

  1. as virsh was not a mandatory dependency for qemu testing, we should not expect the ip addresses could always be obtained via libvirt tooling (virsh).
  2. since avocado-vt supported user-customized networks, even if virsh was installed, in some cases we still can not obtain ip addresses because we merely queried the dhcp leases from the default network managed by libvirt.

@luckyh
Copy link
Contributor

luckyh commented Aug 22, 2024

@TasmiyaNalatwad @lmr do you have any plan to fix the issue? otherwise, I will revert this commit for the sake of qemu users.

@TasmiyaNalatwad
Copy link
Contributor Author

@luckyh Thanks for the review and inputs. I have a plan to get the IP address without using virsh commands. How about using arp table inspection that lists all known IP-MAC address mappings. This way we can get the ip address of guest for respective Mac address.
arp command run on host : arp -n

@luckyh @lmr Requesting your inputs on this approach.

@luckyh
Copy link
Contributor

luckyh commented Aug 26, 2024

Requesting your inputs on this approach.

@TasmiyaNalatwad as we also need to deal with ipv6 addresses for this context I'd suggest using ip neigh instead (according to arp's manual page [1]). However, it is still not to be the perfect replacement to the serial login approach, because in some cases we could not get the guest ip addresses by inspecting the arp/neighbor table from the host.

Instead, I'd suggest the following approach if the original issue only happened to libvirt testing (--vt_type=libvirt), that is to overwrite the _get_address method to the subclass of the libvirt_vm module via calling virsh domifaddr $domain_name (which is similiar to the approach you used but this time we merely need to do the inspection on the specified domain), for example:

diff --git a/virttest/libvirt_vm.py b/virttest/libvirt_vm.py
index 825674e2f..7ce748804 100644
--- a/virttest/libvirt_vm.py
+++ b/virttest/libvirt_vm.py
@@ -378,6 +378,14 @@ class VM(virt_vm.BaseVM):
             LOG.error("Failed to backup xml file:\n%s", detail)
             return ""
 
+    def _get_address(self, index=0, ip_version="ipv4", session=None, timeout=60.0):
+        try:
+            return super()._get_address(index, ip_version, session, timeout)
+        except Exception:
+            output = virsh.domifaddr(self.name)
+            # parse the output and return a valid ip
+            ...
+
     def clone(
         self,
         name=None,

Let me know your opinion.

[1] arp manual page

NOTE
       This program is obsolete. For replacement check ip neigh.

@luckyh
Copy link
Contributor

luckyh commented Sep 2, 2024

Hi @TasmiyaNalatwad , finally I sent the PR #3989 in favor of qemu testing, as I feel that the current virsh approach had to be replaced after all. Feel free to issue a new approach for resolving the original issue, thanks.

@TasmiyaNalatwad
Copy link
Contributor Author

Hi @TasmiyaNalatwad , finally I sent the PR #3989 in favor of qemu testing, as I feel that the current virsh approach had to be replaced after all. Feel free to issue a new approach for resolving the original issue, thanks.

Hi @luckyh , Ok The approach you shared above _get_address method to the subclass of the libvirt_vm module via calling virsh domifaddr $domain_name Looks good to us, this would fix the issue without affecting qemu users. Will share the patch soon regarding the same. Thank you!

mac = self.get_mac_address(nic_index).lower()
ipaddr = utils_net.obtain_guest_ip_from_dhcp_leases(mac)
# updating cache with the latest ip address value
self.address_cache[mac] = ipaddr
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @TasmiyaNalatwad Could you please help redesign this part? Because of the current usage let all the releated
nic_hotplug cases failed which blocking my test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @yanglei-rh there is patch already shared by @luckyh #3989 to revert this patch, Which would unblock your tests. Also i will share the new patch with the approach we had discussed in above comments soon.

Thank you.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, thanks a lot.

@TasmiyaNalatwad
Copy link
Contributor Author

@yanglei-rh I have raised a new PR #4002 which would not affect any other user than libvirt.

@yanglei-rh
Copy link
Contributor

@yanglei-rh I have raised a new PR #4002 which would not affect any other user than libvirt.

Got it, thanks for help sync this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants