Oracle netplan v2 migration

[a-dubs]

Proposed Commit Message

feat(oracle): update Oracle Datasource to use netplan v2

Additional Context

Test Steps

Ran the full integration test suite against Noble using a custom deb created from this feature branch.

Merge type

• Squash merge using "Proposed Commit Message"

[TheRealFalcon]

See my inline comments. I think you added some good code, but much of it can move into tests.

Additionally, I forget, why are we moving to support both v1 and v2 now? Once we move to v2, is there any reason we would need to keep the v1 support around?

[a-dubs]

@TheRealFalcon thanks for the feedback! I guess the idea here was that by keeping the old v1 code around, we could do the comparisons on live systems that way if anyone ever finds a weird edge case then it will log a warning and they could file a bug. But honestly, now that you mention it, I do kinda prefer the idea of just having a super comprehensive unit test suite to verify it once and be done and then move on with our lives.

@holmanb thoughts? since this was your idea originally?

[holmanb]

@TheRealFalcon thanks for the feedback! I guess the idea here was that by keeping the old v1 code around, we could do the comparisons on live systems that way if anyone ever finds a weird edge case then it will log a warning and they could file a bug. But honestly, now that you mention it, I do kinda prefer the idea of just having a super comprehensive unit test suite to verify it once and be done and then move on with our lives.

@holmanb thoughts? since this was your idea originally?

The only benefit that I see of a runtime check is that it might catch edge cases that couldn't be predicted with a battery of unit tests. I'm fine with just getting some good test coverage instead if that's preferred.

[holmanb]

@a-dubs I left some comments below.

[Copilot]

PR Overview

This PR migrates the Oracle datasource configuration from netplan v1 to netplan v2 and improves test coverage for helper functions.

• Updates the Oracle datasource to use the new netplan v2 configuration schema.
• Adds a set of comprehensive unit tests for the dict comparison helper function.
• Introduces YAML import and logger initialization in the test helpers to support improved debugging.

Reviewed Changes

File	Description
tests/unittests/helpers.py	Adds YAML import, logger initialization, and the dicts_are_equal helper function.
tests/unittests/test_helpers.py	Introduces unit tests for the dicts_are_equal function.
cloudinit/sources/DataSourceOracle.py	Migrates Oracle datasource configuration from netplan v1 to v2 and refactors related methods.

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

[TheRealFalcon]

I left some inline comments.

[TheRealFalcon]

This is good and really nice having a bunch of tests for it, but pytest magic already has this built in. Behold:

class TestDictsAreEqual:
    """Tests for dicts_are_equal function."""

    def test_identical_dicts(self):
        """Test that identical dicts return True."""
        dict1 = {"a": 1, "b": 2}
        dict2 = {"a": 1, "b": 2}
        assert dict1 == dict2

    def test_different_dicts(self):
        """Test that different dicts return False."""
        dict1 = {"a": 1, "b": 2}
        dict2 = {"a": 1, "b": 3}
        assert dict1 != dict2

    def test_different_keys(self):
        """Test that dicts with different keys return False."""
        dict1 = {"a": 1, "b": 2}
        dict2 = {"a": 1, "c": 2}
        assert dict1 != dict2

    def test_different_order_same_content(self):
        """
        Test that dicts with same content but different order return True.
        """
        dict1 = {"a": 1, "b": 2}
        dict2 = {"b": 2, "a": 1}
        assert dict1 == dict2

    def test_nested_dicts_equal(self):
        """Test that nested dicts with identical content return True."""
        dict1 = {"a": 1, "b": {"c": 3, "d": 4}}
        dict2 = {"a": 1, "b": {"c": 3, "d": 4}}
        assert dict1 == dict2

    def test_nested_dicts_different(self):
        """Test that nested dicts with different content return False."""
        dict1 = {"a": 1, "b": {"c": 3, "d": 4}}
        dict2 = {"a": 1, "b": {"c": 3, "d": 5}}
        assert dict1 != dict2

    def test_nested_dict_different_order(self):
        """
        Test that nested dicts with different order & same content return True.
        """
        dict1 = {"a": 1, "b": {"c": 3, "d": 4}}
        dict2 = {"b": {"d": 4, "c": 3}, "a": 1}
        assert dict1 == dict2

    def test_list_values_equal(self):
        """Test that dicts with list values that are identical return True."""
        dict1 = {"a": [1, 2, 3], "b": 2}
        dict2 = {"a": [1, 2, 3], "b": 2}
        assert dict1 == dict2

    def test_list_values_different(self):
        """Test that dicts with list values that are different return False."""
        dict1 = {"a": [1, 2, 3], "b": 2}
        dict2 = {"a": [1, 2, 4], "b": 2}
        assert dict1 != dict2

    def test_list_order_matters(self):
        """
        Test that dicts with list values in different orders return False.
        """
        dict1 = {"a": [1, 2, 3], "b": 2}
        dict2 = {"a": [3, 2, 1], "b": 2}
        assert dict1 != dict2

    def test_empty_dicts(self):
        """Test that empty dicts are equal."""
        dict1 = {}
        dict2 = {}
        assert dict1 == dict2

    def test_dict_with_none_values(self):
        """Test dicts containing None values."""
        dict1 = {"a": None, "b": 2}
        dict2 = {"a": None, "b": 2}
        assert dict1 == dict2

    def test_nested_lists_with_dicts(self):
        """Test dicts containing lists of dicts."""
        dict1 = {"a": [{"x": 1}, {"y": 2}]}
        dict2 = {"a": [{"x": 1}, {"y": 2}]}
        assert dict1 == dict2

    def test_nested_lists_with_dicts_different(self):
        """Test dicts containing lists of dicts that differ."""
        dict1 = {"a": [{"x": 1}, {"y": 2}]}
        dict2 = {"a": [{"x": 1}, {"y": 3}]}
        assert dict1 != dict2

    def test_debug_message_content(self, caplog):
        """Test that debug log contains expected diff content."""
        dict1 = {"a": 1, "b": 2}
        dict2 = {"a": 1, "b": 3}

        assert dict1 != dict2
        # Verify a debug message containing 'Dictionaries do not match' logged
        # assert "Dictionaries do not match" in caplog.text

        # assert "-b: 2" in caplog.text
        # assert "+b: 3" in caplog.text

    @pytest.mark.parametrize(
        "dict1,dict2,expected",
        [
            ({"a": 1}, {"a": 1}, True),
            ({"a": 1}, {"a": 2}, False),
            ({"a": 1, "b": 2}, {"b": 2, "a": 1}, True),
            ({"a": {"b": 2}}, {"a": {"b": 2}}, True),
            ({"a": {"b": 2}}, {"a": {"b": 3}}, False),
            ({"a": [1, 2]}, {"a": [1, 2]}, True),
            ({"a": [1, 2]}, {"a": [2, 1]}, False),
        ],
    )
    def test_various_dicts(self, dict1, dict2, expected):
        """Parameterized test for multiple dictionary comparisons."""
        if expected:
            assert dict1 == dict2
        else:
            assert dict1 != dict2

I modified your tests accordingly. If you run that through pytest, they'll all succeed. You also get pretty output (with -vv) when a comparison fails.

I think we can scrap this function and its tests.

[TheRealFalcon]

If we follow the call path here, we're taking the klibc definition from initramfs, rendering v1 config from it, and then converting it to v2. I think that is ok because we have another call site that uses that v1 config and best not to have to maintain two independent implementations.

However, I'm wondering if we could push the conversion code down a bit. Maybe under this function, could we move the v1 to v2 conversion there and then update the calls accordingly? The reason I think it makes more sense there is because _find_networking_config() is expecting a v1 config, and if future us ever wants to change that to use v2, it'll be much more obvious that it is the only consumer using the v1 config and that Oracle no longer cares about v1.

This is all assuming we keep the conversion (see my other comment).

[TheRealFalcon]

I don't actually think this is safe long term. Since v2 is a subset of netplan, it's possible that we'll render something that is valid netplan but not valid v2. I'm wondering if we might just need a separate klibc->v2 renderer rather than trying to support all of v1->v2.

[a-dubs]

This is being closed in favor of the new approach in #6097