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

M4rtinK · 2021-10-13T16:39:13Z

…rror

Previously Satellite provisioning was only rolled back when the
"Unregister" button has been pressed.

Turns out this is not sufficient, as provisioning happens quite early,
yet the registration and subscription attempt can still end with an
error. The system will then be provisioned for Satellite, but not
registered to it, so no "Unregister" button would be available to
roll back the provisioning.

So instead, make sure Satellite provisioning is always rolled back
if registration to satellite fails in one of tis phases.

And while we are at it refactor the RHSM config dict flattening method
to a function in an utility module and cover all the new and changed
code with unit tests.

Resolves: rhbz#2010865

pep8speaks · 2021-10-13T16:39:18Z

Hello @M4rtinK! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-10-20 15:29:53 UTC

M4rtinK · 2021-10-13T16:43:20Z

/kickstart-test --testtype smoke

poncovka · 2021-10-13T17:45:59Z

tests/nosetests/pyanaconda_tests/modules/subscription/utils_test.py

+
+    def nested_dict_test(self):
+        """Test the flattening function can handle a nested dict being passed."""
+        self.assertEqual(flatten_rhsm_nested_dict({"foo": {"bar": "baz"}}), {"foo.bar": "baz"})


I find it weird that we have to do this step. Could the RHSM service eventually provide a new API?

Yeah, its a bit strange that the API is asymmetric like this - that GetAll() returns a nested dict and SetAll() only accepts the flat one. I'll try to ask them about that on one of our future meetings - we might be the only users of the SetAll() API, so I guess they could just change it if they agree with the suggestion.

VladimirSlavik

Looks good to me. Thank you!

VladimirSlavik · 2021-10-14T11:33:20Z

tests/nosetests/pyanaconda_tests/modules/subscription/subscription_tasks_test.py

+        # make the register task throw an exception
+        register_task.return_value.run_with_signals.side_effect = RegistrationError()
+        # run the main task, epxect registration error
+        with self.assertRaises(RegistrationError):


It might be better to use pytest.raises, the self.assert* stuff will eventually go away.

Leave it as it is. I have to take this into account after I'll create switch to pytest on rhel-9 branch.

Leave it as it is. I have to take this into account after I'll create switch to pytest on rhel-9 branch.

ACK

VladimirSlavik · 2021-10-14T11:34:44Z

tests/nosetests/pyanaconda_tests/modules/subscription/subscription_tasks_test.py

+            rhsm_register_server_proxy=rhsm_register_server,
+            organization='foo_org',
+            activation_keys=['key1', 'key2', 'key3']
+


All register_task calls in this file have an empty line after the parameters. Not sure if that is something to change though.

Indeed! Must have been some sort of copy-paste error, lets fix it. :)

jkonecny12

Please also fix https://github.com/rhinstaller/anaconda/blob/rhel-9/pyanaconda/modules/subscription/runtime.py#L757 docstring in this commit. It's not really related but it makes a bad issues in syntax highlighting and setting a new bug on this would be too cumbersome.

jkonecny12

None of the below are blockers. Just a notes.

jkonecny12 · 2021-10-15T15:38:27Z

pyanaconda/modules/subscription/utils.py

+# Red Hat, Inc.
+#
+
+def flatten_rhsm_nested_dict(nested_dict):


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

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.

jkonecny12 · 2021-10-15T15:39:06Z

tests/nosetests/pyanaconda_tests/modules/subscription/subscription_tasks_test.py

@@ -1325,7 +1325,9 @@ def provision_system_for_satellite_download_error_test(self, download_task):
    @patch("pyanaconda.core.util.restart_service")
    @patch("pyanaconda.modules.subscription.runtime.RunSatelliteProvisioningScriptTask")
    @patch("pyanaconda.modules.subscription.runtime.DownloadSatelliteProvisioningScriptTask")
-    def provision_sat_run_error_test(self, download_task, run_script_task, restart_service):
+    @patch("pyanaconda.modules.subscription.runtime.BackupRHSMConfBeforeSatelliteProvisioningTask")
+    def provision_sat_run_error_test(self, backup_task, download_task, run_script_task,


I don't much like the sat abbreviation.

I'm afraid sometimes its hard to not hit the line length limits without such shortenings. Also IIRC I've see similar abbreviation being used by the RHSM team, so at least we are consistent. :)

I would really like to have satellite instead of sat here for a few reasons:

easy to find in results by grep/search

you are using satellite word below in the test names so this is not really consistent with the rest of the change

the few characters you will save here won't really help you because even now the parameters are split to two lines

Fair enough, changed. :)

jkonecny12 · 2021-10-15T15:41:46Z

pyanaconda/modules/subscription/runtime.py

+        # 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


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

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.

M4rtinK · 2021-10-18T13:55:21Z

Please also fix https://github.com/rhinstaller/anaconda/blob/rhel-9/pyanaconda/modules/subscription/runtime.py#L757 docstring in this commit. It's not really related but it makes a bad issues in syntax highlighting and setting a new bug on this would be too cumbersome.

Fixed. :)

M4rtinK · 2021-10-18T14:05:03Z

/kickstart-test --testtype smoke

M4rtinK · 2021-10-20T11:43:12Z

/kickstart-test --testtype smoke

…rror Previously Satellite provisioning was only rolled back when the "Unregister" button has been pressed. Turns out this is not sufficient, as provisioning happens quite early, yet the registration and subscription attempt can still end with an error. The system will then be provisioned for Satellite, but not registered to it, so no "Unregister" button would be available to roll back the provisioning. So instead, make sure Satellite provisioning is always rolled back if registration to satellite fails in one of tis phases. And while we are at it refactor the RHSM config dict flattening method to a function in an utility module and cover all the new and changed code with unit tests. Resolves: rhbz#2010865

M4rtinK · 2021-10-20T15:30:04Z

/kickstart-test --testtype smoke

jkonecny12

Looks good to me.

M4rtinK · 2022-09-08T14:09:08Z

This can be ported to Fedora only once Satellite support has been ported.

M4rtinK added port to Fedora rhel-9 labels Oct 13, 2021

M4rtinK force-pushed the rhel-9-rollback_satellite_provisioning_on_failure branch from dd42b0c to 8958202 Compare October 13, 2021 16:40

poncovka reviewed Oct 13, 2021

View reviewed changes

VladimirSlavik approved these changes Oct 14, 2021

View reviewed changes

jkonecny12 reviewed Oct 15, 2021

View reviewed changes

M4rtinK force-pushed the rhel-9-rollback_satellite_provisioning_on_failure branch from 8958202 to d393d92 Compare October 18, 2021 13:55

M4rtinK force-pushed the rhel-9-rollback_satellite_provisioning_on_failure branch from d393d92 to 1f7ffbc Compare October 20, 2021 11:37

M4rtinK force-pushed the rhel-9-rollback_satellite_provisioning_on_failure branch from 1f7ffbc to ccafafc Compare October 20, 2021 15:29

jkonecny12 approved these changes Oct 21, 2021

View reviewed changes

M4rtinK added the ready to merge The PR can be merged. It should have all BZ flags required for releasing set (usually release+). label Oct 25, 2021

M4rtinK mentioned this pull request Oct 25, 2021

Backport pytest migration to rhel-9 #3664

Merged

rvykydal merged commit b9ee598 into rhinstaller:rhel-9 Nov 2, 2021

M4rtinK added the blocked Don't merge this pull request! label Sep 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

M4rtinK commented Oct 13, 2021

pep8speaks commented Oct 13, 2021 •

edited

M4rtinK commented Oct 13, 2021

poncovka Oct 13, 2021

M4rtinK Oct 14, 2021

VladimirSlavik left a comment

VladimirSlavik Oct 14, 2021

jkonecny12 Oct 15, 2021

M4rtinK Oct 18, 2021

VladimirSlavik Oct 14, 2021

M4rtinK Oct 18, 2021

jkonecny12 left a comment •

edited

jkonecny12 left a comment

jkonecny12 Oct 15, 2021

M4rtinK Oct 18, 2021 •

edited

jkonecny12 Oct 15, 2021

M4rtinK Oct 18, 2021

jkonecny12 Oct 18, 2021

M4rtinK Oct 20, 2021

jkonecny12 Oct 15, 2021

M4rtinK Oct 18, 2021

M4rtinK commented Oct 18, 2021

M4rtinK commented Oct 18, 2021

M4rtinK commented Oct 20, 2021

M4rtinK commented Oct 20, 2021

jkonecny12 left a comment

M4rtinK commented Sep 8, 2022

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

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

Conversation

M4rtinK commented Oct 13, 2021

pep8speaks commented Oct 13, 2021 • edited

Comment last updated at 2021-10-20 15:29:53 UTC

M4rtinK commented Oct 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VladimirSlavik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkonecny12 left a comment • edited

Choose a reason for hiding this comment

jkonecny12 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

M4rtinK Oct 18, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

M4rtinK commented Oct 18, 2021

M4rtinK commented Oct 18, 2021

M4rtinK commented Oct 20, 2021

M4rtinK commented Oct 20, 2021

jkonecny12 left a comment

Choose a reason for hiding this comment

M4rtinK commented Sep 8, 2022

pep8speaks commented Oct 13, 2021 •

edited

jkonecny12 left a comment •

edited

M4rtinK Oct 18, 2021 •

edited