feat: add etcd lock and single API orchestration

[patriciareinoso]

Summary

This PR introduces the etcd sync and async locking mechanism for rolling ops together with a single API that fallbacks to the peer-relation solution in case of etcd failure or unavailability.

Description

etcd async lock

• An background process is spawn on lock request
• It performs the lock acquisition and triggers a lock_granted juju custom hook
• The charm observes this event and executes the corresponding operation and indicates when it finished using etcd keys.

etcd sync lock

• Implemented a distributed lock using etcd (lease + transactional lock key).
• We use the :sync as the owner of the lock to differentiate from the lock trying to be acquired in the background process. Meaning that the sync lock may take priority over operations on the async lock.
• Lock acquisition uses retries (via tenacity) on etcdctl commands.
• The lock is tied to a lease and automatically released if the lease expires.

common API

• The RollingOpsManager is the public API for advanced rolling ops
• It decides on which backend the operation will run: etcd or the peer-relation
• If etcd operations fail (e.g. etcdctl errors, lease issues), the system falls back to the peer-relation-based implementation.
• This ensures operations can continue even when etcd is not usable.
• If there is any failure on the etcd background process a juju custom hook is trigger, this way the RollingOpsManager knows it needs to fallback to peer-relations
• Every operation request is:
  • written to etcd
  • also duplicated into the peer relation databag
• After execution, the operation state is updated in both places.
• This ensures:
  • no operations are lost during fallback
  • scheduling can continue seamlessly regardless of backend
• The system only switches back to etcd when the operation queue is empty
• Before resuming etcd usage any remaining etcd queue state is cleaned up. This guarantees a clean slate and avoids inconsistencies

Peer sync lock

The SyncLockBackend defines the interface that any charm author would need to implement in order to use sync locking in the context of the peer-relation solution given that using the peer-solutions we are not able to guarantee mutual exclusion when tearing down.

[Gu1nness]

Long review again, but I really love the way this is coming up!
It's mostly nitpicking and improving a bit the logging/stability.
Notes on the data modelling but I don't think we have time to rewrite the whole data modelling so I guess we'll go with that for now? Maybe except the from_string, to_string that really looks like hacking around json/poorly using json.

Nearly there!

[Gu1nness]

Note for the future if we want to improve all of our error frameworks: Each error should have a unique error code and a message so that we can log it and it would improve tracing and monitoring.

No need to do it now, just food for thoughts for the future.

Idea of implementation:

from pydantic import BaseModel, ConfigDict
from dataclasses import dataclass, field
class CommonExceptionModel(BaseModel):
	# Model config
	model_config = ConfigDict(from_attributes=True)

	# Attributes
    code: int
    message: str
    name: str


@dataclass
class CommonException(Exception):
    message: str
    code: int = 0
    name: str = field(init=False)

    def __post_init__(self):
        try:
            self.name = self.__class__.__name__
        except AttributeError:
			# For inheritance scenarios.
            pass
        super().__init__(self.message)

    def __str__(self):
        return str(self.message)

    def serialize(self):
            return CommonExceptionModel.model_validate(self).model_dump_json()

[patriciareinoso]

Nice, it could be implemented later

[Gu1nness]

@patriciareinoso About the data modelling comment, and after discussion with Mehdi I paste here a more detailed input:

I think we're hacking around json and building a lot of complexity by re-inventig data serialization/deserialization instead of using proper tools to do that.

An example that we have is:

@dataclass
class Operation:
    …
    def from_string(cls, data: str) -> Operation:
        obj = json.loads(data)
        return cls.from_dict(obj)

    def to_string() -> str:
        return json.dumps(self._to_dict(), separators=(',', ':'))
       

class OperationQueue: # Beware, not event a dataclass
    def __init__(self, operations: list[Operation] | None = None):
        self.operations: list[Operation] = list(operations or [])

    def to_string(self):
        items = [op.to_string() for op in self.operations]
        return json.dumps(items, separators=(',', ':'))

   def from_string(cls, data: str):
        try:
            items = json.loads(data)
        except:
            …
        if not isinstance(items, list) or not all(isinstance(s, str) for s in items):
            raise …
        operations = [Operation.from_string(s) for s in items]
        return cls(operations)

And that builds something weird.

Instead of having:

[
    {"callback_id": "...", "requested_at": "…",}, …
]

We have:

[
    "{\"callback_id\": \"…\", \"requested_at\": \"…\"}", …
]

This implies:

• two json deser instead of one
• Non standard format
• Custom parsing
• Risks in the future if all those nested quotes become re-escaped by something downhill

Possibilities to improve this:

1. Improve the from_string and to_string so they are are less complex, using the power of json and dataclasses (aka build proper json of nested structures).
2. (better IMHO)Use dedicated library for data serialization and deserialization that automatically does the parsing and validation (because I haven't even talked about the manual validation that we end up doing with this implem). An idea could be pydantic which is already used in the background by Data interfaces V1 on which you rely already.

[patriciareinoso]

@Gu1nness
Thank you for the review.

The main change is the use of pydantic for the Operation and OperationQueue classes.
These were the most critical since the stored object was incorrect.
This change also simplify all the / escaping when writing to etcd so thanks for that catch.

As for the rest of the databag modelling I think it can wait because the fields are simple and the change on the logic should not change the content of it. So I'll try to do it later.

While testing I caught some issues which I fixed: we were killing the process before the lease was revoke and the lock was released in case of rollingops_etcd_failed hook. And I improved the update-status reconciliation.

I will review the lease process because if the parent process die, it will not die. So that comment is not yet answered.

[Gu1nness]

This is so much better now!
Congrats!

[Gu1nness]

Nit: Why did this one slip to a hardcoded string from a constant ?

[sinclert-canonical]

👋🏻 Hey @patriciareinoso

I know we just spoke offline about the rollingops v0/v1 charmlib and its port to a standard Python package, which I assume this PR is doing. Please, let me know if that is not the case.

I wanted to share a possible improvement regarding log file locations, in the context of root-less Kubernetes charms (those where the workload container user does not have access to /var/lib, /var/log... unless explicitly granted). The improvement would be to allow the clients to specify the location for their log-file, instead of hard-coding them. I think it would make the library easier to integrate with that type of Kubernetes charms.

---

Disclaimer:

I do not know in which review stage this PR is. Feel free to completely ignore my feedback if the current approach has already been agreed upon. The last thing I want is to delay the porting of the charmlib to a standard Python package.

[patriciareinoso]

These 2 parameter should be optional.

[Mehdi-Bendriss]

Thank you Patricia! This is massive and really good work.
I have 2 points that are not blocking the merge - as it targets a feature branch:

• Will you follow with a next PR for patching setup_logging with a logfile path (and making the etcd relation name optional)?
• Can you fix the docs CI steps that are failing?

[patriciareinoso]

@Mehdi-Bendriss check #445

It answers boths questions