-
Notifications
You must be signed in to change notification settings - Fork 48
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
fix: check lspci and dmidecode specs on x86_64 #257
base: master
Are you sure you want to change the base?
Conversation
c8c1941
to
c8f4c66
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This partially papers over bugs in insights-core; see my latest reply to RHINENG-10737.
common_specs.extend( | ||
[ | ||
"insights.specs.Specs.dmidecode.json", | ||
"insights.specs.Specs.lspci.json", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not correct: PCI is available on basically every mainstream and new architecture nowadays.
This is a problem in insights-core, which assumes an empty output of lspci
is a bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As talked in Slack, this ContentException('Empty content')
is expected for insights-core. Although the empty output of lspci
is valid for s390x, it (the empty lspci file in insights-archive) is useless for insights, even if it's empty on x86_64
, such a ContentException
will also be recorded in its meta_data JSON errors
section. The errors
section in the JSON file indicates why this file is not collected in the archive, but doesn't mean this is an error
of the collection.
On s390x or other archs, in most cases (particular on the test VM machine), its output is always empty. so, here in this test, we can ignore check it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This issue is pending for a long time without update from @ptoscano . I don't see any problem showing ContentException in the log as this exception is defined for empty content on purpose. It is working exactly as what we expect in the design. Please kindly merge this request in case it blocks IQE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is working exactly as what we expect in the design.
As I said in other conversations (since this was discussed outside of this PR, sigh): I consider that design broken, since it has expectations that are not actually what happens in real life systems. If the spec collection considers something that can actually happen (no PCI devices) as "error", then the collection ought to be fixed.
Although the empty output of lspci is valid for s390x, it (the empty lspci file in insights-archive) is useless for insights, even if it's empty on x86_64, such a
ContentException
will also be recorded in its meta_data JSON errors section.
Then there are two ways to fix this:
- consider an empty
lspci
output an error ONLY on x86_64; on other architectures, do not collect the empty file and do not report any error - simply send the empty file in case it is there, and let Insights/Advisory/whatever decide how to consider the situation (valid, invalid) based on the rest of the collected details of a system
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ptoscano I think you mixed the use of "error" with "exception" in your expressions. "Exception" does not equal to "error". To insights-core, ContentException is designed for such cases like lspci and dmidcode. I don't think it is worth nor a good idea to force all Insights services to change their code logic using data collection with what the ContentException is designed for.
c8f4c66
to
21685ff
Compare
No description provided.