feat(helpers): support custom datasource vendor merge strategies - #6944
feat(helpers): support custom datasource vendor merge strategies#6944goldberl wants to merge 1 commit into
Conversation
a312d1f to
9cb7f50
Compare
There was a problem hiding this comment.
Thank you much for continued work on this @goldberl. The vendor-data vs user-data merge story is an old one with undesirable or poorly documented behavior. I have the general inclination that we don't want to continue to expose more merge_how primitives more publicly than they already are because that syntax is fairly tedious to get correct and has multiple behavioral implications that are complex. If we can think of ways to simplify this config setting from a datasource perspective with aliases for the details merge_how settings, I think that may make this easier to consume by image/platform creators.
One other question I'd like to visit is:
- how a user can disable or disregard the merge suggestions provided by MAAS default datasource config. I know cloud-config userdata can with the big hammer:
#cloud-config
vendordata: {enabled: false}
But, I think we may want to provide more discrete handling, since this is merge-order related behavior, not full inclusion or exclusion. I wonder what folks think about defining relative merge order of vendordata or vendordata2 with something like
#cloud-config
vendordata:
enabled: true
userdata_merge: (default|prepend|append|replace|default)
The following could allow users to override the maas vendordata merge defaults to sometime that suits their preference for a given image if it differs from the platform default. This 2nd suggestion likely overengineering for a corner case we don't really need to support at this point in time. But, I think that user-data expressing that it wants vendordata merged with user-data in a specific way may lead to broader support for issues such as #6268. In that case, user-data containing a vendordata: userdata_merge ...` value could establish a practice that duplicated vendordata keys get included/prepended instead of dropped/replaced.
| # When merging vendor-data with user-data, append list values | ||
| # (e.g. write_files, runcmd) so that vendor networking config | ||
| # is not silently dropped when user-data defines the same key. | ||
| vendor_merge_how = "list(append)+dict(no_replace,recurse_list)+str()" |
There was a problem hiding this comment.
I wonder if we can make this datasource config option value simpler to avoid exposing too much flexibility and avoid likelihood of image creator misuse.
What do you think of naming this specifically vendordata_usersdata_merge avoiding all the merge_how syntax and limiting values to something like:
-
default(current cloud-init behavior, replaces and duplicated config keys in VD and UD) -
prepend(prepend duplicated VD key values before UD) -
append(append VD values after UD)
I'm hesitant to continue to expose more merge_how operations and syntax 'publicly' as configuration options here because we have found our historic merge_how syntax is fairly confusing to use properly and exposes unique internal behavior changes that seem to require significant testing to assert the merge behaves as intended.
There was a problem hiding this comment.
These operations above may also help juju use-cases which provided the juju CLI flag preruncmd and postruncmd to try ordering cloud-init user-data before or after what should have been juju vendor-data to avoid collisions on the duplicated runcmd config values in both platform vendor-data and the client's user-data.
| "vendor_cloud_config", | ||
| "vendor2_cloud_config", | ||
| ): | ||
| vendor_merge = getattr( |
There was a problem hiding this comment.
If vendor_merge value is set to anything other than default, as a user I would want a debug message in logs that vendor_data is being prepended before userdata due to datasource_config option X or some such message.
| # When merging vendor-data with user-data, append list values | ||
| # (e.g. write_files, runcmd) so that vendor networking config | ||
| # is not silently dropped when user-data defines the same key. | ||
| vendor_merge_how = "list(append)+dict(no_replace,recurse_list)+str()" |
There was a problem hiding this comment.
Previously, merge_how really only appeared to work within the scope of separate user-data files. The config option should be better named to indicate this merge operation applies to the merge of vendor and userdata somehow.
There was a problem hiding this comment.
Thanks, name is currently append_vendor_list_data
| i_cfgs.append(util.read_conf(cc_fn)) | ||
| cfg = util.read_conf(cc_fn) | ||
| if cc_p in ( | ||
| "vendor_cloud_config", |
There was a problem hiding this comment.
Given the changes to helpers, can we get a unittest specifically that exercises differences in vendor_data and vendor_data2. As it stands the unittests defined test only userdata vs vendordata.
9cb7f50 to
31e0b09
Compare
When deploying a MAAS node with custom user-data, top-level lists
in vendor-data (e.g., `write_files`) are dropped if user-data defines
the same keys. This breaks networking by dropping the MAAS Netplan
configuration (`/etc/netplan/50-maas.yaml`).
This change introduces a boolean flag to configure vendor list
merging:
* `DataSourceMAAS` sets `append_vendor_list_data = True`.
* `ConfigMerger` checks the active datasource for this flag when
loading `vendor_cloud_config` and `vendor2_cloud_config`, logs a
debug message, and applies a non-default merge strategy to append
list items.
Signed-off-by: Leah Goldberg <leah.goldberg@canonical.com>
Fixes canonicalGH-6268
LP: #2158442
31e0b09 to
a09653d
Compare
2 participants
Proposed Commit Message
Additional Context
This PR fixes an issue where top-level lists in vendor-data (such as
write_files) are silently dropped when user-data defines the same keys, which breaks MAAS deployments by wiping out the Netplan network configuration (/etc/netplan/50-maas.yaml).To resolve this without altering global merge behavior across other cloud providers,
ConfigMergernow inspects the active datasource for avendor_merge_howattribute.DataSourceMAASsets this attribute tolist(append)+dict(no_replace,recurse_list)+str(), ensuring MAAS-generated vendor lists are appended to user lists.For example:
user-data
vendor-data
The current way of merging is:
Notice how the
write_filesfrom the vendor-data is ignored since the user-data already definedwrite_files.This PR would change the merge for DataSourceMAAS to be:
This ensures list values are appended instead of dropped.
Test Steps
How to reproduce the bug
Actual behavior
The deployed machine only has the PXE interface (
enp5s0) configured.Expected behavior
10.20.0.1 (enp5s0.100)
The expected behavior is for both the PXE interface (
enp5s0) and the MAAS-configured VLAN interface (enp5s0.100) to remain configured after deployment.How to test this fix
Note: I tested this on MAAS 3.5.12 (snap) which uses Ubuntu 22.04 (Jammy).
You should see networking is set up properly now:
Related Links
Merge type