Ubuntu/devel

[blackboxsw]

do not squash merge

Queue Recommends dependency on python3-apt for apt_pkg module since cc_apt_configure uses it for some operations.
Cloud-init falls back to apt-config dump command when python module not present.

When this PR is merged, I will reflect it to Focal/Jammy/Lunar branches

Proposed commit messages

update changelog

    d/contol: add python3-apt to Recommends
    
    cc_apt_configure will use apt_pkg from this deb if present to
    read apt configuration for Dir::Etc::sourceparts/sourcelist values.
    
    When absent, rely on the command apt-config dump.

Additional Context

Test Steps

Checklist:

• My code follows the process laid out in the documentation
• I have updated or added any unit tests accordingly
• I have updated or added any documentation accordingly

[holmanb]

@blackboxsw What value is added by this dependency? Looking at the code it looks like both the apt_pkg and the apt-config dump codepaths do the same thing - is that correct?

[holmanb]

it looks like both the apt_pkg and the apt-config dump codepaths do the same thing - is that correct?

The PR discussion, commit message, and code comments don't explain why this was added either. If we can accomplish the same thing without a extra dependency, why wouldn't we just always do that? Maintaining multiple codepaths that do the same thing adds maintenance cost - what do we/users get in exchange for this cost?

Edit: Sorry this feedback didn't get into the initial review. This PR looks fine, but I would like at a minimum to see an explanation for the tradeoffs made here documented for future developers - describing the tradeoffs being made here in comments in the code would be fine. And if the functionality really is the same (outside of "library vs subp call"), then I don't think that I agree that we should go forward with making this Recommends, and in that case I would also prefer if we drop the alternative code path from our code.

[TheRealFalcon]

Performance-wise, tradeoff is about 7ms on the critical-chain, though not on the "time to ssh" path.

Adding it as Recommends isn't going to bloat anything. The library already exists in our minimal images.

Is an extra 5 lines of code (plus tests) worth 7ms?

[holmanb]

Performance-wise, tradeoff is about 7ms on the critical-chain, though not on the "time to ssh" path.

Nice. Did I miss this in a comment in the PR?

Adding it as Recommends isn't going to bloat anything. The library already exists in our minimal images.

Agreed, package bloat was not my concern for a Recommends dependency.

Is an extra 5 lines of code (plus tests) worth 7ms?

@TheRealFalcon I'm not opposed to keeping it if there is a performance benefit. Let's just please document the tradeoffs being made in this code while the reasons are fresh.

Looking at the fallback code just makes me ask "but why?", because tradeoffs are clearly being made but not explained by the code or commit message. The intent is non-obvious, so it deserves a little context in an easy-to-find place (preferably right in the code as a comment).

I didn't know performance was the reason for this until your comment (and think I read all of the comments in the original PR), and shelling out vs a library that reads files from disk isn't an obvious performance benefit - synchronous reads from disk are required for both of these code paths.

[TheRealFalcon]

I'm ok with this as-is, but I agree with Brett that we should document why this is ok.

[blackboxsw]

v

I'm ok with this as-is, but I agree with Brett that we should document why this is ok.

I've added documentation about the performance tradeoff and Recommends: state of python3-apt vs subp in PR #4484