test: Rewrite tests_bond_options.yml in new testing format #671

liangwen12year · 2024-01-30T20:26:25Z

Enhancement:

Reason:

Result:

Issue Tracker Tickets (Jira or BZ if any):

codecov · 2024-01-30T20:31:52Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (6c74dee) 20.50% compared to head (9c23735) 20.50%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #671   +/-   ##
=======================================
  Coverage   20.50%   20.50%           
=======================================
  Files          10       10           
  Lines        1478     1478           
  Branches      433      433           
=======================================
  Hits          303      303           
  Misses       1174     1174           
  Partials        1        1

Flag	Coverage Δ
sanity	`20.50% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

liangwen12year · 2024-01-30T20:56:43Z

[citest]

liangwen12year · 2024-01-31T19:41:35Z

[citest]

liangwen12year · 2024-01-31T20:59:14Z

[citest]

richm · 2024-02-01T14:35:10Z

well, that didn't help

TASK [Install dnsmasq] *********************************************************
task path: /WORKDIR/git-bond_options_rewritem7luhouy/tests/playbooks/tasks/create_test_interfaces_with_dhcp.yml:3
Wednesday 31 January 2024  23:34:50 +0000 (0:00:00.034)       0:01:33.565 ***** 
FAILED - RETRYING: [sut]: Install dnsmasq (6 retries left).
FAILED - RETRYING: [sut]: Install dnsmasq (5 retries left).
FAILED - RETRYING: [sut]: Install dnsmasq (4 retries left).
FAILED - RETRYING: [sut]: Install dnsmasq (3 retries left).
FAILED - RETRYING: [sut]: Install dnsmasq (2 retries left).
FAILED - RETRYING: [sut]: Install dnsmasq (1 retries left).
fatal: [sut]: FAILED! => {
    "attempts": 6,
    "changed": false,
    "rc": 1,
    "results": []
}

MSG:

Failed to download metadata for repo 'epel': Cannot prepare internal mirrorlist: Curl error (6): Couldn't resolve host name for https://mirrors.fedoraproject.org/metalink?repo=epel-8&arch=x86_64&infra=stock&content=centos [Could not resolve host: mirrors.fedoraproject.org]

The test prior to this is tests_bond_nm.yml - https://dl.fedoraproject.org/pub/alt/linuxsystemroles/logs/lsr-citool_network-671-2123ffc_CentOS-Stream-8_20240201-000417/artifacts/tests_bond_nm-PASSED.log

TASK [Delete the device 'nm-bond'] *********************************************
task path: /WORKDIR/git-bond_options_rewritem7luhouy/tests/playbooks/tests_bond.yml:114
Wednesday 31 January 2024  23:33:36 +0000 (0:00:00.258)       0:00:26.636 ***** 
ok: [sut] => {
    "changed": false,
    "cmd": [
        "ip",
        "link",
        "del",
        "nm-bond"
    ],
    "delta": "0:00:00.006632",
    "end": "2024-01-31 23:33:36.426429",
    "failed_when_result": false,
    "rc": 1,
    "start": "2024-01-31 23:33:36.419797"
}

STDERR:

Cannot find device "nm-bond"

Not sure if this is a problem - perhaps the other cleanup removes this interface (which is why this task has ignore_errors: true)

It would be useful to add to the cleanup section something which verifies DNS lookups are still working (maybe add an explicit getent hosts mirrors.fedoraproject.org and getent hosts mirrors.centos.org - something like this)

- name: Verify DNS and ping still work
  shell: |
    set -euxo pipefail
    for host in mirrors.fedoraproject.org mirrors.centos.org; do
      if ! getent hosts "$host"; then
        # show whatever diagnostic information you need to determine why DNS isn't working
        # not sure what that would be
        # see if dnsmasq is still running and intercepting DNS requests?
        # and maybe ip a and ip route?  what other information do you need here for debugging?
        exit 1
      fi
      if ! ping "$host"; then
        # show whatever diagnostic information you need to determine why ping isn't working
        # not sure what that would be
        # and maybe ip a and ip route?  what other information do you need here for debugging?
        exit 1
      fi
    done
  when: ansible_facts["distribution"] == "CentOS"
  changed_when: false

liangwen12year · 2024-02-02T12:44:52Z

[citest]

liangwen12year · 2024-02-02T17:37:43Z

[citest]

tyll · 2024-02-05T10:36:55Z

tests/playbooks/tests_bond_options.yml

-              changed_when: false
-            - name: Import the task 'remove_test_interfaces_with_dhcp.yml'
-              import_tasks: tasks/remove_test_interfaces_with_dhcp.yml
+            lsr_description: I can reconfigure the bond options


I wrote the description in user story format before I knew about acceptance criteria. Probably, it makes more sense to write this as an acceptance criteria.

I wrote the lsr_description as acceptance criteria.

tyll · 2024-02-05T10:38:17Z

tests/tasks/assert_bond_options.yml

+    - {key: 'mode', value: '802.3ad'}
+    - {key: 'ad_actor_sys_prio', value: '65535'}
+    - {key: 'ad_actor_system', value: '00:00:5e:00:53:5d'}
+    - {key: 'ad_select', value: 'stable'}
+    - {key: 'ad_user_port_key', value: '1023'}
+    # wokeignore:rule=slave
+    - {key: 'all_slaves_active', value: '1'}
+    - {key: 'downdelay', value: '0'}
+    - {key: 'lacp_rate', value: 'slow'}
+    - {key: 'lp_interval', value: '128'}
+    - {key: 'miimon', value: '110'}
+    - {key: 'num_grat_arp', value: '64'}
+    - {key: 'resend_igmp', value: '225'}
+    - {key: 'updelay', value: '0'}
+    - {key: 'use_carrier', value: '1'}
+    - {key: 'xmit_hash_policy', value: 'encap2+3'}


Is it possible to assign this as a variable in tests/playbooks/tests_bond_options.yml?

It is possible.

Then please do so. It seems to me, that it will help making the playbooks easier to understand. What do you think? It should also reduce the amount of needed task files.

Yes, we do not need to keep some redundant task files.

liangwen12year · 2024-02-06T16:03:02Z

[citest]

liangwen12year · 2024-02-06T17:01:23Z

[citest]

liangwen12year · 2024-02-06T18:23:36Z

[citest]

tyll · 2024-02-06T19:42:10Z

tests/playbooks/tests_bond_options.yml

+            lsr_description: Given the system administrators using the network
+              role, when they reconfigure the bond options as specified in
+              `create_bond_profile.yml`, then I can verify that the
+              bond options are configured correctly in sysfs filesystem.


Actually, the acceptance criteria should describe the steps in lsr_setup, lsr_test and lsr_assert:

A start would be:

Given two DHCP-enabled network interfaces,
When creating a bond profile with them,
Then the controller device and bond port profiles are present and the specified bond options are set for the controller device.

liangwen12year · 2024-02-06T20:42:53Z

[citest]

liangwen12year · 2024-02-06T21:41:38Z

[citest]

tyll · 2024-02-06T22:01:47Z

tests/tasks/create_bond_profile.yml

+          mode: 802.3ad
+          ad_actor_sys_prio: 65535
+          ad_actor_system: 00:00:5e:00:53:5d
+          ad_select: stable
+          ad_user_port_key: 1023
+          all_ports_active: true
+          downdelay: 0
+          lacp_rate: slow
+          lp_interval: 128
+          miimon: 110
+          min_links: 0
+          num_grat_arp: 64
+          primary_reselect: better
+          resend_igmp: 225
+          updelay: 0
+          use_carrier: true
+          xmit_hash_policy: encap2+3


Not sure if it was clear, these options also belong in a variable in the playbook.

Aha, thanks for making it clearer, I will use a variable to hold the bond options. Bonus from this, I think that we can have fewer task files.

tyll · 2024-02-06T22:03:00Z

tests/playbooks/tests_bond.yml

+    - name: Verify DNS and ping still work
+      shell: |
+        set -euxo pipefail
+        for host in mirrors.fedoraproject.org mirrors.centos.org; do
+          if ! getent hosts "$host"; then
+            exit 1
+          fi
+          if ! ping -c5 "$host"; then
+            exit 1
+          fi
+        done
+      when: ansible_facts["distribution"] == "CentOS"
+      changed_when: false


If you intend to keep this code in, then it should be its own task file.

I am not planning to keep this code in, but only for temporary debugging purposes.

liangwen12year · 2024-02-09T13:02:26Z

[citest]

liangwen12year · 2024-02-09T15:33:40Z

[citest]

tests/playbooks/tests_network_state.yml

liangwen12year · 2024-02-13T23:39:52Z

[citest]

liangwen12year · 2024-02-14T02:08:39Z

[citest]

tests/playbooks/tests_bond.yml

tests/playbooks/tests_bond_cloned_mac.yml

tests/playbooks/tests_bond_deprecated.yml

tests/playbooks/tests_bond_removal.yml

tests/tasks/check_network_dns.yml

liangwen12year · 2024-02-14T16:45:24Z

[citest]

The new testing format is more concise and easier to debug when test failure happens. Signed-off-by: Wen Liang <[email protected]>

Sometimes the rpm download returns a 403, which is likely caused by too many parallel jobs attempt the download from the same controller in too short a period of time, so the epel server throttles additional downloads - use a retry here to mitigate. Signed-off-by: Wen Liang <[email protected]>

In order to guarantee each test is cleaned up properly in the end, it is important to add a post-test check to each test checking that: - Routes and DNS are restored. - Network connectivity to certain hosts are preserved. Signed-off-by: Wen Liang <[email protected]>

Without purging the DNS testing config at the end `tests_network_state.yml`, the managed hosts can not properly resolve certain hosts (e.g. mirrors.fedoraproject.org, mirrors.centos.org ) in the package installation task of other tests. Signed-off-by: Wen Liang <[email protected]>

liangwen12year · 2024-02-14T20:57:04Z

[citest]

liangwen12year · 2024-02-14T21:00:04Z

@richm, thanks for the hint and contribution to this PR.

https://github.com/liangwen12year/network/pull/2/commits

richm · 2024-02-15T03:20:46Z

There were 3 tests that hung:

network 671 CentOS-8/ansible-2.9 tests_bond_deprecated_nm.yml UNDEFINED 2024-02-14T14:03:44
network 671 Fedora-39/ansible-2.16 tests_ethernet_nm.yml UNDEFINED 2024-02-14T13:59:44
network 671 Fedora-38/ansible-2.16 tests_bond_removal_nm.yml UNDEFINED 2024-02-14T09:49:21

the time now is 2024-02-14T20:18:44, so you can see that those tests are quite hung. I'm going to merge this PR, and we will see if those 3 tests are flakes or are persistent.

liangwen12year force-pushed the bond_options_rewrite branch from fba403d to bd93e78 Compare January 30, 2024 20:29

liangwen12year force-pushed the bond_options_rewrite branch from bd93e78 to 3f31b1d Compare January 30, 2024 20:39

liangwen12year force-pushed the bond_options_rewrite branch from 2fcb873 to 2123ffc Compare January 31, 2024 20:58

liangwen12year force-pushed the bond_options_rewrite branch from 9289f31 to 1651fd3 Compare February 2, 2024 15:27

tyll reviewed Feb 5, 2024

View reviewed changes

liangwen12year force-pushed the bond_options_rewrite branch from 1651fd3 to e7de3d6 Compare February 6, 2024 16:02

liangwen12year force-pushed the bond_options_rewrite branch from e7de3d6 to 0e64478 Compare February 6, 2024 16:59

liangwen12year force-pushed the bond_options_rewrite branch from 0e64478 to 7ac12e6 Compare February 6, 2024 18:22

tyll reviewed Feb 6, 2024

View reviewed changes

liangwen12year force-pushed the bond_options_rewrite branch from 7ac12e6 to 2d41ef1 Compare February 6, 2024 20:41

liangwen12year force-pushed the bond_options_rewrite branch from 2d41ef1 to 4a4b620 Compare February 6, 2024 21:41

tyll reviewed Feb 6, 2024

View reviewed changes

liangwen12year force-pushed the bond_options_rewrite branch from 4a4b620 to 9e6419a Compare February 9, 2024 13:01

liangwen12year force-pushed the bond_options_rewrite branch from 9e6419a to 065eaef Compare February 9, 2024 15:33

richm reviewed Feb 13, 2024

View reviewed changes

tests/playbooks/tests_network_state.yml Show resolved Hide resolved

richm previously approved these changes Feb 13, 2024

View reviewed changes

liangwen12year dismissed richm’s stale review via e1bc458 February 13, 2024 23:38

liangwen12year force-pushed the bond_options_rewrite branch from e1bc458 to d5eed58 Compare February 14, 2024 02:08