Skip to content

worker: disconnect dbus from NameOwnerChanged signal (POO #183833)#6578

Merged
mergify[bot] merged 1 commit intoos-autoinst:masterfrom
AdamWill:worker-dbus-quota-fix
Jul 10, 2025
Merged

worker: disconnect dbus from NameOwnerChanged signal (POO #183833)#6578
mergify[bot] merged 1 commit intoos-autoinst:masterfrom
AdamWill:worker-dbus-quota-fix

Conversation

@AdamWill
Copy link
Contributor

@AdamWill AdamWill commented Jul 9, 2025

This avoids us piling up signals we never handle and eventually exceeding a dbus quota limit and causing all subsequent tap jobs to fail.

New Net::DBus instances are automatically connected to this signal on creation, you cannot change this. If the instance is kept around but nothing iterates the dbus event loop once in a while, that signal will just keep piling up messages indefinitely. We encountered this once before in os-autoinst and "solved" it by not persisting the dbus connection, but in this case it doesn't seem a good idea to re-initialize that connection every time we want to check on the status of a tap worker. So instead let's disconnect it from the signal.

The 1 here is a bit magical. The Net::DBus connect_to_signal method issues signal numbers on a first-come, first-served basis, starting at 1 (0 is never used), and returns the number it issued. When Net::DBus _new calls it, though, it doesn't record the return value. So we have to know/guess that this will always be the first signal connected.

Related ticket: https://progress.opensuse.org/issues/183833

@AdamWill
Copy link
Contributor Author

AdamWill commented Jul 9, 2025

I'm not 100% sure this is un-hacky enough to upstream, but figured I'd submit it at least. It'd be great if @berrange could comment. Relevant perl-net-dbus code areas:

@AdamWill
Copy link
Contributor Author

AdamWill commented Jul 9, 2025

The last time we ran into a very similar issue was https://progress.opensuse.org/issues/90872 , which we 'fixed' with os-autoinst/os-autoinst#1641 .

@AdamWill AdamWill force-pushed the worker-dbus-quota-fix branch 2 times, most recently from 48d115b to ea30444 Compare July 9, 2025 18:53
@berrange
Copy link

berrange commented Jul 9, 2025

I'm not 100% sure this is un-hacky enough to upstream, but figured I'd submit it at least. It'd be great if @berrange could comment. Relevant perl-net-dbus code areas:

Yes it is a bit hacky, but at the same time I think it is likely the least worst option, given the constraints imposed by the perl-net-dbus implementation. Since the perl-net-dbus project is largely dormant, it is unlikely to see new changes that would invalidate the assumption made here.

@AdamWill AdamWill force-pushed the worker-dbus-quota-fix branch 3 times, most recently from d38f66b to 32857f4 Compare July 9, 2025 19:47
This avoids us piling up signals we never handle and eventually
exceeding a dbus quota limit and causing all subsequent tap jobs
to fail.

New Net::DBus instances are automatically connected to this signal
on creation, you cannot change this. If the instance is kept
around but nothing iterates the dbus event loop once in a while,
that signal will just keep piling up messages indefinitely. We
encountered this once before in os-autoinst and "solved" it by not
persisting the dbus connection, but in this case it doesn't seem a
good idea to re-initialize that connection every time we want to
check on the status of a tap worker. So instead let's disconnect
it from the signal.

The 1 here is a bit magical. The Net::DBus connect_to_signal
method issues signal numbers on a first-come, first-served basis,
starting at 1 (0 is never used), and returns the number it
issued. When Net::DBus _new calls it, though, it doesn't record
the return value. So we have to know/guess that this will always
be the first signal connected.

Related ticket: https://progress.opensuse.org/issues/183833

Signed-off-by: Adam Williamson <[email protected]>
@AdamWill AdamWill force-pushed the worker-dbus-quota-fix branch from 32857f4 to 7c71f3f Compare July 9, 2025 20:15
@codecov
Copy link

codecov bot commented Jul 9, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.11%. Comparing base (371ad64) to head (7c71f3f).
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6578      +/-   ##
==========================================
- Coverage   99.11%   99.11%   -0.01%     
==========================================
  Files         399      399              
  Lines       40707    40714       +7     
==========================================
+ Hits        40347    40353       +6     
- Misses        360      361       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mergify mergify bot merged commit 6ca3853 into os-autoinst:master Jul 10, 2025
51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants