patch: advanced rolling ops using etcd by patriciareinoso · Pull Request #364 · canonical/charmlibs

patriciareinoso · 2026-03-16T20:00:25Z

Context

All the code on this PR is new

This implementation is based on DA241 Spec

The content of charmlibs/advanced-rollingops/src/charmlibs/advanced_rollingops/_dp_interfaces_v1.py belongs to another library that is currently being migrated to charmlibs so you can ignore it for now.

Summary

This PR is the first part of the implementation of advanced rolling ops at cluster level.

This PR includes:

Management of the client certificate used to connect to etcd
- The leader unit creates a self-signed certificate with live of 20 years
- Share the certificate with the other units using a peer relation
Implementation of the integration with etcd
- Share the mtls certificate
- Observe the resource_created event
- Observe the endpoints_changed event
Management of a env file needed to connecto etcd via etcdctl

This PR does not implement the locking mechanism. In here we only test that we can connect to etcd from each unit.

Current workflow:

The unit make a request
A new background process is spawn
The background process dispatches a Juju hook
The unit observes that hook
The unit writes and read a value in etcd
If the unit was able to connect to etcd, it executes the "restart" function.

This is a very simplified workflow to be able to test that the units from different apps can reach etcd.

To do

Replace the content of charmlibs/advanced-rollingops/src/charmlibs/advanced_rollingops/_dp_interfaces_v1.py with the the actual library once it is migrated to charmlibs
Implement the actual locking mechanism
Figure out how to properly install etcdctl

advanced-rollingops/tests/integration/test_etcd_rolling_ops.py

james-garner-canonical · 2026-03-16T20:11:43Z

Hi @patriciareinoso thanks for opening this PR. I haven't looked through the code yet, but I wanted to start with a request for some additional context first.

Please update the PR to describe whether this is a migration of an existing codebase or should be treated as all new. If it's a migration, please link to the existing library, and describe what's changing/open to change. If there was a spec involved in the design of this new lib, please also link to it.

You've probably already seen it, but I just want to also drop a link to our migration guide https://documentation.ubuntu.com/charmlibs/how-to/migrate/

If you'd like to talk about any of this synchronously, feel free to set up a meeting, my availability is in my calendar. Otherwise we'll proceed with asynchronous review when we have the context.

PS: If the PR isn't ready for review yet, you can mark it as a draft and manually re-request review when ready.

Gu1nness

Mostly questions around security and try/catch errors.
Excellent work!

advanced-rollingops/src/charmlibs/advanced_rollingops/__init__.py

rollingops/src/charmlibs/rollingops/_certificates.py

Gu1nness · 2026-03-17T13:16:26Z

rollingops/src/charmlibs/rollingops/_certificates.py

+
+        ca_key = PrivateKey.generate(key_size=4096)
+        ca_attributes = CertificateRequestAttributes(
+            common_name='rollingops-client-ca', is_ca=True


Shouldn't that be common_name ? Or something decided by the charm integrating ?
Or is it because it can't be chosen in a distributed way ?

In addition to Neha's comment on the common_name, it might be worth adding add_unique_id_to_subject_name=False to the attributes.

rollingops/src/charmlibs/rollingops/_certificates.py

Gu1nness · 2026-03-17T14:38:10Z

rollingops/src/charmlibs/rollingops/_manager.py

Pinning to this file but it's a more generic question: Is is possible for a unit to request a lock for another unit by "hacking" the etcdctl calls that request a lock ? What would be the consequences ?
Is there a way to protect from that ?

Gu1nness · 2026-03-17T14:43:09Z

rollingops/src/charmlibs/rollingops/_etcdctl.py

+        os.chmod(cls.ENV_FILE, 0o600)
+
+    @classmethod
+    def load_env(cls) -> dict[str, str]:


That function scares me.
I trust that it probably works but it does scare me that we're trying to parse some exports that way.
Why do we need to go through a dedicated file to do that ?
It seems to me that we're writing to the file just to read it as an environment provider for the command runner.

Isn't there another way to do that?

The parameters that are passed as env=cls.load_env() to the subprocess command could instead be appended to the command itself (example).

This would eliminate the need to write and load an env file, but would require access to relation data at runtime.

Gu1nness · 2026-03-17T14:45:28Z

rollingops/src/charmlibs/rollingops/_manager.py

+    def _on_install(self, event: InstallEvent) -> None:
+        subprocess.run(['apt-get', 'update'], check=True)
+        subprocess.run(['apt-get', 'install', '-y', 'etcd-client'], check=True)


Use https://github.com/canonical/charmlibs/tree/main/apt ?
Not sure this would work in airgapped env also.
Maybe good as a first start but that raises the question of migration in the future.

advanced-rollingops/src/charmlibs/advanced_rollingops/_worker.py

Gu1nness · 2026-03-17T14:52:41Z

rollingops/src/charmlibs/rollingops/_manager.py

+        """Stop the etcd worker process in the current unit."""
+        unit = event.departing_unit
+        if unit == self.model.unit:
+            self.worker.stop()


So to be sure:

worker spawns one process and dies

Process triggers one dispatch and dies

Dispatch from the process prevents this event (relation-departed) from running (because only one event at a time).
Is it to cover race conditions ? Or are the workers incomplete for now ?

Gu1nness · 2026-03-17T15:15:36Z

Also I think you need to add a changelog file (cf https://documentation.ubuntu.com/charmlibs/explanation/charmlibs-publishing/ )

#364 has revealed that CI breaks with packages with a `requires-python` lower bound that's higher than 3.10 (it has `requires-python>=3.12`). This is because we would run `just integration-k8s` and `just integration-machine` in the `init` step to check if they have any tests, and that uses our default `justfile` Python version of 3.10. This PR fixes this by recording the minimum supported Python version when we calculate the Python versions to test the library with, and using that version in the failing step.

james-garner-canonical · 2026-03-17T23:23:34Z

CI failure should be fixed in #366

CODEOWNERS

advanced-rollingops/pyproject.toml

rollingops/src/charmlibs/rollingops/_manager.py

…E-9349-rolling-ops

james-garner-canonical · 2026-03-18T12:22:53Z

Integration tests should be fixed for real now with #373 merged, please merge main to pull in the fix.

reneradoi

In general the implementation looks good for the scope of the PR. I've left a few comments, mostly about error handling, and a few questions.

reneradoi · 2026-03-18T10:27:45Z

rollingops/src/charmlibs/rollingops/_certificates.py

+
+        ca_key = PrivateKey.generate(key_size=4096)
+        ca_attributes = CertificateRequestAttributes(
+            common_name='rollingops-client-ca', is_ca=True


In addition to Neha's comment on the common_name, it might be worth adding add_unique_id_to_subject_name=False to the attributes.

reneradoi · 2026-03-18T10:30:58Z

rollingops/src/charmlibs/rollingops/_certificates.py

+        client_key = PrivateKey.generate(key_size=4096)
+
+        csr_attributes = CertificateRequestAttributes(
+            common_name=common_name, add_unique_id_to_subject_name=False


I would recommend to add Subject Alternative Names, as certificate verification based on common name has been deprecated.

reneradoi · 2026-03-18T11:37:02Z

rollingops/src/charmlibs/rollingops/_etcdctl.py

+        os.chmod(cls.ENV_FILE, 0o600)
+
+    @classmethod
+    def load_env(cls) -> dict[str, str]:


The parameters that are passed as env=cls.load_env() to the subprocess command could instead be appended to the command itself (example).

This would eliminate the need to write and load an env file, but would require access to relation data at runtime.

reneradoi · 2026-03-18T11:39:46Z

rollingops/src/charmlibs/rollingops/_etcdctl.py

+        )
+
+    @classmethod
+    def get_first_key_value(cls, key_prefix: str) -> tuple[str, dict[str, str]] | None:


This doesn't seem to be used - what is it needed for?

reneradoi · 2026-03-18T12:06:28Z

rollingops/src/charmlibs/rollingops/_worker.py

+
+        pid = int(pid_str)
+        try:
+            os.kill(pid, signal.SIGINT)


Should we use SIGTERM instead, as SIGINT usually means keyboard interrupt?

reneradoi · 2026-03-18T12:39:10Z

rollingops/src/charmlibs/rollingops/_relations.py

+        app_data = relation.data[self.model.app]
+        secret_id = app_data.get(SECRET_FIELD)
+        if secret_id:
+            return


Can be simplified with := and would be nice to have a comment here to understand that this means the certificate already exists.

reneradoi · 2026-03-18T12:44:01Z

rollingops/src/charmlibs/rollingops/_relations.py

+            logger.error('No etcd endpoints available')
+            return
+
+        self.shared_certificates.sync_to_local_files()


Why updating certs if endpoints on etcd side have changed?

reneradoi · 2026-03-18T12:50:21Z

rollingops/src/charmlibs/rollingops/_etcdctl.py

+        """
+        cls.BASE_DIR.mkdir(parents=True, exist_ok=True)
+
+        cls.SERVER_CA.write_text(tls_ca_pem or '')


I wouldn't do or '' to be honest. It means silently continuing in a situation that is problematic, rather make it apparent that there is no CA.

reneradoi · 2026-03-18T12:56:18Z

rollingops/src/charmlibs/rollingops/_etcdctl.py

+        cls.ENV_FILE.unlink(missing_ok=True)
+
+    @classmethod
+    def run(


General comment on the subprocess.run command: This might raise a CalledProcessError if check is enabled, these errors should be caught.

I think it is in general a code practice to always have check=true and catch the error instead of checking the output of the run command (see also in the other methods of this class).

reneradoi · 2026-03-18T12:58:01Z

rollingops/src/charmlibs/rollingops/_etcdctl.py

+        return env
+
+    @classmethod
+    def ensure_initialized(cls):


As this method may raise, we should make sure to catch the errors and act appropriately where it is called, otherwise this could result in uncaught errors at runtime.

dimaqq · 2026-03-19T04:35:16Z

rollingops/src/charmlibs/rollingops/_dp_interfaces_v1.py

+try:
+    import psycopg2
+except ImportError:
+    psycopg2 = None


I wonder about vendoring this library as is.

From what I can see, postgres is not used by the rolling ops functionality, and this import is referenced in a method that's not ultimately used.

patriciareinoso added 2 commits March 16, 2026 15:39

patch: etcd rolling ops version

70a4cca

first working version

b32878c

patriciareinoso requested a review from a team as a code owner March 16, 2026 20:00

patriciareinoso requested a review from james-garner-canonical March 16, 2026 20:00

patriciareinoso commented Mar 16, 2026

View reviewed changes

advanced-rollingops/tests/integration/test_etcd_rolling_ops.py Outdated Show resolved Hide resolved

patriciareinoso commented Mar 16, 2026

View reviewed changes

advanced-rollingops/tests/integration/test_etcd_rolling_ops.py Outdated Show resolved Hide resolved

patriciareinoso added 4 commits March 17, 2026 09:09

fix format

bf58a2d

fix linting

4d99740

add tenacity to integration test

a29b85d

remove unnecessary logs

de57674

Gu1nness requested changes Mar 17, 2026

View reviewed changes

add dataplatform as reviewes

effafe4

james-garner-canonical mentioned this pull request Mar 17, 2026

ci: support libs with requires-python higher than 3.10 #366

Merged

james-garner-canonical reviewed Mar 18, 2026

View reviewed changes

CODEOWNERS Outdated Show resolved Hide resolved

james-garner-canonical reviewed Mar 18, 2026

View reviewed changes

advanced-rollingops/pyproject.toml Outdated Show resolved Hide resolved

patriciareinoso requested review from Mehdi-Bendriss, reneradoi and skourta March 18, 2026 09:03

rename and add integration tests

3948b94

patriciareinoso commented Mar 18, 2026

View reviewed changes

rollingops/src/charmlibs/rollingops/_manager.py Show resolved Hide resolved

patriciareinoso added 2 commits March 18, 2026 12:04

Merge branch 'main' of https://github.com/canonical/charmlibs into DP…

1726070

…E-9349-rolling-ops

linting and rebase

b4f700c

james-garner-canonical mentioned this pull request Mar 18, 2026

ci: fix min Python version in integration tests #373

Merged

reneradoi reviewed Mar 18, 2026

View reviewed changes

dimaqq reviewed Mar 19, 2026

View reviewed changes

Conversation

patriciareinoso commented Mar 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Summary

Current workflow:

To do

Uh oh!

Uh oh!

Uh oh!

james-garner-canonical commented Mar 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Gu1nness left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Gu1nness commented Mar 17, 2026

Uh oh!

james-garner-canonical commented Mar 17, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

james-garner-canonical commented Mar 18, 2026

Uh oh!

reneradoi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

patriciareinoso commented Mar 16, 2026 •

edited

Loading

james-garner-canonical commented Mar 16, 2026 •

edited

Loading