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

Roll back Satellite provisioning if subscription attempt fails with e… #3651

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
41 changes: 37 additions & 4 deletions pyanaconda/modules/subscription/runtime.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
SERVER_HOSTNAME_NOT_SATELLITE_PREFIX
from pyanaconda.modules.subscription.subscription_interface import \
RetrieveOrganizationsTaskInterface
from pyanaconda.modules.subscription.utils import flatten_rhsm_nested_dict
from pyanaconda.anaconda_loggers import get_module_logger

import gi
Expand Down Expand Up @@ -754,7 +755,7 @@ def name(self):
return "Restore RHSM configuration after Satellite provisioning"

def run(self):
"Restore the full RHSM configuration back to clean values."""
"""Restore the full RHSM configuration back to clean values."""
# the SetAll() RHSM DBus API requires a dict of variants
config_dict = {}
for key, value in self._rhsm_configuration.items():
Expand Down Expand Up @@ -801,6 +802,7 @@ def __init__(self, rhsm_observer, subscription_request, system_purpose_data,
self._rhsm_observer = rhsm_observer
self._subscription_request = subscription_request
self._system_purpose_data = system_purpose_data
self._rhsm_configuration = {}
# callback for nested tasks
self._registered_callback = registered_callback
self._registered_to_satellite_callback = registered_to_satellite_callback
Expand Down Expand Up @@ -883,7 +885,16 @@ def _provision_system_for_satellite(self):
backup_task = BackupRHSMConfBeforeSatelliteProvisioningTask(
rhsm_config_proxy=rhsm_config_proxy
)
self._config_backup_callback(backup_task.run())
# Run the task and flatten the returned configuration
# (so that it can be fed to SetAll()) now, so we don't have to do that later.
flat_rhsm_configuration = {}
nested_rhsm_configuration = backup_task.run()
if nested_rhsm_configuration:
flat_rhsm_configuration = flatten_rhsm_nested_dict(nested_rhsm_configuration)
self._config_backup_callback(flat_rhsm_configuration)
# also store a copy in this task, in case we encounter an error and need to roll-back
# when this task is still running
self._rhsm_configuration = flat_rhsm_configuration
Comment on lines +888 to +897
Copy link
Member

Choose a reason for hiding this comment

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

I don't like how this method is bigger and bigger. I think it would like a bit more love and abstraction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some of this is blocked by the payload not yet being modular, but some improvements are planed already for the upstream version, especially related to the unsightly bucket of callbacks we need to pass there at the moment.


# now run the Satellite provisioning script we just downloaded, so that the installation
# environment can talk to the Satellite instance the user has specified via custom
Expand All @@ -909,9 +920,19 @@ def _provision_system_for_satellite(self):
# which is an unrecoverable error, so we end there.
raise e

def _roll_back_satellite_provisioning(self):
"""Something failed after we did Satellite provisioning - roll it back."""
log.debug("registration attempt: rolling back Satellite provisioning")
rollback_task = RollBackSatelliteProvisioningTask(
rhsm_config_proxy=self._rhsm_observer.get_proxy(RHSM_CONFIG),
rhsm_configuration=self._rhsm_configuration
)
rollback_task.run()
log.debug("registration attempt: Satellite provisioning rolled back")

def run(self):
"""Try to register and subscribe the installation environment."""

provisioned_for_satellite = False
# check authentication method has been set and credentials seem to be
# sufficient (though not necessarily valid)
register_task = None
Expand Down Expand Up @@ -950,6 +971,7 @@ def run(self):
# environment for Satellite
log.debug("registration attempt: provisioning system for Satellite")
self._provision_system_for_satellite()
provisioned_for_satellite = True
# if we got there without an exception being raised, it was a success!
log.debug("registration attempt: system provisioned for Satellite")

Expand All @@ -959,12 +981,16 @@ def run(self):
except (RegistrationError, MultipleOrganizationsError) as e:
log.debug("registration attempt: registration attempt failed: %s", e)
log.debug("registration attempt: skipping auto attach due to registration error")
if provisioned_for_satellite:
self._roll_back_satellite_provisioning()
raise e
log.debug("registration attempt: registration succeeded")
else:
log.debug(
"registration attempt: credentials insufficient, skipping registration attempt"
)
if provisioned_for_satellite:
self._roll_back_satellite_provisioning()
raise RegistrationError(_("Registration failed due to insufficient credentials."))

# try to attach subscription
Expand All @@ -975,7 +1001,14 @@ def run(self):
rhsm_attach_proxy=rhsm_attach_proxy,
sla=sla
)
subscription_task.run()
try:
subscription_task.run()
except SubscriptionError as e:
# also roll back Satellite provisioning if registration was successfull
# but auto-attach failed
if provisioned_for_satellite:
self._roll_back_satellite_provisioning()
raise e
# if we got this far without an exception then subscriptions have been attached
self._subscription_attached_callback(True)

Expand Down
36 changes: 6 additions & 30 deletions pyanaconda/modules/subscription/subscription.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
RegisterAndSubscribeTask, UnregisterTask, SystemPurposeConfigurationTask, \
RetrieveOrganizationsTask
from pyanaconda.modules.subscription.rhsm_observer import RHSMObserver
from pyanaconda.modules.subscription.utils import flatten_rhsm_nested_dict


from pykickstart.errors import KickstartParseWarning
Expand Down Expand Up @@ -102,7 +103,7 @@ def __init__(self):
self._satellite_provisioning_script = None
self.registered_to_satellite_changed = Signal()
self._registered_to_satellite = False
self._rhsm_conf_before_satellite_provisioning = None
self._rhsm_conf_before_satellite_provisioning = {}

# registration status
self.registered_changed = Signal()
Expand Down Expand Up @@ -587,26 +588,6 @@ def rhsm_observer(self):
"""
return self._rhsm_observer

def _flatten_rhsm_nested_dict(self, nested_dict):
"""Convert the GetAll() returned nested dict into a flat one.

RHSM returns a nested dict with categories on top
and category keys & values inside. This is not convenient
for setting keys based on original values, so
let's normalize the dict to the flat key based
structure similar to what's used by SetAll().

:param dict nested_dict: the nested dict returned by GetAll()
:return: flat key/value dictionary, similar to format used by SetAll()
:rtype: dict
"""
flat_dict = {}
for category_key, category_dict in nested_dict.items():
for key, value in category_dict.items():
flat_key = "{}.{}".format(category_key, key)
flat_dict[flat_key] = value
return flat_dict

def get_rhsm_config_defaults(self):
"""Return RHSM config default values.

Expand Down Expand Up @@ -636,7 +617,7 @@ def get_rhsm_config_defaults(self):
# turn the variant into a dict with get_native()
nested_dict = get_native(proxy.GetAll(""))
# flatten the nested dict
flat_dict = self._flatten_rhsm_nested_dict(nested_dict)
flat_dict = flatten_rhsm_nested_dict(nested_dict)
self._rhsm_config_defaults = flat_dict
return self._rhsm_config_defaults

Expand All @@ -659,16 +640,11 @@ def unregister_with_task(self):

:return: a DBus path of an installation task
"""
# if we have a configuration backup, flatten the clean RHSM config so that the task
# can feed it to SetAll()
flat_rhsm_configuration = {}
if self._rhsm_conf_before_satellite_provisioning:
flat_rhsm_configuration = self._flatten_rhsm_nested_dict(
self._rhsm_conf_before_satellite_provisioning
)
# the configuration backup is already flattened by the task that fetched it,
# we can directly feed it to SetAll()
task = UnregisterTask(rhsm_observer=self.rhsm_observer,
registered_to_satellite=self.registered_to_satellite,
rhsm_configuration=flat_rhsm_configuration)
rhsm_configuration=self._rhsm_conf_before_satellite_provisioning)
# apply state changes on success
task.succeeded_signal.connect(self._system_unregistered_callback)
return task
Expand Down
39 changes: 39 additions & 0 deletions pyanaconda/modules/subscription/utils.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
#
# Utility functions for network module
#
# Copyright (C) 2021 Red Hat, Inc.
#
# This copyrighted material is made available to anyone wishing to use,
# modify, copy, or redistribute it subject to the terms and conditions of
# the GNU General Public License v.2, or (at your option) any later version.
# This program is distributed in the hope that it will be useful, but WITHOUT
# ANY WARRANTY expressed or implied, including the implied warranties of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General
# Public License for more details. You should have received a copy of the
# GNU General Public License along with this program; if not, write to the
# Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
# 02110-1301, USA. Any Red Hat trademarks that are incorporated in the
# source code or documentation are not subject to the GNU General Public
# License and may only be used or replicated with the express permission of
# Red Hat, Inc.
#

def flatten_rhsm_nested_dict(nested_dict):
Copy link
Member

Choose a reason for hiding this comment

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

I'm still thinking if this shouldn't be an object of RHSM configuration with flatten method.

Copy link
Contributor Author

@M4rtinK M4rtinK Oct 18, 2021

Choose a reason for hiding this comment

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

Ideally we can coordinate with RHSM team to have the Set* and Get* methods use the same dictionary format, as suggested by @poncovka , then we don't need any conversion at all.

"""Convert the GetAll() returned nested dict into a flat one.

RHSM returns a nested dict with categories on top
and category keys & values inside. This is not convenient
for setting keys based on original values, so
let's normalize the dict to the flat key based
structure similar to what's used by SetAll().

:param dict nested_dict: the nested dict returned by GetAll()
:return: flat key/value dictionary, similar to format used by SetAll()
:rtype: dict
"""
flat_dict = {}
for category_key, category_dict in nested_dict.items():
for key, value in category_dict.items():
flat_key = "{}.{}".format(category_key, key)
flat_dict[flat_key] = value
return flat_dict