[MISC] Check for peer relation when getting the primary endpoint #366

dragomirp · 2024-02-22T19:59:26Z

Try to speed up and stabilise the tests. Based of the shared buffers PR.

This reverts commit 8184dfa.

codecov · 2024-02-22T20:01:27Z

Codecov Report

Attention: Patch coverage is 40.90909% with 13 lines in your changes are missing coverage. Please review.

Project coverage is 81.25%. Comparing base (bf39c67) to head (f3e3d33).
Report is 1 commits behind head on dpe-3591-fix-shared-buffers-validation.

Files	Patch %	Lines
src/charm.py	40.90%	12 Missing and 1 partial ⚠️

Additional details and impacted files

@@                            Coverage Diff                             @@
##           dpe-3591-fix-shared-buffers-validation     #366      +/-   ##
==========================================================================
- Coverage                                   81.43%   81.25%   -0.19%     
==========================================================================
  Files                                          10       10              
  Lines                                        2214     2235      +21     
  Branches                                      356      360       +4     
==========================================================================
+ Hits                                         1803     1816      +13     
- Misses                                        343      347       +4     
- Partials                                       68       72       +4

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

…peer-rel-gh-runner

…vailable

…or-peer-rel-gh-runner

dragomirp · 2024-02-24T12:56:58Z

src/charm.py

+        if not self._peers:
+            logger.debug("primary endpoint early exit: Peer relation not joined yet.")
+            return None


If the peer relation is not yet joined, don't try to find the primary.

dragomirp · 2024-02-24T12:59:20Z

src/charm.py

+        except PostgreSQLListUsersError:
+            logger.warning("Deferriing on_start: Unable to list users")
+            event.defer()
+            return


Skipping the wait in primary_endpoint could lead o exceptions here.

src/charm.py

dragomirp · 2024-02-24T13:04:36Z

src/charm.py

+                        # TODO figure out why peer data is not available
+                        if (
+                            primary_endpoint
+                            and len(self._units_ips) == 1
+                            and len(self._peers.units) > 1
+                        ):
+                            logger.warning(
+                                "Possibly incoplete peer data: Will not map primary IP to unit IP"
+                            )
+                            return primary_endpoint


On juju 3 we don't seem to be getting the peer data used to get the IP addr when a new unit is joining (e.g. when scaling in the test_db suite).

It smells dangerous...

dragomirp · 2024-02-24T13:06:51Z

src/charm.py

+                        logger.debug(
+                            "primary endpoint early exit: Primary IP not in cached peer list."
+                        )
+                        primary_endpoint = None


_units_ips is a property and will not update on retry. The Patroni response might change while retrying, but IMHO it's better to leave that to the topology observer script.

taurus-forever · 2024-02-26T11:21:08Z

src/charm.py

+                        # TODO figure out why peer data is not available
+                        if (
+                            primary_endpoint
+                            and len(self._units_ips) == 1
+                            and len(self._peers.units) > 1
+                        ):
+                            logger.warning(
+                                "Possibly incoplete peer data: Will not map primary IP to unit IP"
+                            )
+                            return primary_endpoint


It smells dangerous...

src/charm.py

taurus-forever · 2024-02-26T11:51:07Z

tests/integration/test_db_admin.py

@@ -34,7 +34,6 @@


 @pytest.mark.group(1)
-@pytest.mark.skip(reason="DB Admin tests are currently broken")


Good to have it re-enable. I suspect it is not related to this PR but enabled as coincident. Right?

I reenabled it here to reduce the amount CI runs.

taurus-forever

Discussed with Dragomir, merging this to have progress with tests stabilisation otherwise shared_buffers test cannot be merged.

* Fix shared buffers validation Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Consider shared buffers config option value when calculating effective cache size value Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Fix PostgreSQL restart not being called Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Add and update unit test Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Update unit test Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Increase test timeout Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Increase test timeout Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Switch to GH runners * Increase stanza timeout * [MISC] Check for peer relation when getting the primary endpoint (#366) * Reenable landscape tests * Try GH runners * Back on self-hosted runner * Don't check for primary if peer didn't join yet * Don't recheck missing primary ip when looking for primary endpoint * Catch list user exception * Catch Psycopg2 error when trying to set config * Try to preflight connection on config validation * Don't retry db connection check * Set the primary endpoint before starting the primary * Revert "Set the primary endpoint before starting the primary" This reverts commit 8184dfa. * Wait for peer before starting primary * Switch to GH runners * Recheck DB connection * Don't check peer on primary startup * Try to skip primary IP validation if not all cluster IPs seem to be available * Comment * Review comments * Unit tests --------- Signed-off-by: Marcelo Henrique Neppel <[email protected]> Co-authored-by: Dragomir Penev <[email protected]> Co-authored-by: Dragomir Penev <[email protected]>

* Fix shared buffers validation Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Consider shared buffers config option value when calculating effective cache size value Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Fix PostgreSQL restart not being called Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Add and update unit test Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Update unit test Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Increase test timeout Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Increase test timeout Signed-off-by: Marcelo Henrique Neppel <[email protected]> * Switch to GH runners * Increase stanza timeout * [MISC] Check for peer relation when getting the primary endpoint (canonical#366) * Reenable landscape tests * Try GH runners * Back on self-hosted runner * Don't check for primary if peer didn't join yet * Don't recheck missing primary ip when looking for primary endpoint * Catch list user exception * Catch Psycopg2 error when trying to set config * Try to preflight connection on config validation * Don't retry db connection check * Set the primary endpoint before starting the primary * Revert "Set the primary endpoint before starting the primary" This reverts commit 8184dfa. * Wait for peer before starting primary * Switch to GH runners * Recheck DB connection * Don't check peer on primary startup * Try to skip primary IP validation if not all cluster IPs seem to be available * Comment * Review comments * Unit tests --------- Signed-off-by: Marcelo Henrique Neppel <[email protected]> Co-authored-by: Dragomir Penev <[email protected]> Co-authored-by: Dragomir Penev <[email protected]>

dragomirp added 14 commits February 13, 2024 17:17

Reenable landscape tests

ac24da8

Try GH runners

98e151e

Merge branch 'main' into reenable-landscape

14b7d3f

Back on self-hosted runner

5d9da4c

Don't check for primary if peer didn't join yet

375c957

Don't recheck missing primary ip when looking for primary endpoint

8c47669

Catch list user exception

1e0b514

Catch Psycopg2 error when trying to set config

fa44790

Try to preflight connection on config validation

7fce785

Don't retry db connection check

d50d669

Set the primary endpoint before starting the primary

8184dfa

Revert "Set the primary endpoint before starting the primary"

5a09131

This reverts commit 8184dfa.

Wait for peer before starting primary

1eb5529

Switch to GH runners

e14d5a1

dragomirp changed the title ~~Check for peer rel gh runner~~ [TEST] Check for peer rel gh runner Feb 22, 2024

github-actions bot added the Libraries: Out of sync label Feb 22, 2024

dragomirp added 8 commits February 22, 2024 22:57

Recheck DB connection

dc4588f

Merge branch 'check-for-peer-rel' into check-for-peer-rel-gh-runner

b89476f

Don't check peer on primary startup

e7e8965

Merge branch 'check-for-peer-rel' into check-for-peer-rel-gh-runner

dbf7deb

Merge branch 'dpe-3591-fix-shared-buffers-validation' into check-for-…

3f1d62e

…peer-rel-gh-runner

Try to skip primary IP validation if not all cluster IPs seem to be a…

5c78ca8

…vailable

Merge remote-tracking branch 'origin/reenable-landscape' into check-f…

1ea2833

…or-peer-rel-gh-runner

Comment

beea41d

dragomirp marked this pull request as ready for review February 24, 2024 12:54

dragomirp requested a review from taurus-forever February 24, 2024 12:55

dragomirp commented Feb 24, 2024

View reviewed changes

src/charm.py Outdated Show resolved Hide resolved

dragomirp commented Feb 24, 2024

View reviewed changes

dragomirp changed the title ~~[TEST] Check for peer rel gh runner~~ [MISC] Check for peer relation when getting the primary endpoint Feb 24, 2024

taurus-forever reviewed Feb 26, 2024

View reviewed changes

Review comments

e520923

taurus-forever approved these changes Feb 26, 2024

View reviewed changes

Unit tests

f3e3d33

dragomirp merged commit a33b39d into dpe-3591-fix-shared-buffers-validation Feb 27, 2024
41 checks passed

dragomirp deleted the check-for-peer-rel-gh-runner branch February 27, 2024 09:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MISC] Check for peer relation when getting the primary endpoint #366

[MISC] Check for peer relation when getting the primary endpoint #366

dragomirp commented Feb 22, 2024 •

edited

Loading

codecov bot commented Feb 22, 2024 •

edited

Loading

dragomirp Feb 24, 2024

dragomirp Feb 24, 2024

dragomirp Feb 24, 2024

taurus-forever Feb 26, 2024

dragomirp Feb 24, 2024

taurus-forever Feb 26, 2024

taurus-forever Feb 26, 2024

dragomirp Feb 26, 2024

taurus-forever left a comment

		@@ -34,7 +34,6 @@


		@pytest.mark.group(1)
		@pytest.mark.skip(reason="DB Admin tests are currently broken")

[MISC] Check for peer relation when getting the primary endpoint #366

[MISC] Check for peer relation when getting the primary endpoint #366

Conversation

dragomirp commented Feb 22, 2024 • edited Loading

codecov bot commented Feb 22, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taurus-forever left a comment

Choose a reason for hiding this comment

dragomirp commented Feb 22, 2024 •

edited

Loading

codecov bot commented Feb 22, 2024 •

edited

Loading