canonical / observability-libs

A collection of charm libraries curated by the Observability team.
https://charmhub.io/observability-libs
Apache License 2.0
3 stars 8 forks source link

Replace leader guard with is_patched guard #13

Closed sed-i closed 2 years ago

sed-i commented 2 years ago

Issue

The k8s patching had a leader guard inside on-install, which always happens before leader-elected. It's possible that juju already elected a leader by the time install fires, but it is technically incorrect to assume so. A unit does not need to be a leader to patch k8s.

Solution

Use is_patched guard instead of leader guard.

Context

This is only a partial solution so far, because we still get these kind of errors:

ERROR juju.worker.caasapplicationprovisioner.runner exited "prometheus-k8s": Operation cannot be fulfilled on pods "prometheus-k8s-0": the object has been modified; please apply your changes to the latest version and try again

Testing Instructions

Deploy e.g. prometheus with this version of the lib.

Release Notes

Replace leader guard with is_patched guard in k8s patching.

jnsgruk commented 2 years ago

Merged, thanks. I think you should have rights to publish this to Charmhub @sed-i?

sed-i commented 2 years ago

So needing to add a patch to the unittests here was an early indication that this change is going to break people's CI:

lightkube.core.exceptions.ConfigError: [Errno 2] No such file or directory: '/var/run/secrets/kubernetes.io/serviceaccount/token'
lightkube.core.exceptions.ConfigError: Configuration file ~/.kube/config not found

Revert and create v2? (Haven't published to charmhub yet.)

jnsgruk commented 2 years ago

Oh interesting. Yeh, I'm away from my computer for a bit, but if it's breaking things then definitely revert - where did you catch it?

I'm interested because in most places I've seen this, the whole class is mocked out for tests?

sed-i commented 2 years ago

where did you catch it

Prometheus unit tests. Although perhaps because I moved too quickly from v0 to v1. Checking.

sed-i commented 2 years ago

Prometheus unit tests mock methods, not the entire class, so reverting is best. Created #14 for this.

rbarry82 commented 2 years ago

So needing to add a patch to the unittests here was an early indication that this change is going to break people's CI:

lightkube.core.exceptions.ConfigError: [Errno 2] No such file or directory: '/var/run/secrets/kubernetes.io/serviceaccount/token'
lightkube.core.exceptions.ConfigError: Configuration file ~/.kube/config not found

Revert and create v2? (Haven't published to charmhub yet.)

Does this actually break "real" code where k8s/microk8s is deployed, or just unit tests?

Since lightkube itself wasn't updated, the "right" way may be catching the exception. It's happening now because Client isn't actually instantiated until patch|is_patched, but:

from lightkube.core import exceptions

...

class KubernetesServicePatch(Object):
    def _patch(self, ...):
        try:
            _client = Client()
        except exceptions.ConfigError:
            logger.warning(...)
            return
        if self.is_patched(client):
            ...