DPE-2654 profile-limits option #331

paulomach · 2023-09-28T20:30:39Z

Issue

main issue: no fine tune for memory allocation
configurations are being set without validation
configurations changes are not triggering changes in the workload

Solution

Add profile-limit-memory config option
Usage of data platform data-models lib for config validation
Usage of rollingOps lib for workload restart orchestration, when the changed configs requires.
- Add a method to defer primary to be last one to restart (mitigating primary switchover)

Chore

refactored get_available_memory to be simpler
charm typing for relations/mysql.py
typo fix
usage of config values as properties, provided by data-models lib

codecov · 2023-09-28T20:35:44Z

Codecov Report

Merging #331 (ea0ecb9) into main (ef2d363) will increase coverage by 0.48%.
The diff coverage is 69.53%.

@@            Coverage Diff             @@
##             main     #331      +/-   ##
==========================================
+ Coverage   64.01%   64.50%   +0.48%     
==========================================
  Files          16       17       +1     
  Lines        2993     3113     +120     
  Branches      389      413      +24     
==========================================
+ Hits         1916     2008      +92     
- Misses        955      979      +24     
- Partials      122      126       +4

Files	Coverage Δ
src/constants.py	`100.00% <100.00%> (ø)`
src/utils.py	`100.00% <100.00%> (ø)`
lib/charms/mysql/v0/mysql.py	`73.22% <78.57%> (+0.47%)`	⬆️
src/relations/mysql.py	`53.44% <33.33%> (+0.86%)`	⬆️
src/mysql_vm_helpers.py	`60.99% <64.70%> (-0.74%)`	⬇️
src/config.py	`85.45% <85.45%> (ø)`
src/charm.py	`50.78% <50.98%> (-1.14%)`	⬇️

... and 2 files with indirect coverage changes

dpe-2654-profile-limit-option # Conflicts: # lib/charms/mysql/v0/mysql.py # src/charm.py # src/mysql_vm_helpers.py

dpe-2654-profile-limit-option

taurus-forever

Nice. The new code it way more pleasant to read!
One question inside, and I hope the answer there is "no", so LGTM.

taurus-forever · 2023-10-10T08:28:22Z

src/charm.py

+        self.app_peer_data.setdefault(
+            "cluster-name", self.config.cluster_name or f"cluster-{common_hash}"
+        )
+        self.app_peer_data.setdefault("cluster-set-domain-name", f"cluster-set-{common_hash}")


Q: will we overwrite cluster-name and cluster-set-domain-name on the leader move (re-elected)?

by using setdefault we avoid overwriting it

carlcsaposs-canonical · 2023-10-10T13:20:51Z

lib/charms/mysql/v0/mysql.py

+
+        # escape variable values when needed
+        if not re.match(r"^[0-9,a-z,A-Z$_]+$", value):
+            value = f"`{value}`"
+


any reason not to always escape?

also do you need to check that

`

is not in the string?

not really: 096fe7f

actually, we cannot escape always - number will fail. rolledback on ea0ecb9

carlcsaposs-canonical · 2023-10-10T14:20:16Z

src/charm.py

+            self._mysql.write_content_to_file(
+                path=MYSQLD_CUSTOM_CONFIG_FILE, content=new_config_content
+            )
+            self.on[f"{self.restart.name}"].acquire_lock.emit()


Suggested change

self.on[f"{self.restart.name}"].acquire_lock.emit()

self.on[self.restart.name].acquire_lock.emit()

nit

it's like that to shut the linter - self.restart.name is typed ad AnyStr and key should be str

suggest str() instead of f-string

src/config.py

tests/integration/helpers.py

shayancanonical · 2023-10-10T17:29:07Z

src/charm.py

+            # the upgrade already restart the daemon
+            return
+
+        if not self._mysql.is_mysqld_running:


this is not a property. should we add () to invoke the method?

shayancanonical · 2023-10-11T12:50:27Z

lib/charms/mysql/v0/mysql.py

@@ -116,15 +116,17 @@ def wait_until_mysql_connection(self) -> None:

 # Increment this PATCH version before using `charmcraft publish-lib` or reset
 # to 0 if you are raising the major API version
-LIBPATCH = 49
+LIBPATCH = 50


please update to 51 as another PR was merged with libpatch 50

dpe-2654-profile-limit-option

carlcsaposs-canonical · 2023-10-12T06:59:02Z

src/charm.py

@@ -773,8 +762,18 @@ def _join_unit_to_cluster(self) -> None:
            self.unit.status = WaitingStatus("waiting to join the cluster")
            logger.debug("Waiting to joing the cluster, failed to acquire lock.")

-    def _restart(self, _: EventBase) -> None:
+    def _restart(self, event: EventBase) -> None:


nit: not all events that isinstance EventBase can be deferred

paulomach · 2023-10-13T18:42:08Z

merging besides the CI error - test passed previously, failing due disk full error. Deal with it on another PR

first pass with minimal unit test fixes

0ba4c82

paulomach added 3 commits September 29, 2023 12:43

increasing coverage

ec89ebd

Merge branch 'main' of github.com:canonical/mysql-operator into feature/

54520a8

dpe-2654-profile-limit-option # Conflicts: # lib/charms/mysql/v0/mysql.py # src/charm.py # src/mysql_vm_helpers.py

defer primary restart to be last

44e92f9

paulomach marked this pull request as ready for review September 29, 2023 21:13

paulomach requested review from shayancanonical, taurus-forever and carlcsaposs-canonical September 29, 2023 21:13

paulomach added 9 commits September 30, 2023 11:21

stable now has profile

b5c4659

escape variables dynamically

e1e9c8c

additional static config and defer when not running

40849eb

simpler method for primary retrieval on tests

20fef55

better description

6ec4096

skip handler on upgrade

dd019d4

adds pre-check since upgrade is in stable

fa71dfe

fix get primary for app name with hyphens

af04042

exact match app name

1468586

juditnovak mentioned this pull request Oct 4, 2023

[DPE-2651] Juju3 pipelines / Testing secrets #333

Merged

paulomach mentioned this pull request Oct 5, 2023

Avoid any actions when a unit is offline and allow mysql GR to recover #334

Merged

paulomach added 2 commits October 6, 2023 08:31

Merge branch 'main' of github.com:canonical/mysql-operator into feature/

6a7fa42

dpe-2654-profile-limit-option

fix bad c-c c-v

79829e3

taurus-forever previously approved these changes Oct 10, 2023

View reviewed changes

carlcsaposs-canonical previously approved these changes Oct 10, 2023

View reviewed changes

shayancanonical reviewed Oct 10, 2023

View reviewed changes

paulomach mentioned this pull request Oct 10, 2023

DPE-2653 profile limit option canonical/mysql-k8s-operator#318

Merged

shayancanonical reviewed Oct 11, 2023

View reviewed changes

paulomach added 3 commits October 11, 2023 10:30

defer on restart and bug fix

740b58e

Merge branch 'main' of github.com:canonical/mysql-operator into feature/

521dc6c

dpe-2654-profile-limit-option

always escape

096fe7f

paulomach dismissed stale reviews from carlcsaposs-canonical and taurus-forever via 096fe7f October 11, 2023 14:20

shayancanonical previously approved these changes Oct 11, 2023

View reviewed changes

escape needs to be conditional

ea0ecb9

paulomach dismissed shayancanonical’s stale review via ea0ecb9 October 11, 2023 19:11

carlcsaposs-canonical reviewed Oct 12, 2023

View reviewed changes

carlcsaposs-canonical approved these changes Oct 12, 2023

View reviewed changes

shayancanonical approved these changes Oct 13, 2023

View reviewed changes

paulomach merged commit 1f72f1b into main Oct 13, 2023

paulomach deleted the feature/dpe-2654-profile-limit-option branch October 13, 2023 18:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DPE-2654 profile-limits option #331

DPE-2654 profile-limits option #331

paulomach commented Sep 28, 2023 •

edited

Loading

codecov bot commented Sep 28, 2023 •

edited

Loading

taurus-forever left a comment

taurus-forever Oct 10, 2023

paulomach Oct 10, 2023

carlcsaposs-canonical Oct 10, 2023

carlcsaposs-canonical Oct 10, 2023

paulomach Oct 11, 2023

paulomach Oct 11, 2023

carlcsaposs-canonical Oct 10, 2023

paulomach Oct 11, 2023

carlcsaposs-canonical Oct 12, 2023

shayancanonical Oct 10, 2023

paulomach Oct 11, 2023

shayancanonical Oct 11, 2023

paulomach Oct 11, 2023

carlcsaposs-canonical Oct 12, 2023

paulomach commented Oct 13, 2023

	self.on[f"{self.restart.name}"].acquire_lock.emit()
	self.on[self.restart.name].acquire_lock.emit()

DPE-2654 profile-limits option #331

DPE-2654 profile-limits option #331

Conversation

paulomach commented Sep 28, 2023 • edited Loading

Issue

Solution

Chore

codecov bot commented Sep 28, 2023 • edited Loading

Codecov Report

taurus-forever left a 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

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

paulomach commented Oct 13, 2023

paulomach commented Sep 28, 2023 •

edited

Loading

codecov bot commented Sep 28, 2023 •

edited

Loading