Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cc_modules: set default meta frequency value when no config available #1457

Merged
merged 4 commits into from
May 16, 2022

Conversation

blackboxsw
Copy link
Collaborator

@blackboxsw blackboxsw commented May 16, 2022

Proposed Commit Message

Each cloud-config module defines meta["frequency"] module attribute.
This frequency will be preferred unless overrides are given in
/etc/cloud/cloud.cfg module definitions.

Additional Context

Test Steps

Checklist:

  • My code follows the process laid out in the documentation
  • I have updated or added any unit tests accordingly
  • I have updated or added any documentation accordingly

Each cloud-config module defines meta["frequency"] module attribute.
This frequency will be preferred unless overrides are given in
/etc/cloud/cloud.cfg module definitions.
@blackboxsw blackboxsw requested a review from TheRealFalcon May 16, 2022 16:20
@TheRealFalcon
Copy link
Member

Additional test if you want it. It's only checking logs, but hopefully our logs aren't lying to us.

diff --git a/tests/integration_tests/modules/test_combined.py b/tests/integration_tests/modules/test_combined.py
index eb45062e..58262ce1 100644
--- a/tests/integration_tests/modules/test_combined.py
+++ b/tests/integration_tests/modules/test_combined.py
@@ -5,12 +5,16 @@ of the test would be unlikely to affect the running of another test using
 the same instance launch. Most independent module coherence tests can go
 here.
 """
+import glob
+import importlib
 import json
 import re
 import uuid
+from pathlib import Path
 
 import pytest
 
+import cloudinit.config
 from tests.integration_tests.clouds import ImageSpecification
 from tests.integration_tests.decorators import retry
 from tests.integration_tests.instances import IntegrationInstance
@@ -223,6 +227,25 @@ class TestCombined:
             class_client.execute("stat -c %N /run/cloud-init/cloud-id")
         )
 
+    def test_run_frequency(self, class_client: IntegrationInstance):
+        log = class_client.read_from_file("/var/log/cloud-init.log")
+        config_dir = Path(cloudinit.config.__file__).parent
+        module_paths = glob.glob(str(config_dir / "cc*.py"))
+        module_names = [Path(x).stem for x in module_paths]
+        found_count = 0
+        for name in module_names:
+            mod = importlib.import_module(f"cloudinit.config.{name}")
+            frequency = mod.meta["frequency"]
+            # cc_ gets replaced with config- in logs
+            log_name = name.replace("cc_", "config-")
+            # Some modules have been filtered out in /etc/cloud/cloud.cfg,
+            if f"running {log_name}" in log:
+                found_count += 1  # Ensure we're matching on the right text
+                assert f"running {log_name} with frequency {frequency}" in log
+        assert (
+            found_count > 10
+        ), "Not enough modules found in log. Did the log message change?"
+
     def _check_common_metadata(self, data):
         assert data["base64_encoded_keys"] == []
         assert data["merged_cfg"] == "redacted for non-root user"

@blackboxsw
Copy link
Collaborator Author

Additional test if you want it. It's only checking logs, but hopefully our logs aren't lying to us.

diff --git a/tests/integration_tests/modules/test_combined.py b/tests/integration_tests/modules/test_combined.py
index eb45062e..58262ce1 100644
--- a/tests/integration_tests/modules/test_combined.py
+++ b/tests/integration_tests/modules/test_combined.py
@@ -5,12 +5,16 @@ of the test would be unlikely to affect the running of another test using
 the same instance launch. Most independent module coherence tests can go
 here.
 """
+import glob
+import importlib
 import json
 import re
 import uuid
+from pathlib import Path
 
 import pytest
 
+import cloudinit.config
 from tests.integration_tests.clouds import ImageSpecification
 from tests.integration_tests.decorators import retry
 from tests.integration_tests.instances import IntegrationInstance
@@ -223,6 +227,25 @@ class TestCombined:
             class_client.execute("stat -c %N /run/cloud-init/cloud-id")
         )
 
+    def test_run_frequency(self, class_client: IntegrationInstance):
+        log = class_client.read_from_file("/var/log/cloud-init.log")
+        config_dir = Path(cloudinit.config.__file__).parent
+        module_paths = glob.glob(str(config_dir / "cc*.py"))
+        module_names = [Path(x).stem for x in module_paths]
+        found_count = 0
+        for name in module_names:
+            mod = importlib.import_module(f"cloudinit.config.{name}")
+            frequency = mod.meta["frequency"]
+            # cc_ gets replaced with config- in logs
+            log_name = name.replace("cc_", "config-")
+            # Some modules have been filtered out in /etc/cloud/cloud.cfg,
+            if f"running {log_name}" in log:
+                found_count += 1  # Ensure we're matching on the right text
+                assert f"running {log_name} with frequency {frequency}" in log
+        assert (
+            found_count > 10
+        ), "Not enough modules found in log. Did the log message change?"
+
     def _check_common_metadata(self, data):
         assert data["base64_encoded_keys"] == []
         assert data["merged_cfg"] == "redacted for non-root user"

+1 , I was adding something similar to unit tests, but also just a general check that we didn't have anything fall through the cracks with a "None" frequency.
I'll pull in yours as I'd appreciate the full integration test on this, maybe we can suppplement with a no None frequency seen.

@TheRealFalcon
Copy link
Member

Here's one for overriding frequency. I put it in tests/integration_tests/modules/test_frequency_override.py:

import pytest

from tests.integration_tests.instances import IntegrationInstance

USER_DATA = """\
#cloud-config
runcmd:
 - echo "hi" >> /var/tmp/hi
"""


@pytest.mark.user_data(USER_DATA)
def test_frequency_override(client: IntegrationInstance):
    # Some pre-checks
    assert (
        "running config-scripts-user with frequency once-per-instance"
        in client.read_from_file("/var/log/cloud-init.log")
    )
    assert client.read_from_file("/var/tmp/hi").strip().count("hi") == 1

    # Change frequency of scripts-user to always
    config = client.read_from_file("/etc/cloud/cloud.cfg")
    new_config = config.replace("- scripts-user", "- [ scripts-user, always ]")
    client.write_to_file("/etc/cloud/cloud.cfg", new_config)

    client.restart()

    # Ensure the script was run again
    assert (
        "running config-scripts-user with frequency always"
        in client.read_from_file("/var/log/cloud-init.log")
    )
    assert client.read_from_file("/var/tmp/hi").strip().count("hi") == 2

Copy link
Member

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Looks good to me as is. I recently commented another integration test you could add for overriding frequency if you'd like.

cloudinit/config/modules.py Show resolved Hide resolved
@blackboxsw blackboxsw merged commit 2eb2cea into canonical:main May 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants