refactor: update handle function of cc_mounts - #5498
Conversation
|
cc @a-dubs |
91dbc79 to
36b3157
Compare
The handle function of cc_mounts was hard to grok and had one of the highest cyclomatic complexity scores in the codebase. Functionally, the code should be unchanged.
36b3157 to
91d15a8
Compare
a-dubs
left a comment
a-dubs
left a comment
There was a problem hiding this comment.
Changes look good! Left one little comment/nit but not going to block on it.
Also I was wondering if it would be worth the time to add further unit tests or integration tests before merging to ensure this refactor didn't break anything?
| def sanitized_devname_is_valid( | ||
| original: str, sanitized: Optional[str], fstab_devs: Dict[str, str] | ||
| ) -> bool: | ||
| """Get if the device name is valid and log if not.""" |
There was a problem hiding this comment.
could we reword this description a little?
The last two commits in #5459 added some coverage for things I knew I was changing here. Got any specific ideas? |
|
@TheRealFalcon After reading back over all of the cc_mounts unit tests, I realize there is indeed coverage for all the functions you have created. My original thought was, "oh wow thats a lot of new helper functions, we should unit test those!!!". But I now see that the unit tests pretty thoroughly test the functions that rely on these helpers, so their functionality is pretty well tested, just not directly. So I think the effort of adding new unit tests is not worth the reward. |
|
This PR still needs another reviewer @holmanb or @blackboxsw |
blackboxsw
left a comment
blackboxsw
left a comment
There was a problem hiding this comment.
Minor nits and I think a potentially significant question on santize_mounts_configuration to see if we are accidentally dropping items from mount config lines if they contain None.
| default_already_exists = any( | ||
| cfgm[0] == default_mount[0] for cfgm in mounts | ||
| ) |
There was a problem hiding this comment.
Nice, I initially wondering if this may be less performant than default_mount[0] in [cfgm[0] for cfgm in mounts] but the any clause gives us a chance to early exit on first True seen so it's probably a bit better than the alternative.
Also another alternative would be to create mount_paths = set(cfgmnt=[0] for cfgmnt in mounts) outside the scope of the for loop to initialize it for use in both iterations of default_mounts.
Anyhow, nothing more than a discussion point. Not critical as far as processing cost.
There was a problem hiding this comment.
Those work too, but I think I prefer it as-is.
| """ | ||
| actlist = [] | ||
| dev_denylist = [] | ||
| for line in mounts[::-1]: |
There was a problem hiding this comment.
Why are we walking through mounts in reverse only to re-reverse the list order once we populate actlist? Is it because we generally expect the None field to be the highest index in our mounts list so we can persist it locally as a dev_denylist?
There was a problem hiding this comment.
Because we have some weird rules that would allow a mount declaration like this:
/dev/sda1 /mnt1
/dev/sda1 None
/dev/sda1 /mnt2
The None on line 2 means "disregard this line and any /dev/sda1 entries that came before it". The first reversal is to make it easy to know which lines to throw out that we come upon. If we previously saw a None having the same first entry, we know to throw it out.
Since we accumulated our act list in reverse order, the last reverse is to put the list back into it's original order (after we've removed the lines we want to remove).
| updated_cfg = add_default_mounts_to_cfg( | ||
| updated_cfg, default_mount_options, fstab_devs, device_aliases, cloud | ||
| ) | ||
| updated_cfg = remove_nonexistent_devices(updated_cfg) |
There was a problem hiding this comment.
Could remove_nonexistent_devices be performed because add_default_mounts_to_cfg since all we are doing in add_default_mounts_to_cfg is adding known valid default mount config for ephemeral and swap if they aren't already present?
| updated_cfg = add_default_mounts_to_cfg( | |
| updated_cfg, default_mount_options, fstab_devs, device_aliases, cloud | |
| ) | |
| updated_cfg = remove_nonexistent_devices(updated_cfg) | |
| updated_cfg = remove_nonexistent_devices(updated_cfg) | |
| updated_cfg = add_default_mounts_to_cfg( | |
| updated_cfg, default_mount_options, fstab_devs, device_aliases, cloud | |
| ) |
There was a problem hiding this comment.
No actually. I could specify ephemeral None to tell cloud-init not to render the epehermal device. On add_default_mounts_to_cfg, it'll see that I have already defined it, and then not touch the mount list, then later remove it on the remove_nonexistent_devices call. If I switch the order, cloud-init will remove it from the list, then add the default back in. We'll get the default added to the mount list when we didn't want it.
There was a problem hiding this comment.
Good, thanks for the correction/explanation there.
| updated_line = [ | ||
| str(token) for token in updated_line if token is not None | ||
| ] |
There was a problem hiding this comment.
This has the side-effect of truncating items in the list that have the value of None. Is this what we want here?
I think we want the following:
| updated_line = [ | |
| str(token) for token in updated_line if token is not None | |
| ] | |
| updated_line = [ | |
| None if token is None else str(token) for token in updated_line | |
| ] |
There was a problem hiding this comment.
Ahh, good find. I reworked the code there a bit to align with what we had before and also updated a test case that should pass on main but fail pre-fixing this.
|
@blackboxsw , this should be ready for re-review |
blackboxsw
left a comment
blackboxsw
left a comment
There was a problem hiding this comment.
LGTM. Thanks for the fixup of potentially redacted values and unittest additions. Validated before and after expected values in /etc/fstab mounts remain unchanged in integration testing and test coverage looks good.
This may have conflicts in merge with upstream/main to resolve.
| device_aliases = cfg.get("device_aliases", {}) | ||
|
|
||
| for i in range(len(cfgmnt)): | ||
| return fstab_lines, fstab_devs, fstab_removed |
There was a problem hiding this comment.
I wondered about defining a namedtuple here for readability and usability of the return value from this function but I don't think it's worth the minor overhead here and we only have one call-site to keep aligned with this return value, so not a lot of reuse potential there.
There was a problem hiding this comment.
Agreed. Since it's just one site it doesn't seem like it would add much value.
The handle function of cc_mounts was hard to grok and had one of the highest cyclomatic complexity scores in the codebase. Functionally, the code should be unchanged.
The handle function of cc_mounts was hard to grok and had one of the highest cyclomatic complexity scores in the codebase. Functionally, the code should be unchanged.
3 participants
Proposed Commit Message
Additional Context
Test Steps
Merge type