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

RHSM: do not use the force D-Bus registration option #812

Merged
merged 1 commit into from
May 11, 2023

Conversation

ptoscano
Copy link
Contributor

@ptoscano ptoscano commented Apr 15, 2023

The 'force' D-Bus registration option used to simply be not implemented, effectively being a no-op; because of that, the system is manually unregistered before the actual registration.

Newer versions of subscription-manager (in 8.8 and 9.2) actually do implement the 'force' option; OTOH, since the system was unregistered before the registration attempt, having the option there does not make any difference. Hence, simply drop the 'force' option altogether.

Improve the comment regarding the unregistration to mention the status of the 'force' option, and the general reason for unconditionally unregistering at that point.

Checklist

  • PR has been tested manually in a VM (either author or reviewer)
  • Jira issue has been made public if possible
  • [RHELC-] is part of the PR title
  • GitHub label has been added to help with Release notes
  • PR title explains the change from the user's point of view
  • Code and tests are documented properly
  • The commits are squashed to as few commits as possible (without losing data)
  • When merged: Jira issue has been updated to Release Pending if relevant

The 'force' D-Bus registration option used to simply be not implemented,
effectively being a no-op; because of that, the system is manually
unregistered before the actual registration.

Newer versions of subscription-manager (in 8.8 and 9.2) actually do
implement the 'force' option; OTOH, since the system was unregistered
before the registration attempt, having the option there does not make
any difference. Hence, simply drop the 'force' option altogether.

Improve the comment regarding the unregistration to mention the status
of the 'force' option, and the general reason for unconditionally
unregistering at that point.

Signed-off-by: Pino Toscano <ptoscano@redhat.com>
@codecov
Copy link

codecov bot commented Apr 17, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (a09e678) 93.52% compared to head (f91f820) 93.52%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #812   +/-   ##
=======================================
  Coverage   93.52%   93.52%           
=======================================
  Files          24       24           
  Lines        3350     3350           
  Branches      589      589           
=======================================
  Hits         3133     3133           
  Misses        152      152           
  Partials       65       65           
Flag Coverage Δ
centos-linux-7 87.94% <100.00%> (ø)
centos-linux-8 88.95% <100.00%> (ø)

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

Impacted Files Coverage Δ
convert2rhel/subscription.py 92.64% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@pr-watson
Copy link
Contributor

/packit test

@pr-watson
Copy link
Contributor

Change looks good to me, just waiting on integration tests to pass

Copy link
Contributor

@pr-watson pr-watson left a comment

Choose a reason for hiding this comment

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

Change looks good to me!

@abadger
Copy link
Member

abadger commented Apr 20, 2023 via email

@bocekm
Copy link
Member

bocekm commented Apr 20, 2023

This change is the outcome of these two BZs about the force option not working through the dbus API:
RHEL 8: https://bugzilla.redhat.com/show_bug.cgi?id=2118486
RHEL 9: https://bugzilla.redhat.com/show_bug.cgi?id=2121350

There's no BZ for RHEL 7 and I believe that even if there was one it would be rejected. RHEL 7 is not being worked on anymore except critical bugs.

As the sub-man is not going to be fixed on RHEL 7, we need to keep un-registering the system ourselves until we stop allowing conversions of RHEL 7 in the tool altogether.

To improve the user experience at least on RHEL 8+, we can use the force option on RHEL 8+ to save one dbus API call, leading to a (perhaps) faster conversion and less info log messages. @abadger, WDYT about that?

@ptoscano
Copy link
Contributor Author

ptoscano commented Apr 20, 2023

To improve the user experience at least on RHEL 8+, we can use the force option on RHEL 8+ to save one dbus API call,

Considering that the current unregister_system() executes subscription-manager unregister, there is no wasted D-Bus call ;-)

leading to a (perhaps) faster conversion and less info log messages.

Nah, it does not really buy you anything. The slowest parts in the runtime of subscription-manager are the network interactions with the registration server (whose time you can count in seconds), you can try yourself how much time a subscription-manager register (to RHSM) takes to complete on an unregistered system (maybe it'll be slightly faster in a local Katello/Satellite). Because of this, an extra command invocation or D-Bus call on the system is almost "noise" in the time spent.

If you really wanted to use the force D-Bus option, you would need to do a "multiple version check" dance, which is not pretty and easy to maintain. I did it in the redhat_subscription Ansible role, and it's hard to get right. Not to mention it doubles your testing, as you have two code paths ("old sub-man" and "new sub-man") that you need to test.

TL;DR: keep the current way, it's easier for convert2rhel.

@bocekm
Copy link
Member

bocekm commented Apr 20, 2023

Considering that the current unregister_system() executes subscription-manager unregister, there is no wasted D-Bus call ;-)

Ah, you got me :)

LT;DR: keep the current way, it's easier for convert2rhel.

I'm ok with that.

@ptoscano
Copy link
Contributor Author

ptoscano commented May 5, 2023

friendly ping ;)

@bocekm bocekm merged commit bccc73d into oamg:main May 11, 2023
@ptoscano ptoscano deleted the subman-drop-force-opt branch May 12, 2023 06:42
@Venefilyn Venefilyn added the kind/bug-fix A bug has been fixed label May 15, 2023
@Venefilyn Venefilyn changed the title RHSM: do not use the 'force' D-Bus registration option RHSM: do not use the force D-Bus registration option May 22, 2023
Venefilyn pushed a commit that referenced this pull request Jun 19, 2023
The 'force' D-Bus registration option used to simply be not implemented,
effectively being a no-op; because of that, the system is manually
unregistered before the actual registration.

Newer versions of subscription-manager (in 8.8 and 9.2) actually do
implement the 'force' option; OTOH, since the system was unregistered
before the registration attempt, having the option there does not make
any difference. Hence, simply drop the 'force' option altogether.

Improve the comment regarding the unregistration to mention the status
of the 'force' option, and the general reason for unconditionally
unregistering at that point.

Signed-off-by: Pino Toscano <ptoscano@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug-fix A bug has been fixed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants