Skip to content
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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

zhangqianqian
Copy link

No description provided.

@zhangqianqian zhangqianqian force-pushed the only_check_lspci_and_dmidecode_on_x86_64 branch from c8c1941 to c8f4c66 Compare June 17, 2024 04:05
Copy link
Contributor

@ptoscano ptoscano left a 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.

integration-tests/test_common_specs.py Outdated Show resolved Hide resolved
common_specs.extend(
[
"insights.specs.Specs.dmidecode.json",
"insights.specs.Specs.lspci.json",
Copy link
Contributor

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.

Copy link

@xiangce xiangce Jun 20, 2024

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.

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.

Copy link
Contributor

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

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.

integration-tests/test_common_specs.py Show resolved Hide resolved
@zhangqianqian zhangqianqian force-pushed the only_check_lspci_and_dmidecode_on_x86_64 branch from c8f4c66 to 21685ff Compare June 20, 2024 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants