pf: close nc file descriptors in killstates test

[franco_opnsense.org]

Summary

On an atf-sh script capture the nc process lingers and blocks
the capturing process.

[kp]

What do you mean by "On an atf-sh script capture"?

[franco_opnsense.org]

Consider running tests from the src tree using atf-sh (I'm using devel/atf but the base one also works with the full path I think):

sh -c 'echo $(atf-sh /usr/src/tests/sys/netpfil/pf/killstate.sh match)'

This hangs until you kill the stray nc processes. It works fine for the build of tests where nothing is forked into the background which seems to be the general issue. Another way would be to kill the nc processes when the test body ends (not in the cleanup as that would still cause it to hang), but a single line approach is likely easier although if you run the test a number of times these processes just keep building up in the system. I'm happy to improve this in any other way.

BTW, the shell-based tests are great. I want to add tests (mainly for pflog at the moment) without the test build and kyua complications.

[kp]

In D47543#1084854, @franco_opnsense.org wrote:

Consider running tests from the src tree using atf-sh (I'm using devel/atf but the base one also works with the full path I think):

sh -c 'echo $(atf-sh /usr/src/tests/sys/netpfil/pf/killstate.sh match)'

This hangs until you kill the stray nc processes. It works fine for the build of tests where nothing is forked into the background which seems to be the general issue. Another way would be to kill the nc processes when the test body ends (not in the cleanup as that would still cause it to hang), but a single line approach is likely easier although if you run the test a number of times these processes just keep building up in the system. I'm happy to improve this in any other way.

That's kind of expected to not work. We don't run tests that way. We always run them via kyua because it does a lot of setup work for us. This starts the process midway, and stops before it's entirely done.

That is, this skips both the head and cleanup functions. It doesn't verify prerequisites (e.g. are we running as the right user e.g. atf_set require.user root, do we have required programs e.g. atf_set require.progs scapy). Failing to run the cleanup function can leave all sorts of things behind (like jails and interfaces for most tests, processes for some others).

Skipping the installation step may work to some extent for some of the shell script tests, but not for anything that builds a binary. I'm not sure what it'd do for the python-based tests, but either way, this is well outside of how the tests are expected to run.

The command above resulted in two jails and on the host an epair interface with 192.0.2.1 assigned to it being left behind. The in-tree atf-sh even explicitly warns that this isn't how it's expecting to get run:

killstate.sh: WARNING: Running test cases outside of kyua(1) is unsupported
killstate.sh: WARNING: No isolation nor timeout control is being applied; you may get unexpected failures; see atf-test-case(4)

Kyua takes care of this. In the case of the pf tests it even nests the tests in a vnet jail so that we don't accidentally affect the host, or other running tests.

There's more information on test writing here: https://freebsdfoundation.org/wp-content/uploads/2019/05/The-Automated-Testing-Framework.pdf , if that's useful.

[franco_opnsense.org]

Thanks, just to be clear you imply the change is wrong even though the test still works?

[kp]

In D47543#1084990, @franco_opnsense.org wrote:

Thanks, just to be clear you imply the change is wrong even though the test still works?

More pointless than wrong. You’re trying to fix incorrect use of the tests by changing the test, rather than by using it correctly.

[franco_opnsense.org]

So regardless of why I already stated this is a technical issue that is by no means "pointless", what do you suggest to improve this particular test to make it more robust? Uncontrolled creation of processes that inherit file descriptors isn't exactly clean design but I can see why you do not want to apply this mere bandaid with that larger issue at hand. I'm happy to do the work since a lot of people were asking for test cases and here I am offering work on test cases to get started. :)

As for wider discussions about how to test let's just focus on this particular test case. Kyua is not a magic remedy for the problems the testing framework has like a lack of test coverage e.g. in the pflog department. Running atf-sh as "unsupported" and having a test pass just fine is no reason to not pursue this, especially since it says "you may get unexpected failures" and we talk about a passing test case. I appreciate the heads up but I still believe allowing test to run in a zero-config fashion from any running system is an actual benefit to the problem of not having enough people work on test cases. It really does not matter how you might see it differently because the test case needs work first.

[vegeta_tuxpowered.net]

In D47543#1085347, @franco_opnsense.org wrote:

So regardless of why I already stated this is a technical issue that is by no means "pointless", what do you suggest to improve this particular test to make it more robust? Uncontrolled creation of processes that inherit file descriptors isn't exactly clean design but I can see why you do not want to apply this mere bandaid with that larger issue at hand.

The creation of processes, fds, and interfaces is not uncontrolled. There already is a clean up mechanism, as long as you use Kyua, which you are deliberately skipping. There is no need for a cleanup, or workarounds for it, inside of the test itself.

I'm happy to do the work since a lot of people were asking for test cases and here I am offering work on test cases to get started. :)

Developing new tests does not really need those workarounds. Create an empty file, add it to proper Makefile, do cd freebsd-src/tests/sys/netpfil; make; make install and you can develop your tests by editing the installed file and running it with kyua debug -k /usr/tests/sys/netpfil/pf/Kyuafile my_test:my_case. It's not that much overhead.

I still believe allowing test to run in a zero-config fashion from any running system is an actual benefit

Your case is already not zero-config as you'd need to cleanup *much* more than that single hanging process. Which, again, is already provided for you when running the tests with Kyua. IMO running tests via Kyua *is* zero-config.

[kp]

In D47543#1085347, @franco_opnsense.org wrote:

So regardless of why I already stated this is a technical issue that is by no means "pointless", what do you suggest to improve this particular test to make it more robust? Uncontrolled creation of processes that inherit file descriptors isn't exactly clean design but I can see why you do not want to apply this mere bandaid with that larger issue at hand. I'm happy to do the work since a lot of people were asking for test cases and here I am offering work on test cases to get started. :)

As for wider discussions about how to test let's just focus on this particular test case. Kyua is not a magic remedy for the problems the testing framework has like a lack of test coverage e.g. in the pflog department. Running atf-sh as "unsupported" and having a test pass just fine is no reason to not pursue this, especially since it says "you may get unexpected failures" and we talk about a passing test case. I appreciate the heads up but I still believe allowing test to run in a zero-config fashion from any running system is an actual benefit to the problem of not having enough people work on test cases. It really does not matter how you might see it differently because the test case needs work first.

I'm failing to understand the point you're trying to make.
Is this test case currently broken for you? Is it passing when it shouldn't be?

Yes, there is room for better test coverage. I welcome new test cases, but that's clearly not this.

[franco_opnsense.org]

I'm trying to understand the entry barrier here. If kyua fixes everything that's alright and thanks for the pointers. From release engineering scope there are a number of kyua integration challenges irrelevant to your argumentation (which I can understand), but you both seem to see the situation too easy from an established development workflow ("just do it right"). Using a custom src.conf already makes kyua defunct, but it is what it is. Going to raise appropriate patches elsewhere then. Thanks!