[DPE-4533] Pause Patroni in the TLS test #588

dragomirp · 2024-07-25T15:50:18Z

Mostly a port of canonical/postgresql-operator#534

Pauses patroni while trying to trigger rewind
Split the TLS test so that the rewind testing will also be executed on arm64

codecov · 2024-07-25T15:52:16Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.77%. Comparing base (3e48255) to head (691133e).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #588      +/-   ##
==========================================
+ Coverage   68.60%   68.77%   +0.17%     
==========================================
  Files          10       10              
  Lines        2978     2985       +7     
  Branches      564      565       +1     
==========================================
+ Hits         2043     2053      +10     
+ Misses        816      814       -2     
+ Partials      119      118       -1

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

lucasgameiroborges

Nice!

marceloneppel

Excellent!

dragomirp · 2024-07-26T14:59:06Z

tests/integration/test_tls.py

+
+@pytest.mark.group(1)
+async def test_remove_tls(ops_test: OpsTest) -> None:
+    async with ops_test.fast_forward():


Splitting off TLS removal as well.

dragomirp · 2024-07-26T14:59:30Z

tests/integration/test_tls.py

-        await ops_test.model.wait_for_idle(apps=[DATABASE_APP_NAME], status="active", timeout=1000)
+        await ops_test.model.wait_for_idle(apps=[DATABASE_APP_NAME], status="active", timeout=1500)


Timeouted 3 times in a row for Juju 2

taurus-forever

Tnx!

dragomirp · 2024-08-02T11:19:10Z

src/charm.py

+            try:
+                container.restart(self._postgresql_service)
+            except ChangeError:
+                logger.exception("Failed to restart patroni")


If there's a dangling Postgresql process, Patroni will be unable to start

dragomirp · 2024-08-02T11:21:34Z

tests/integration/test_tls.py

+        try:
+            await run_command_on_unit(ops_test, primary, "/charm/bin/pebble stop postgresql")
+        except Exception as e:
+            # pebble stop on juju 2 errors out and leaves dangling PG processes
+            if juju_major_version > 2:
+                raise e
+            await run_command_on_unit(ops_test, primary, "pkill --signal SIGTERM -x postgres")


On Juju 2 if the pebble stop command fails, the Patroni process is terminated, but a dangling Postgresql process remains and prevents Patroni from starting again.

Alternatively, we can do this check and termination in the charm code, but I would prefer not to add Juju 2 specific live code. Thoughts?

On 2.9 if patroni stop failed: kill -SIGTERM $postgresql_pid ?

On 2.9 if patroni stop failed: kill -SIGTERM $postgresql_pid ?

Pretty much. We can do this in the ChangeError in the restart check. Question is do we want to add that code for Juju 2's sake?

I prefer that we keep this SIGTERM call in the test code and not in the charm code. Even if it is a SIGTERM, I'd prefer either to wait for the database process to gracefully finish by itself or analyze the situation manually before doing any action to avoid any data corruption.

dragomirp · 2024-08-02T11:24:21Z

TLS test succeeded 5 times in a row on juju 2: https://github.com/canonical/postgresql-k8s-operator/actions/runs/10212523498/job/28261962481?pr=588

taurus-forever

LGTM, one educational question.

taurus-forever · 2024-08-02T11:24:56Z

tests/integration/ha_tests/helpers.py

            primary_name = await get_primary(ops_test, app)
            unit_ip = await get_unit_address(ops_test, primary_name)
-            configuration_info = requests.get(f"http://{unit_ip}:8008/config")
+            configuration_info = requests.get(f"{schema}://{unit_ip}:8008/config", verify=not tls)


Please share the reason for verify=not tls?

taurus-forever · 2024-08-02T11:27:17Z

tests/integration/test_tls.py

+        try:
+            await run_command_on_unit(ops_test, primary, "/charm/bin/pebble stop postgresql")
+        except Exception as e:
+            # pebble stop on juju 2 errors out and leaves dangling PG processes
+            if juju_major_version > 2:
+                raise e
+            await run_command_on_unit(ops_test, primary, "pkill --signal SIGTERM -x postgres")


On 2.9 if patroni stop failed: kill -SIGTERM $postgresql_pid ?

lucasgameiroborges

LGTM for the changes, but why so many failures on install hook on CI? :(

dragomirp · 2024-08-02T12:43:31Z

LGTM for the changes, but why so many failures on install hook on CI? :(

Test app is borked due to charmcraft 3 release.

marceloneppel · 2024-08-02T13:04:47Z

tests/integration/test_tls.py

+        try:
+            await run_command_on_unit(ops_test, primary, "/charm/bin/pebble stop postgresql")
+        except Exception as e:
+            # pebble stop on juju 2 errors out and leaves dangling PG processes
+            if juju_major_version > 2:
+                raise e
+            await run_command_on_unit(ops_test, primary, "pkill --signal SIGTERM -x postgres")


I prefer that we keep this SIGTERM call in the test code and not in the charm code. Even if it is a SIGTERM, I'd prefer either to wait for the database process to gracefully finish by itself or analyze the situation manually before doing any action to avoid any data corruption.

Pause Patroni in the TLS test

9158ea6

github-actions bot added the Libraries: Out of sync label Jul 25, 2024

dragomirp added 6 commits July 25, 2024 19:10

Wrong arg

3c41be3

Disable verify

8e7db4d

Other wrong arg

0b04b40

Retry stopping

55925a0

Split tls test

7b5cd24

Abort on failed rewind test

c1ad590

dragomirp marked this pull request as ready for review July 26, 2024 12:02

dragomirp requested review from marceloneppel, taurus-forever and lucasgameiroborges July 26, 2024 12:02

lucasgameiroborges approved these changes Jul 26, 2024

View reviewed changes

marceloneppel approved these changes Jul 26, 2024

View reviewed changes

Split off remove TLS test

704ebd7

dragomirp commented Jul 26, 2024

View reviewed changes

dragomirp added 2 commits July 26, 2024 18:00

Merge branch 'main' into dpe-4533-flaky-test

4fe6fc3

Idle in the rewind test

a3c2464

marceloneppel approved these changes Jul 26, 2024

View reviewed changes

taurus-forever approved these changes Jul 29, 2024

View reviewed changes

dragomirp added 8 commits July 30, 2024 23:17

Merge branch 'main' into dpe-4533-flaky-test

a0161fa

Merge branch 'main' into dpe-4533-flaky-test

fa105fb

Merge branch 'main' into dpe-4533-flaky-test

02c651e

Catch restart error

68948f5

manually start service

89dba90

Revert test tweaks

c005133

Check tls relation when checking for certificates

85812f3

Update config when restarting leader

bcac1b9

dragomirp marked this pull request as draft August 1, 2024 10:02

dragomirp added 4 commits August 1, 2024 17:16

Revert restart changes and juju 2 overkill

7eafe84

Directly TERM the postgres process

cbadff1

Merge branch 'main' into dpe-4533-flaky-test

844655d

Ignore volume errors

5ec3edf

dragomirp commented Aug 2, 2024

View reviewed changes

dragomirp marked this pull request as ready for review August 2, 2024 11:21

dragomirp requested review from taurus-forever, marceloneppel and lucasgameiroborges August 2, 2024 11:21

taurus-forever approved these changes Aug 2, 2024

View reviewed changes

lucasgameiroborges approved these changes Aug 2, 2024

View reviewed changes

marceloneppel approved these changes Aug 2, 2024

View reviewed changes

dragomirp added 3 commits August 2, 2024 17:50

Unit tests

bbc6e77

Merge branch 'main' into dpe-4533-flaky-test

4de0ac1

Lint

691133e

dragomirp merged commit 978d0ec into main Aug 2, 2024

dragomirp deleted the dpe-4533-flaky-test branch August 2, 2024 22:32

		await ops_test.model.wait_for_idle(apps=[DATABASE_APP_NAME], status="active", timeout=1000)
		await ops_test.model.wait_for_idle(apps=[DATABASE_APP_NAME], status="active", timeout=1500)

[DPE-4533] Pause Patroni in the TLS test #588

[DPE-4533] Pause Patroni in the TLS test #588

Uh oh!

Conversation

dragomirp commented Jul 25, 2024 • edited by taurus-forever Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jul 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

lucasgameiroborges left a comment

Choose a reason for hiding this comment

Uh oh!

marceloneppel 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!

taurus-forever 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!

dragomirp commented Aug 2, 2024

Uh oh!

taurus-forever 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!

lucasgameiroborges left a comment

Choose a reason for hiding this comment

Uh oh!

dragomirp commented Aug 2, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dragomirp commented Jul 25, 2024 •

edited by taurus-forever

Loading

codecov bot commented Jul 25, 2024 •

edited

Loading