atf_python: Miscellaneous minor improvements #865
Conversation
tests/atf_python/sys/net/vnet.py
Outdated
| @@ -180,6 +180,7 @@ def create_iface(self, alias_name: str, iface_name: str) -> List[VnetInterface]: | |||
|
|
|||
| @staticmethod | |||
| def is_autodeleted(iface_name: str) -> bool: | |||
| return "lo0" not in iface_name | |||
There was a problem hiding this comment.
Doesn't this unconditionally return, i.e., the subsequent lines are never executed?
There was a problem hiding this comment.
yeah, this would need an if to be actually conditional.
There was a problem hiding this comment.
Yes. The overall way it works is that for each interface, it checks if is_autodeleted, so I wan it to return False if the interface is lo0, it will call this method once for each interface.
Previously any loopback interface was added to the list of interfaces to be automatically deleted.
This change will produce the lo0 to be skipped, for example:
...
sbin/ping/test_ping.py::TestPing::test_pinger[_0_0_special_wrong] ==== vnet cleanup ====
# topology_id: 'TestPing:test_pinger[_0_0_special_wrong]'
Skipping interface pytest:TestPing:test_pinger__0_0_special_wrong:lo0
run: '/usr/sbin/jexec pytest:TestPing:test_pinger__0_0_special_wrong ifconfig epair1a destroy'
run: '/usr/sbin/jexec pytest:TestPing:test_pinger__0_0_special_wrong ifconfig tun0 destroy'
run: '/usr/sbin/jail -r pytest:TestPing:test_pinger__0_0_special_wrong'
PASSED
Else `ifconfig lo0 destroy` will throw an:
ifconfig: SIOCIFDESTROY: Invalid argument
| @@ -180,6 +180,8 @@ def create_iface(self, alias_name: str, iface_name: str) -> List[VnetInterface]: | |||
|
|
|||
| @staticmethod | |||
| def is_autodeleted(iface_name: str) -> bool: | |||
| if iface_name == "lo0": | |||
| return False | |||
| iface_type = re.split(r"\d+", iface_name)[0] | |||
| return iface_type in IfaceFactory.AUTODELETE_TYPES | |||
There was a problem hiding this comment.
I think the AUTODELETE_TYPES thing is not a great idea in general. It requires the test framework to maintain a list of interface types which will get stale over time. I'm not sure why it doesn't include if_bridge or if_wg for example.
Is the intent here to destroy all interfaces belonging to a vnet before destroying the vnet itself?
There was a problem hiding this comment.
I think the AUTODELETE_TYPES thing is not a great idea in general. It requires the test framework to maintain a list of interface types which will get stale over time. I'm not sure why it doesn't include
if_bridgeorif_wgfor example.
Probably because there are no tests that need these types at the moment.
Is the intent here to destroy all interfaces belonging to a vnet before destroying the vnet itself?
Yes, similarly to what is done in atf-sh (80fc250)
There was a problem hiding this comment.
Hmm, ok. The problem that motivated commit 80fc250 is now fixed, so I'm still kind of skeptical, but alright.
There was a problem hiding this comment.
Ahh.. OK, I believe @AlexanderChernikov's words were at the time:
There is a known problem resulting from a corner case in the kernel delayed object reclamation model.
I'll try test destroying everything from the host... but I remember half-epairs remaining and something similar to https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=270628 when testing with VLANs.
| ) | ||
| for iface_name in ifaces_lst.split(): | ||
| if not self.is_autodeleted(iface_name): | ||
| if iface_name not in self._list_ifaces(): | ||
| print("Skipping interface {}:{}".format(vnet_name, iface_name)) | ||
| continue | ||
| run_cmd( | ||
| "/usr/sbin/jexec {} ifconfig {} destroy".format(vnet_name, iface_name) | ||
| "/usr/sbin/jexec {} /sbin/ifconfig {} destroy".format(vnet_name, iface_name) |
There was a problem hiding this comment.
Is your intent just to fix the inconsistency that some places specify a full path and others don't? I don't think it matters too much one way or the other, but the commit log message should be a bit more clear.
There was a problem hiding this comment.
Right, I'll change the commit message. Chances are very low that someone overrides these in the PATH, especially within a VNET.
These are all minor changes, maybe changing all string formatting to f-strings (as these are arguably more readable and slightly better performant) is next. They can all be dropped if you don't consider it is worth the effort.
There was a problem hiding this comment.
I think the changes are fine, I'm asking questions in part because I'm not too familiar with atf_python.
There was a problem hiding this comment.
I have no problem at all with questions!
As we already know, it makes pytest slow once you add --atf to its parameters, as you well pointed out, if there are low hanging-fruits that could it speed up... I'm just trying to comb it a little.
pytest runs twice per test:
- To run the test:
# pytest -vv -p no:cacheprovider -s --atf --confcutdir=/usr/tests --atf-source-dir=/usr/tests/atf_python <name of single test here>
- To cleanup:
# pytest -vv -p no:cacheprovider -s --atf --confcutdir=/usr/tests --atf-cleanup --atf-source-dir=/usr/tests/atf_python <name of single test here>
In case you want to experiment with pytest directly.
I have not found anything obvios at the moment, but one minor thing that I found confusing when creating atf_python tests was that the debugging information is not immediately clear, I'm trying to familiarize with pytest's internals a little more as well.
Usually tests are run in sterile environments; however, there is a slight chance that the PATH overrides the utilities used for testing. Pedantically use absolute paths, even inside VNETs, to avoid ambiguity. Chiefly, jexec -> /usr/sbin/jexec, and ifconfig -> /sbin/ifconfig.
To be replaced with pytest's section/add_report_section.
|
Merged, thank you. |
Else `ifconfig lo0 destroy` will throw an:
ifconfig: SIOCIFDESTROY: Invalid argument
Reviewed by: markj
MFC after: 1 week
Pull Request: #865
Reviewed by: markj MFC after: 1 week Pull Request: #865
Usually tests are run in sterile environments; however, there is a slight chance that the PATH overrides the utilities used for testing. Pedantically use absolute paths, even inside VNETs, to avoid ambiguity. Chiefly, jexec -> /usr/sbin/jexec, and ifconfig -> /sbin/ifconfig. Reviewed by: markj MFC after: 1 week Pull Request: #865
Reviewed by: markj MFC after: 1 week Pull Request: #865
To be replaced with pytest's section/add_report_section. Reviewed by: markj MFC after: 1 week Pull Request: #865
Else `ifconfig lo0 destroy` will throw an:
ifconfig: SIOCIFDESTROY: Invalid argument
Reviewed by: markj
MFC after: 1 week
Pull Request: freebsd/freebsd-src#865
Reviewed by: markj MFC after: 1 week Pull Request: freebsd/freebsd-src#865
Usually tests are run in sterile environments; however, there is a slight chance that the PATH overrides the utilities used for testing. Pedantically use absolute paths, even inside VNETs, to avoid ambiguity. Chiefly, jexec -> /usr/sbin/jexec, and ifconfig -> /sbin/ifconfig. Reviewed by: markj MFC after: 1 week Pull Request: freebsd/freebsd-src#865
Reviewed by: markj MFC after: 1 week Pull Request: freebsd/freebsd-src#865
To be replaced with pytest's section/add_report_section. Reviewed by: markj MFC after: 1 week Pull Request: freebsd/freebsd-src#865
Sorry, but these are too small for me to manage in Phabricator.
This is the sad part of GitHub, the CODEOWNERS file is not 100% effective:
cc/ @markjdb @AlexanderChernikov @ngie-eign @emaste