Enable type checking #5936

holmanb · 2024-12-18T20:02:47Z

Proposed Commit Message

chore: enable type checking for core Python modules

Add various datastructures and type annotations to main.py and stages.py
to support type checking and simplify logic.

Refactor logic in Init().

This change reduces McCabe complexity index in main.py:

    main_init: 31 -> 28
    status_wrapper: 20 -> 13

Remove the following from the mypy override list:

    cloudinit.analyze
    cloudinit.cmd.main
    cloudinit.config.modules
    cloudinit.distros
    cloudinit.distros.ubuntu
    cloudinit.helpers
    cloudinit.net.cmdline
    cloudinit.sources
    cloudinit.ssh_util
    cloudinit.stages
    cloudinit.templater

Additional context

Note to reviewers: most of the file changes should be self-explanatory, with two exceptions:

stages.py
---------

- Init.__init__(): signature change to accept new Stage type and to set cache_mode 
- renamed methods
- some new helper methods to simplify logic
- add module-level Stage variables to reduce magic string literals

main.py
-------

- add TypedDict to define status.json internal datastructures
- replace magic literal use with Stage types from stages.py
- stop using dsmode strings for controlling non-datasource control flow

Merge type

Squash merge using "Proposed Commit Message"
Rebase and merge unique commits. Requires commit messages per-commit each referencing the pull request number (#<PR_NUM>)

holmanb · 2024-12-18T20:25:15Z

cloudinit/analyze/show.py

@@ -122,7 +122,7 @@ class SystemctlReader:

    def __init__(self, property, parameter=None):
        self.epoch = None
-        self.args = [subp.which("systemctl"), "show"]
+        self.args = ["systemctl", "show"]


subp.which can return None, this is redundant

holmanb · 2024-12-19T00:57:00Z

cloudinit/stages.py

+    check = "check"
+
+
+# used in status.json and some log strings


This section defines a variety of magic strings that were previously scattered throughout main.py and stages.py.

holmanb · 2024-12-19T00:59:03Z

cloudinit/stages.py

+
+# These module variables store static data that is associated with each stage.
+# https://docs.cloud-init.io/en/latest/explanation/boot.html
+local: Final = BootStage(


These dataclasses hold all the (formerly) magic strings. Note that dsmode is not included because dsmode is a datasource & runtime concept, not a thing that is known before running the code.

holmanb · 2024-12-19T00:59:48Z

cloudinit/stages.py

+
+        stage: which Stage Init() is currently running in
+        reporter: a reporter object
+        cache_mode: force a specific CacheMode


cache_mode is known before instantiation, so set it on instantiation

holmanb · 2024-12-19T01:08:42Z

cloudinit/cmd/main.py

@@ -276,36 +305,6 @@ def attempt_cmdline_url(path, network=True, cmdline=None) -> Tuple[int, str]:
    )


-def purge_cache_on_python_version_change(init):


This makes more sense as an Init() method. A few other functions in this file should probably be moved as well (_should_bring_up_interfaces(), _maybe_persist_instance_data(), _maybe_set_hostname()), but this PR is already too big.

holmanb · 2024-12-19T01:10:00Z

cloudinit/cmd/main.py

    if args.local:
-        deps = [sources.DEP_FILESYSTEM]
+        stage = stages.local


stage holds all of the (formerly) magic strings as well as deps - this is all statically defined data that is known prior to runtime

holmanb · 2024-12-19T01:13:42Z

cloudinit/cmd/main.py

        sys.stderr.write("%s\n" % (netinfo.debug_info()))
    else:
-        existing = "check"


This logic still exists. See Init().cache_mode

holmanb · 2024-12-19T01:19:16Z

cloudinit/cmd/main.py


-    if mode == sources.DSMODE_NETWORK:


Using dsmode variables for flow control that isn't actually relevant to the dsmode is unnecessarily convoluted.

All stage-specific flow control* now uses the local stage variable and the stages.{local,network,config,final} dataclasses.

*with the exception of the (two?) spots in the code where datasource.dsmode modifies the flow of code

holmanb · 2024-12-19T01:20:49Z

cloudinit/cmd/main.py


    # Stage 5
    bring_up_interfaces = _should_bring_up_interfaces(init, args)
    try:
-        init.fetch(existing=existing)
-        # if in network mode, and the datasource is local
+        init.fetch()


cache_mode is either set at Init() instantiation, or discovered at runtime in Init().cache_mode

holmanb · 2024-12-19T01:23:19Z

cloudinit/cmd/main.py

        # Before network comes up, set any configured hostname to allow
        # dhcp clients to advertize this hostname to any DDNS services
        # LP: #1746455.
-        _maybe_set_hostname(init, stage="local", retry_stage="network")
-
-    init.apply_network_config(bring_up=bring_up_interfaces)


This still gets called in both branches, just a small refactor to eliminate redundant branches.

holmanb · 2024-12-19T01:26:09Z

cloudinit/cmd/main.py

            LOG.debug(
-                "[%s] Exiting. datasource %s not in local mode.",


The [%s] was redundant. If the datasource is not in local mode, then it is in network mode. The surrounding logs also hold this information.

holmanb · 2024-12-19T01:26:34Z

cloudinit/cmd/main.py

                init.datasource,
            )
            return (init.datasource, [])
        else:
            LOG.debug(
-                "[%s] %s is in local mode, will apply init modules now.",
-                mode,
+                "%s is in local mode, will apply init modules now.",


The [%s] was redundant.

holmanb · 2024-12-19T19:48:44Z

cloudinit/stages.py

@@ -342,9 +587,7 @@ def _get_datasources(self):
        cfg_list = self.cfg.get("datasource_list") or []
        return (cfg_list, pkg_list)

-    def _restore_from_checked_cache(self, existing):
-        if existing not in ("check", "trust"):


Since Init() is annotated with the stages.CacheMode, and all callsites of Init() are checked by mypy, we shouldn't need to do these runtime checks anymore.

holmanb · 2024-12-19T19:50:38Z

cloudinit/stages.py

@@ -431,7 +679,7 @@ def _get_ipath(self, subname=None):
            )
        return instance_dir

-    def _write_network_config_json(self, netcfg: dict):
+    def _write_network_config_json(self, netcfg: Optional[dict]):


Because self._find_networking_config() has codepaths that return None as the first argument.

holmanb · 2024-12-19T19:54:59Z

cloudinit/subp.py

    else:
        raise_on_invalid_command(args)
        bytes_args = [
            x if isinstance(x, bytes) else x.encode("utf-8") for x in args
        ]
    try:
        with performance.Timed(
-            "Running {}".format(logstring if logstring else args)


Mypy didn't like the multiple types that this could be. Standardize above to List[bytes] and log it as a List[string].

github-actions · 2025-01-07T00:24:51Z

Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close.

If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging TheRealFalcon, and he will ensure that someone takes a look soon.

(If the pull request is closed and you would like to continue working on it, please do tag TheRealFalcon to reopen it.)

github-actions · 2025-01-22T00:24:10Z

Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close.

If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging TheRealFalcon, and he will ensure that someone takes a look soon.

(If the pull request is closed and you would like to continue working on it, please do tag TheRealFalcon to reopen it.)

TheRealFalcon

First pass. Overall some really nice changes. I left some comments inline.

Additionally, I know you mentioned it earlier, but how hard is it to split out some of the changes into commits? In particular, the BootStage changes along with read_cfg_paths changes have some far reaching implications, so it's hard to verify those with all of the other interleaved changes.

TheRealFalcon · 2025-01-08T21:18:03Z

cloudinit/stages.py

@@ -63,6 +75,180 @@
 )


+@dataclass


Is there any reason not to keep this a named tuple?

TheRealFalcon · 2025-01-08T21:20:58Z

cloudinit/stages.py

+]
+
+
+@dataclass


(frozen=True) for the dataclasses defined here?

TheRealFalcon · 2025-01-08T21:25:06Z

cloudinit/sources/__init__.py

+# [DEP_FILESYSTEM, DEP_NETWORK] - this means Network stage
+#
+# XXX: TODO - simplify this logic
+Deps = Literal["FILESYSTEM", "NETWORK"]


I don't really see a value add here. Other than the next two lines, there's a single call point that could define the literal there.

TheRealFalcon · 2025-01-08T21:31:47Z

cloudinit/sources/__init__.py

-DSMODE_NETWORK = "net"
-DSMODE_PASS = "pass"
+DsMode = Literal["disabled", "local", "net", "pass"]
+DSMODE_DISABLED: Final[DsMode] = "disabled"


I don't see how typing these variables here as Literal helps us at all. Why not move the Literal typing to line 212?

TheRealFalcon · 2025-01-27T22:01:44Z

cloudinit/sources/__init__.py


-UNSET = "_unset"
-METADATA_UNKNOWN = "unknown"
+UNSET: Final[dict] = {}


This variable is referenced in a bunch of places outside of this file. I think I'd rather see us replace instances of this one by one rather than trying to do a big bang update here.

TheRealFalcon · 2025-01-27T22:05:09Z

cloudinit/distros/__init__.py

@@ -583,7 +583,7 @@ def update_hostname(self, hostname, fqdn, prev_hostname_fn):
        # Remove duplicates (incase the previous config filename)
        # is the same as the system config filename, don't bother
        # doing it twice
-        update_files = set([f for f in update_files if f])
+        update_files = list(set([f for f in update_files if f]))


Why is this necessary?

TheRealFalcon · 2025-01-27T22:25:08Z

cloudinit/stages.py

+
+
+# used in status.json and some log strings
+UserLongName = Literal[


Defining all of these string literals feels like overkill to me. As far as I can tell, after the BootStage refactor (which is really nice btw), the boot stages are all initialized once right below these definitions. Do we really need to define every possible string value that a welcome value can take or the internal name that we log? How does that benefit us?

My preference would be to kill all of the of the Literal defintions here, and have a single BootStage class defined as:

class BootStage: # Or whatever we wanna call it deps: List[sources.Deps] description: str welcome: str name: str report: str long: str status: str internal: str

The other stages can use a "" for fields that aren't used for those stages. I don't think the benefit of getting rid of 3 unused fields (that only get logged/printed anyway) is worth the complexity of a whole class hierarchy.

stappersg · 2025-02-09T16:12:47Z

Just a silly update to prevent that the git-actions-bot marks this merge request as stale. (Otherwise would human intervention be needed to remove the stale pr label.)

holmanb added 3 commits December 18, 2024 12:55

wip

ab7cea8

Merge remote-tracking branch 'upstream/main' into HEAD

d87869f

fix import

4252b89

holmanb commented Dec 18, 2024

View reviewed changes

holmanb added 4 commits December 18, 2024 14:33

wip

c0bd068

dedupe main.py name styles, stop using dsmode for non-dsmode logic

df5b5fd

wip

7eafec8

wip

aeacf62

holmanb commented Dec 19, 2024

View reviewed changes

holmanb added 2 commits December 18, 2024 18:30

wip

544890a

wip

674b94f

holmanb commented Dec 19, 2024

View reviewed changes

wip

d4ed0ce

holmanb marked this pull request as ready for review December 19, 2024 23:52

holmanb mentioned this pull request Dec 20, 2024

Decide on scheduled-daily #5929

Open

github-actions bot added the stale-pr Pull request is stale; will be auto-closed soon label Jan 7, 2025

holmanb removed the stale-pr Pull request is stale; will be auto-closed soon label Jan 7, 2025

github-actions bot added the stale-pr Pull request is stale; will be auto-closed soon label Jan 22, 2025

holmanb removed the stale-pr Pull request is stale; will be auto-closed soon label Jan 22, 2025

TheRealFalcon requested changes Jan 27, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable type checking #5936

Enable type checking #5936

holmanb commented Dec 18, 2024 •

edited

Loading

holmanb Dec 18, 2024

holmanb Dec 19, 2024

holmanb Dec 19, 2024

holmanb Dec 19, 2024

holmanb Dec 19, 2024

holmanb Dec 19, 2024

holmanb Dec 19, 2024

holmanb Dec 19, 2024

holmanb Dec 19, 2024

holmanb Dec 19, 2024

holmanb Dec 19, 2024

holmanb Dec 19, 2024

holmanb Dec 19, 2024

holmanb Dec 19, 2024

holmanb Dec 19, 2024

github-actions bot commented Jan 7, 2025

github-actions bot commented Jan 22, 2025

TheRealFalcon left a comment •

edited

Loading

TheRealFalcon Jan 8, 2025

TheRealFalcon Jan 8, 2025

TheRealFalcon Jan 8, 2025

TheRealFalcon Jan 8, 2025

TheRealFalcon Jan 27, 2025

TheRealFalcon Jan 27, 2025

TheRealFalcon Jan 27, 2025

stappersg commented Feb 9, 2025 •

edited

Loading

		@@ -276,36 +305,6 @@ def attempt_cmdline_url(path, network=True, cmdline=None) -> Tuple[int, str]:
		)


		def purge_cache_on_python_version_change(init):



		# used in status.json and some log strings
		UserLongName = Literal[

		]


		@dataclass

Enable type checking #5936

Are you sure you want to change the base?

Enable type checking #5936

Conversation

holmanb commented Dec 18, 2024 • edited Loading

Proposed Commit Message

Additional context

Merge type

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

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

Choose a reason for hiding this comment

github-actions bot commented Jan 7, 2025

github-actions bot commented Jan 22, 2025

TheRealFalcon left a comment • edited Loading

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

Choose a reason for hiding this comment

stappersg commented Feb 9, 2025 • edited Loading

holmanb commented Dec 18, 2024 •

edited

Loading

TheRealFalcon left a comment •

edited

Loading

stappersg commented Feb 9, 2025 •

edited

Loading