YAML consolidation: final patch set for configmanager.py (FR-702)#255
Merged
slyon merged 15 commits intocanonical:mainfrom Jun 30, 2022
Merged
YAML consolidation: final patch set for configmanager.py (FR-702)#255slyon merged 15 commits intocanonical:mainfrom
configmanager.py (FR-702)#255slyon merged 15 commits intocanonical:mainfrom
Conversation
Contributor
Author
schopin-pro
force-pushed
the
yaml-consolidation-full
branch
2 times, most recently
from
5c07c61 to
4f3e276
Compare
5 tasks
5 tasks
schopin-pro
force-pushed
the
yaml-consolidation-full
branch
from
4f3e276 to
259a22f
Compare
Codecov Report
@@ Coverage Diff @@
## main #255 +/- ##
==========================================
- Coverage 99.19% 98.93% -0.27%
==========================================
Files 61 61
Lines 11206 11131 -75
==========================================
- Hits 11116 11012 -104
- Misses 90 119 +29
Continue to review full report at Codecov.
|
5 tasks
schopin-pro
force-pushed
the
yaml-consolidation-full
branch
from
259a22f to
e905416
Compare
slyon
suggested changes
May 9, 2022
Contributor
slyon
left a comment
slyon
left a comment
There was a problem hiding this comment.
Big kudos to @schopin-pro !
Thank you very much for this final PR in a long series of changes.
This YAML parser consolidation makes the netplan internals so much cleaner and easier to maintain!
I did an initial round of review and added several comments inline that we should address. I need to have another go on this big change, though, especially the "netplan: migration configmanager away from PyYAML" refactoring commit (0993272) – The testsuite looks all good, but I'd like to double check that we don't accidentally change the behavior here.
slyon
reviewed
May 10, 2022
Contributor
slyon
left a comment
slyon
left a comment
There was a problem hiding this comment.
Adding some more inline comments, after having a 2nd look.
This was an internal API so this shouldn't create too much problem. Leaving the symbol as internal for now, pending publication in a subsequent PR.
Those attributes are generic enough to be useful in a number of situations, but will mostly be used when reimplementing the configmanager.py features.
V2: Rewrite the matching algorithm to take the new driver list feature
into account.
schopin-pro
force-pushed
the
yaml-consolidation-full
branch
from
e905416 to
748b2a6
Compare
The whole sriov.py file needs detailed informations from the net definitions. It used to get them from configmanager, but those are going away really soon! V2: * Use correct uint return type and explicitly check against UINT_MAX for invalid VLAN ID. * Prefix all new C symbols with an underscore to adhere to the new API/ABI conventions. * Remove superfluous trivial_compound_definition property.
There are a couple of inconsistencies in the test data. The test.yaml netplan config tries to configure a vlan interface on eth99, which hasn't been defined, and the merged config tries to add two new links on patcha and patchb, while they're already in a link together. While the current code doesn't catch this, parser consolidation will result in stricter validation, making the merge test fail.
…open I suspect that the fallback is needed because configmanager opens the netplan conf file in Python, but that won't necessarily be the case when the parsing is done via libnetplan.
schopin-pro
force-pushed
the
yaml-consolidation-full
branch
2 times, most recently
from
c96012b to
f939394
Compare
This change has far reaching impacts, as it changes the fundamental data types of the class. V2: * Split off the matching algorithm evolution which does NOT belong to this patch. * Move some small test data changes into their own patch to give it more context. * Add a test covering the error path on invalid input, since we now have some validation. * Some whitespace fixes in the tests. * apply.py: rework the whole matching loop to better fit the new data model * try.py: fix the missing extra-config application when checking for revertability.
Those old functions have been written out in favor of the user-friendlier State and Parser classes, so we now clean them up. This means that we now have more code in abi_compat.c that isn't covered by the test suite. I think that's OK, as they are simple wrappers around functions that *are* covered. V2: remove superfluous LCOV_EXCL_START statement
schopin-pro
force-pushed
the
yaml-consolidation-full
branch
from
f939394 to
b321115
Compare
Contributor
Author
|
I finally took the time to go through the review comments. I left a couple of discussions open as there are still unanswered questions there. Sorry, I forgot to keep a global log of the new version, although each commit should have a local changelog. |
slyon
approved these changes
Jun 27, 2022
Contributor
slyon
left a comment
slyon
left a comment
There was a problem hiding this comment.
Thank you very much for all the effort you've put into this @schopin-pro!
All my remarks have been addressed, with some comments still open/unresolved. But those are about exporting the symbols according to the new libnetplan API spec and we agreed upon that this adoption will be done separately in an additional PR.
All unit-tests, ABI checks and integration-/autopkgtests PASS, so this is ready for merging! \o/
Contributor
|
I've created a PPA build including this PR + #254 + #275: https://launchpad.net/~slyon/+archive/ubuntu/netplan-yaml-parser-pr255/+packages In addition to running some official autopkgtests against this build, I'd like to ask @mvo5 to maybe run the |
Contributor
|
Fwiw, I build a core22 snap that includes this netplan PPA: When I install that in a core22 image as a local install and run our tests (https://github.com/snapcore/snapd/blob/master/tests/core/netplan-cfg/task.yaml#L31) I get: |
Contributor
|
Thanks for testing, this is a good catch! I've built a unit-test case around it. I think we need to add a new private/internal diff --git a/netplan/cli/commands/try_command.py b/netplan/cli/commands/try_command.py
index 2e0b1af..20b8cbf 100644
--- a/netplan/cli/commands/try_command.py
+++ b/netplan/cli/commands/try_command.py
@@ -43,6 +43,7 @@ class NetplanTry(utils.NetplanCommand):
leaf=True)
self.configuration_changed = False
self.new_interfaces = None
+ self.config_file = None
self._config_manager = None
self.t_settings = None
self.t = None
@@ -52,7 +53,7 @@ class NetplanTry(utils.NetplanCommand):
@property
def config_manager(self): # pragma: nocover (called by later commands)
if not self._config_manager:
- self._config_manager = ConfigManager()
+ self._config_manager = ConfigManager(prefix=self._rootdir)
return self._config_manager
def clear_ready_stamp(self):
@@ -146,7 +147,7 @@ class NetplanTry(utils.NetplanCommand):
def cleanup(self): # pragma: nocover (requires user input)
self.config_manager.cleanup()
- def is_revertable(self): # pragma: nocover (requires user input)
+ def is_revertable(self):
'''
Check if the configuration is revertable, if it doesn't contain bits
that we know are likely to render the system unstable if we apply it,
@@ -161,7 +162,7 @@ class NetplanTry(utils.NetplanCommand):
'''
extra_config = []
- if self.config_file:
+ if self.config_file: # pargma: nocover
extra_config.append(self.config.file)
np_state = self.config_manager.parse(extra_config=extra_config)
revert_unsupported = []
diff --git a/tests/test_cli_units.py b/tests/test_cli_units.py
index 48fe2d9..b235aeb 100644
--- a/tests/test_cli_units.py
+++ b/tests/test_cli_units.py
@@ -34,6 +34,7 @@ class TestCLI(unittest.TestCase):
def setUp(self):
self.tmproot = tempfile.mkdtemp()
os.mkdir(os.path.join(self.tmproot, 'run'))
+ os.makedirs(os.path.join(self.tmproot, 'etc/netplan'))
os.environ['DBUS_TEST_NETPLAN_ROOT'] = self.tmproot
def tearDown(self):
@@ -103,3 +104,28 @@ class TestCLI(unittest.TestCase):
self.assertTrue(os.path.isfile(stamp_file))
self.assertTrue(cmd.clear_ready_stamp())
self.assertFalse(os.path.isfile(stamp_file))
+
+ def test_netplan_try_is_revertable(self):
+ with open(os.path.join(self.tmproot, 'etc/netplan/test.yaml'), 'w') as f:
+ f.write('''network:
+ bridges:
+ br54:
+ dhcp4: false
+''')
+ cmd = NetplanTry()
+ self.assertTrue(cmd.is_revertable())
+
+ def test_netplan_try_is_not_revertable(self):
+ with open(os.path.join(self.tmproot, 'etc/netplan/test.yaml'), 'w') as f:
+ f.write('''network:
+ ethernets:
+ eth0:
+ dhcp4: true
+ bonds:
+ bn0:
+ interfaces: [eth0]
+ parameters:
+ mode: balance-rr
+''')
+ cmd = NetplanTry()
+ self.assertFalse(cmd.is_revertable()) |
Contributor
Author
|
Eh, I guess we finally found out why there was this I guess this code must have regressed during one of the many rebases and reworks of the patchset. I'll fix this in a jiffy, along with the tests you've nicely written. |
Instead of forcing users to cast (which was done through a macro in netplan.c anyway), we now use the proper type to say "give us a pointer to whatever, we promise we won't touch its data". This function will be used to expose a bit of the dirtiness of the bonds and bridges properties to Python, which in turn will tell us how revertable a given config is.
It marked the wrong pointer, instead marking the address at the end of the input string as dirty, which doesn't make much sense (and would be prone to conflicts down the line as allocations are reused).
The underlying implementation uses the dirtiness infrastructure for this, as this allows for a rather generic implementation, and shows us that there are actual parameters touched by the user config, whereas the explicit boolean `custom_bridging` only tells us that there's a parameters block, which might very well be empty.
The code hadn't been fully migrated to the new libnetplan-backed configmanager. This commit also adds some unit tests for this function, which were sorely lacking. Co-authored-by: Lukas Märdian <lukas.maerdian@canonical.com>
schopin-pro
force-pushed
the
yaml-consolidation-full
branch
from
86b427d to
d8b25d0
Compare
Contributor
Author
|
The issue should be fixed. Thanks for the tests, @mvo5! Not only was this particular CLI codepath buggy, but fixing it led me to uncover another issue deep in the parsing code that's now fixed in the branch as well. |
Contributor
|
I've updated the PPA and built a core22 base snap & qemu image, as described by mvo above and ran the tests manually (from https://github.com/snapcore/snapd/blob/master/tests/core/netplan-cfg/task.yaml). All passed, looking good to me! |
Contributor
|
mvo replied out-of-band that there's nothing else to be tested from the snapd side. And Simon's latest changes LGTM. Merging. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This patch set, the last one in the YAML consolidation series, ports ConfigManager to use libnetplan's YAML parser. In order to do so, we had to expose a lot of the internal data of the Netplan state to the Python bindings, as the
netplan applylogic needs to inspect the definitions to know what to do.Checklist
make checksuccessfully.make check-coverage).