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

2132242: [1.28] Outsource uploading DNF profile to rhsmcertd #3242

Merged

Conversation

jirihnidek
Copy link
Contributor

@jirihnidek jirihnidek commented Apr 4, 2023

  • Backport to 1.28 branch.
  • BZ: https://bugzilla.redhat.com/show_bug.cgi?id=2132242
  • Card ID: ENT-5530
  • The subscription-manager tries to upload combined DNF profile at the end of registration process. Gathering of this profile can take very long time. It can be several minutes in some edge cases. This blocked registration process and the activity of gathering DNF profile was not communicated using live status message like other activities.
  • This commit uses novel approach. The process of gathering and uploading of DNF process is outsourced to rhsmcertd service. The subscription-manager sends SIGUSR1 signal to rhsmcertd service to trigger gathering and uploading DNF profile. When rhsmcertd is not running, then old blocking approach is used, but this activity is communicated to user using live status message.
  • The rhsmcertd can receive SIGUSR1 signal and it starts Python script package-profile-uploader, which is already part of installation.
  • The information about rhsmcertd PID is written to lock file of rhsmcertd /var/lock/subsys/rhsmcertd
  • When rhsmcertd is shutting down, then it tries to delete lock file
  • glib functions are used for handling signals and spawning new process (only uploading of profile ATM).
  • Fixes one issue with not supported macro in old glib
  • Updated some unit tests
  • rhsm-package-profile-uploader is always spawned with --force-upload CLI option
  • rhsmcertd reads log levels from rhsm.conf

@github-actions
Copy link

github-actions bot commented Apr 5, 2023

Coverage

Coverage (computed on Fedora latest) •
FileStmtsMissCoverMissing
rhsm
   profile.py2297169%28–29, 33–34, 43, 48, 68, 103–107, 109–113, 115–117, 119–126, 145–147, 151–153, 189, 202, 216–217, 227–228, 234, 244, 256, 266–267, 269–270, 276, 326, 339, 372–376, 388–390, 394–395, 403, 414–417, 424, 429, 445–448
subscription_manager
   managercli.py213471766%224, 228, 246, 256–269, 315, 319–320, 333, 355, 359, 361, 401, 439–441, 455–457, 482, 502, 528, 534–535, 537–538, 555, 557–558, 560–562, 564, 639–640, 652, 677, 690–691, 693, 716–717, 726–727, 729, 828, 831–833, 836–839, 847–848, 850–852, 855–857, 860–862, 870–871, 873–875, 881, 904–906, 908–912, 914, 920, 935–938, 940, 949–952, 954, 965, 967, 989–990, 997, 1033, 1035, 1037–1038, 1040, 1050–1051, 1059, 1082–1087, 1097, 1109–1110, 1113, 1189–1193, 1201–1202, 1209, 1212–1213, 1218–1220, 1222–1231, 1233–1236, 1238–1239, 1249–1250, 1252, 1255, 1274–1278, 1302, 1304, 1310, 1313–1315, 1318–1319, 1321–1328, 1330–1333, 1335–1342, 1344, 1346–1347, 1351, 1353–1354, 1356–1359, 1363, 1365, 1367–1368, 1371–1377, 1391, 1393–1394, 1396–1406, 1409, 1411–1416, 1443, 1446, 1452, 1455, 1459, 1466, 1468–1469, 1471, 1473, 1492, 1499–1503, 1505–1512, 1514, 1516–1521, 1524, 1540–1546, 1548–1550, 1592–1593, 1596–1597, 1600–1601, 1603, 1606, 1608–1609, 1611, 1649–1650, 1656–1657, 1659, 1673–1674, 1678–1684, 1687–1693, 1695–1703, 1705–1715, 1726–1728, 1730–1731, 1736, 1741–1747, 1749, 1751, 1754–1755, 1757, 1759–1766, 1768, 1771–1781, 1783, 1837, 1839, 1847, 1852, 1884–1886, 1888–1889, 1896, 1899–1905, 1911–1912, 1914–1915, 1918, 1929, 1933, 1940–1943, 1947–1948, 1952–1953, 1964, 1987, 1991–1993, 2002, 2006, 2027, 2045–2046, 2052, 2084, 2087, 2115, 2119, 2134–2136, 2145, 2171, 2181–2182, 2186–2187, 2189–2193, 2203, 2208, 2214–2215, 2228–2229, 2268–2269, 2275–2276, 2278, 2281–2282, 2284–2285, 2288–2293, 2297–2298, 2300–2302, 2304, 2325–2329, 2336, 2362–2364, 2372, 2380–2384, 2386–2387, 2390–2394, 2461, 2482–2484, 2496–2497, 2500–2501, 2506–2508, 2510–2511, 2513–2515, 2518–2521, 2523–2525, 2527, 2529–2530, 2532–2533, 2535–2546, 2548–2550, 2553, 2555, 2557–2559, 2562, 2564, 2568–2571, 2575–2576, 2580, 2585, 2587–2588, 2590–2591, 2593–2599, 2601, 2603–2604, 2609, 2611, 2613–2614, 2618, 2620, 2675, 2684, 2689, 2693, 2713–2714, 2722, 2738–2744, 2767–2768, 2803–2804, 2807–2808, 2811–2812, 2814–2816, 2818–2822, 2824–2834, 2849, 2852, 2854–2859, 2862–2864, 2866, 2868, 2870, 2873, 2875–2876, 2879–2880, 2884, 2886–2887, 2889, 2891–2893, 2895, 3015–3016, 3066, 3093–3094, 3096, 3169, 3177–3178, 3182, 3237–3238, 3243–3244, 3296, 3300, 3302, 3308, 3338, 3395, 3411–3412, 3420, 3427, 3450–3452, 3456–3457, 3461–3462, 3467, 3502, 3573, 3575, 3577–3579, 3583, 3585, 3587, 3589–3590, 3592–3595, 3597–3598, 3600–3606, 3609, 3611, 3614–3616, 3618–3622, 3625, 3674–3678, 3699–3702, 3733, 3735, 3766–3769, 3780–3781, 3814, 3836–3838, 3840–3845, 3849
   utils.py3377777%63, 66, 93, 145–148, 151–152, 154–156, 159–164, 177, 181, 195–196, 240–247, 251–253, 255–264, 266–267, 270, 282, 284–287, 318–319, 323–325, 329, 351, 357–361, 363–364, 366, 368–370, 372, 406, 414, 424, 621, 623, 653–654
TOTAL23007996756% 

Tests Skipped Failures Errors Time
2355 10 💤 0 ❌ 0 🔥 51.073s ⏱️

src/daemons/rhsmcertd.c Outdated Show resolved Hide resolved
@jirihnidek jirihnidek force-pushed the jhnidek/2132242_1.28_gather_dnf_profile branch from 5390b6d to 0b438fa Compare April 18, 2023 12:46
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 is mostly OK, I think there are couple of fixups needed:

  • regarding commit 2132242: [1.28] Outsource uploading DNF profile to rhsmcertd:
    • this contains also the backport of commit b70c0d5; can you please backport it as separate commit, so it's easier to review and to audit in the git history?
  • regarding commit rhsmcertd reads default_log_level from rhsm.conf:
    • this contains also the backport of commit 3a8e42a; can you please backport it as separate commit, so it's easier to review and to audit in the git history?
  • if I remember the testing that happened back then, I think you need to also backport commit 1bb1850 to avoid SELinux issues when rhsm-package-profile-uploader runs

@ptoscano
Copy link
Contributor

(and while you are there, please also rebase it, as there were other commits pushed to the 1.28 branch in the meanwhile)

@jirihnidek jirihnidek force-pushed the jhnidek/2132242_1.28_gather_dnf_profile branch from 92cba9d to e619502 Compare April 27, 2023 12:59
cnsnyder and others added 7 commits April 28, 2023 11:35
* Backport to 1.28 branch.
  * Original PR: #3139
  * Original commit: cc3f24c
* BZ: https://bugzilla.redhat.com/show_bug.cgi?id=2132242
* Card ID: ENT-5530
* The subscription-manager tries to upload combined DNF profile
  at the end of registration process. Gathering of this
  profile can take very long time. It can be several minutes
  in some edge cases. This blocked registration process and
  the activity of gathering DNF profile was not communicated
  using live status message like other activities.
* This commit uses novel approach. The process of gathering
  and uploading of DNF process is outsourced to rhsmcertd
  service. The subscription-manager sends SIGUSR1 signal to
  rhsmcertd service to trigger gathering and uploading
  DNF profile. When rhsmcertd is not running, then old
  blocking approach is used, but this activity is communicated
  to user using live status message.
* The rhsmcertd can receive SIGUSR1 signal and it
  starts Python script package-profile-uploader, which is
  already part of installation.
* The information about rhsmcertd PID is written to lock
  file of rhsmcertd /var/lock/subsys/rhsmcertd
* When rhsmcertd is shutting down, then it tries to delete
  lock file
* glib functions are used for handling signals and
  spawning new process (only uploading of profile ATM).
* Updated some unit tests
* TODO:
  * Use same approach for DNF plugin subscription-manager
* Macro is re-implemented in rhsmcertd, because it is really
  simple and it is only used for suppressing GCC warnings
* The macro is re-implemented for glib older than 2.58
* When log directory /var/log/rhsm does not exist, then
  rhsmcertd tries to create this directory before
  it tries to print anything to log file

(cherry picked from commit 3a8e42a)
* When default_log_level was set to DEBUG or any other value,
  then it was just not read by rhsmcertd. Thus it was not
  possible to get debug messages to rhsm.log from rhsmcertd,
  when rhsmcertd was started using:
  `systemctl start rhsmcertd.service` without hacking
  rhsmcertd.service file
* The rhsmcertd is able to read default_log_level from
  rhsm.conf now. It supports following log levels: ERROR,
  WARM, INFO and DEBUG. Default value is INFO.
* When logging.rhsmcertd is set, then value of this option
  overrides logging.default_log_level
* When rhsmcertd is started from command line with --debug
  option, then it overrides log level specified in
  rhsm.conf
* Added line and number to log message
* Added few more usages of debug () for testing purpose.
* Card ID: ENT-5532
* When configuration option report_package_config is set to 0,
  then package profile should not be usually reported.
  It only has to be reported during registration regardless
  the value of report_package_config option.
  For this reason the rhsm-package-profile-uploader is always
  spawned with --force-upload CLI option.
* Note: if you want to use this approach for other cases, then
  it is necessary to check if value of report_package_config
  is equal and then it is possible to send SIGUSR1 signal to
  rhsmcertd.
* Small changes:
  - Removed unused definition in rhsmcertd.c
  - Emphasize that update_check() is called with force=True
Merely importing "pkg_resources" will perform its initialization, which
is not exactly lightweight. "pkg_resources" is used to get the version
of subscription-manager as installed, in case there is no valid version
in the internal "version" module, which usually happens in
developer-only setups.

Since the build system takes care of filling "pkg_version" in the
"version" module, a distro build of subscription-manager does not need
to query "pkg_resources" for the version. Hence, import "pkg_resources"
only when we actually need to use it for getting the version.
@ptoscano ptoscano force-pushed the jhnidek/2132242_1.28_gather_dnf_profile branch from e619502 to 3068a8a Compare April 28, 2023 09:58
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.

LGTM -- the CI failure is npm crashing in Fedoraw Rawhide, which can be ignored for something targeted at RHEL 8.

@ptoscano ptoscano merged commit 423a602 into subscription-manager-1.28 Apr 28, 2023
11 of 12 checks passed
@ptoscano ptoscano deleted the jhnidek/2132242_1.28_gather_dnf_profile branch April 28, 2023 10:12
@jirihnidek
Copy link
Contributor Author

Thanks 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants