Page MenuHomeFreeBSD

kyua: Add "kyua debug -p" option
ClosedPublic

Authored by igoro on Mar 23 2025, 6:42 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Oct 4, 2:10 AM
Unknown Object (File)
Thu, Oct 2, 4:00 AM
Unknown Object (File)
Tue, Sep 23, 1:52 PM
Unknown Object (File)
Mon, Sep 22, 10:58 PM
Unknown Object (File)
Sep 12 2025, 11:58 PM
Unknown Object (File)
Sep 11 2025, 2:35 PM
Unknown Object (File)
Sep 11 2025, 1:32 AM
Unknown Object (File)
Sep 7 2025, 3:19 AM

Details

Summary

It allows the test engine to be paused right before the test cleanup
routine to help with test debugging.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

igoro requested review of this revision.Mar 23 2025, 6:42 PM

Whoa, nice. :)

What happens if tests are running in parallel? The behaviour I would naively expect is that other running tests will keep going, but new ones will not be scheduled until kyua is resumed.

contrib/kyua/cli/cmd_debug.cpp
92

Is it possible to also print the path to the test's workdir?

Whoa, nice. :)

What happens if tests are running in parallel? The behaviour I would naively expect is that other running tests will keep going, but new ones will not be scheduled until kyua is resumed.

Well, this is for starters only :). It logically extends existing "kyua debug" sub-command which is designed to debug a single test case. So, this is for the cases when the annoying failed test is known or a test is under development. Let's see how this one goes.

It is expected to be another bigger patch for the cases when it is unknown beforehand, i.e. we want to run our usual "kyua test ..." with a new flag like the same "-p" to get it stuck upon any failed test. And the parallelism will require discussion, and extra care if we need maximum comfort.

contrib/kyua/cli/cmd_debug.cpp
92

Sure, this is an obvious thing to have.

  • Could you please verify that this doesn't regress Linux/MacOS?
  • Something like using debugger_ptr_t = std::shared_ptr<debugger> then converting all of the code to use debugger_ptr_t (or maybe just debugger_ptr? I forget the style ATF/kyua uses since I flushed that out of my cache in favor of other garbage :D..) seems like a good idea for clarity/brevity.

I actually have a different proposal.. what if the "automatic cleanup" functionality was opt-out and enabled by default or by a custom policy, e.g.,

  • I want the results from the passed test cases,
  • I want the results from the failed test cases only, or
  • I don't care, clean it up when you're done.

Also, I think that it's honestly better to get the process core and continue on personally. Otherwise you could have resources tied up waiting for a human to push a button.

contrib/kyua/cli/cmd_debug.cpp
94–95

This kind of behavior is not documented in the proposed manpage change. Please thoroughly document the feature so it doesn't confuse folks about the behavior might be like.

Also, this kind of thing seems like it would be best driven via kyua.conf (for starters), but the user should be able to override it on the CLI.

contrib/kyua/engine/debugger.hpp
43

Drive-by comment: I wonder if this was something @jmmv did because std::optional didn't exist at the time this was written...

I love this.

It'd be very nice to have the actual output before it pauses, but this is a very nice improvement already, and it's not hard to run the test without '-p' first. That's one of the nice things about having tests, that it's very easy to reproduce specific scenarios.

I'm insufficiently familiar with kyua to offer much detailed feedback on the implementation, other than "I love this feature".

contrib/kyua/model/test_case.cpp
241–244

Could you please use a more descriptive name than d?

Also, writing a variable and having const on the method seems a bit odd; I'm kind of surprised clang didn't gripe about it being an issue, but then again it's probably because this method manages a different object instance than _pimpl points to (_pimpl was global state, right? It's been a while since I looked at that code..).

Generally speaking something modifying object state shouldn't be using const.

  • Could you please verify that this doesn't regress Linux/MacOS?
  • Something like using debugger_ptr_t = std::shared_ptr<debugger> then converting all of the code to use debugger_ptr_t (or maybe just debugger_ptr? I forget the style ATF/kyua uses since I flushed that out of my cache in favor of other garbage :D..) seems like a good idea for clarity/brevity.

I actually have a different proposal.. what if the "automatic cleanup" functionality was opt-out and enabled by default or by a custom policy, e.g.,

  • I want the results from the passed test cases,
  • I want the results from the failed test cases only, or
  • I don't care, clean it up when you're done.

Also, I think that it's honestly better to get the process core and continue on personally. Otherwise you could have resources tied up waiting for a human to push a button.

The whole part about processing cores is not really functional outside of FreeBSD with the default kern.corefile sysctl value. MacOS works ok for now since the path to the core file is hacked into Kyua, but Apple likes breaking undocumented stuff in the base OS; Linux requires some extra touch to break OS provided infra because of recent changes made in the Linux kernel 5.x series around managing it via /proc or /sys (it was an odd change). Ubuntu 24.04 and some other distros also install a crash reporter which interferes with how cores are processed.

Also, I think that it's honestly better to get the process core and continue on personally. Otherwise you could have resources tied up waiting for a human to push a button.

I think we're coming at this from different perspectives. For an atf-c test case I'd agree with you that just dumping core for post-mortem debugging with lldb would be more useful.

The use case I have in mind is atf-sh, and specifically the pf tests where we often build setups with multiple vnet jails so we can send traffic between them. In that case being able to access the jails to manually repeat the traffic, run tcpdump, look at state tables, attach dtrace, ... are very common debugging steps. In that scenario keeping the jails running is much more useful.

kevans added inline comments.
contrib/kyua/model/test_case.cpp
241–244

Right, const on the method just says it won't modify this directly, not that it won't modify global state (or something via a pointer that belongs to this)

Well, it was a quick one hour patch, now it's time for polishing.

Polish and apply the suggestions

  • Could you please verify that this doesn't regress Linux/MacOS?

I usually check FreeBSD and Linux, thanks for reminding about macOS build testing.

  • Something like using debugger_ptr_t = std::shared_ptr<debugger> then converting all of the code to use debugger_ptr_t (or maybe just debugger_ptr? I forget the style ATF/kyua uses since I flushed that out of my cache in favor of other garbage :D..) seems like a good idea for clarity/brevity.

Sure, we already have things like test_program_ptr, so that debugger_ptr seems to be our option.

I actually have a different proposal.. what if the "automatic cleanup" functionality was opt-out and enabled by default or by a custom policy, e.g.,

  • I want the results from the passed test cases,
  • I want the results from the failed test cases only, or
  • I don't care, clean it up when you're done.

Also, I think that it's honestly better to get the process core and continue on personally. Otherwise you could have resources tied up waiting for a human to push a button.

Yeah, as others commented the intention is a little bit different and related to system/e2e tests or so. I've reflected this in the man page for clarity.

In D49463#1128246, @kp wrote:

It'd be very nice to have the actual output before it pauses, but this is a very nice improvement already, and it's not hard to run the test without '-p' first. That's one of the nice things about having tests, that it's very easy to reproduce specific scenarios.

Yes, as I mentioned in Slack it is a known issue -- the stdout/stderr are printed only after that, by current design. I propose to think of this as a separate patch due to it can be disputable and it may postpone this already useful feature.

contrib/kyua/cli/cmd_debug.cpp
94–95

You got me, I was thinking exactly about the same but was rushing to the working patch first :) Now the man page is updated to mention that such way it becomes interactive,

Regarding the kyua.conf. As long as it's just an ad-hoc option for a single test case debugging there are no much use cases for having it coming from the config file.

contrib/kyua/engine/debugger.hpp
43

Yes, Kyua has its own optional.

contrib/kyua/model/test_case.cpp
241–244

Could you please use a more descriptive name than d?

Sure.

Also, writing a variable and having const on the method seems a bit odd; I'm kind of surprised clang didn't gripe about it being an issue, but then again it's probably because this method manages a different object instance than _pimpl points to (_pimpl was global state, right? It's been a while since I looked at that code..).

Generally speaking something modifying object state shouldn't be using const.

Well, it is intentional :) The encapsulation is not an easy part of software design, you never know where the project is going to be tomorrow. I worked with big open-source OOP-based projects with famous companies behind when something was hidden or sealed by design with good reasoning and later users find this like a pain when a trivial fix or a simple extension requires extra effort and workarounds to apply.

Here we have a test case concept which is immutable-like after creation by design, and test_program::find() as the main provider yields const. I decided not to touch this long standing design part. That's why the method is called attach_debugger instead of a usual setter name like set_debugger to emphasize that it's not about changing the test case itself. But yes, from academical perspective it should not be done like this.

What do you think?

This feature can be refined, but I don’t want to get in the way of the utility of this change for folks like @kp, who need the debuggability. Let’s go forward with this change and refine things/sand down rough edges on GitHub.

This revision is now accepted and ready to land.May 3 2025, 7:55 PM
contrib/kyua/drivers/debug_test.cpp
96

What if the value returned is ‘.end()’ (not found)?

igoro added inline comments.
contrib/kyua/drivers/debug_test.cpp
96

Good point, and the current design is that the find() itself would through an exception. Actually, if the flow reached this point then it is not expected to be a case.

igoro added inline comments.
contrib/kyua/drivers/debug_test.cpp
96

throw*

contrib/kyua/doc/kyua-debug.1.in
42

Should these lines be moved after 37 for sorting?

120

Should this be moved after 88 for sorting?

igoro marked an inline comment as done.

Fix the man page format

This revision now requires review to proceed.May 10 2025, 7:38 PM
igoro added inline comments.
contrib/kyua/doc/kyua-debug.1.in
42

Good catch.

120

Sure. Thanks for helping to keep the quality of the project's documentation.

This feature can be refined, but I don’t want to get in the way of the utility of this change for folks like @kp, who need the debuggability. Let’s go forward with this change and refine things/sand down rough edges on GitHub.

Thanks for your time. Deal. This patch will land in the coming days. I've opened the respective GitHub PR to continue with internals there: https://github.com/freebsd/kyua/pull/266.

This revision is now accepted and ready to land.May 10 2025, 9:10 PM
contrib/kyua/doc/kyua-debug.1.in
94
95
97
101

Improve the man page wording

This revision now requires review to proceed.May 11 2025, 4:56 PM
igoro added inline comments.
contrib/kyua/doc/kyua-debug.1.in
101

Many thanks for the help.

A couple more man page nits, otherwise this seems fine to me, modulo the other comment. I didn't carefully review the code changes.

contrib/kyua/doc/kyua-debug.1.in
90

Or, "Causes kyua to pause right before ..."

91
92

I missed this constraint last time I read through this.

Is it possible to have this work even if there is no cleanup routine? The reason is that sometimes there are useful artifacts left in the test's sandbox directory (I think this is usually /tmp/kyua.XXXXXX), and I often want to have access to it when debugging. No cleanup routine is needed to remove that directory since kyua handles it automatically.

I don't mean for this comment to block the patch, since the functionality is obviously useful. Sorry for bringing it up so late.

igoro added inline comments.
contrib/kyua/doc/kyua-debug.1.in
92

Actually, this is how it must be done. Somehow I ended up focusing mostly on network tests which have explicit test specific artifacts to be removed by their cleanup routines. Many thanks for spotting this.

This revision was not accepted when it landed; it landed in state Needs Review.May 17 2025, 9:33 AM
This revision was automatically updated to reflect the committed changes.
igoro marked an inline comment as done.