Sysctl library #99

zmraul · 2023-07-12T10:02:38Z

PoC integration with Kafka: canonical/kafka-operator#118
Addresses DPE-2111

This draft will be updated with feedback from #98 and Kafka PR before setting it to ready to review.

rgildein

I think that we could improve some small thinks, but it's overall it looks good to me.
I will also suggest to add more logging, since we are not doing much of that.

rgildein · 2023-07-20T13:58:24Z

lib/charms/operator_libs_linux/v0/sysctl.py

+
+CHARM_FILENAME_PREFIX = "90-juju-"
+SYSCTL_DIRECTORY = Path("/etc/sysctl.d")
+SYSCTL_FILENAME = Path(f"{SYSCTL_DIRECTORY}/95-juju-sysctl.conf")


not-blocking:

Suggested change

SYSCTL_FILENAME = Path(f"{SYSCTL_DIRECTORY}/95-juju-sysctl.conf")

SYSCTL_FILENAME = SYSCTL_DIRECTORY / "95-juju-sysctl.conf"

rgildein · 2023-07-20T13:59:08Z

lib/charms/operator_libs_linux/v0/sysctl.py

+SYSCTL_FILENAME = Path(f"{SYSCTL_DIRECTORY}/95-juju-sysctl.conf")
+SYSCTL_HEADER = f"""# This config file was produced by sysctl lib v{LIBAPI}.{LIBPATCH}
+#
+# This file represents the output of the sysctl lib, which can combine multiple


Are we providing paths to original files here? I could not found it in the code.

I added this as part of file creation, see this commit: 0cd9816

rgildein · 2023-07-20T14:10:52Z

lib/charms/operator_libs_linux/v0/sysctl.py

+        # NOTE: case where own charm calls update() more than once.
+        if self.charm_filepath.exists():
+            self._merge(add_own_charm=False)


I think we need to provide better description in doc-string or change logic here, because this function is called update and it's not following real update pattern. I'll try to explain it with example.
Calling update twice as shown bellow, does not ensure that 95-juju-sysctl.conf will have vm.max_map_count configured. It will be applied, but there will be no information and after reboot it will be lost.

self.sysctl.update({"vm.max_map_count": 262144}) self.sysctl.update({"vm.swappiness": 50})

From this situation I would expect that 95-juju-sysctl.conf to have both vm.max_map_count=262144 and vm.swappiness=50.

Yes, maybe update is not the correct term here. It's more a configure scenario, where we change everything, not adding.

Yes, if the behaviour here is to write an entire config, I agree with @rgildein -- this name is confusing. It's kind of like dict.update, but does something very different. What about renaming it configure, as you suggest?

rgildein · 2023-07-20T14:16:02Z

lib/charms/operator_libs_linux/v0/sysctl.py

+        self._merge()
+
+    def remove(self) -> None:
+        """Remove config for charm."""


I would prefer to add note about not applying anything (since we are not doing any change in sysctl configuration, not reverting any value) with this remove, so charm developer will not expect that after calling sysctl.remove().

Added as part of the description

rgildein · 2023-07-20T14:20:27Z

lib/charms/operator_libs_linux/v0/sysctl.py

+
+    def _create_snapshot(self) -> Dict[str, str]:
+        """Create a snaphot of config options that are going to be set."""
+        return {key: self._sysctl([key, "-n"])[0] for key in self._desired_config.keys()}


Like this we will call subprocess to many times and this can easily took 10+ seconds. Can we call it only once or load those values from 95-juju-sysctl.cong?

Simplified call into a single one. Thanks!

rgildein · 2023-07-20T14:23:40Z

lib/charms/operator_libs_linux/v0/sysctl.py

+
+        return config
+
+    def _parse_line(self, line: str) -> Dict[str, str]:


To me it would be much cleaner if this function returns Tuple[str, str] and not dictionary with single key and value.

Yes, I updated the parsing

rgildein · 2023-07-20T14:27:56Z

tests/unit/test_sysctl.py

+
+class TestSysctlConfig(unittest.TestCase):
+    def setUp(self) -> None:
+        self.desired_values = {"vm.swappiness": {"value": 0}}


this variable is never used, can we remove it

rgildein · 2023-07-20T14:35:15Z

tests/unit/test_sysctl.py

+
+
+class TestSysctlConfig(unittest.TestCase):
+    def setUp(self) -> None:


I really think that we should mocked SYSCTL_DIRECTORY (probably also SYSCTL_FILENAME) to ensure that this tests will not create such file or change it. Right now I did not spotted any place where we are not mocking load or write config, but this can easily happen in future and we should avoid touching system configuration in unit tests.

rgildein · 2023-07-20T14:40:21Z

lib/charms/operator_libs_linux/v0/sysctl.py

+        except CalledProcessError as e:
+            msg = f"Error executing '{cmd}': {e.stdout}"
+            logger.error(msg)
+            raise SysctlError(msg)


Suggested change

raise SysctlError(msg)

raise SysctlError(msg) from error

rgildein · 2023-07-20T14:41:51Z

lib/charms/operator_libs_linux/v0/sysctl.py

+    """Raised when there's an error running sysctl command."""
+
+
+class SysctlPermissionError(Error):


not-blocking: Can we stay on same page here and use ApplyError as I did in grub lib? To me it's more obvious than some permission error.

benhoyt

I haven't looked at the test thoroughly yet, but I added a few initial comments on the code.

benhoyt · 2023-07-21T05:35:01Z

lib/charms/operator_libs_linux/v0/sysctl.py

+
+    def __repr__(self):
+        """Represent the Error."""
+        return "<{}.{} {}>".format(type(self).__module__, type(self).__name__, self.args)


__repr__ is usually supposed to be in the format that could be turned back into the object if sent through eval(), in other words, the Python representation. Exception subclasses already have a nice __repr__ that does this:

>>> class FooError(Exception): pass ... >>> repr(FooError(1, 'foo')) "FooError(1, 'foo')"

So I don't think we need __repr__ here.

I'm guilty of copying the exception format from the other modules in here :) I'll change it to remove unused parts. Thanks for the explanation!

benhoyt · 2023-07-21T05:35:11Z

lib/charms/operator_libs_linux/v0/sysctl.py

+        return "<{}.{} {}>".format(type(self).__module__, type(self).__name__, self.args)
+
+    @property
+    def name(self):


Why is this needed?

benhoyt · 2023-07-21T05:37:25Z

lib/charms/operator_libs_linux/v0/sysctl.py

+        return self.args[0]
+
+
+class SysctlError(Error):


I don't think this subclass adds any value. This package is already called sysctl, so it seems like raising sysctl.Error is already enough? Or at least the name is confusing given the overlap with the package name (sysctl.SysctlError). If we need to distinguish sysctl command errors from other errors, maybe it can be sysctl.CommandError or similar?

I think CommandError might be best here

lib/charms/operator_libs_linux/v0/sysctl.py

benhoyt · 2023-07-21T05:46:22Z

lib/charms/operator_libs_linux/v0/sysctl.py

+        cmd = [f"{key}={value}" for key, value in self._desired_config.items()]
+        result = self._sysctl(cmd)
+        expr = re.compile(r"^sysctl: permission denied on key \"([a-z_\.]+)\", ignoring$")
+        failed_values = [expr.match(line) for line in result if expr.match(line)]


If you're compiling the regex every time you can just use re.match("pattern", line). But it's usually better to compile the regex just once, and save it as a class-level "constant". I usually do (at the class level):

class Config: _apply_re = re.compile("...pattern...")

Also, re.match() only matches at the start of the string, so no need for the ^ in the re.

Good to know, I'll move this to the top

benhoyt · 2023-07-21T05:49:31Z

lib/charms/operator_libs_linux/v0/sysctl.py

+        """Apply values to machine."""
+        cmd = [f"{key}={value}" for key, value in self._desired_config.items()]
+        result = self._sysctl(cmd)
+        expr = re.compile(r"^sysctl: permission denied on key \"([a-z_\.]+)\", ignoring$")


Is there a way to make sysctl fail with a non-zero exit code in this case and just use stderr in that case, instead of parsing error output?

benhoyt · 2023-07-21T05:52:10Z

lib/charms/operator_libs_linux/v0/sysctl.py

+
+    def _parse_line(self, line: str) -> Dict[str, str]:
+        """Parse a line from juju-sysctl.conf file."""
+        if line.startswith("#") or line == "\n":


According to the man page, ; is a valid command line too, so maybe line.startswith(("#", ";")). Also, maybe or not line.strip() is better than the above, to ensure a blank line with spaces in it doesn't count either.

Also, we should probably check or '=' not in line.

benhoyt · 2023-07-21T05:55:57Z

lib/charms/operator_libs_linux/v0/sysctl.py


-# TODO: add your code here! Happy coding!
+        param, value = line.split("=")


This will cause a ValueError unpacking if the value contains a =, as it will be split into more than two parts. You want param, value = line.split("=", maxsplit=1) or perhaps param, _, value = line.partition("=").

benhoyt · 2023-07-21T05:57:56Z

lib/charms/operator_libs_linux/v0/sysctl.py


-# TODO: add your code here! Happy coding!
+        param, value = line.split("=")
+        return {param.strip(): value.strip()}


It's odd and inefficient to return a one-value dictionary. A tuple (param, value) would be better. Though then you have to return None if there's no value.

Also, I don't think having this factored out into its own method, _parse_line, really helps with clarity. I think it'd be clearer just inline in load function. That would simplify the error/None/{} case too, because you'd just continue in the loop if the line was a comment or invalid.

This was done initially on _load as you point out, but it looked something like:

{ param.strip(): value.strip() for line in f.read().splitlines() if line and not line.startswith('#') for param, value in [line.split('=')] }

which is quite a mouthful :/

nvm, I merged _parse_line into load function now.

benhoyt · 2023-07-21T06:00:43Z

tests/unit/test_sysctl.py

+    @patch("builtins.open", new_callable=mock_open, read_data="vm.max_map_count=25500\n")
+    @patch("charms.operator_libs_linux.v0.sysctl.check_output")
+    @patch("charms.operator_libs_linux.v0.sysctl.Config._create_charm_file")
+    @patch("charms.operator_libs_linux.v0.sysctl.Config._load_data")


Yeah, this is a lot of mocking... I think we'd be better to just change the directory and use real file operations. We're testing a lot more that way. We can have one test with mocked file operations if you want, to ensure that we're actually writing to the right path if the path isn't overridden.

It is a lot, but I was trying to avoid creating files here. This happens more on the update call which is the one that holds most of the logic. Would adding integration tests (and leaving unit tests like this) cover your concern here?

It's more that mocking is messy and fragile. Messy, because no one wants to see six @patch decorators :-), and fragile, because we're depending a lot on implementation details, so if our implementation changes slightly, the tests break. I normally don't mock local filesystem operations in unit tests, because local filesystem operations are fast and reliable.

That said, yes, some integration tests would be very valuable for this lib -- good idea!

I'm not against so many @patch, but I still think that we should change SYSCTL_DIRECTORY to some tmp path to be sure that we will not change anything in system sysctl config and maybe also mocked all subprocess calls so unit tests will not call sysctl key=value. I meant in setUp function.

benhoyt

Thanks for the updates. The main thing I'd want to see changed is the signature of update to not use the inner {"value": x} one-key dict (and probably change the name).

I'd also much prefer the tests to avoid so much mocking of internals (and filesystem operations): if we could just mock the subprocess call and use real filesystem operations for the file handling, the tests would be more robust in the face of implementation changes, and we'd need fewer integration tests.

Let's also get some end-to-end / integration tests here. I think we only need a few, especially if we update the unit tests to use real filesystem operations.

benhoyt · 2023-07-24T14:22:19Z

lib/charms/operator_libs_linux/v0/sysctl.py

+        Args:
+            config: dictionary with keys to update:
+        ```
+        {"vm.swappiness": {"value": 10}, ...}


Hmm, this is very strange. Why isn't the value just the value of the dictionary, so cfg.update({"vm.swappiness": 10})? It's very strange to use a one-value dict with a fixed key like this.

It's a good point. This was initially done differently. The key (here vm.swappiness) could also have a rule associated with it, so we would have the subdict with value and rule keys. But that was not implemented in this version.

I would suggest making a good API for what we have now then (that is, avoid the "value" dict), and then later, if we extend it, we could use something like {"vm.swappiness": Rule(foo, bar)}.

benhoyt · 2023-07-24T14:25:28Z

lib/charms/operator_libs_linux/v0/sysctl.py

+        # NOTE: case where own charm calls update() more than once.
+        if self.charm_filepath.exists():
+            self._merge(add_own_charm=False)


Yes, if the behaviour here is to write an entire config, I agree with @rgildein -- this name is confusing. It's kind of like dict.update, but does something very different. What about renaming it configure, as you suggest?

benhoyt · 2023-07-24T14:28:46Z

lib/charms/operator_libs_linux/v0/sysctl.py

+        """Write the charm file."""
+        charm_params = [f"{key}={value}\n" for key, value in self._desired_config.items()]
+        with open(self.charm_filepath, "w") as f:
+            f.writelines(charm_params)


It's a minor point for small config files like this, but it's usually better to write a single line at a time, avoiding the whole file being instantiated in memory at once. It's also a bit simpler. Something like this:

with open(self.charm_filepath, "w") as f: for key, value in self._desired_config.items(): f.write(f"{key}={value}\n")

benhoyt · 2023-07-24T14:30:07Z

lib/charms/operator_libs_linux/v0/sysctl.py

+        result = {}
+        for key, value in config.items():
+            result[key] = str(value["value"])
+        self._desired_config: Dict[str, str] = result


Nit: good use case for a dictionary comprehension:

self._desired_config = {k: str(v) for k, v in config.items()}

benhoyt · 2023-07-24T14:33:30Z

lib/charms/operator_libs_linux/v0/sysctl.py

+            raise ApplyError(msg)
+
+    def _create_snapshot(self) -> Dict[str, str]:
+        """Create a snaphot of config options that are going to be set."""


Nit: snaphot -> snapshot

benhoyt · 2023-07-24T14:36:49Z

tests/unit/test_sysctl.py

+    @patch("builtins.open", new_callable=mock_open, read_data="vm.max_map_count=25500\n")
+    @patch("charms.operator_libs_linux.v0.sysctl.check_output")
+    @patch("charms.operator_libs_linux.v0.sysctl.Config._create_charm_file")
+    @patch("charms.operator_libs_linux.v0.sysctl.Config._load_data")


It's more that mocking is messy and fragile. Messy, because no one wants to see six @patch decorators :-), and fragile, because we're depending a lot on implementation details, so if our implementation changes slightly, the tests break. I normally don't mock local filesystem operations in unit tests, because local filesystem operations are fast and reliable.

That said, yes, some integration tests would be very valuable for this lib -- good idea!

rgildein

Two not-blocking comment + one about unit tests.

rgildein · 2023-07-24T18:41:26Z

lib/charms/operator_libs_linux/v0/sysctl.py

+        for path in paths:
+            with open(path, "r") as f:
+                data += f.readlines()
+        with open(SYSCTL_FILENAME, "w") as f:
+            f.writelines(data)


not-blocking: follow up from @benhoyt comment about creating charm file.

Suggested change

for path in paths:

with open(path, "r") as f:

data += f.readlines()

with open(SYSCTL_FILENAME, "w") as f:

f.writelines(data)

with open(SYSCTL_FILENAME, "w") as f:

f.write(SYSCTL_HEADER)

for path in paths:

with open(path, "r") as charm_file:

f.write(charm_file.readlines())

rgildein · 2023-07-24T18:49:05Z

lib/charms/operator_libs_linux/v0/sysctl.py

+                if line.startswith(("#", ";")) or not line.strip() or "=" not in line:
+                    continue
+
+                key, _, value = line.partition("=")


not-blocking:

Suggested change

key, _, value = line.partition("=")

key, value = line.split("=", 1)

rgildein · 2023-07-24T18:55:19Z

tests/unit/test_sysctl.py

+    @patch("builtins.open", new_callable=mock_open, read_data="vm.max_map_count=25500\n")
+    @patch("charms.operator_libs_linux.v0.sysctl.check_output")
+    @patch("charms.operator_libs_linux.v0.sysctl.Config._create_charm_file")
+    @patch("charms.operator_libs_linux.v0.sysctl.Config._load_data")


I'm not against so many @patch, but I still think that we should change SYSCTL_DIRECTORY to some tmp path to be sure that we will not change anything in system sysctl config and maybe also mocked all subprocess calls so unit tests will not call sysctl key=value. I meant in setUp function.

zmraul · 2023-07-25T14:14:02Z

@benhoyt

Reworked some unit tests that used excessive mocking to use actual file operations.
Added a couple integration tests.
Renamed principal function to configure and changed API data scheme to be more expected.

Thanks for the guidance here!

benhoyt

This looks a lot better to me now, thanks!

rgildein

LGTM

benhoyt · 2023-07-25T19:10:16Z

All good to merge, or does anyone else need to review this? If not, I can merge.

zmraul · 2023-07-26T07:40:01Z

All good to merge, or does anyone else need to review this? If not, I can merge.

@benhoyt I think we are good to merge. Thanks!

zmraul added 7 commits July 4, 2023 16:08

draft

842e079

add snapshot creation

9a46eed

restructure lib

9f22676

remove name validation

619467b

add unit tests and update lib

b954fda

add conditional merge option

2d3bff8

fix lint

7257317

zmraul marked this pull request as ready for review July 19, 2023 11:31

zmraul added 3 commits July 19, 2023 16:49

change type of Config value to str

aa727f2

fix typing and var name

afaffa7

fix typing on tests

fe7eb54

rgildein reviewed Jul 20, 2023

View reviewed changes

benhoyt reviewed Jul 21, 2023

View reviewed changes

zmraul added 5 commits July 21, 2023 09:34

improve error naming

e0572f7

add PR feedback

82b7ddd

remove _parse_line

651eed3

fix lint

befeda3

add file naming

0cd9816

benhoyt requested changes Jul 24, 2023

View reviewed changes

rgildein reviewed Jul 24, 2023

View reviewed changes

zmraul added 2 commits July 25, 2023 11:10

rename to configure and add feedback

c0fcd1c

remove mocks from unit tests, add integration tests

fa9fa65

zmraul requested a review from benhoyt July 25, 2023 15:03

benhoyt approved these changes Jul 25, 2023

View reviewed changes

rgildein approved these changes Jul 25, 2023

View reviewed changes

benhoyt merged commit d8e07fd into canonical:main Jul 26, 2023
4 checks passed

benhoyt changed the title ~~Sysctl init~~ Sysctl library Jul 26, 2023

dashmage mentioned this pull request Jan 10, 2024

Setting nf_conntrack under sysctl config fails. canonical/charm-sysconfig#33

Closed

	SYSCTL_FILENAME = Path(f"{SYSCTL_DIRECTORY}/95-juju-sysctl.conf")
	SYSCTL_FILENAME = SYSCTL_DIRECTORY / "95-juju-sysctl.conf"


		return config

		def _parse_line(self, line: str) -> Dict[str, str]:



		class TestSysctlConfig(unittest.TestCase):
		def setUp(self) -> None:

		"""Raised when there's an error running sysctl command."""


		class SysctlPermissionError(Error):


		# TODO: add your code here! Happy coding!
		param, value = line.split("=")

	key, _, value = line.partition("=")
	key, value = line.split("=", 1)

Sysctl library #99

Sysctl library #99

Conversation

zmraul commented Jul 12, 2023 • edited by jira bot Loading

rgildein left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benhoyt left a 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

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

benhoyt left a 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

rgildein left a 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

zmraul commented Jul 25, 2023

benhoyt left a comment

Choose a reason for hiding this comment

rgildein left a comment

Choose a reason for hiding this comment

benhoyt commented Jul 25, 2023

zmraul commented Jul 26, 2023 • edited Loading

zmraul commented Jul 12, 2023 •

edited by jira bot

Loading

zmraul commented Jul 26, 2023 •

edited

Loading