[RHELC-1033, RHELC-1036, RHELC-1006] Stop uninstalling subscription-mananager and unregistering the system by default #869
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
abadger
force-pushed
the
subscription-not-needed
branch
from
09b0e03
to
95066a9
Compare
The registration is retained. Just the conf needs to be restored manually. At least in my case because the conf contained |
abadger
force-pushed
the
subscription-not-needed
branch
from
95066a9
to
cd304d4
Compare
r0x0d
reviewed
Aug 2, 2023
r0x0d
reviewed
Aug 2, 2023
r0x0d
reviewed
Aug 2, 2023
convert2rhel/toolopts.py
Outdated
Comment on lines
408
to
440
| elif not tool_opts.should_subscribe: | ||
| loggerinst.warning( | ||
| "Ignoring the --serverurl option. It has no effect when no credentials to subscribe the system were given." | ||
| ) |
There was a problem hiding this comment.
If we deprecate the no_rhsm option, we will just remove the above statement and use this one?
I'm wondering if it is not easier to merge the two issues right now and remove the no_rhsm option.
There was a problem hiding this comment.
Yeah, I' working on adding norhsm into the mix now, then working on unittests afterthat.
There was a problem hiding this comment.
So actually, it is --keep-rhsm that is being deprecated, not --no-rhsm. So the above code is still present.
r0x0d
reviewed
Aug 2, 2023
convert2rhel/toolopts.py
Outdated
| @@ -78,6 +75,29 @@ def set_opts(self, supported_opts): | |||
| if value is not None and hasattr(self, key): | |||
| setattr(self, key, value) | |||
|
|
|||
| @property | |||
| def should_subscribe(self): | |||
There was a problem hiding this comment.
This feels to me that it shouldn't belong here... This feels that it should be placed in the subscription.py module, but, we are using this same property in another part of the toolopts as well, so I'm not sure how to feel about it.
I see how useful the property is here, but still, it feels to be that it should belong to subscription.py, as we are not really taking the CLI option and parsing it to assign with the global toolopts object, but rather, we are saying "take those already defined CLI options and made a decision with it".
Any thoughts? I'm cool to leave the property here, but still, I think I should bring this comment just to make sure there is nothing we can do to move it to a "more appropriate module".
There was a problem hiding this comment.
I have the same uncertainty as you here. I think asking the question "Should I subscribe" does feel like it should be answered by subscription.py. OTOH, I think that toolopts.py should definitely not import subscription. So I think that leaves us with these choices:
- Just have the code here and public.
- Put code which does this check in both toolopts.py (private, just for validating the other arguments) and subscription.py (for public consumption by other code).
- Put the code into a "utils" module. Note that this would be a separate file from the current utils so that utils can keep the stdlib as its only dependency. (Maybe making utils into a directory and making a subscription module inside of that.) I started down this path so I know it's workable; I backed it out because it is more intrusive
The first of these is what is currently implemented. The last one is my preference if we think this belongs somewhere else.
What do you think?
There was a problem hiding this comment.
The last one is also my preference given the options. I agree that the solution 2 is not anywhere near to be the best one... I wouldn't mind leaving the code as-is, but if you can implement the last option, then I think it would look better for maintainability.
There was a problem hiding this comment.
I have moved this although I went with a variation on solution 2 that doesn't duplicate the body of the code. I implemented should_subscribe as a private function in toolopts.py so that it can be used there. Then I created a public identifier for should_subscribe in subscription.py which just references the private function with appropriate arguments.
The reason I decided not to do utils isn't technical but maintainability. I couldn't figure out a good way to show people that utils.py only requires the stdlib but subscription_utils.py would require convert2rhel.toolopts. If I named it toolopts_utils.py to signify that it required toolopts, then we would have the same feeling that it belonged in something related to subscriptions, not in something related to toolopts.
|
Not related, but if you can take care of that, would be nice: https://github.com/abadger/convert2rhel/blob/pr/abadger/869/convert2rhel/subscription.py#L23 not being used |
abadger
force-pushed
the
subscription-not-needed
branch
from
27d4d08
to
46f8e1b
Compare
r0x0d
reviewed
Aug 8, 2023
convert2rhel/subscription.py
Outdated
| RHSM product certificate. This is the reason that we need to call this function. | ||
| """ | ||
| cmd = ["subscription-manager", "refresh"] | ||
| output, ret_code = utils.run_subprocess(cmd, print_output=True) |
There was a problem hiding this comment.
Do we really want to show the output after executing the command or is it only by debug purposes?
There was a problem hiding this comment.
You are right. This is fixed.
r0x0d
reviewed
Aug 8, 2023
bocekm
changed the title
abadger
force-pushed
the
subscription-not-needed
branch
from
8d8f203
to
0dd9097
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #869 +/- ##
==========================================
- Coverage 94.34% 94.10% -0.25%
==========================================
Files 41 41
Lines 4053 4142 +89
Branches 715 737 +22
==========================================
+ Hits 3824 3898 +74
- Misses 159 173 +14
- Partials 70 71 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
abadger
force-pushed
the
subscription-not-needed
branch
from
ede186e
to
99e22cc
Compare
abadger
force-pushed
the
subscription-not-needed
branch
5 times, most recently
from
b918127
to
6f70c86
Compare
abadger
added
need-integration-tests
tests/all
hosekadam
reviewed
Sep 6, 2023
| """ | ||
| # Check whether any package owns the certificate file. It is | ||
| # possible that a new rpm package was installed after we copied the | ||
| # certificate into place which owns the certificate. If that is |
There was a problem hiding this comment.
Suggested change
| # certificate into place which owns the certificate. If that is | |
| # certificate into place which owns the certificate. If that is |
Nope, this is new (I merged all of my unittest fixes already. A couple notes on this one:
|
hosekadam
reviewed
Sep 6, 2023
| loggerinst.critical("The subscription-manager package is not installed correctly.") | ||
| if not pkghandler.get_installed_pkg_information("subscription-manager"): | ||
| loggerinst.critical( | ||
| "The subscription-manager package is not installed correctly. You could try manually installing it before running convert2rhel" |
There was a problem hiding this comment.
Suggested change
| "The subscription-manager package is not installed correctly. You could try manually installing it before running convert2rhel" | |
| "The subscription-manager package is not installed correctly. You could try manually installing it before running convert2rhel" |
hosekadam
reviewed
Sep 6, 2023
|
|
||
|
|
||
| # subscription is the natural place to look for should_subscribe but it | ||
| # is needed by toolopts. So define it as a private function in toolopts but |
There was a problem hiding this comment.
Suggested change
| # is needed by toolopts. So define it as a private function in toolopts but | |
| # is needed by toolopts. So define it as a private function in toolopts but |
|
/packit test |
Signed-off-by: Daniel Diblik <ddiblik@redhat.com>
danmyway
force-pushed
the
subscription-not-needed
branch
from
74ca715
to
8cf3b3a
Compare
|
/packit test |
danmyway
reviewed
Sep 6, 2023
tests/integration/tier0/non-destructive/subscription-manager/main.fmf
Outdated
Show resolved
Hide resolved
danmyway
reviewed
Sep 6, 2023
tests/integration/tier0/non-destructive/subscription-manager/test_sub_man_pre_register.py
Outdated
Show resolved
Hide resolved
kokesak
reviewed
Sep 7, 2023
tests/integration/tier0/non-destructive/subscription-manager/test_sub_man_pre_register.py
Outdated
Show resolved
Hide resolved
tests/integration/tier0/non-destructive/subscription-manager/test_sub_man_pre_register.py
Outdated
Show resolved
Hide resolved
|
|
||
| # Validate it matches with UUID prior to the conversion | ||
| assert original_registration_uuid == post_c2r_registration_uuid | ||
| del os.environ["C2R_TESTS_CHECK_RHSM_UUID_MATCH"] |
There was a problem hiding this comment.
I do not like the fact that we are deleting the environment variable in a function (fixture) that was set outside this function. Due to time limitations, I would keep it as it is, but in the future, I would prefer that we parametrize our fixtures so we do not rely on environment variables. This way we would get rid of all these and just stick with regular parameters of the function. This could be done using the indirect parametrization (https://docs.pytest.org/en/7.1.x/example/parametrize.html#indirect-parametrization)
There was a problem hiding this comment.
I agree, with that.
Even asserting for the uuids in the fixture is sketchy considering when the assertion error is raised, the test itself passes, but there is an Error in the fixture teardown.
If you don't mind, please create an issue to cover this @kokesak , thanks.
There was a problem hiding this comment.
Created ticket: https://issues.redhat.com/browse/RHELC-1134
There was a problem hiding this comment.
I've created the issue @kokesak https://issues.redhat.com/browse/RHELC-1135
danmyway
force-pushed
the
subscription-not-needed
branch
from
5e7e9fb
to
ef5dad6
Compare
|
/packit test |
* address review comment * tweak incomplete_rollback_in_analyze test to not be any action dependent, just validate the envar related message Signed-off-by: Daniel Diblik <ddiblik@redhat.com>
danmyway
force-pushed
the
subscription-not-needed
branch
from
ef5dad6
to
633f22b
Compare
|
/packit test --labels non-destructive |
|
all destructive tests are have passed previously |
Venefilyn
changed the title
abadger
changed the title
Andrew-ang9
pushed a commit
to Andrew-ang9/convert2rhel
that referenced
this pull request
Oct 10, 2023
…amg#869) * Make subscription from within convert2rhel optional. Previously, we would unregister a system if it was registered and then re-register with subscription-manager. The only time we didn't do this would be when the user specified --no-rhsm but that does too much. It causes convert2rhel to avoid the use of subscription-manager altogether (it doesn't install the package or configure the RHEL repos) For our purposes, we just don't want to replace the registration if the system already has one. The strategy we're implementing here is: * If the user specifies (username and password) or (activation_key and organization) then (re)register the system. * If not, then attempt to do the conversion without registering the system first. There are several distinct changes made to enable this functionality: * subscription.{subscribe_system,rollback}() were moved ito a RestorableChange (called RestorableSystemSubscription). This is mainly because deciding whether to unregister the system in case of a rollback now needs to have some saved state to know whether we registered it in the first place. * replace_subscription_manager() no longer unregisters the system as part of uninstalling the current subscription-manager packages and installing the RHEL ones. * A new property was added to toolopts, should_subscribe. This property tells whether the user has specified options which ask us to (re)register the system or not. * In pre_ponr_changes/subscription.py we now use should_subscribe to decide whether to (re)subscribe the system instead of only no_rhsm. We also use RestorableSystemSubscribe instead of calling subscribe_system(). * In main.py we no longer have to call subscription.rollback() because the backup framework (backup.backup_control.pop_all()) will take care of rolling back the subscription. * Reviewed places where tool_opts.no_rhsm was used and change places that were used only to subscribe the system (as opposed to installing subscription-manager, which is also used for other things) to subscription.should_subscribe() instead: * actions.pre_ponr_changes.subscription.PreSubscription: This action makes sure that /usr/bin/subscription-manager can run so it needs to run even if we don't subscribe the system. * actions.system_checks.dbus.DbusIsRunning: We currently only use DBus as part of subscribing the system so should_subscribe is appropriate here. * toolopts.CLI: use should_subscribe to tell the user we're ignoring the presence of --serverurl. * Deprecate --keep-rhsm and use a heuristic to decide whether to register with subscription-manager. The heuristic is to check whether the user gave us the necessary credentials to register the system with subscription-manager. If so, then we use rhsm to do so. If not, then we do not attempt to register the system. Fixes: https://issues.redhat.com/browse/RHELC-1036 * cert.py: * make cert.SystemCert() work with the backup system. Now it will only install the certificate if it isn't already present on the system and only remove the certificate if it was convert2rhel which installed it. * Give SystemCert the ability to install to different directories. This allows us to use it for both the susbcription-manager product certificate and the yum repo certificate (needed for the convert2rhel yum repo). * main.py: Use the backup system to uninstall the certificates if we rollback instead of making a manual call to remove it. * subscription.py: * The RestorableChange for registering and unregistering the system no longer needs to check keep_rhsm. We will always unsubscribe the system if we subsribed it but by default we are no longer subscribing it in the first place. * Remove replace_subscription_manager() and remove_original_subscription_manager() as we don't need anything that uninstalls the subscription-manager package from the vendor anymore. * Add detect_subscription_manager() to check whether subscription-manager is installed (the entrypoint packages that we need for conversion, basically '*subscription-manager*'.) * Move a check for whether subscription-manager packages for RHEL have been downloaded successfully from replace_subscription_manager() to install_rhel_subscription_manager(). This entails calling logger.critical() if the rpms are not present. In the past, we would fail in the same way, just at an earlier stage (in replace[..]() instead of install[..](). * Move the installation of the redhat-uep.pem for the yum repos into a pre_ponr_change instead of here. It's graduating from a hack to something that we need to ensure is installed. (Remove _check_and_install_redhat_uep_pem()). * verify_rhsm_installed(): Remove the keep_rhsm check. Now, we will always fail at this point if verify_rhsm_installed() was called and subscription-manager is not installed. (The equivalent check is now done by the caller) * download_rhsm_pkgs(): Do not check for keep_rhsm. Now, when this is called, we will always download the packages. (The equivalent check is now done by the caller) * toolopts.py: Remove the keep_rhsm toolopts attribute. When parsing the CLI, warn if --keep-rhsm is passed and ignore the option. * actions/pre_ponr_changes/subscription.py: * Create and action for installing yum repo certs with code migrated from subscription._check_and_install_redhat_uep_pem() * Move installation of the gpg key from the PreSubscription Action and make both Presubscription and pre_ponr_changes.transaction.ValidatePackageManagerTransaction depend on it. (TODO from r0x0d) * Now only download and install subscription-manager if they are not already installed. * Remove an unused namedtuple import * Remove unused toolopts.credentials_thru_cli When the RegistrationCommand was refactored into a class, we stopped needing credentials_thru_cli. Remove that now. * Make sure subscription-manager knows about the product certificate. Add a function to call subscription-manager refresh. Use it to make sure subscription-manager knows about any RHSM product certificate the user may have put on the system since the last time subscription-manager refreshed its cache. * Add subscription-manager package installation to the backup controller. This is needed so that the packages can be uninstalled after we have unregistered the system during rollback. Without this change thte tiing was wrong because: * some packages (those which were removed) need to be rolled back early (before file rollbacks happen) * The packages here (installed subscription-manager packages) need to be rolled back later, after unregistering the system. * Unregistering the system is now handled by the backup framework and the backup framework rollback is a single step. Since the backup framework rollback can't exist both before and after the other rollback steps, adding the subscription-manager package rollbacks to the framework is the only way to fix this. * Use get_installed_pkg_information() instead of get_installed_pkg_objects() for subscription-manager installation. On RHEL8, those two functions deal with architecture specification differently. Using get_installed_pkg_information everywhere gives the right answer on both RHEL7 and RHEL8. * Make subscription-manager installation work with old versions of deps. We are trying to get convert2rhel to only install the subset of subscription-manager packages which it requires. However, when we do have to install packages, we are getting them from the UBI repositories where the version of subscription-manager may need a newer vrsion of dependencies than the vendor has. For this reason, we may need to install some dependencies from the UBI repositories even though the vendor versions of them are already installed. Currently, python-syspurpose and json-c are the only problematic packages so make sure that they are added to the install set. .. seealso:: Bug report illustrating the version problem: oamg#494 * syspurpose and jsonc should not be uninstalled on rollback if they were installed to begin with. Fix this by sending both packages to install and packages to update to RestorablePackageSet. When we call restore(), packages which were updated are not uninstalled. (In the future, we could backup and restore those packages). The PR fixing this in the past was: oamg#393 Co-authored-by: Rodolfo Olivieri <rodolfo.olivieri3@gmail.com> Co-authored-by: Michal Bocek <mbocek@redhat.com> Co-authored-by: E Gustavsson <eric@spytec.se> Co-authored-by: Martin 'kokesak' Litwora <mlitwora@redhat.com> * Fix for poor error message when a cert has already been removed in rollback. If something (for instance, package removal) removes a certificate that we installed, then we will try to reove the cert during rollback but the file won't exist. Add some code to give a nice message in this case. Also fix the integration test test_rhsm_error_logged(). It was checking for a prior bad error message (an exception). That won't happen in the same circumstance with the new code so adapt it to fit the new reality. * Hack to fix the order of rollbacks while we still have items that aren't ported to the backup framework. * Fix package backup on a pre-registered system When the system is registered to RHSM before running convert2rhel, the redhat.repo file is not generated because of the missing RHSM product cert on non-RHEL system. However convert2rhel installs this cert and when we do a package backup, the subscription-manager yum plugin generates the redhat.repo file. Having the RHEL repos defined and enabled during the package backup causes a failure when accessing the RHEL repos due to the $releasever being different on RHEL and RHEL-clones. Besides that, when backing up packages we want to be accessing only the RHEL-clone repos, not RHEL repos. Also, fix the check whether the packages to be removed have been really removed. We were comparing strings to objects which led to a false flag warning that packages that were in fact removed haven't been removed. Also, update dependencies of a few Actions to make sure: - we backup system repos before pkg backup because we're using the backed up repos for the pkg backups - we remove pkgs that contain repofiles only after installing sub-man pkgs because the sub-man pkg installation may require dependencies from the original vendor repos --------- Signed-off-by: Daniel Diblik <ddiblik@redhat.com> Co-authored-by: Rodolfo Olivieri <rodolfo.olivieri3@gmail.com> Co-authored-by: Michal Bocek <mbocek@redhat.com> Co-authored-by: E Gustavsson <eric@spytec.se> Co-authored-by: Martin 'kokesak' Litwora <mlitwora@redhat.com> Co-authored-by: Daniel Diblik <ddiblik@redhat.com>
10 tasks
Venefilyn
pushed a commit
that referenced
this pull request
Mar 7, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Make subscription from within convert2rhel optional.
Previously, we would unregister a system if it was registered and then re-register with
subscription-manager. The only time we didn't do this would be when the user specified --no-rhsm
but that does too much. It causes convert2rhel to avoid the use of subscription-manager altogether
(it doesn't install the package or configure the RHEL repos) For our purposes, we just don't want to
replace the registration if the system already has one.
The strategy we're implementing here is:
There are several distinct changes made to enable this functionality:
RestorableSystemSubscription). This is mainly because deciding whether to unregister the system
in case of a rollback now needs to have some saved state to know whether we registered it in the
first place.
current subscription-manager packages and installing the RHEL ones.
specified options which ask us to (re)register the system or not.
the system instead of only no_rhsm. We also use RestorableSystemSubscribe instead of calling
subscribe_system().
(backup.backup_control.pop_all()) will take care of rolling back the subscription.
Things that still need to be done:
toolopts.should_subscribe instead.
whether removing unregistration from sub-man is okay
RHEL versions installed will keep the pre-uninstall registration and
/etc/rhsm/rhsm.conf.rpmsavewhen the subscription-manager package is removed. This.rpmsavefile would need to be restored when the new package is installed./etc/rhsm/rhsm.conf, we can try to implement: https://issues.redhat.com/browse/RHELC-1036 as doing that will mean we don't have to uninstall the package at all.early before while backup_control.pop_all() is run as the last item. We may need to
pop_all() to be one of the first things run on rollback or even add more of the ad hoc
rollback functions into the backup framework so that they can be done in a specific order.
Jira Issues: RHELC-1033, RHELC-1036, [RHELC-1006] (https://issues.redhat.com/browse/RHELC-1006)
Checklist
[RHELC-]is part of the PR titleRelease Pendingif relevant