Add new testsuite for firewalld #3882
Conversation
This commit adds a new testsuite for firewalld(a firewall daemon with d-bus interface) in LISA. The suite runs the compiled testsuite binary provided by the firewalld-test rpm. - the test is supported only for AzureLinux 3.0. - there are known test failures because of missing kernel configs. the test checks the failure list against this known list for setting status.
There was a problem hiding this comment.
Pull Request Overview
This pull request adds a new testsuite for the firewalld daemon on Azure Linux 3.0, enabling validation of firewalld functionality through the provided firewalld-test rpm.
- Introduces a new TestSuite class (FirewalldSuite) and related helper functions.
- Implements configuration checks, log parsing, and test result updates.
Comments suppressed due to low confidence (1)
microsoft/testsuites/firewalld/firewalldsuite.py:160
- [nitpick] The log message 'results dir B' is ambiguous. Consider clarifying the message to clearly indicate which directory is being removed.
log.info("Removing firewalld testsuite results dir B")
| ] | ||
|
|
||
| status = TestStatus.PASSED | ||
| if known_fail_testcase_num != fail_testcase_num: |
There was a problem hiding this comment.
The direct list comparison on line 88 may fail due to differences in ordering even when the same test failures occur. To improve robustness, consider comparing sorted lists or using set equality.
| if known_fail_testcase_num != fail_testcase_num: | |
| if set(known_fail_testcase_num) != set(fail_testcase_num): |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
The known_fail_testcase_num is a sorted list.
The fail_testcase_num is created by listing the test directory which logs all failure test numbers as sub-dirs, which also appears sorted with ls.
There was a problem hiding this comment.
This comment of copilot looks valid to me. Maintaining the order in the code is not reliable, and the total count is the same doesn't means every element is the same in the list. for example, [0, 1] is the same length as [1, 2], but they are different.
There was a problem hiding this comment.
My bad for the naming. The type of the compared variables is List[str], where each list contains the failed test numbers, ex: ["01", "02"] and not the count. If it still makes sense, I will make the changes :)
There was a problem hiding this comment.
Thanks for the explanation. Please compare the set instead of the sorted list. This makes maintenance easier. Even if a new failure ID is added out of order, the logic will still hold.
Aarch64 kernel has additional FIB configs enabled compared to x86_64 kernel. The failure list is thus a subset of the known list in x86_64.
| description=""" | ||
| This test case runs the firewalld testsuite. | ||
| The test suite is a collection of tests which run against | ||
| a local firewalld installation. |
There was a problem hiding this comment.
Please put test suite description in TestSuiteMetadata, here put the test case description is sufficient.
| if not _is_ipv6_rpfilter_supported(node): | ||
| raise SkippedException( | ||
| "Skipping tests. Needs kernel config CONFIG_FIB_INET &" | ||
| "CONFIG_FIB_IPV6 enabled to use fib based expressions for" |
There was a problem hiding this comment.
"CONFIG_FIB_IPV6 enabled to use fib based expressions for " add one space in the end
| ) | ||
|
|
||
| # Update the test result data | ||
| result.set_status( |
There was a problem hiding this comment.
Remove the requirement to set the test case status to "PASSED". It's not recommended to add error messages to passed test cases. If detailed subtest information is needed, send the subtest results instead.
There was a problem hiding this comment.
@squirrelsc Idea here is to have a summary of the tests (Total/Failed/Skipped) which is available from the logs, Could you suggest how to do that here? The summary looks like below:
2025-06-24 05:38:22.145[124637967710016][INFO] lisa.RootRunner FirewalldSuite.verify_firewalld: PASSED TOTAL: 306
FAILED: 15
SKIPPED: 19
There was a problem hiding this comment.
Please search send_sub_test_result_message to find examples in other test suites.
| # arg to set number of jobs | ||
| jobs_arg = "-j4" | ||
|
|
||
| clean_command = f"{test_suite_binary} {clean_arg} {set_logs_dir_arg}" |
There was a problem hiding this comment.
Move this line just before node.execute(clean_command, sudo=True, shell=False) to make it clearer. Do the same for test_command.
The changes add a new testsuite for firewalld(a firewall daemon with d-bus interface) in LISA. The suite runs the compiled test binary provided by the firewalld-test rpm.
Test Methodology:
Used
microsoft/runbook/ready.yamlto run this particular test on a provisioned Azure Linux 3.0 VM.NOTE:
Currently, the firewalld daemon fails to start in Azl 3.0. This PR is tracking the changes to fix this error.
In order to run the LISA test, the ready VM contains the fixed rpms under a local repo.
Verified the test status on multiple runs:
Install dependencies
Test Result