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

Replace snap channel config for hard coded revision config #79

Merged
merged 4 commits into from
Apr 24, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 6 additions & 8 deletions charms/worker/charmcraft.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,6 @@ bases:
architectures: [amd64]
config:
options:
channel:
default: edge
type: string
description: Snap channel of the k8s snap
Comment on lines -45 to -48
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, not going to use snap channels anymore

labels:
default: ""
type: string
Expand All @@ -59,15 +55,17 @@ parts:
charm:
build-packages: [git]
charm-entrypoint: k8s/src/charm.py
lib:
# move the ./k8s/lib path to ./lib since
# charmcraft assumes it to be there once the charm runs
promote:
# move paths out of ./k8s to ./ since
# charmcraft assumes ./lib to be there
# charmcraft assumes ./templates to be there
after: [charm]
plugin: nil
source: ./
override-prime: |
rm -rf $CRAFT_PRIME/lib
rm -rf $CRAFT_PRIME/lib $CRAFT_PRIME/templates
mv $CRAFT_PRIME/k8s/lib $CRAFT_PRIME/lib
mv $CRAFT_PRIME/k8s/templates $CRAFT_PRIME/templates
Comment on lines -69 to +68
Copy link
Contributor Author

@addyess addyess Apr 24, 2024

Choose a reason for hiding this comment

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

This moves the templates folder from ./k8s/templates to ./templates once installed on the k8s-worker units, along with the ./lib folder


provides:
cos-agent:
Expand Down
4 changes: 0 additions & 4 deletions charms/worker/k8s/charmcraft.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -51,10 +51,6 @@ bases:
architectures: [amd64]
config:
options:
channel:
default: edge
type: string
description: Snap channel of the k8s snap
Comment on lines -54 to -57
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, revisions are hard coded

datastore:
default: dqlite
type: string
Expand Down
15 changes: 7 additions & 8 deletions charms/worker/k8s/src/charm.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
from urllib.parse import urlparse

import charms.contextual_status as status
import charms.operator_libs_linux.v2.snap as snap_lib
import ops
import yaml
from charms.contextual_status import WaitingStatus, on_error
Expand All @@ -50,10 +51,9 @@
)
from charms.kubernetes_libs.v0.etcd import EtcdReactiveRequires
from charms.node_base import LabelMaker
from charms.operator_libs_linux.v2.snap import SnapError, SnapState
from charms.operator_libs_linux.v2.snap import ensure as snap_ensure
from charms.reconciler import Reconciler
from cos_integration import COSIntegration
from snap import management as snap_management
from token_distributor import ClusterTokenType, TokenCollector, TokenDistributor, TokenStrategy

# Log messages can be retrieved using juju debug-log
Expand Down Expand Up @@ -198,12 +198,11 @@ def get_cloud_name(self) -> str:
"""
return self.xcp.name or ""

@on_error(ops.BlockedStatus("Failed to install k8s snap."), SnapError)
def _install_k8s_snap(self):
"""Install the k8s snap package."""
@on_error(ops.BlockedStatus("Failed to install snaps."), snap_lib.SnapError)
def _install_snaps(self):
"""Install snap packages."""
status.add(ops.MaintenanceStatus("Ensuring snap installation"))
log.info("Ensuring k8s snap version")
snap_ensure("k8s", SnapState.Latest.value, self.config["channel"])
snap_management()
Comment on lines -201 to +205
Copy link
Contributor Author

Choose a reason for hiding this comment

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

let's move all the logic about managing the snap revision to another module


@on_error(WaitingStatus("Waiting to apply snap requirements"), subprocess.CalledProcessError)
def _apply_snap_requirements(self):
Expand Down Expand Up @@ -527,7 +526,7 @@ def _reconcile(self, event):
return

self._apply_proxy_environment()
self._install_k8s_snap()
self._install_snaps()
self._apply_snap_requirements()
self._check_k8sd_ready()
if self.lead_control_plane:
Expand Down
117 changes: 117 additions & 0 deletions charms/worker/k8s/src/snap.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
#!/usr/bin/env python3

# Copyright 2024 Canonical Ltd.
# See LICENSE file for licensing details.

# Learn more at: https://juju.is/docs/sdk

"""Snap Installation Module."""


import logging
import subprocess
from pathlib import Path
from typing import List, Literal, Optional, Union

import charms.operator_libs_linux.v2.snap as snap_lib
import yaml
from pydantic import BaseModel, Field, ValidationError, parse_obj_as
from typing_extensions import Annotated

# Log messages can be retrieved using juju debug-log
log = logging.getLogger(__name__)


class SnapFileArgument(BaseModel):
"""Structure to install a snap by file.

Attributes:
install_type (str): literal string defining this type
name (str): The name of the snap after installed
filename (Path): Path to the snap to locally install
classic (bool): If it should be installed as a classic snap
dangerous (bool): If it should be installed as a dangerouse snap
devmode (bool): If it should be installed as with dev mode enabled
"""

install_type: Literal["file"] = Field("file", alias="install-type", exclude=True)
name: str = Field(exclude=True)
filename: Optional[Path] = None
classic: Optional[bool] = None
devmode: Optional[bool] = None
dangerous: Optional[bool] = None
Comment on lines +25 to +42
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This looks like all the arguments in snap_lib.install_local(...) All the names and type mimic those arguments intentionally.

two arguments name and install_type do not exist as arguments to that method -- so they are marked with exclude=True. When this.dict(...) is called -- those arguments won't show up in the call



class SnapStoreArgument(BaseModel):
"""Structure to install a snap by snapstore.

Attributes:
install_type (str): literal string defining this type
name (str): The type of the request.
state (SnapState): a `SnapState` to reconcile to.
classic (bool): If it should be installed as a classic snap
devmode (bool): If it should be installed as with dev mode enabled
channel (bool): the channel to install from
cohort (str): the key of a cohort that this snap belongs to
revision (int): the revision of the snap to install
"""

install_type: Literal["store"] = Field("store", alias="install-type", exclude=True)
name: str = Field(exclude=True)
classic: Optional[bool] = None
devmode: Optional[bool] = None
state: Optional[snap_lib.SnapState] = Field(snap_lib.SnapState.Present)
channel: Optional[str] = None
cohort: Optional[str] = None
revision: Optional[int] = None
Comment on lines +45 to +66
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This looks like all the arguments in snap_lib.Snap.ensure(...) All the names and types mimic those arguments intentionally.

Again, the two arguments name and install_type do not exist as arguments to that method -- so they are marked with exclude=True. When this.dict(...) is called -- those arguments won't show up in the call



SnapArgument = Annotated[
Union[SnapFileArgument, SnapStoreArgument], Field(discriminator="install_type")
Comment on lines +69 to +70
Copy link
Member

Choose a reason for hiding this comment

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

Nice! I love the discriminator logic here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is what we have to work with in pydantic 1.x. This has been changed in pydantic 2.x a bit
https://docs.pydantic.dev/latest/migration/#introduction-of-typeadapter

]
Comment on lines +69 to +71
Copy link
Contributor Author

@addyess addyess Apr 24, 2024

Choose a reason for hiding this comment

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

This is pydantic's fancy way of creating a type which can parse data as either SnapFileArgument or SnapStoreArgument

the Field(discriminator="install_type") indicates to the parser how to determine which type of object it should be parsed as.



def _parse_management_arguments() -> List[SnapArgument]:
"""Parse snap management arguments.

Raises:
SnapError: when the management issue cannot be resolved

Returns:
Parsed arguments list for the specific host architecture
"""
revision = Path("templates/snap_installation.yaml")
if not revision.exists():
raise snap_lib.SnapError(f"Failed to find file={revision}")
try:
with revision.open() as f:
body = yaml.safe_load(f)
except yaml.YAMLError as e:
log.error("Failed to load file=%s, %s", revision, e)
raise snap_lib.SnapError(f"Failed to load file={revision}")
dpkg_arch = ["dpkg", "--print-architecture"]
arch = subprocess.check_output(dpkg_arch).decode("UTF-8").strip()
Comment on lines +92 to +93
Copy link
Contributor Author

@addyess addyess Apr 24, 2024

Choose a reason for hiding this comment

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

the snap_installation.yaml will have sections based on architecture. each architecture can specify a list of snaps to install (not just one). This lets us have the flexibility to add multiple snaps in the future if necessary. Maybe we want to add kubelet with its own set of args. Or maybe by default install k9s or ponysay.


if not (isinstance(body, dict) and (arch_spec := body.get(arch))):
log.warning("Failed to find revision for arch=%s", arch)
raise snap_lib.SnapError(f"Failed to find revision for arch={arch}")

try:
args: List[SnapArgument] = [parse_obj_as(SnapArgument, arg) for arg in arch_spec] # type: ignore[arg-type]
Copy link
Contributor Author

@addyess addyess Apr 24, 2024

Choose a reason for hiding this comment

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

so, for every arg in the architecture specification (arch_spec) we want to parse the dict object into either a SnapFileArgument or a SnapStoreArgument.

Since we made that nice annotated type we can use parse_obj_as(...) asking pydantic to figure out which object type it is.

except ValidationError as e:
log.warning("Failed to validate args=%s (%s)", arch_spec, e)
raise snap_lib.SnapError("Failed to validate snap args")

return args


def management():
"""Manage snap installations on this machine."""
cache = snap_lib.SnapCache()
for args in _parse_management_arguments():
which = cache[args.name]
if isinstance(args, SnapFileArgument) and which.revision != "x1":
snap_lib.install_local(**args.dict(exclude_none=True))
elif isinstance(args, SnapStoreArgument):
log.info("Ensuring %s snap version", args.name)
which.ensure(**args.dict(exclude_none=True))
Comment on lines +111 to +117
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This management() will get called on every reconcile hook. We need to parse the snap_installation.yaml, determine the args, and either locally install or run ensure with the given arguments. Pydantic has validated the arguments match what's in snap_lib, so we can just turn the args back into a dict, and call the methods with keyword arguments.

We don't want to keep installing the local file over and over and over, -- so this checks for the revision x1 but that might not be the cleanest way. This would limit the number of times you can do a install_local to just one revision. Maybe there's a TODO needed here

Copy link
Member

Choose a reason for hiding this comment

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

Definitely, let's add a TODO here

6 changes: 6 additions & 0 deletions charms/worker/k8s/templates/snap_installation.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
amd64:
addyess marked this conversation as resolved.
Show resolved Hide resolved
- name: k8s
channel: edge
arm64:
- name: k8s
channel: edge
2 changes: 1 addition & 1 deletion charms/worker/k8s/tests/unit/test_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ def mock_reconciler_handlers(harness):
"""
handler_names = {
"_evaluate_removal",
"_install_k8s_snap",
"_install_snaps",
"_apply_snap_requirements",
"_check_k8sd_ready",
"_join_cluster",
Expand Down
122 changes: 122 additions & 0 deletions charms/worker/k8s/tests/unit/test_snap.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
# Copyright 2024 Canonical Ltd.
# See LICENSE file for licensing details.

# Learn more about testing at: https://juju.is/docs/sdk/testing

# pylint: disable=duplicate-code,missing-function-docstring
"""Unit tests snap module."""

import io
import unittest.mock as mock
from pathlib import Path

import pytest
import snap


@mock.patch("pathlib.Path.exists", mock.Mock(return_value=False))
def test_parse_no_file():
"""Test no file exists."""
with pytest.raises(snap.snap_lib.SnapError):
snap._parse_management_arguments()


@mock.patch("pathlib.Path.exists", mock.Mock(return_value=True))
@mock.patch("pathlib.Path.open")
def test_parse_invalid_file(mock_open):
"""Test file is invalid."""
mock_open().__enter__.return_value = io.StringIO("example: =")
with pytest.raises(snap.snap_lib.SnapError):
snap._parse_management_arguments()


@mock.patch("pathlib.Path.exists", mock.Mock(return_value=True))
@mock.patch("pathlib.Path.open")
@mock.patch("subprocess.check_output")
def test_parse_invalid_arch(mock_checkoutput, mock_open):
"""Test file has invalid arch."""
mock_open().__enter__.return_value = io.StringIO("{}")
mock_checkoutput().decode.return_value = "amd64"
with pytest.raises(snap.snap_lib.SnapError):
snap._parse_management_arguments()


@mock.patch("pathlib.Path.exists", mock.Mock(return_value=True))
@mock.patch("pathlib.Path.open")
@mock.patch("subprocess.check_output")
def test_parse_validation_error(mock_checkoutput, mock_open):
"""Test file cannot be parsed."""
mock_open().__enter__.return_value = io.StringIO("amd64:\n- {}")
mock_checkoutput().decode.return_value = "amd64"
with pytest.raises(snap.snap_lib.SnapError):
snap._parse_management_arguments()


@mock.patch("pathlib.Path.exists", mock.Mock(return_value=True))
@mock.patch("pathlib.Path.open")
@mock.patch("subprocess.check_output")
def test_parse_valid_store(mock_checkoutput, mock_open):
"""Test file parses as store content."""
content = """
amd64:
- install-type: store
name: k8s
channel: edge
"""
mock_open().__enter__.return_value = io.StringIO(content)
mock_checkoutput().decode.return_value = "amd64"
args = snap._parse_management_arguments()
assert args == [
snap.SnapStoreArgument(name="k8s", channel="edge"),
]


@mock.patch("pathlib.Path.exists", mock.Mock(return_value=True))
@mock.patch("pathlib.Path.open")
@mock.patch("subprocess.check_output")
def test_parse_valid_file(mock_checkoutput, mock_open):
"""Test file parses as file content."""
content = """
amd64:
- install-type: file
name: k8s
filename: path/to/thing
"""
mock_open().__enter__.return_value = io.StringIO(content)
mock_checkoutput().decode.return_value = "amd64"
args = snap._parse_management_arguments()
assert args == [
snap.SnapFileArgument(name="k8s", filename=Path("path/to/thing")),
]


@mock.patch("snap._parse_management_arguments")
@mock.patch("snap.snap_lib.install_local")
@mock.patch("snap.snap_lib.SnapCache")
def test_management_installs_local(cache, install_local, args):
"""Test installer uses local installer."""
cache.return_value.__getitem__.return_value = mock.MagicMock(spec=snap.snap_lib.Snap)
args.return_value = [
snap.SnapFileArgument(name="k8s", filename=Path("path/to/thing")),
]
snap.management()
cache.called_once_with()
cache["k8s"].ensure.assert_not_called()
install_local.assert_called_once_with(filename=Path("path/to/thing"))


@mock.patch("snap._parse_management_arguments")
@mock.patch("snap.snap_lib.install_local")
@mock.patch("snap.snap_lib.SnapCache")
def test_management_installs_store(cache, install_local, args):
"""Test installer uses store installer."""
cache.return_value.__getitem__.return_value = mock.MagicMock(spec=snap.snap_lib.Snap)
args.return_value = [
snap.SnapStoreArgument(name="k8s", channel="edge"),
]
snap.management()
cache.called_once_with()
install_local.assert_not_called()
cache()["k8s"].ensure.assert_called_once_with(
state=snap.snap_lib.SnapState.Present, channel="edge"
)
Loading