Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dbus: Remove the upper limit on try timeout (LP: #1967084) #271

Merged
merged 1 commit into from
Apr 11, 2022

Conversation

schopin-pro
Copy link
Contributor

@schopin-pro schopin-pro commented Apr 11, 2022

Also, increase the default to 10s.

If the caller sees fit to specifiy a 30s timeout for their try call, it
stands to reason to respect that decision, as they (probably) know
better.

This issue came up in the Core testsuite, where the netplan
configuration step failed because netplan try took 6s, cf
LP: #1967084. Their DBus call specifies a timeout of 300s, but the call
failed after a "relatively short time", which was this internal timeout.

Note that the mere fact that netplan try takes more than 5s on such a
simple configuration is probably due to the following patch:

https://git.launchpad.net/ubuntu/+source/netplan.io/tree/debian/patches/0006-cli-apply-give-some-extra-time-for-networkctl-reload.patch?h=applied/ubuntu/focal-updates

Since the bridge is empty, it might be in a "configuring" state forever.

Granted, that's not something one would do on production systems,
however creating an empty bridge interface is routinely done in test
suites, including our own.

Checklist

  • Runs make check successfully.
  • Retains 100% code coverage (make check-coverage).
  • (Optional) Closes an open bug in Launchpad.

@codecov-commenter
Copy link

codecov-commenter commented Apr 11, 2022

Codecov Report

Merging #271 (c72bf8c) into main (d692ce7) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #271   +/-   ##
=======================================
  Coverage   99.07%   99.07%           
=======================================
  Files          61       61           
  Lines       10864    10864           
=======================================
  Hits        10764    10764           
  Misses        100      100           
Impacted Files Coverage Δ
src/dbus.c 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d692ce7...c72bf8c. Read the comment docs.

Also, increase the default to 10s.

If the caller sees fit to specifiy a 30s timeout for their try call, it
stands to reason to respect that decision, as they (probably) know
better.

This issue came up in the Core testsuite, where the netplan
configuration step failed because `netplan try` took 6s, cf
LP: #1967084. Their DBus call specifies a timeout of 300s, but the call
failed after a "relatively short time", which was this internal timeout.

Note that the mere fact that `netplan try` takes more than 5s on such a
simple configuration is probably due to the following patch:

https://git.launchpad.net/ubuntu/+source/netplan.io/tree/debian/patches/0006-cli-apply-give-some-extra-time-for-networkctl-reload.patch?h=applied/ubuntu/focal-updates

Since the bridge is empty, it might be in a "configuring" state forever.

Granted, that's not something one would do on production systems,
however creating an empty bridge interface is routinely done in test
suites, including our own.
Copy link
Collaborator

@sil2100 sil2100 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In principle this sounds fine. At least right now I can't think of a reason why we would only allow overriding the timeout but only up to the default value that we provide. So +1.

@sil2100
Copy link
Collaborator

sil2100 commented Apr 11, 2022

After this is merged, I'm thinking of cherry-picking this fix and including it in the ubuntu-image PPA used by the coreXX builds. But this depends on the severity - if @mvo5 thinks it's not needed, then we can just wait for this to go through the usual SRU route or the root cause being fully identified.

@schopin-pro schopin-pro merged commit a738597 into canonical:main Apr 11, 2022
@schopin-pro schopin-pro deleted the dbus-timeout-fix branch April 11, 2022 13:30
@mvo5
Copy link
Contributor

mvo5 commented Apr 11, 2022

Thanks, this looks fine. Let's not forge/ignoret the other findings in LP: 1967084 though after merging this, I think there is more to this than just increasing the timeout :) (either on the snapd or the netplan side, a bit unclear to me at this point)

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