[DPE-2882] Addressing issues 108 and 111 #113

juditnovak · 2023-11-02T19:06:49Z

Lib tries to access app databag from non-leader unit · Issue #111 · canonical/data-platform-libs
Updated secret still at earlier content in observers (that labelled them) · Issue #108 · canonical/data-platform-libs HIGH IMPORTANCE !!!!!!!!!!!!!!!!!!

juditnovak · 2023-11-13T22:56:21Z

lib/charms/data_platform_libs/v0/data_interfaces.py

+                try:
+                    self._secret_content = self.meta.get_content(refresh=True)
+                except (ValueError, ModelError):
+                    # Due to: ValueError: Secret owner cannot use refresh=True


Turns out this is only a bug on the test framework... But --as much as I hate to do this-- I rather do a global safety measure here than disable corresponding, important unittests. (I know this is incorrect.)

I do my best to keep an eye -- and clean this up as the issue is fixed.

(Until than any volunteer is welcome to find a safe workaround to fix broken tests in a way that they return a meaninful, reliable result -- just pls do it fast, as MySQL Router is currently broken on the fix delivered here... ;-) )

is the ModelError something else? catching all ModelErrors could be dangerous

Great catch, thank you!!!!
I think it's left there by mistake.

Just for the reference: no, it's not.
See https://github.com/canonical/data-platform-libs/pull/113/files#r1392489501

It might be worth looking at the modelerror message and re-raising the exception if it's not what you expect—I believe modelerror is fairly generic

All right I've followed up the thing, and remember now. ModelError comes from another Juju (actually ops) bug ;-)

canonical/operator#1058 / https://bugs.launchpad.net/juju/+bug/2042596

Ah, and to explain: the case above is somewhat NOT triggered when w/o refresh. That's why catching and retrying with simple get_content() actually solves the issue.

juditnovak · 2023-11-14T10:37:58Z

Tested against the following scenario, where local builds (mysql-router-k8s and mysql-test-appare current latest/edge WITH this version of the library):

# Deploy
juju  deploy --trust  ./mysql-router-k8s_ubuntu-22.04-amd64.charm --resource mysql-router-image=ghcr.io/canonical/charmed-mysql@sha256:0f5fe7d7679b1881afde24ecfb9d14a9daade790ec787087aa5d8de1d7b00b21
juju deploy mysql-k8s --channel=8.0/edge
juju deploy --trust ./mysql-test-app_ubuntu-22.04-amd64_ubuntu-20.04-amd64.charm 

# Relate backend and router
juju relate mysql-k8s mysql-router-k8s

# Relate router test-app
juju relate mysql-test-app mysql-router-k8s

# Break backend-router relation
juju remove-relation mysql-router-k8s mysql-k8s

# Restore backnd-router relation
juju relate  mysql-router-k8s mysql-k8s

mysql-test-app continued function with no issues, everyone happy:

Model    Controller          Cloud/Region        Version  SLA          Timestamp
router3  microk8s-localhost  microk8s/localhost  3.1.6    unsupported  11:37:35+01:00

App               Version                  Status  Scale  Charm             Channel   Rev  Address         Exposed  Message
mysql-k8s         8.0.34-0ubuntu0.22.04.1  active      1  mysql-k8s         8.0/edge  110  10.152.183.107  no       
mysql-router-k8s  8.0.34-0ubuntu0.22.04.1  active      1  mysql-router-k8s              0  10.152.183.54   no       
mysql-test-app    0.0.2                    active      1  mysql-test-app                0  10.152.183.200  no       

Unit                 Workload  Agent  Address       Ports  Message
mysql-k8s/0*         active    idle   10.1.244.115         Primary
mysql-router-k8s/0*  active    idle   10.1.244.98          
mysql-test-app/0*    active    idle   10.1.244.116         

Integration provider                 Requirer                             Interface           Type     Message
mysql-k8s:database                   mysql-router-k8s:backend-database    mysql_client        regular  
mysql-k8s:database-peers             mysql-k8s:database-peers             mysql_peers         peer     
mysql-k8s:restart                    mysql-k8s:restart                    rolling_op          peer     
mysql-k8s:upgrade                    mysql-k8s:upgrade                    upgrade             peer     
mysql-router-k8s:database            mysql-test-app:database              mysql_client        regular  
mysql-router-k8s:mysql-router-peers  mysql-router-k8s:mysql-router-peers  mysql_router_peers  peer     
mysql-router-k8s:upgrade-version-a   mysql-router-k8s:upgrade-version-a   upgrade             peer     
mysql-test-app:application-peers     mysql-test-app:application-peers     application-peers   peer

carlcsaposs-canonical · 2023-11-14T12:07:46Z

lib/charms/data_platform_libs/v0/data_interfaces.py

@@ -526,7 +526,11 @@ def get_content(self) -> Dict[str, str]:
        """Getting cached secret content."""
        if not self._secret_content:
            if self.meta:
-                self._secret_content = self.meta.get_content()
+                try:
+                    self._secret_content = self.meta.get_content(refresh=True)


is it safe to call with refresh=True for secret owner?

wondering given

This parameter is only meaningful for secret observers, not owners.

https://ops.readthedocs.io/en/latest/#ops.Secret.get_content

With the try-catch is's safe for sure 🤣

Joke aside according to Juju Team it only affects the Harness: https://chat.charmhub.io/charmhub/pl/aekfocgk8tbtfcsda7311uurxe

Gotta confess I didn't verify that -- however with the security measure this PR is safe for sure. I'd confirm if I get a moment.

I might be misreading—my understanding of that thread is that a different error is thrown at runtime

I'm not sure exactly what error is raised on a real Juju deployment.

https://chat.charmhub.io/charmhub/pl/js96bnfhiifqtjyhsx9fwksqhe

but I might be missing something

All right, you will be right. And the error raised for real was the ModelError.
(Sry, I wrote this code a few days ago, and forgot these details.)
I took ModelError from a real-life failure.

So all in all: I think this is the answer for following https://github.com/canonical/data-platform-libs/pull/113/files#r1391790655

Yes, this way it is safe to use refresh=True -- even for secret owners.
(There are actually a bunch of intergation tests addressing this part of the code.)

carlcsaposs-canonical · 2023-11-14T12:09:21Z

lib/charms/data_platform_libs/v0/data_interfaces.py

+                try:
+                    self._secret_content = self.meta.get_content(refresh=True)
+                except (ValueError, ModelError):
+                    # Due to: ValueError: Secret owner cannot use refresh=True


is the ModelError something else? catching all ModelErrors could be dangerous

carlcsaposs-canonical · 2023-11-14T12:11:24Z

tests/integration/conftest.py

+
+@pytest.fixture(autouse=True)
+async def without_errors(caplog):
+    """This fixture is to list all those errors that mustn't occur during execution."""


nit: maybe it should be the other way around? specify which errors are okay, otherwise if error unknown: fail the test

You're totally right. I just checked integration test logs -- we don't seem to have fake ones.
I'm addressing this.

All right, the previous version of this actually wasn't functional.
The new one is looking at juju logs.

I'm planning to re-view/re-fine this method (not flaky, etc) in my upcoming task next week.
https://warthogs.atlassian.net/browse/DPE-2994

But I think it's a good attempt to ensure we have nothing but "expected" errors

carlcsaposs-canonical

while testing, saw one issue with relation breaking
unit-mysql-router-k8s-0: 10:50:30 ERROR unit.mysql-router-k8s/0.juju-log backend-database:6: Non-existing secret was attempted to be removed 8, database

otherwise, looks good!

juditnovak force-pushed the DPE-2882_addresssing_issues branch 7 times, most recently from c74c664 to b390723 Compare November 8, 2023 09:39

juditnovak requested a review from delgod November 8, 2023 09:52

juditnovak marked this pull request as ready for review November 8, 2023 09:52

juditnovak force-pushed the DPE-2882_addresssing_issues branch 3 times, most recently from 092d18f to 5732814 Compare November 13, 2023 08:26

juditnovak commented Nov 13, 2023

View reviewed changes

juditnovak requested review from welpaolo and removed request for welpaolo November 13, 2023 23:00

delgod approved these changes Nov 14, 2023

View reviewed changes

juditnovak requested a review from carlcsaposs-canonical November 14, 2023 10:38

carlcsaposs-canonical approved these changes Nov 14, 2023

View reviewed changes

juditnovak force-pushed the DPE-2882_addresssing_issues branch 9 times, most recently from 07f5bb4 to d2c2655 Compare November 16, 2023 15:00

delgod approved these changes Nov 17, 2023

View reviewed changes

juditnovak requested a review from carlcsaposs-canonical November 17, 2023 13:09

juditnovak force-pushed the DPE-2882_addresssing_issues branch 2 times, most recently from d2c2655 to 560d868 Compare November 17, 2023 16:04

juditnovak added 4 commits November 17, 2023 17:09

Addressing issues 108 and 111

407af56

Removing xfail from tests for issue 108

369ec07

Fixing error message

ee9055e

Checking no unexpected errors in logs

55e88c1

juditnovak force-pushed the DPE-2882_addresssing_issues branch from 560d868 to 55e88c1 Compare November 17, 2023 16:09

carlcsaposs-canonical added a commit to canonical/mysql-router-k8s-operator that referenced this pull request Nov 20, 2023

Sync with canonical/data-platform-libs#113

515f8aa

carlcsaposs-canonical added a commit to canonical/mysql-test-app that referenced this pull request Nov 20, 2023

Sync with canonical/data-platform-libs#113

9605a0d

carlcsaposs-canonical approved these changes Nov 20, 2023

View reviewed changes

delgod merged commit d7fc9e0 into main Nov 20, 2023
21 checks passed

delgod deleted the DPE-2882_addresssing_issues branch November 20, 2023 13:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DPE-2882] Addressing issues 108 and 111 #113

[DPE-2882] Addressing issues 108 and 111 #113

juditnovak commented Nov 2, 2023

juditnovak Nov 13, 2023 •

edited

Loading

carlcsaposs-canonical Nov 14, 2023

juditnovak Nov 14, 2023

juditnovak Nov 14, 2023

carlcsaposs-canonical Nov 15, 2023

juditnovak Nov 17, 2023

juditnovak Nov 17, 2023

juditnovak commented Nov 14, 2023

carlcsaposs-canonical Nov 14, 2023

juditnovak Nov 14, 2023

carlcsaposs-canonical Nov 14, 2023

juditnovak Nov 14, 2023

carlcsaposs-canonical Nov 14, 2023

carlcsaposs-canonical Nov 14, 2023

juditnovak Nov 14, 2023

juditnovak Nov 14, 2023

carlcsaposs-canonical left a comment

[DPE-2882] Addressing issues 108 and 111 #113

[DPE-2882] Addressing issues 108 and 111 #113

Conversation

juditnovak commented Nov 2, 2023

juditnovak Nov 13, 2023 • edited Loading

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

juditnovak commented Nov 14, 2023

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

carlcsaposs-canonical left a comment

Choose a reason for hiding this comment

juditnovak Nov 13, 2023 •

edited

Loading