Return UnknownDevice for assignments where device is unknown#400
Return UnknownDevice for assignments where device is unknown#400marmarta wants to merge 1 commit intoQubesOS:mainfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #400 +/- ##
=======================================
Coverage 76.65% 76.66%
=======================================
Files 53 53
Lines 9407 9411 +4
=======================================
+ Hits 7211 7215 +4
Misses 2196 2196 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Fix tests so that now they will fail if there is a bad reaction to an unknown PCI device. Clean-up handling unknown PCI devices by settigs. requires QubesOS/qubes-core-admin-client#400 fixes QubesOS/qubes-issues#10409
| raise ProtocolError("No devices matches to assignment") | ||
| return UnknownDevice(port=self.port, | ||
| device_id=self.device_id, | ||
| attachment=self) |
There was a problem hiding this comment.
First, check carefully places where this attribute is used (in global config, VM settings, widgets, ansible etc). The behavior change may be unexpected in some, especially since the old behavior was explicitly documented in the docstring.
Then, update docstring.
And finally, I don't think attachment attribute should be set. Its docstring says "VM to which device is attached (frontend domain)", which cannot be true for not present device.
There was a problem hiding this comment.
I think you're implying this is a bad idea? I think it makes sense not to raise a ProtocolError if we have an UnknownDevice for those purposes, but I can try to think about tacking this problem in another way.
(and about the attachment attribute, it is incorrect to set here self, yes, but this is a property of a DeviceAssignment that knows its frontend domain, shouldn't we set it?)
There was a problem hiding this comment.
I think the change makes sense in general. But since it's behavior change, it needs at least checking if other places don't rely on the old behavior. For example attach_device in qubesadmin/tools/qvm_device.py seems to rely on this exception. I'm sure there are other places too (maybe also in other repositories...)
(and about the attachment attribute, it is incorrect to set here self, yes, but this is a property of a DeviceAssignment that knows its frontend domain, shouldn't we set it?)
"attachment" is about where the device is attached (like, currently connected), not assigned (potentially attached, automatically or not, may be even multiple assignments for the same device)
There was a problem hiding this comment.
Oh, and note this file is also copied to core-admin, so check if that side doesn't rely on this exception too.
|
I tried to find uses of this in other modules, didn't find reliance on the exception outside of core-admin (QubesOS/qubes-core-admin#799) and core-admin-client. |
|
I'm not sure here; the tests failed when I did a change to react to this change in qvm_detach, but I'm not sure if it's not because the tests do not take into account the concurrent change in core-admin? |
OpenQA test summaryComplete test suite and dependencies: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2026050404-devel&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 tests14 failures
Fixed failuresCompared to: https://openqa.qubes-os.org/tests/170766#dependencies 30 fixed
Unstable testsDetailsPerformance TestsPerformance degradation:No issues Remaining performance tests:No remaining performance tests |
3 participants
references QubesOS/qubes-issues#10409