Refactor provider options by Cristi1324 · Pull Request #503 · cloudbase/coriolis

Cristi1324 · 2026-08-19T14:50:21Z

Register common destination options and apply them during OS morphing.

Add data_transfer_mechanism, set_dhcp, and retain_user_credentials under
common_target_options. The worker merges those rows onto each provider
list, injects the fields into the destination schema, and removes core
names before the provider call. A request that names only core options
returns only those rows. OS morphing stores set_dhcp as nics_set_dhcp
and stores retain_user_credentials on the morphing parameters.

Summary

The worker adds data_transfer_mechanism, set_dhcp, and retain_user_credentials to every provider destination option list.
The worker adds those fields to the destination environment schema.
The worker removes those names before the provider call.
A request that names only those options returns only those rows.
OS morphing stores set_dhcp as nics_set_dhcp.
OS morphing stores retain_user_credentials on the morphing parameters.
A destination environment value overrides the stored provider value.
The stored provider value overrides [common_target_options].

Test plan

Test a deployment with no destination option filter. Confirm the wizard lists data_transfer_mechanism, set_dhcp, and retain_user_credentials.
Test a deployment with a destination options request that names only set_dhcp. Confirm the response contains only that row.
Test a deployment with set_dhcp set to false. Confirm the guest keeps its static addresses.
Test a deployment with retain_user_credentials set to true. Confirm the destination platform keeps the existing guest credentials.
Test a deployment with set_dhcp omitted. Confirm OS morphing uses the [common_target_options] value for nics_set_dhcp.

Depends on: https://github.com/cloudbase/coriolis-provider-cloudstack/pull/6

Dany9966 · 2026-09-28T09:50:48Z


    nics_info = osmorphing_info.get('nics_info')
-    set_dhcp = osmorphing_info.get('nics_set_dhcp', True)
+    set_dhcp = osmorphing_parameters.get('set_dhcp', True)


This will be properly handled in: https://github.com/cloudbase/coriolis/pull/534/files.
This PR will need to be rebased eventually, and is therefore blocked in the meantime.

Is blocked by: #534

Dany9966 · 2026-10-15T11:37:22Z

+        "title": "Set DHCP",
+        "description": (
+            "Sets whether or not to configure the VM to use DHCP "
+            "during the OSMorphing stage."


Nit:

Suggested change

"during the OSMorphing stage."

"during the OSMorphing stage. "

"When disabled, guest static IPs will be preserved after the migration."

Dany9966 · 2026-10-15T13:16:49Z

+            "Keep the guest user accounts during OS morphing. "
+            "When false, OS morphing creates the cloud-init user."


Suggested change

"Keep the guest user accounts during OS morphing. "

"When false, OS morphing creates the cloud-init user."

"Prevents the destination cloud provisioning from resetting the pre-existing credentials. "

"When disabled, the destination platform may also create a provisioned cloud guest user."

Dany9966 · 2026-10-15T13:17:34Z

+opts = [
+    cfg.ListOpt(
+        "cloudbase_init_plugins",
+        default=None,


Why don't you use the config_defaults from CORE_DESTINATION_OPTIONS as well?

If we set a default for cloudbase_init_plugins, it will be applied over the provider default because the provider default is not handled in apply_core_destination_overrides. Maybe we should pass enabled_plugins in osmorphing_parameters as cloudbase_init_plugins, but that means that this will no longer be a drop in replacement.
I can still set defaults from CORE_DESTINATION_OPTIONS for the other options.

Dany9966 · 2026-10-15T13:31:28Z

+LOG = logging.getLogger(__name__)
+
+
+def merge_core_destination_options(provider_options, option_names=None):


Please set type hints for all introduced method args and return types.

Dany9966 · 2026-10-15T13:47:29Z

+        """
+        if plugins is None:
+            return None
+        if isinstance(plugins, str):


At what point does this plugins be a string? IMO it shouldn't reach this point and then make a list out of the string, especially since the schema expects it to be a list.
If that happens, it means a schema validation is either not being called or unfunctional.
All of this logic is supposed to be done by the schema validator.

Dany9966 · 2026-10-15T13:48:16Z

              "type": "boolean",
-              "default": false
+              "default": false,
+              "description": "Keep the guest user accounts during OS morphing. When false, OS morphing creates the cloud-init user."


Please set the new descriptions here as well.

Dany9966 · 2026-10-15T13:52:51Z

+# Used when dest-env and the destination provider both omit the key.
+# cloudbase_init_plugins = cloudbaseinit.plugins.common.mtu.MTUPlugin,cloudbaseinit.plugins.windows.ntpclient.NTPClientPlugin
+# Disk copy mechanism for destination worker VMs: HTTPS (TCP/5566) or SSH (TCP/22).
+# data_transfer_mechanism = HTTPS


since there's not only osmorphing options in here, maybe we should move these somewhere else, and not in osmorphing_conf?

I'd also like to see these in a separate section, not under DEFUALT. Something like common_target_option.

Dany9966

Left some comments, but I'm not overall happy with how complicated this got. I'm worried about how this code could possibly be maintained and understood. I think we'll need to think about this one some more before proceeding.

What I particularly don't like about it is that there's a lot of handling/hacks core-side to make the common options work and account for plugin defined or missing options. It would also be helpful (as in easier to review) if we'd refactor common options before adding new ones (cloudbase_init_plugins).

Dany9966 · 2026-10-16T08:44:56Z


 from coriolis.osmorphing import windows

+COMMON_TARGET_OPTION_GROUP = "common_target_option"


NIT:

Suggested change

COMMON_TARGET_OPTION_GROUP = "common_target_option"

COMMON_TARGET_OPTION_GROUP = "common_target_options"

Dany9966 · 2026-10-16T10:06:49Z

+              "default": false,
+              "description": "Prevents the destination cloud provisioning from resetting the pre-existing credentials. When disabled, the destination platform may also create a provisioned cloud guest user."
+            },
+            "cloudbase_init_plugins": {


Let's please rename this option into custom_cloudbase_init_plugins, since it will list all the values and users can easily mess it up. By adding the Custom here, it will be more obvious that this is something to be messed with only in special cases.

Dany9966 · 2026-10-16T10:13:10Z

+              "items": {
+                "type": "string"
+              },
+              "description": "Cloudbase-Init plugin class names for Windows guests. When set, this list is written into the guest. When omitted, the provider plugin list is used, then coriolis.conf, then the Windows morphing default."


Suggested change

"description": "Cloudbase-Init plugin class names for Windows guests. When set, this list is written into the guest. When omitted, the provider plugin list is used, then coriolis.conf, then the Windows morphing default."

"description": "Custom Cloudbase-Init plugin class names for Windows guests. This overrides the destination provider's list of cloudbase-init plugins that get enabled after migrating Windows guests."

Dany9966 · 2026-10-21T14:30:23Z

+        if provider_plugins is not None:
+            return provider_plugins
+
+        from coriolis.osmorphing import conf as osmorphing_conf


I'd much rather have the cloudbase init plugin defaults moved to conf than having the risk of circular imports. That will ensure that windows module import conf module safely, and conf module never (and should never) import windows or other modules.

Dany9966 · 2026-10-21T14:32:08Z

            remote_script_path,
        )

+    def _parse_cloudbase_init_plugins(self, plugins):


Why no type hints for these private methods?

Dany9966 · 2026-10-21T14:37:24Z

+    ``_resolve_destination_option``.
+    """
+    if not isinstance(osmorphing_info, dict):
+        osmorphing_info = osmorphing_info or {}


If this must be a dict, than assign a dict to it. It's even worse that down you set osmorphing_info = dict(osmorphing_info) which could've literally thrown a ValueError pretty easily.

Suggested change

osmorphing_info = osmorphing_info or {}

osmorphing_info = {}

Might be better to just raise InvalidInput when it's not a dict. I'll change all of the not isSomething to raise InvalidInput.

Dany9966 · 2026-10-21T14:58:41Z

+    },
+}
+
+CORE_DESTINATION_OPTIONS = {


I still feel like this whole module shouldn't live under osmorphing since it will not hold only osmorphing env options.
Please refactor it elsewhere. Suggestion: conf.py module under providers

Add data_transfer_mechanism, set_dhcp, and retain_user_credentials under common_target_options. The worker merges those rows onto each provider list, injects the fields into the destination schema, and removes core names before the provider call. A request that names only core options returns only those rows. OS morphing stores set_dhcp as nics_set_dhcp and stores retain_user_credentials on the morphing parameters.

claudiubelu reviewed Aug 24, 2026

View reviewed changes

Cristi1324 force-pushed the refactor-target-options branch 2 times, most recently from b03c3c2 to d799e6f Compare August 25, 2026 12:39

Cristi1324 force-pushed the refactor-target-options branch 2 times, most recently from 70b1df5 to d757307 Compare September 2, 2026 09:27

Cristi1324 changed the title ~~WIP Refactor provider options~~ Refactor provider options Sep 2, 2026

Cristi1324 marked this pull request as ready for review September 2, 2026 09:35

Dany9966 reviewed Sep 28, 2026

View reviewed changes

Cristi1324 marked this pull request as draft October 9, 2026 10:53

Cristi1324 force-pushed the refactor-target-options branch 2 times, most recently from 6de4af7 to 328497f Compare October 13, 2026 11:56

Cristi1324 marked this pull request as ready for review October 13, 2026 11:59

Cristi1324 requested a review from Dany9966 October 15, 2026 09:47

Dany9966 requested changes Oct 15, 2026

View reviewed changes

Cristi1324 force-pushed the refactor-target-options branch 2 times, most recently from 6cc5e62 to b89fa1b Compare October 16, 2026 10:51

Dany9966 requested changes Oct 21, 2026

View reviewed changes

Cristi1324 force-pushed the refactor-target-options branch from b89fa1b to afd830d Compare October 22, 2026 11:51

Cristi1324 commented Oct 22, 2026

View reviewed changes

Comment thread coriolis/providers/conf.py

Cristi1324 marked this pull request as draft October 22, 2026 12:04

Cristi1324 force-pushed the refactor-target-options branch from afd830d to 873a876 Compare October 22, 2026 12:11

Cristi1324 mentioned this pull request Oct 22, 2026

Add cloudbase init plugins common option #549

Open

7 of 10 tasks

Cristi1324 marked this pull request as ready for review October 22, 2026 12:32

	"during the OSMorphing stage."
	"during the OSMorphing stage. "
	"When disabled, guest static IPs will be preserved after the migration."

		"Keep the guest user accounts during OS morphing. "
		"When false, OS morphing creates the cloud-init user."

		LOG = logging.getLogger(__name__)


		def merge_core_destination_options(provider_options, option_names=None):


		from coriolis.osmorphing import windows

		COMMON_TARGET_OPTION_GROUP = "common_target_option"

	"description": "Cloudbase-Init plugin class names for Windows guests. When set, this list is written into the guest. When omitted, the provider plugin list is used, then coriolis.conf, then the Windows morphing default."
	"description": "Custom Cloudbase-Init plugin class names for Windows guests. This overrides the destination provider's list of cloudbase-init plugins that get enabled after migrating Windows guests."

Conversation

Cristi1324 commented Aug 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dany9966 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Cristi1324 commented Aug 19, 2026 •

edited

Loading