Skip to content

[DPE-6965] Storage pools #852

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

Merged
merged 14 commits into from
May 5, 2025
Merged

[DPE-6965] Storage pools #852

merged 14 commits into from
May 5, 2025

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 <[email protected]>
@marceloneppel marceloneppel added the enhancement New feature, UI change, or workload upgrade label Apr 23, 2025
Copy link

codecov bot commented Apr 23, 2025

Codecov Report

Attention: Patch coverage is 34.28571% with 23 lines in your changes missing coverage. Please review.

Project coverage is 70.83%. Comparing base (f60f4ef) to head (4dcc9ec).
Report is 1 commits behind head on 16/edge.

Files with missing lines Patch % Lines
src/relations/async_replication.py 0.00% 14 Missing ⚠️
src/charm.py 28.57% 5 Missing ⚠️
src/backups.py 76.92% 1 Missing and 2 partials ⚠️
src/upgrade.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           16/edge     #852      +/-   ##
===========================================
- Coverage    71.03%   70.83%   -0.21%     
===========================================
  Files           15       15              
  Lines         3870     3888      +18     
  Branches       571      580       +9     
===========================================
+ Hits          2749     2754       +5     
- Misses         938      949      +11     
- Partials       183      185       +2     

☔ 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 <[email protected]>
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
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 +1728 to +1731
if len(data_directory_contents) == 1 and data_directory_contents[0] == "pg_wal":
os.remove(os.path.join(POSTGRESQL_DATA_PATH, "pg_wal"))
logger.info("PostgreSQL data directory was not empty. Removed pg_wal")
return True
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 was needed for async replication, when sometimes the pg_wal folder got recreated at some point with old data.

@@ -126,9 +126,12 @@ bootstrap:
initdb:
- encoding: UTF8
- data-checksums
- waldir: /var/snap/charmed-postgresql/common/data/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 +133 to +134
basebackup:
- waldir: /var/snap/charmed-postgresql/common/data/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.

@@ -147,6 +150,7 @@ postgresql:
ssl_cert_file: {{ conf_path }}/cert.pem
ssl_key_file: {{ conf_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 +27 to +35
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-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.

await ops_test.model.deploy(
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.

await ops_test.model.deploy(
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:53
Copy link
Contributor

@taurus-forever taurus-forever left a comment

Choose a reason for hiding this comment

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

IMHI, we can merge this tested PR and rediscuss binaries on Monday.

Signed-off-by: Marcelo Henrique Neppel <[email protected]>
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
Signed-off-by: Marcelo Henrique Neppel <[email protected]>
@marceloneppel marceloneppel merged commit ff02253 into 16/edge May 5, 2025
66 checks passed
@marceloneppel marceloneppel deleted the dpe-6965-storage-pools branch May 5, 2025 11:24
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