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

Sysctl library #99

Merged
merged 17 commits into from
Jul 26, 2023
295 changes: 279 additions & 16 deletions lib/charms/operator_libs_linux/v0/sysctl.py
Original file line number Diff line number Diff line change
@@ -1,24 +1,84 @@
"""TODO: Add a proper docstring here.
# Copyright 2023 Canonical Ltd.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

This is a placeholder docstring for this charm library. Docstrings are
presented on Charmhub and updated whenever you push a new version of the
library.
"""Handler for the sysctl config.

Complete documentation about creating and documenting libraries can be found
in the SDK docs at https://juju.is/docs/sdk/libraries.
This library allows your charm to create and update sysctl config options to the machine.

See `charmcraft publish-lib` and `charmcraft fetch-lib` for details of how to
share and consume charm libraries. They serve to enhance collaboration
between charmers. Use a charmer's libraries for classes that handle
integration with their charm.
Validation and merge capabilities are added, for situations where more than one application
are setting values. The following files can be created:

Bear in mind that new revisions of the different major API versions (v0, v1,
v2 etc) are maintained independently. You can continue to update v0 and v1
after you have pushed v3.
- /etc/sysctl.d/90-juju-<app-name>
Requirements from one application requesting to update the values.

Markdown is supported, following the CommonMark specification.
- /etc/sysctl.d/95-juju-sysctl.conf
Merged file resulting from all other `90-juju-*` application files.


A charm using the sysctl lib will need a data structure like the following:
```yaml
vm.swappiness:
value: 1
vm.max_map_count:
value: 262144
vm.dirty_ratio:
value: 80
vm.dirty_background_ratio:
value: 5
net.ipv4.tcp_max_syn_backlog:
value: 4096
```

Now, it can use that template within the charm, or just declare the values directly:

```python
from charms.operator_libs_linux.v0 import sysctl

class MyCharm(CharmBase):

def __init__(self, *args):
...
self.sysctl = sysctl.Config(self.meta.name)

self.framework.observe(self.on.install, self._on_install)
self.framework.observe(self.on.remove, self._on_remove)

def _on_install(self, _):
# Altenatively, read the values from a template
sysctl_data = {"net.ipv4.tcp_max_syn_backlog": {"value": 4096}}

try:
self.sysctl.update(config=sysctl_data)
except (sysctl.SysctlPermissionError, sysctl.ValidationError) as e:
logger.error(f"Error setting values on sysctl: {e.message}")
self.unit.status = BlockedStatus("Sysctl config not possible")
except sysctl.SysctlError:
logger.error("Error on sysctl")

def _on_remove(self, _):
self.sysctl.remove()
```
"""

import logging
import re
from pathlib import Path
from subprocess import STDOUT, CalledProcessError, check_output
from typing import Dict, List

logger = logging.getLogger(__name__)

# The unique Charmhub library identifier, never change it
LIBID = "17a6cd4d80104d15b10f9c2420ab3266"

Expand All @@ -27,6 +87,209 @@

# Increment this PATCH version before using `charmcraft publish-lib` or reset
# to 0 if you are raising the major API version
LIBPATCH = 1
LIBPATCH = 2

CHARM_FILENAME_PREFIX = "90-juju-"
SYSCTL_DIRECTORY = Path("/etc/sysctl.d")
SYSCTL_FILENAME = 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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

# configurations into a single file like.
"""


class Error(Exception):
"""Base class of most errors raised by this library."""

@property
def message(self):
"""Return the message passed as an argument."""
return self.args[0]


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


class ApplyError(Error):
"""Raised when there's an error applying values in sysctl."""


class ValidationError(Error):
"""Exception representing value validation error."""


class Config(Dict):
"""Represents the state of the config that a charm wants to enforce."""

_apply_re = re.compile(r"sysctl: permission denied on key \"([a-z_\.]+)\", ignoring$")

def __init__(self, name: str) -> None:
self.name = name
self._data = self._load_data()

def __contains__(self, key: str) -> bool:
"""Check if key is in config."""
return key in self._data

def __len__(self):
"""Get size of config."""
return len(self._data)

def __iter__(self):
"""Iterate over config."""
return iter(self._data)

def __getitem__(self, key: str) -> str:
"""Get value for key form config."""
return self._data[key]

@property
def charm_filepath(self) -> Path:
"""Name for resulting charm config file."""
return SYSCTL_DIRECTORY / f"{CHARM_FILENAME_PREFIX}{self.name}"

def update(self, config: Dict[str, dict]) -> None:
"""Update sysctl config options with a desired set of config params.

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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)}.

```
"""
self._parse_config(config)

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?


conflict = self._validate()
if conflict:
raise ValidationError(f"Validation error for keys: {conflict}")

snapshot = self._create_snapshot()
logger.debug("Created snapshot for keys: %s", snapshot)
try:
self._apply()
except ApplyError:
self._restore_snapshot(snapshot)
raise

self._create_charm_file()
self._merge()

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

The removal process won't apply any sysctl configuration. It will only merge files from
remaining charms.
"""
self.charm_filepath.unlink(missing_ok=True)
logger.info("Charm config file %s was removed", self.charm_filepath)
self._merge()

def _validate(self) -> List[str]:
"""Validate the desired config params against merged ones."""
common_keys = set(self._data.keys()) & set(self._desired_config.keys())
conflict_keys = []
for key in common_keys:
if self._data[key] != self._desired_config[key]:
benhoyt marked this conversation as resolved.
Show resolved Hide resolved
logger.warning(
"Values for key '%s' are different: %s != %s",
key,
self._data[key],
self._desired_config[key],
)
conflict_keys.append(key)

return conflict_keys

def _create_charm_file(self) -> None:
"""Write the charm file."""
charm_params = [f"# {self.name}\n"] + [
f"{key}={value}\n" for key, value in self._desired_config.items()
]
with open(self.charm_filepath, "w") as f:
f.writelines(charm_params)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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")


def _merge(self, add_own_charm=True) -> None:
"""Create the merged sysctl file.

Args:
add_own_charm : bool, if false it will skip the charm file from the merge.
"""
# get all files that start by 90-juju-
data = [SYSCTL_HEADER]
paths = set(SYSCTL_DIRECTORY.glob(f"{CHARM_FILENAME_PREFIX}*"))
if not add_own_charm:
paths.discard(self.charm_filepath.as_posix())

for path in paths:
with open(path, "r") as f:
data += f.readlines()
with open(SYSCTL_FILENAME, "w") as f:
f.writelines(data)
Comment on lines +225 to +229
Copy link
Contributor

Choose a reason for hiding this comment

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

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())


# Reload data with newly created file.
self._data = self._load_data()

def _apply(self) -> None:
"""Apply values to machine."""
cmd = [f"{key}={value}" for key, value in self._desired_config.items()]
result = self._sysctl(cmd)
failed_values = [
self._apply_re.match(line) for line in result if self._apply_re.match(line)
]
logger.debug("Failed values: %s", failed_values)

if failed_values:
msg = f"Unable to set params: {[f.group(1) for f in failed_values]}"
logger.error(msg)
raise ApplyError(msg)

def _create_snapshot(self) -> Dict[str, str]:
"""Create a snaphot of config options that are going to be set."""
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: snaphot -> snapshot

cmd = ["-n"] + list(self._desired_config.keys())
values = self._sysctl(cmd)
return dict(zip(list(self._desired_config.keys()), values))

def _restore_snapshot(self, snapshot: Dict[str, str]) -> None:
"""Restore a snapshot to the machine."""
values = [f"{key}={value}" for key, value in snapshot.items()]
self._sysctl(values)

def _sysctl(self, cmd: List[str]) -> List[str]:
"""Execute a sysctl command."""
cmd = ["sysctl"] + cmd
logger.debug("Executing sysctl command: %s", cmd)
try:
return check_output(cmd, stderr=STDOUT, universal_newlines=True).splitlines()
except CalledProcessError as e:
msg = f"Error executing '{cmd}': {e.stdout}"
logger.error(msg)
raise CommandError(msg)

def _parse_config(self, config: Dict[str, dict]) -> None:
"""Parse a config passed to the lib."""
result = {}
for key, value in config.items():
result[key] = str(value["value"])
self._desired_config: Dict[str, str] = result
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: good use case for a dictionary comprehension:

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


def _load_data(self) -> Dict[str, str]:
"""Get merged config."""
config = {}
if not SYSCTL_FILENAME.exists():
return config

with open(SYSCTL_FILENAME, "r") as f:
for line in f:
if line.startswith(("#", ";")) or not line.strip() or "=" not in line:
continue

key, _, value = line.partition("=")
Copy link
Contributor

Choose a reason for hiding this comment

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

not-blocking:

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

config[key.strip()] = value.strip()

# TODO: add your code here! Happy coding!
return config
Loading