Skip to content

Conversation

@lucasgameiroborges
Copy link

@lucasgameiroborges lucasgameiroborges commented Nov 21, 2024

Part of the 4-PR bundle to merge PostgreSQL 16. See doc for further info: https://docs.google.com/document/d/1K4xMxKDOiHfdCTc_hxMh6bW1DCZ6HC6wa6g8tAJAtno/edit?tab=t.0

@codecov
Copy link

codecov bot commented Nov 21, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 16 lines in your changes missing coverage. Please review.

Project coverage is 71.46%. Comparing base (5dd067f) to head (8fff98f).
Report is 1 commits behind head on 16/edge.

Files with missing lines Patch % Lines
src/charm.py 55.00% 8 Missing and 1 partial ⚠️
src/cluster.py 71.42% 3 Missing and 1 partial ⚠️
src/relations/postgresql_provider.py 57.14% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           16/edge     #680      +/-   ##
===========================================
- Coverage    71.92%   71.46%   -0.46%     
===========================================
  Files           15       14       -1     
  Lines         3476     3287     -189     
  Branches       535      491      -44     
===========================================
- Hits          2500     2349     -151     
+ Misses         845      815      -30     
+ Partials       131      123       -8     

☔ 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.

github-actions bot and others added 4 commits March 11, 2025 15:01
Co-authored-by: GitHub Actions <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
* Use `charmcraft test` & concierge (#762)

* Update charmcraft.yaml build tools (#760)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* [DPE-6020] Better promote-to-primary unit scope error handling (#759)

* Bump libs

* Flip default scope

* Better action failure

* Wrong attr

* Revert scope

* Bump libs

* Handle async replica switchover

* Unit tests

* Bump cosl

* Disable Nextcloud test (#767)

* Update canonical/data-platform-workflows action to v30 (#770)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* Use _promote_charm.yaml (#771)

Use `charmcraft promote` and auto-generate release notes

* [DPE-5827] Set all nodes to synchronous replicas (#672)

* Set all nodes to synchronous replicas

* Fix template var

* Also change config patching

* Update sync nodes during upgrade

* Revert are_writes_increasing changes

* Add back logging

* Try without logs

* Tactical sleep

* Log removal error

* Remove logs

* Tweak replication test

* Pass down unit

* Wait for test app to idle

* Add comment

* Port config changes

* Copy policy test

* Fix import

* Missed param removal

* Unit test

* Missing attr

* Add logs

* Add timeout to connection

* Log conn str

* Fix num of standbys

* Charm fixture

* Remove stepdown hook

* Config description

* Revert conn str

* Add async scaling test

* Typo

* Don't remove standby and primary

* Update dependency psutil to v7 (#772)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* Update dependency cryptography to v44.0.1 [SECURITY] (#764)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* Update canonical/data-platform-workflows action to v30.0.2 (#765)

* Update canonical/data-platform-workflows action to v30.0.2

* Update promote.yaml

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Carl Csaposs <carl.csaposs@canonical.com>

* [DPE-6323] Handle missing stanza output (#727)

* Handle missing stanza output

* Update libs

* Unit tests

* Update canonical/has-signed-canonical-cla action to v2 (#773)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* [MISC] Define charm constants (#774)

* Lock file maintenance Python dependencies (#743)

* Lock file maintenance Python dependencies

* Backoff boto3 1.36

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Dragomir Penev <dragomir.penev@canonical.com>

* Update charmcraft.yaml build tools (#768)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* Update canonical/data-platform-workflows action to v30.1.3 (#776)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* Update dependency uv to v0.6.3 (#780)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* [MISC] Sanitize PostgreSQL extra-user-roles arg (#782)

* [MISC] Fix PostgreSQL lib function signature (#786)

* [MISC] Skip backup and subordinate tests without creds (#789)

* Bump libs

* Skip backup tests without creds

* Skip subordinate tests

* Update tests/integration/test_subordinates.py

Co-authored-by: Carl Csaposs <carl.csaposs@canonical.com>

---------

Co-authored-by: Carl Csaposs <carl.csaposs@canonical.com>

* Update dependency jinja2 to v3.1.6 [SECURITY] (#788)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* Disable cache

* Reduce required approvals on Renovate pull requests by 1 (#787)

* Sync docs from Discourse (#748)

Co-authored-by: GitHub Actions <41898282+github-actions[bot]@users.noreply.github.com>

* Cleanup juju 2 tests

* Linting

* Integration test diffs

* Try with series for ubuntu pro subordinate

* Filter terminated units

* Bump PG version

* Disable pgaudit for timescale and postgis

* Linting

* Remove tests

* Remove param for secrets

* Linting

* Idle when disabling pgaudit

* Actually disable audit

* Disable timescale in object test

* Try to disable plugins between tests

* Update canonical/data-platform-workflows action to v30.2.0 (#792)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* Try to disable pgaudit in general

---------

Co-authored-by: Carl Csaposs <carl.csaposs@canonical.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Sinclert Pérez <sinclert.perez@canonical.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Comment on lines +102 to +103
# TODO Figure out how to deal with pgaudit
config={"profile": "testing", "plugin_audit_enable": "False"},
Copy link
Contributor

Choose a reason for hiding this comment

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

Generally disable pgaudit for this test, until we decide how to handle incompatibilities.

Copy link
Contributor

Choose a reason for hiding this comment

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

Talk to upstream to address incompatibility ASAP?

Copy link
Contributor

Choose a reason for hiding this comment

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

Upstream bug is timescale/timescaledb#7667. There should be fixes for this in Timescale 2.18.2, but there seems to be additional issues on pgaudit's side. We can update our PPA to 2.18.2 and see if it helps.

Comment on lines +48 to +50
# TODO switch back to series when pylib juju can figure out the base:
# https://github.com/juju/python-libjuju/issues/1240
series="noble",
Copy link
Contributor

Choose a reason for hiding this comment

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

Base doesn't work as expected here.

Comment on lines 355 to 360
connection = self._connect_to_database(database=database)
connection.autocommit = True
with connection.cursor() as cursor:
for extension, enable in ordered_extensions.items():
if extension == "postgis":
cursor.execute("SET pgaudit.log = 'none';")
Copy link
Contributor

Choose a reason for hiding this comment

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

Reverting this change to keep the lib consistent with main, we should have to address compatibilities between extensions either way.

@dragomirp dragomirp self-requested a review March 13, 2025 12:53
Comment on lines +964 to +968

# When removing all units sometimes the last unit remain in the list
current_units = {unit.name for unit in ops_test.model.applications[app].units}
original_units.intersection_update(current_units)
assert original_units.issubset(current_units), "New unit not added to model"
Copy link
Contributor

Choose a reason for hiding this comment

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

Annoying failure in the restore_cluster test. The last unit of a deployment sometimes remains in some libjuju cache. Will backport to main in another PR.

should_fail = database == "postgres"
cursor.execute(f"CREATE TABLE {random_name}(data TEXT);")
should_fail = database == DATABASE_DEFAULT_NAME
cursor.execute(f"CREATE SCHEMA test; CREATE TABLE test.{random_name}(data TEXT);")
Copy link
Contributor

Choose a reason for hiding this comment

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

PG 16 doesn't give access to the public schema by default. See canonical/postgresql-k8s-operator#788 (comment)

@dragomirp dragomirp self-requested a review March 14, 2025 13:33
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.

LGTM, one question (typo found on promote).

Copy link
Contributor

Choose a reason for hiding this comment

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

@a-velasco we need documentation versioning ASAP here, otherwise PG16 has PG14 docs without any chances to separate them. :-(

@pytest.mark.abort_on_fail
async def test_pre_upgrade_check(ops_test: OpsTest) -> None:
"""Test that the pre-upgrade-check action runs successfully."""
# TODO remove once we release to stable
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 should we have ticket here? Or prepared PR?

We can promote the current 16/edge to 16/beta and use it here.

Copy link
Member

Choose a reason for hiding this comment

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

A prepared PR pointing to 16/beta would be fine. So we can test it before we promote to stable.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can promote with the dispatch flows to test that they work once we merge.

Comment on lines +102 to +103
# TODO Figure out how to deal with pgaudit
config={"profile": "testing", "plugin_audit_enable": "False"},
Copy link
Contributor

Choose a reason for hiding this comment

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

Talk to upstream to address incompatibility ASAP?

Copy link
Member

@marceloneppel marceloneppel left a comment

Choose a reason for hiding this comment

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

I commented on minor details. Apart from that, LGTM! Thanks for the great work!

* [Users]
* [Logs]
* [Juju]
* [Legacy charm]
Copy link
Member

Choose a reason for hiding this comment

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

Does the legacy charm have a 16-version? I don't have a strong opinion about whether we should remove this section.

Copy link
Contributor

Choose a reason for hiding this comment

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

Haven't touched the docs. We should discuss with @a-velasco what the plan is for 16 docs.

@pytest.mark.abort_on_fail
async def test_pre_upgrade_check(ops_test: OpsTest) -> None:
"""Test that the pre-upgrade-check action runs successfully."""
# TODO remove once we release to stable
Copy link
Member

Choose a reason for hiding this comment

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

A prepared PR pointing to 16/beta would be fine. So we can test it before we promote to stable.

dragomirp and others added 2 commits March 19, 2025 16:16
Co-authored-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
@dragomirp dragomirp merged commit 8f3c42a into 16/edge Mar 19, 2025
61 checks passed
@dragomirp dragomirp deleted the test-pg-16 branch March 19, 2025 19:25
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: OK

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants