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

refactor: remove legacy collection #4009

Merged
merged 22 commits into from
Sep 30, 2024
Merged

refactor: remove legacy collection #4009

merged 22 commits into from
Sep 30, 2024

Conversation

xiangce
Copy link
Contributor

@xiangce xiangce commented Feb 2, 2024

  • and remove all dependent codes and relevant tests
  • see RHINENG-6982

All Pull Requests:

Check all that apply:

  • Have you followed the guidelines in our Contributing document, including the instructions about commit messages?
  • Is this PR to correct an issue?
  • Is this PR an enhancement?

Complete Description of Additions/Changes:

Add your description here

@xiangce xiangce mentioned this pull request Feb 2, 2024
3 tasks
@xiangce xiangce force-pushed the remove_legacy_collection branch 2 times, most recently from 08267a6 to bfe8912 Compare February 2, 2024 08:13
@xiangce xiangce force-pushed the remove_legacy_collection branch from bfe8912 to fd791b4 Compare February 27, 2024 04:09
@xiangce
Copy link
Contributor Author

xiangce commented Feb 27, 2024

This PR requires corresponding modifications to the IQE test. Before this, the IQE test will always fail.

Comment on lines -23 to -24
Core collection should be disabled by default, unless
the RPM version 3.1 or above
Copy link
Contributor

Choose a reason for hiding this comment

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

So, how does this work with RHEL 6, which was shipped with Client 3.0.14?

Copy link
Contributor Author

@xiangce xiangce May 10, 2024

Choose a reason for hiding this comment

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

For RHEL 6, or RHEL 7, any client installed with a version older than 3.0.14, core_collect will be set to True by default, after this change.

Copy link
Contributor Author

@xiangce xiangce May 15, 2024

Choose a reason for hiding this comment

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

Per my test, the oldest version of the insights-client insights-client-3.0.0-4 that we provided to the customers, works well with core_collect=True on RHEL 6/7, for details please check the new comment to the RHINENG-6982

@candlepin-bot
Copy link

Can one of the admins verify this patch?

@xiangce xiangce force-pushed the remove_legacy_collection branch from b13ab9d to d322759 Compare May 10, 2024 06:42
@JoySnow
Copy link
Collaborator

JoySnow commented May 21, 2024

Hi @xiangce , the legacy collection change looks good to me. And I tried to run cli command insights-collect, it broken into an circular import error related to circular import, while this command works fine with the code in master branch.

FYI, the detail command output example:

$ insights-collect --help
Traceback (most recent call last):
  File "/root/Work/insights-core/venv39/bin/insights-collect", line 33, in <module>
    sys.exit(load_entry_point('insights-core', 'console_scripts', 'insights-collect')())
  File "/root/Work/insights-core/venv39/bin/insights-collect", line 25, in importlib_load_entry_point
    return next(matches).load()
  File "/usr/lib64/python3.9/importlib/metadata.py", line 86, in load
    module = import_module(match.group('module'))
  File "/usr/lib64/python3.9/importlib/__init__.py", line 127, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 1030, in _gcd_import
  File "<frozen importlib._bootstrap>", line 1007, in _find_and_load
  File "<frozen importlib._bootstrap>", line 986, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 680, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 850, in exec_module
  File "<frozen importlib._bootstrap>", line 228, in _call_with_frames_removed
  File "/root/Work/insights-core/insights/collect.py", line 23, in <module>
    from insights.core.spec_cleaner import Cleaner
  File "/root/Work/insights-core/insights/core/spec_cleaner.py", line 24, in <module>
    from insights.client.constants import InsightsConstants as constants
  File "/root/Work/insights-core/insights/client/__init__.py", line 14, in <module>
    from . import client
  File "/root/Work/insights-core/insights/client/client.py", line 22, in <module>
    from .connection import InsightsConnection
  File "/root/Work/insights-core/insights/client/connection.py", line 39, in <module>
    from insights.core.spec_cleaner import Cleaner
ImportError: cannot import name 'Cleaner' from partially initialized module 'insights.core.spec_cleaner' (most likely due to a circular import) (/root/Work/insights-core/insights/core/spec_cleaner.py)

@xiangce xiangce force-pushed the remove_legacy_collection branch 2 times, most recently from a9d0e47 to 07854e1 Compare May 21, 2024 08:17
@xiangce
Copy link
Contributor Author

xiangce commented May 21, 2024

Hi @JoySnow - thanks for reviewing. The issue you found was fixed in the latest commit, please have a look again.

Copy link
Contributor

@m-horky m-horky left a comment

Choose a reason for hiding this comment

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

I've been able to run this Core on RHEL 6.10 system with Client 3.0.14. I don't think I have notes.

Copy link
Collaborator

@JoySnow JoySnow left a comment

Choose a reason for hiding this comment

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

Thanks for the update @xiangce . The insights-collect command works well now.

$ insights-collect 
/tmp/insights-xiaoxwan-test321-rhel91.default-20240521082408

@xiangce xiangce added the WIP Includes: Approved but Not Ready for Merging/Releasing label May 28, 2024
@xiangce xiangce force-pushed the remove_legacy_collection branch from 07854e1 to 09c8bd8 Compare May 30, 2024 06:51
@xiangce xiangce force-pushed the remove_legacy_collection branch from 09c8bd8 to c1af04c Compare June 20, 2024 01:56
@xiangce xiangce force-pushed the remove_legacy_collection branch from 0f7bd27 to ee9368e Compare July 16, 2024 02:13
@xiangce xiangce force-pushed the remove_legacy_collection branch from ee9368e to 9d7bfed Compare August 7, 2024 07:12
@xiangce
Copy link
Contributor Author

xiangce commented Aug 26, 2024

test me

2 similar comments
@xiangce
Copy link
Contributor Author

xiangce commented Aug 27, 2024

test me

@xiangce
Copy link
Contributor Author

xiangce commented Aug 28, 2024

test me

skontar and others added 6 commits September 5, 2024 19:43
* Enhance httpd_conf mapper and shared reducer

Mapper:
- improved full_data field to store not only last found
  value, but also all previous ones in a list

Reducer:
- store parsed data in data field not only the last value,
  but also all found values as the list.
- added methods get_active_setting, get_setting_list to get
  more complex data structures with more data than backward
  compatible method get_valid_setting

https://bugzilla.redhat.com/show_bug.cgi?id=1455516

* Resolved review comments

* Remove unused import
* Enhance httpd_conf to support nested sections
- IfModule section is nest-able
- Support all the sections (includes nested sections)
- Re-define section name with a tuple in format of:('type', 'name')
  e.g.: ('IfModule', 'prefork.c')
- Remove the useless data and rename the `full_data` to `data`
- Remove the deprecated `get_valid_setting` from master branch
  `insights-plugin` does not depend on the master branch
- Re-design the `get_active_setting`
- Remove the filter: Do not filter httpd_conf from now on

* WIP: Version 1
- supported all kinds of sections includes nest-able sections
- revert get_setting_list() and enhance get_active_setting()

* Move `ParsedData` to parser `httpd_conf.py` as a global type
- by moving `ParsedData` simplify the logic of the two 'get_' in combiner
- Improve the dostrings and examples
- Enhance test coverage to 100%

* Enhance get_setting_list and get_active_setting
- get_setting_list returns empty list instead of None when nothing is found
- get_active_setting:
  returns an empty list when section is specified and nothing is found
  returns None when section is not specified and nothing is found
* Adding method from LogFileOutput

* Testing new method

* Now use timestamp formats for error and regular times

* Updated to time_format property, added standard and error formats for ProxyLog
- It's necessary to remove the `filters` because of some plugins, 
  such as load_nfsmod_failed, need to check if their content is empty.
* New method in HttpdConfAll combiner to list sections.
Needed by upcoming httpd hardening rule.

* Fixed docstring, simplified code.

* Fixed a docstring.
csams and others added 12 commits September 5, 2024 19:43
* Support mode was missing some config being passed in
The mounts combiner has one performance issue for IBM gpfs running hosts. As this combiner is not necessary, remove it by this MR. Use parsers.mount when you need mount information.
   IBM GPFS internal mount: https://www.ibm.com/support/pages/gpfs-internal-and-external-mount-states

Signed-off-by: Chen lizhong <[email protected]>
Signed-off-by: Xiangce Liu <[email protected]>
- and remove all dependent codes and relevant tests
- update the script that generates the filters.yaml
- see RHINENG-6982

Signed-off-by: Xiangce Liu <[email protected]>
Signed-off-by: Xiangce Liu <[email protected]>
Signed-off-by: Xiangce Liu <[email protected]>
- as it's just deprecated but should supported for a while

Signed-off-by: Xiangce Liu <[email protected]>
@xiangce xiangce force-pushed the remove_legacy_collection branch from 6bc58a1 to a6683ba Compare September 5, 2024 11:43
@codecov-commenter
Copy link

codecov-commenter commented Sep 29, 2024

Codecov Report

Attention: Patch coverage is 57.89474% with 40 lines in your changes missing coverage. Please review.

Project coverage is 76.88%. Comparing base (751e55d) to head (7a927d6).

Files with missing lines Patch % Lines
insights/client/core_collector.py 51.94% 37 Missing ⚠️
insights/client/config.py 0.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4009      +/-   ##
==========================================
+ Coverage   76.69%   76.88%   +0.19%     
==========================================
  Files         759      757       -2     
  Lines       41788    41342     -446     
  Branches     9603     9526      -77     
==========================================
- Hits        32049    31787     -262     
+ Misses       8639     8478     -161     
+ Partials     1100     1077      -23     
Flag Coverage Δ
unittests 76.86% <57.89%> (+0.19%) ⬆️

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.

- when collecting in py26 env, if the foreach_execute specs depends
  on DatasourceProvider, the provider will be in unicode type and causes
  the followed validate() fail
- this change converted the unicode to string by force for all 'cmd' passed to
  CommandOutputProvider

Signed-off-by: Xiangce Liu <[email protected]>

rh-pre-commit.version: 2.3.1
rh-pre-commit.check-secrets: ENABLED
@xiangce xiangce removed the WIP Includes: Approved but Not Ready for Merging/Releasing label Sep 30, 2024
@xiangce xiangce merged commit 7ff8c5e into master Sep 30, 2024
11 checks passed
@xiangce xiangce deleted the remove_legacy_collection branch September 30, 2024 08:14
xiangce added a commit that referenced this pull request Oct 10, 2024
- and remove all dependent codes and relevant tests
- update the script that generates the filters.yaml
- see RHINENG-6982

Signed-off-by: Xiangce Liu <[email protected]>
(cherry picked from commit 7ff8c5e)
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.