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

Collect system information (New) #760

Merged
merged 32 commits into from
Jan 8, 2024
Merged

Collect system information (New) #760

merged 32 commits into from
Jan 8, 2024

Conversation

Hook25
Copy link
Collaborator

@Hook25 Hook25 commented Oct 12, 2023

Description

We have decided in CR041 that we are going to rely, among others, on inxi to fetch the hardware of a system. The best place to inject this new step is the bootstrap sequence before any resource job is collected and executed.

This should result in the collection being included as a map in the session storage and dumped at the end of the execution under a new key of the submission.json file in this form:

If a tool runs successfully

"system_information" : {
  "tool_name" : { 
    “tool_version” :  tool_version,
     "success" : True,
     “outputs” : { 
       "payload" : tool_json_output,
       "stderr" : tool_stderr
    }
  } 
}

If a tool fails or produced an invalid json output:

"system_information" : {
  "tool_name" : { 
    “tool_version” :  tool_version, 
    "success" : False,
    “outputs” : { 
      “stdout” : tool_stdout,
      “stderr” : tool_stderr
    }
  } 
}

Resolved issues

Resolves: https://warthogs.atlassian.net/browse/CHECKBOX-900

Documentation

WIP

Tests

The change was tested via metabox both in snaps and source runs

@codecov
Copy link

codecov bot commented Oct 12, 2023

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (2e0f376) 36.58% compared to head (d37eb71) 36.78%.
Report is 9 commits behind head on main.

Files Patch % Lines
...box-ng/plainbox/impl/session/system_information.py 93.25% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #760      +/-   ##
==========================================
+ Coverage   36.58%   36.78%   +0.20%     
==========================================
  Files         310      311       +1     
  Lines       34639    34759     +120     
  Branches     5965     5976      +11     
==========================================
+ Hits        12671    12786     +115     
- Misses      21399    21405       +6     
+ Partials      569      568       -1     
Flag Coverage Δ
checkbox-ng 62.92% <95.04%> (+0.27%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@kissiel kissiel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many things need changing, most of them posted below.
Generally to much unnecessary tight coupling, for something that's practically static.
Now for the whole keeping the state of the session in the state object -> why?
The sysinfo is gathered at the start of the session, and kept there and submitted with the submission. Awesome. But it may be done at the end of the session as well, and wouldn't require any change to the suspend/resume in Checkbox. "what if sysinfo changes during the session", well, with the code submitted here we also wouldn't notice.

checkbox-ng/plainbox/vendor/system_information/__init__.py Outdated Show resolved Hide resolved
checkbox-ng/plainbox/impl/session/state.py Outdated Show resolved Hide resolved
checkbox-ng/plainbox/impl/session/assistant.py Outdated Show resolved Hide resolved
@yphus
Copy link
Contributor

yphus commented Oct 17, 2023

The sysinfo is gathered at the start of the session

Getting hw info at the start of the session is IMHO a good idea, to reuse the same data for resources expression (whatever the form of such expression) or to combine one collector with another, or even with the hw manifest. Needless to call again inxi if the data collected at session start can be turned into something jobs requirements can consume.

@Hook25 Hook25 marked this pull request as ready for review October 18, 2023 09:23
@Hook25 Hook25 requested a review from kissiel October 18, 2023 09:49
Copy link
Contributor

@kissiel kissiel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I found something in need of deletion

checkbox-ng/plainbox/impl/session/system_information.py Outdated Show resolved Hide resolved
@kissiel
Copy link
Contributor

kissiel commented Nov 29, 2023

Have you tested how it affects other subcommands that create temporary sessions?
I remember other subcommands creating a "throw-away" sessions, and if this collection is run when, for instance, listing units using checkbox-cli list it would become extremely slow.

Minor: also port the fix (no reconnecting message) to inxi job
Now the collection is done automatically upon the first get call
if no set was done previously. This should be effectively the same as
before, the first checkpoint dumps the information, reading it, while
a non-fresh session will load a checkpoint setting before reading it

Minor: Update tests to mock/check the new path
Minor: moved return_code to output
Minor: moved success to property of CollectorOutput
Minor: moved str cast to actual cmd
Minor: updated tests
Minor: Better metaclass docstr
Minor: exclude build dir from the installation
Minor: updated docstring in init to be up to date to the new content
@Hook25 Hook25 requested a review from kissiel January 5, 2024 09:25
Copy link
Contributor

@kissiel kissiel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks great. The problem I've got is I think its complexity is unnecessary in places.
It may be best if we sit on it together, so one of two outcomes happen:

  • I see what I'm missing, and why the things included here are necessary
  • the outputs gets simpler

checkbox-ng/plainbox/impl/session/README.md Outdated Show resolved Hide resolved
checkbox-ng/plainbox/impl/session/README.md Outdated Show resolved Hide resolved
checkbox-ng/plainbox/impl/session/README.md Outdated Show resolved Hide resolved
checkbox-ng/plainbox/impl/session/README.md Outdated Show resolved Hide resolved
checkbox-ng/plainbox/impl/session/README.md Outdated Show resolved Hide resolved
checkbox-ng/plainbox/impl/session/README.md Outdated Show resolved Hide resolved
checkbox-ng/plainbox/impl/session/README.md Outdated Show resolved Hide resolved
kissiel
kissiel previously approved these changes Jan 8, 2024
Copy link
Contributor

@kissiel kissiel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Monumental work!
Thanks for all the tweaks and improvements!

@Hook25 Hook25 enabled auto-merge (squash) January 8, 2024 16:05
Copy link
Contributor

@kissiel kissiel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Hook25 Hook25 merged commit bdbd9be into main Jan 8, 2024
19 checks passed
@Hook25 Hook25 deleted the collect_sys_info branch January 8, 2024 17:10
LiaoU3 pushed a commit to LiaoU3/checkbox that referenced this pull request Jan 9, 2024
* Init implenation of system info gathering

* System information collector init and vendorized inxi

* Store actual infos gathered by system_information in the state

* Make system_information in state persistent

* Include system_information in submission.json

* Tests for system_information collectors

* Metabox tests to check system_information

Minor: also port the fix (no reconnecting message) to inxi job

* Updated plainbox tests for new v8 state

* Update to overall class style

* Test class that resumes the session

* Avoid test_assistant's collection of system_information

* Test also build_SessionState

* Minor: small test for collect

* Make system_information executable

* Moved responsibility of system_information collection

Now the collection is done automatically upon the first get call
if no set was done previously. This should be effectively the same as
before, the first checkpoint dumps the information, reading it, while
a non-fresh session will load a checkpoint setting before reading it

Minor: Update tests to mock/check the new path

* Refactored Collectors to use metaclasses

Minor: moved return_code to output
Minor: moved success to property of CollectorOutput
Minor: moved str cast to actual cmd
Minor: updated tests

* Test also CollectorMeta

* Change REGISTER_NAME -> COLLECTOR_NAME

Minor: Better metaclass docstr

* Include inxi in the setup MANIFEST.in

Minor: exclude build dir from the installation

* Removed success from OutputSuccess and added super

* Mock inxi in metabox tests

* Removed additional system_information module

Minor: updated docstring in init to be up to date to the new content

* Documentation about System Information Collection

* Fixed paragraphs headers

* Address review comments

* Rename json_output -> payload

* Avoid building json in jinja

* Fix typing mistake and update main with the new impl.

* Versioned collector outputs

Minor: fixed typo

* Updated path system_information in vendor

* don't jsonify json

* Fixed loading mistyping the result of collection
LiaoU3 pushed a commit to LiaoU3/checkbox that referenced this pull request Mar 20, 2024
* Init implenation of system info gathering

* System information collector init and vendorized inxi

* Store actual infos gathered by system_information in the state

* Make system_information in state persistent

* Include system_information in submission.json

* Tests for system_information collectors

* Metabox tests to check system_information

Minor: also port the fix (no reconnecting message) to inxi job

* Updated plainbox tests for new v8 state

* Update to overall class style

* Test class that resumes the session

* Avoid test_assistant's collection of system_information

* Test also build_SessionState

* Minor: small test for collect

* Make system_information executable

* Moved responsibility of system_information collection

Now the collection is done automatically upon the first get call
if no set was done previously. This should be effectively the same as
before, the first checkpoint dumps the information, reading it, while
a non-fresh session will load a checkpoint setting before reading it

Minor: Update tests to mock/check the new path

* Refactored Collectors to use metaclasses

Minor: moved return_code to output
Minor: moved success to property of CollectorOutput
Minor: moved str cast to actual cmd
Minor: updated tests

* Test also CollectorMeta

* Change REGISTER_NAME -> COLLECTOR_NAME

Minor: Better metaclass docstr

* Include inxi in the setup MANIFEST.in

Minor: exclude build dir from the installation

* Removed success from OutputSuccess and added super

* Mock inxi in metabox tests

* Removed additional system_information module

Minor: updated docstring in init to be up to date to the new content

* Documentation about System Information Collection

* Fixed paragraphs headers

* Address review comments

* Rename json_output -> payload

* Avoid building json in jinja

* Fix typing mistake and update main with the new impl.

* Versioned collector outputs

Minor: fixed typo

* Updated path system_information in vendor

* don't jsonify json

* Fixed loading mistyping the result of collection
binli pushed a commit to binli/checkbox that referenced this pull request Mar 22, 2024
* Init implenation of system info gathering

* System information collector init and vendorized inxi

* Store actual infos gathered by system_information in the state

* Make system_information in state persistent

* Include system_information in submission.json

* Tests for system_information collectors

* Metabox tests to check system_information

Minor: also port the fix (no reconnecting message) to inxi job

* Updated plainbox tests for new v8 state

* Update to overall class style

* Test class that resumes the session

* Avoid test_assistant's collection of system_information

* Test also build_SessionState

* Minor: small test for collect

* Make system_information executable

* Moved responsibility of system_information collection

Now the collection is done automatically upon the first get call
if no set was done previously. This should be effectively the same as
before, the first checkpoint dumps the information, reading it, while
a non-fresh session will load a checkpoint setting before reading it

Minor: Update tests to mock/check the new path

* Refactored Collectors to use metaclasses

Minor: moved return_code to output
Minor: moved success to property of CollectorOutput
Minor: moved str cast to actual cmd
Minor: updated tests

* Test also CollectorMeta

* Change REGISTER_NAME -> COLLECTOR_NAME

Minor: Better metaclass docstr

* Include inxi in the setup MANIFEST.in

Minor: exclude build dir from the installation

* Removed success from OutputSuccess and added super

* Mock inxi in metabox tests

* Removed additional system_information module

Minor: updated docstring in init to be up to date to the new content

* Documentation about System Information Collection

* Fixed paragraphs headers

* Address review comments

* Rename json_output -> payload

* Avoid building json in jinja

* Fix typing mistake and update main with the new impl.

* Versioned collector outputs

Minor: fixed typo

* Updated path system_information in vendor

* don't jsonify json

* Fixed loading mistyping the result of collection
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.

3 participants