fix pulseaudio re-start command line

[lyakh]

When running a kmod-load-unload test locally on a Ubuntu 20.04 machine, it couldn't re-start pulseaudio, the reason was, that the script was trying to start the command in the form ' pulseaudio -D' i.e. with several spaces in front of the executable name and that didn't work. Whereas just using the line as obtained from "ps" output works.

I'm not sure how reliable this solution is. But it also boils down to whether we should use ps output to re-start pulseaudio or just use pulseaudio --start. Maybe the latter would be more reliable.

[marc-hb]

Thanks @lyakh for looking at the pulseaudio situation, I'm afraid you don't realize what you're getting yourself into :-)
For now the only "officially supported" pulseaudio configuration is apt remove pulseaudio, see conclusion in #289. This is what we have in CI for now.

Now if you think you can (gradually) improve this situation then great! Can you first try to (partially?) revert c7d2477? I think it was not well tested and buggy, see discussion about this commit in PR #266 which maybe related to yours.

I think extracting pulseaudio arguments from the output of ps to save them and restart pulseaudio is crazy: it's extremely unlikely to be enough to restore pulseaudio the same way as I already mentioned in PR #159. Ideally we should use proper OS configuration like autospawn = no in /etc/pulse/client.conf and any other required systemctl magic as indicated by @juimonen at #289 (comment)

BTW I created a new label "pulseaudio" in sof-test for convenience, please click on it on the right side of this page.

[lyakh]

@marc-hb not sure I wanted to go as far as changing the world (TM) by fixing pulseaudio ;-) There might be slightly more or less correct ways of doing something, e.g. of stopping and re-starting PA, specifying or not specifying environment, missing some command-line arguments, starting as a wrong user, etc. But not starting at all because of a wrong process name should be fixed IMHO.

[marc-hb]

Can you first try to (partially?) revert c7d2477?

not sure I wanted to go as far as changing the world (TM) by fixing pulseaudio ;-)

Understood. I would still like someone (you?) to explore the history of lib.sh a little bit because I'm fairly convinced it recently got worse and that there are better starting points in the relatively recent git history than the current tip of the branch.

[lyakh]

Can you first try to (partially?) revert c7d2477?

not sure I wanted to go as far as changing the world (TM) by fixing pulseaudio ;-)

Understood. I would still like someone (you?) to explore the history of lib.sh a little bit because I'm fairly convinced it recently got worse and that there are better starting points in the relatively recent git history than the current tip of the branch.

I looked at c7d2477 Though it doesn't strike me as a feat of elegance, it says it's fixing some error checking bug and by just looking at it I'm not sure I know what that bug was. So, reverting it completely or even some not fully understood and unverified parts of it might bring that bug back.
There are many places in our CI bash scripts that I would do differently, but I'm not 100% sure, that all those my alternative implementations would actually be improvements. I'm also not sure we should now start such a global clean up or just fix bugs 1 by 1 as we detect them.

[marc-hb]

I spent a bit more time looking at the pulseaudio situation. I think the entire approach in this script is broken, as somewhat confirmed by #289 (comment)

We tried a lot of ways to deal with pulseaudio, but in vain.

It's broken because on Ubuntu (and probably many others), pulseaudio is started by systemd's pulseaudio.socket as described by @juimonen in #289

This implies at least two issues:

• pulseaudio will restart even after having been "disabled" by func_lib_disable_pulseaudio() which should really be called func_lib_fail_to_kill_pulseaudio(). A simple aplay command is enough to restart pulseaudio, I just checked.

• As already discussed in #266, systemd starts pulseaudio with --daemonize=no which causes various issues when re-using that parameter.

Instead of these functions we should use the systemd one-liners suggested by @juimonen in #289.
In the meantime the only configuration officially supported by sof-test is apt remove pulseaudio

All this being said, I'm approving because if you think it works for you sometimes then why not. These functions are considered "not in service" anyway.

[lyakh]

All this being said, I'm approving because if you think it works for you sometimes then why not. These functions are considered "not in service" anyway.

@marc-hb thanks, without this patch I cannot run tests at all, so, yes, it seems like an improvement to me.

[marc-hb]

@marc-hb thanks, without this patch I cannot run tests at all, so, yes, it seems like an improvement to me.

Before running sof-test you should either apt remove pulseaudio or use the systemd --user disable ... commands above.

[lyakh]

@marc-hb thanks, without this patch I cannot run tests at all, so, yes, it seems like an improvement to me.

Before running sof-test you should either apt remove pulseaudio or use the systemd --user disable ... commands above.

@marc-hb that doesn't sound like a good idea to me. Firstly we want to test with pulseaudio, that's how SOF has to function, secondly if I understand correctly sof-test only tries to disable PA for module loading-unloading tests and only unless a parameter is specified explicitly to block that behaviour.

[marc-hb]

Firstly we want to test with pulseaudio, that's how SOF has to function,

There are different levels of testing. For the lowest level and most fundamental and most important level of testing we don't want pulseaudio to interfere with the tests (it has been for a very long time)

secondly if I understand correctly sof-test only tries to disable PA for module loading-unloading tests and only unless a parameter is specified explicitly to block that behaviour.

That's not my understanding.

that doesn't sound like a good idea to me.

As @aiChaoSONG mentioned there has already been an enormous amount of time spent trying to disable and re-enable pulseaudio dynamically. Since it has failed repeatedly (and inconsistently), pulseaudio is now removed. See above where I could reproduce one of these undesired autospawns in minutes. I'm afraid these attempts didn't use systemd and/or the autospawn setting though, so maybe there is hope.

[lyakh]

Firstly we want to test with pulseaudio, that's how SOF has to function,

There are different levels of testing. For the lowest level and most fundamental and most important level of testing we don't want pulseaudio to interfere with the tests (it has been for a very long time)

secondly if I understand correctly sof-test only tries to disable PA for module loading-unloading tests and only unless a parameter is specified explicitly to block that behaviour.

That's not my understanding.

that doesn't sound like a good idea to me.

As @aiChaoSONG mentioned there has already been an enormous amount of time spent trying to disable and re-enable pulseaudio dynamically. Since it has failed repeatedly (and inconsistently), pulseaudio is now removed. See above where I could reproduce one of these undesired autospawns in minutes. I'm afraid these attempts didn't use systemd and/or the autospawn setting though, so maybe there is hope.

@kv2019i I think it was you who suggested, that since we now always run with PA enabled "in the field" i.e. on Linux laptops and the like, that's also how we should test? And that even some use-cases won't work without it?

[paulstelian97]

First of all test without PA to ensure the driver and firmware are okay, then test with as well to compare. Some issues are due to PA configuration and the UCM...

[plbossart]

I agree with @lyakh that you cannot ask testers to remove pulseaudio on their target machines.
What you do for CI is a different story, you may remove PulseAudio if you feel like it.
Don't force everyone involved in testing to remove PulseAudio please.

[marc-hb]

CI systems would ideally have as few differences as possible with real world systems. Unfortunately removing pulseaudio on them is the best we can do for now. Patches welcome!

PS: removing and re-installing pulseaudio with apt takes seconds.

[kv2019i]

To @lyakh, ack on @paulstelian97 's reply that it is perfectly valid to have both -- e.g. to have a set of tests where Pulseaudio should be disabled and another with the full vertical stack (including Pulseaudio).

Given we target a more limited set of distros, disabling Pulseaudio from the script in a robust way, should still be a doable task (and yes, I understand patches are welcome). In wider context of upstream bugs, I usually ask the bug reporter to rename the PA binary and kill all PA processes. This is a minimal procedure that works 100% of the time, but for our CI scripts, I believe we can have something cleaner... and not require to uninstall PA.

[lgirdwood]

fwiw systemd may be respawning pulseaudio too (since it seems to ignore it's own "no respawn" config).

[kv2019i]

@lgirdwood wrote:

fwiw systemd may be respawning pulseaudio too (since it seems to ignore it's own "no respawn" config).

That's one aspect that complicates life in wider context of Linux systems, but in our case (mostly Ubuntu/Fedora), Pulseaudio is run as part of the user session and not as a system-wide daemon. So these instructions to stop Pulseaudio and disabling autospawn should work for all CI cases: https://wiki.debian.org/PulseAudio . But yeah, in wider context, there are a lot more options -- Arch Linux installs a systemd service and e.g. Pulseaudio Snap for Ubuntu adds a systemd unit for some reason -- and then you have to worry about the separate spawn mechanisms of systemd.

[marc-hb]

I forgot to remove pulseaudio and my test script (as root) just hung forever because:

Ctrl-Z
ps f
sudo -u user /usr/bin/pulseaudio --daemonize=no --log-target=journal

I admittedly did NOT have autospawn and systemd configured not to start it.

I just found this other knob by chance:

systemctl list-unit-files | grep -i pulse
pulseaudio-enable-autospawn.service masked enabled