dom0-update: In download mode, only send specifically requested packages to dom0#638
dom0-update: In download mode, only send specifically requested packages to dom0#638crass wants to merge 2 commits intoQubesOS:mainfrom
Conversation
crass
force-pushed
the
fix-dom0-update-packages
branch
from
603b811 to
880e804
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #638 +/- ##
==========================================
+ Coverage 69.89% 71.71% +1.81%
==========================================
Files 3 3
Lines 495 502 +7
==========================================
+ Hits 346 360 +14
+ Misses 149 142 -7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
marmarek
left a comment
marmarek
left a comment
There was a problem hiding this comment.
This looks quite fragile IMO, the output format isn't guaranteed to stay the same...
Can you avoid this magic file selection if --clean was used (in which case old packages are removed first anyway)? Just in case it breaks at some point, there would be an option to bypass it. And for that effect, maybe add hint about it to the "package requested but not downloaded" error message?
|
|
||
| if ls "$DOM0_UPDATES_DIR"/packages/*.rpm > /dev/null 2>&1; then | ||
| case "$UPDATE_ACTION" in | ||
| download|upgrade|install) |
There was a problem hiding this comment.
This breaks all the other actions - like distro-sync or downgrade, or some future action not known by dnf yet. Please add fallback to the old approach for other actions.
There was a problem hiding this comment.
Yes, I agree this is a good idea. I'd also like to note that actions under the fallback will have the same issue that this is trying to fix. I suppose it can not be helped.
There was a problem hiding this comment.
I've added distro-sync and downgrade and tested them. There are command aliases that could be added, but I'm not sure how much we care. These are the common commands. A fallback has been added for when there is an error in the new package gathering or if it comes back with no packages. If the RPMS var is set to the empty string for upgrade, this could be a parsing problem that doesn't cause an error or just that there are no packages to update. In this case it doesn't hurt to send all cached packages, though it can be the unnecessary sending of hundreds of MBs of package data to dom0. At some point we could optimize that.
There was a problem hiding this comment.
TBH, I'd prefer the less common commands to fallback to the old method, which is more reliable (doesn't depend on specific dnf output formatting). This is especially relevant for distro-sync which is used for distribution upgrade and may include less common lines like changing arch of a package, forced downgrades, obsoletes etc. If output processing finds some packages, but not all, the upgrade as a whole will fail...
But also, please go directly to the old method in case --clean was used. There are no other packages downloaded in this case, and it would give a way to upgrade even if output processing misses something.
There was a problem hiding this comment.
Ok, I was confused by the use of fallback, which, to my mind, means a path taken on some kind of failure in the regular code path. But I'm now understanding you to mean that distro-sync should unconditionally use the old/current code path. Correct? Perhaps also, I could add an argument, say --use-all-cached-packages, that forces the old code path.
There was a problem hiding this comment.
But I'm now understanding you to mean that
distro-syncshould unconditionally use the old/current code path. Correct?
Yes
Perhaps also, I could add an argument, say
--use-all-cached-packages, that forces the old code path.
I'd say doing it when --clean is specified is enough. --clean also avoids the original issue this PR is fixing (at the expense of potentially downloading some packages again).
There was a problem hiding this comment.
Perhaps also, I could add an argument, say
--use-all-cached-packages, that forces the old code path.I'd say doing it when --clean is specified is enough. --clean also avoids the original issue this PR is fixing (at the expense of potentially downloading some packages again).
This is still pending.
There was a problem hiding this comment.
Thanks for the reminder. I've not had the time to work on this. Just pushed changes. I wonder if distro-sync should use the new code path and the old path can be chosen by passing --clean.
crass
force-pushed
the
fix-dom0-update-packages
branch
2 times, most recently
from
952ab07 to
4455d33
Compare
…ges to dom0 Save output of the package manager command. Then, if called with an action that downloads packages, parse out the requested packages and only send those to dom0. Fallback to the old method if --clean was specified. This fixes an issue where packages could be installed that were not intended or where packages could not be installed due to missing dependencies from packages not explicitly requested by the user. Fixes QubesOS/qubes-issues#10716
crass
force-pushed
the
fix-dom0-update-packages
branch
from
4455d33 to
5822cd0
Compare
|
Now with no updates pending ( The old code had |
Hmm, I don't see why qfile-agent is being run. There is a |
|
That's probably related to the fallback: if there are no packages there, it will set |
If there are no downloaded packages, do not assign literal "*.rpm" to the RPMS variable - the later part of the scripts would actually try to send a file with this name, and fail.
OpenQA test summaryComplete test suite and dependencies: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2026042316-4.3&flavor=pull-requests Test run included the following:
New failures, excluding unstableCompared to: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2026032404-devel&flavor=update
Failed tests43 failures
Fixed failuresCompared to: https://openqa.qubes-os.org/tests/170766#dependencies 32 fixed
Unstable testsDetails
Performance TestsPerformance degradation:11 performance degradations
Remaining performance tests:100 tests
|
|
I tried to fix your version, but quite quickly found several more issues, mostly due to not handling various output lines. For example: or extra lines between list of packages: The fallback saved the day, but still, if it's not going to handle even standard update, I don't think it's worth complicating things. |
3 participants
Currently
qubes-download-dom0-updates.shwill send to dom0 all files with a.rpmfile extension to dom0 to be installed by the package manager. Because interrupting the qubes-dom0-update does not stop the UpdateVM from downloading requested packages (QubesOS/qubes-issues#10722), the files sent back to dom0 to be installed by the package manager may be more than was actually requested. This could cause unintended installs or installation failures.This PR changes
qubes-download-dom0-updates.shto only send packages that were requested. This is done by parsing the output of the package manager to extract package filenames. This was tested on Qubes 4.3, with the UpdateVM being sys-whonix.sys-whonixusesdnf, but it appears that the script needs to supportyumanddnf5. Those have not been explicitly tested. dnf5 from fedora 41 appears to have sufficiently similar output and I'd expect the output to be pretty stable.Fixes QubesOS/qubes-issues#10716