updater: dom0 non-iteractive updater#191
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #191 +/- ##
==========================================
- Coverage 72.63% 69.33% -3.31%
==========================================
Files 10 10
Lines 1162 1275 +113
==========================================
+ Hits 844 884 +40
- Misses 318 391 +73 ☔ 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 is based on initial reading the code, I haven't tested it yet.
vmupdate/agent/source/dnf/dnf_api.py
Outdated
| super().__init__(log_handler, log_level, agent_type) | ||
|
|
||
| if self.type == AgentType.UPDATE_VM: | ||
| dnfconf = "/var/lib/qubes/dom0-updates/etc/dnf/dnf.conf" |
There was a problem hiding this comment.
Use self.UPDATE_VM_INSTALLROOT like in the other places?
vmupdate/agent/source/dnf/dnf_api.py
Outdated
|
|
||
| self.base = dnf.Base(conf) | ||
| if self.type == AgentType.UPDATE_VM: | ||
| self.base._allow_erasing = True |
There was a problem hiding this comment.
Looking at the dnf source, the only place where it's used in practice is an argument to base.resolve(). So, you can use it there (in upgrade_internal) instead of setting a private attribute.
| QVMRUN_OPTS=(--quiet --filter-escape-chars --nogui --pass-io) | ||
|
|
||
| if [ "$PROGRESS_REPORTING" == "1" ]; then | ||
| CMD="/usr/lib/qubes/qubes-download-dom0-updates-init.sh" |
There was a problem hiding this comment.
This needs to handle the case of old template too. It's okay to refuse this mode in such a case, but it needs a proper error message instead of a file not found error. Something like "Progress reporting requires updatevm based on a template with Qubes 4.3 packages", or something like this (maybe include template name and/or name of the updatevm?)
Whether it is new enough, you can check either by handling exit code 127 (file not found), or (better) by announcing some supported feature in the core-agent-linux PR and checking it here.
| from .exit_codes import EXIT | ||
|
|
||
|
|
||
| class AgentType(enum.Enum): |
There was a problem hiding this comment.
Nit: Is there some more specific and descriptive name than AgentType/self.type/agent_type? It's very overloadable... In particular self.type can become problematic.
How about for example AgentDomain/self.domain/agent_domain?
|
The test is still running at https://openqa.qubes-os.org/tests/144378, but I see already some exception (this is updating just templates for now): |
|
I tried to play with it and I have some questions/remarks:
PS my approach to get some updates pending is this: |
piotrbartman
force-pushed
the
vmupdater/dom0
branch
from
c53a1ac to
ff887e8
Compare
|
openQArun PR_LABEL=openqa-group-1 TEST=system_tests_network,system_tests_network_updates,system_tests_gui_tools MACHINE=64bit |
1 similar comment
|
openQArun PR_LABEL=openqa-group-1 TEST=system_tests_network,system_tests_network_updates,system_tests_gui_tools MACHINE=64bit |
Is it intentional change? It should fail (trying to update without network access), but it used to fail with different error code. |
|
In dnf5, building the repo sack is more automated. When loading repositories, libdnf5 decides internally whether to refresh the data or if the cache is fresh enough (like dnf cli). This gives us less control over the metadata refresh step, even though it is required to build the repo sack object. Currently, RN the first try of network connection happens during the update step, which explains the different error code. IMO we should update the test to accept both error codes for all package managers (as is already done for Arch), rather than modifying libdnf5 to mimic apt. Thoughts? |
|
Fair enough. |
DNF5 gives less control over when metadata is refreshed. Accept both error codes in the test. More details at QubesOS/qubes-core-admin-linux#191 (comment)
|
openQArun PR_LABEL=openqa-group-1 TEST=system_tests_network,system_tests_network_updates,system_tests_gui_tools MACHINE=64bit |
marmarek
left a comment
marmarek
left a comment
There was a problem hiding this comment.
Besides two minor issues, error handling is broken:
dom0 (no updates): 0%| | 0/100 [00:06<?, ?it/s]
( ' d o m 0 : e r r : U s i n g s y s - f i r e w a l l a s U p d a t e V M f o r D o m 0 ' , )
( ' d o m 0 : e r r : D o w n l o a d i n g u p d a t e s . T h i s m a y t a k e a w h i l e . . . ' , )
( ' d o m 0 : e r r : / b i n / b a s h : l i n e 1 : / u s r / l i b / q u b e s / q u b e s - d o w n l o a d - d o m 0 - u p d a t e s - i n i t . s h : N o s u c h f i l e o r d i r e c t o r y ' , )
( ' d o m 0 : o u t : [ 0 ; 3 1 m [ 0 m d o m 0 : o u t : R e f r e s h i n g p a c k a g e i n f o ' , )
( ' d o m 0 : o u t : s y s - f i r e w a l l : o u t : ' , )
( ' d o m 0 : o u t : s y s - f i r e w a l l : e r r : ' , )
( ' d o m 0 : o u t : d o m 0 : o u t : N o t h i n g t o d o . ' , )
( ' d o m 0 : e r r : d o m 0 d o n e n o _ u p d a t e s ' , )
( < v m u p d a t e . a g e n t . s o u r c e . s t a t u s . F o r m a t e d L i n e o b j e c t a t 0 x 7 1 1 8 f 8 2 b 4 c 5 0 > , )
( < v m u p d a t e . a g e n t . s o u r c e . s t a t u s . F o r m a t e d L i n e o b j e c t a t 0 x 7 1 1 8 f 8 2 b 4 c 5 0 > , )
[user@dom0 ~]$ echo $?
0
failure of the script (here because it wasn't there, but applies to other cases too) should be reported as an error, not "no updates"
vmupdate/update_manager.py
Outdated
| def print(self, *args): | ||
| if self.buffered: | ||
| self.buffer += ' '.join(args) + '\n' | ||
| self.buffer += " ".join(str(args)) + "\n" |
There was a problem hiding this comment.
Didn't you mean something like map(str, args)?
dom0-updates/qubes-dom0-update
Outdated
| CMD="script --quiet --return --command '${CMD//\'/\'\\\'\'}' /dev/null" | ||
| fi | ||
| if [ "$PROGRESS_REPORTING" == "1" ]; then | ||
| qvm-run "${QVMRUN_OPTS[@]}" -- "$UPDATEVM" "$CMD" < /dev/null | sed "s/``^/$(hostname):out: /" |
There was a problem hiding this comment.
| qvm-run "${QVMRUN_OPTS[@]}" -- "$UPDATEVM" "$CMD" < /dev/null | sed "s/``^/$(hostname):out: /" | |
| qvm-run "${QVMRUN_OPTS[@]}" -- "$UPDATEVM" "$CMD" < /dev/null | sed "s/^/$(hostname):out: /" |
?
DNF5 gives less control over when metadata is refreshed. Accept both error codes in the test. More details at QubesOS/qubes-core-admin-linux#191 (comment)
There was a problem hiding this comment.
pacman plugin needs an updated __init__ arguments too
There was a problem hiding this comment.
oh, it needs more than that, AttributeError: 'PACMANCLI' object has no attribute 'PROGRESS_REPORTING'
introduce agent_type: for local (dom0), remote (template vms etc.) and proxy (update vm) actions. `qubes-vm-update` uses `qubes-dom0-update --just-print-progress` for non-interactive update, only small subset of action are supported.
…tions to be compliant with DNF's progress reporting.
VMs with the above features should be excluded from updating.
piotrbartman
force-pushed
the
vmupdater/dom0
branch
from
ff887e8 to
a7074f9
Compare
dom0-updates/qubes-dom0-update
Outdated
| --quiet) | ||
| exec > /dev/null | ||
| ;; |
There was a problem hiding this comment.
This breaks salt, see QubesOS/qubes-issues#10543 (comment)
There was a problem hiding this comment.
I believe the easiest way would be to rename this flag, --silent ?
There was a problem hiding this comment.
Sounds good, --silent is not a valid dnf option, so should be safe.
5 participants
qubes-vm-updateusesqubes-dom0-update --just-print-progressfor non-interactive update, only small subset of action are supported.requires: QubesOS/qubes-core-agent-linux/pull/576