Skip to content

Conversation

@marceloneppel
Copy link
Member

@marceloneppel marceloneppel commented Apr 23, 2025

Issue

The new PG 16 charm needs 4 different storage areas to separate the different kinds of data produced by the database system. More details are explained at DPE-2603.

Solution

Modify the charm to have 4 different storages instead of 1 and adapt the charm code to configure PostgreSQL to use 3 of them (data, logs and temp) for the needed purposes and keep the last one (archive) for local backups.

Notes:

  • Some tests are failing, and I'm still investigating why and will fix them while you review the PR.

Checklist

  • I have added or updated any relevant documentation.
  • I have cleaned any remaining cloud resources from my accounts.

Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
@marceloneppel marceloneppel added the enhancement New feature, UI change, or workload upgrade label Apr 23, 2025
@codecov
Copy link

codecov bot commented Apr 23, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.79%. Comparing base (1c90b97) to head (66829e4).
Report is 1 commits behind head on 16/edge.

Additional details and impacted files
@@             Coverage Diff             @@
##           16/edge     #941      +/-   ##
===========================================
+ Coverage    73.74%   73.79%   +0.04%     
===========================================
  Files           12       12              
  Lines         3554     3560       +6     
  Branches       509      511       +2     
===========================================
+ Hits          2621     2627       +6     
  Misses         738      738              
  Partials       195      195              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
…ools

Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
Comment on lines +709 to +713
if temp_location is not None:
cursor.execute("SELECT TRUE FROM pg_tablespace WHERE spcname='temp';")
if cursor.fetchone() is None:
cursor.execute(f"CREATE TABLESPACE temp LOCATION '{temp_location}';")
cursor.execute("GRANT CREATE ON TABLESPACE temp TO public;")
Copy link
Member Author

Choose a reason for hiding this comment

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

Configure PostgreSQL to use the temp storage for temporary tablespaces.

Comment on lines +983 to +1002
container.exec([
"chown",
f"{WORKLOAD_OS_USER}:{WORKLOAD_OS_GROUP}",
"/var/lib/postgresql/archive",
]).wait()
container.exec([
"chown",
f"{WORKLOAD_OS_USER}:{WORKLOAD_OS_GROUP}",
self._storage_path,
]).wait()
container.exec([
"chown",
f"{WORKLOAD_OS_USER}:{WORKLOAD_OS_GROUP}",
"/var/lib/postgresql/logs",
]).wait()
container.exec([
"chown",
f"{WORKLOAD_OS_USER}:{WORKLOAD_OS_GROUP}",
"/var/lib/postgresql/temp",
]).wait()
Copy link
Member Author

Choose a reason for hiding this comment

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

Set ownership of storage volumes folders because on clouds like AWS and GCP, the ownership might be different.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh testing this on Clouds will surpise us... diffrerent storages on different AZ/NAS/Performance.... we will have fun :-)

Can we add AWS test here? can be separate backlog task.

- auth-local: trust
- encoding: UTF8
- data-checksums
- waldir: /var/lib/postgresql/logs
Copy link
Member Author

Choose a reason for hiding this comment

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

Mapping the WAL directory to the logs storage in the primary.

Comment on lines +110 to +111
basebackup:
- waldir: /var/lib/postgresql/logs
Copy link
Member Author

Choose a reason for hiding this comment

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

Mapping the WAL directory to the logs storage in the replicas.

ssl_cert_file: {{ storage_path }}/cert.pem
ssl_key_file: {{ storage_path }}/key.pem
{%- endif %}
temp_tablespaces: temp
Copy link
Member Author

Choose a reason for hiding this comment

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

Configure PostgreSQL to use the temp storage for temporary tablespaces.

Comment on lines +30 to +38
logger.info("Checking charm storages")
expected_storages = ["archive", "data", "logs", "temp"]
storages = await ops_test.model.list_storage()
assert len(storages) == 4, f"Expected 4 storages, got: {len(storages)}"
for index, storage in enumerate(storages):
assert (
storage["attachments"]["unit-postgresql-k8s-0"].__dict__["storage_tag"]
== f"storage-{expected_storages[index]}-{index}"
), f"Storage {expected_storages[index]} not found"
Copy link
Member Author

Choose a reason for hiding this comment

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

Testing the presence of multiple storages.

Comment on lines +41 to +42
# TODO: rollback to ops_test.model.deploy syntax when we release to edge.
await ops_test.juju(
Copy link
Member Author

Choose a reason for hiding this comment

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

This will be rolled back as soon as a revision is published to the 16/edge channel.

base=CHARM_BASE,
),
# TODO: rollback to ops_test.model.deploy syntax when we release to stable.
await ops_test.juju(
Copy link
Member Author

Choose a reason for hiding this comment

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

This will be rolled back as soon as a revision is published to the 16/beta channel.

@marceloneppel marceloneppel marked this pull request as ready for review April 29, 2025 11:52
Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
Comment on lines +983 to +1002
container.exec([
"chown",
f"{WORKLOAD_OS_USER}:{WORKLOAD_OS_GROUP}",
"/var/lib/postgresql/archive",
]).wait()
container.exec([
"chown",
f"{WORKLOAD_OS_USER}:{WORKLOAD_OS_GROUP}",
self._storage_path,
]).wait()
container.exec([
"chown",
f"{WORKLOAD_OS_USER}:{WORKLOAD_OS_GROUP}",
"/var/lib/postgresql/logs",
]).wait()
container.exec([
"chown",
f"{WORKLOAD_OS_USER}:{WORKLOAD_OS_GROUP}",
"/var/lib/postgresql/temp",
]).wait()
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh testing this on Clouds will surpise us... diffrerent storages on different AZ/NAS/Performance.... we will have fun :-)

Can we add AWS test here? can be separate backlog task.

Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
…ools

Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
@marceloneppel marceloneppel merged commit 274abc1 into 16/edge May 5, 2025
66 checks passed
@marceloneppel marceloneppel deleted the dpe-6964-storage-pools branch May 5, 2025 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature, UI change, or workload upgrade Libraries: Out of sync

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants