Conversation
There was a problem hiding this comment.
Pull request overview
Introduces a new composite GitHub Action under gh-actions/infra/database-backup to select an appropriate Juju unit (based on health/role) and execute a database charm backup workflow, reporting results via the GitHub step summary.
Changes:
- Added
backup.pyandbackup_helpers.pyimplementing unit selection,juju statusparsing, backup execution, and summary reporting. - Added a Python unit test suite with
juju statusJSON fixtures covering selection modes, degraded fallback, and failure paths. - Added a GitHub workflow to syntax-check and run the unit tests for this action.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
gh-actions/infra/database-backup/backup.py |
Core unit-selection logic and Juju action execution with summary output |
gh-actions/infra/database-backup/backup_helpers.py |
Shared helpers for command execution, JSON parsing, and summary writing |
gh-actions/infra/database-backup/action.yaml |
Composite action definition wiring inputs to backup.py via env vars |
gh-actions/infra/database-backup/README.md |
Usage docs and input/selection behavior documentation |
gh-actions/infra/database-backup/tests/test_database_backups.py |
Unit tests for selection logic, command handling, and summary behavior |
gh-actions/infra/database-backup/tests/fixtures/healthy-three.json |
Fixture: healthy 3-unit deployment with a single primary |
gh-actions/infra/database-backup/tests/fixtures/unhealthy-replicas.json |
Fixture: replicas unhealthy to validate degraded fallback behavior |
gh-actions/infra/database-backup/tests/fixtures/single-unit.json |
Fixture: single-unit deployment behavior |
gh-actions/infra/database-backup/tests/fixtures/ambiguous-primary.json |
Fixture: invalid status with multiple primaries |
gh-actions/infra/database-backup/tests/fixtures/agent-unhealthy.json |
Fixture: lost agent exclusion behavior |
.github/workflows/test-database-backup.yaml |
CI workflow to run py_compile and unittest for this action |
.gitignore |
Ignores Python __pycache__/ artifacts |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
77ccf2c to
9706015
Compare
92623ec to
fbf7883
Compare
CarlosNihelton
left a comment
There was a problem hiding this comment.
Awesome work. I have only minor nitpicks for your consideration, otherwise LGTM!
| warning: str | None | ||
| workload: str | ||
| agent: str | ||
| excluded: tuple[tuple[str, tuple[str, ...]], ...] |
There was a problem hiding this comment.
This construct is very hard to parse even for a C++ programmer :)
I believe another small dataclass is worth the cost. Something like:
@dataclass(frozen=True)
class Exclusion:
unit: str
reasons: tuple[str, ...]| action=values.get("ACTION", "create-backup"), | ||
| parameters=parameters, | ||
| unit_role=values.get("UNIT_ROLE", "non-primary"), | ||
| timeout=values.get("TIMEOUT", "6h"), |
There was a problem hiding this comment.
Can't we take those default values from granted given they come from the action inputs instead of repeating ourselves for the third time?
There was a problem hiding this comment.
Indeed, we should probably just take those defaults from the action itself and handle the case where these aren't passed as bad calls or broken implementations.
| if len(parsed_units) == 1: | ||
| selected = eligible[0] | ||
| elif unit_role == "any": | ||
| selected = eligible[0] |
There was a problem hiding this comment.
really nitpick: don't you find and or expression easier to read?
| if len(parsed_units) == 1: | |
| selected = eligible[0] | |
| elif unit_role == "any": | |
| selected = eligible[0] | |
| if len(parsed_units) == 1 or unit_role == "any": | |
| selected = eligible[0] |
There was a problem hiding this comment.
Also, if there is only one parsed and eligible unit, we're selecting it without checking the requested role, what also implies we're not considering whether we should or should not mark degraded. For instance, if the input was unit_role == "non-primary" and we get to this point (assuming the solitaire unit is Primary), per the README that would mean a degraded fallback from requested non-primary into a Primary unit due lack of alternatives, right?
Take me with a grain of salt here because I'm not super familiar with the intricacies of the semantics of Juju.
There was a problem hiding this comment.
There are a few things that only having one eligible unit could mean. Either we only had one unit to start with, all replica units are not eligible (in error states or the like), or all units but one replica unit are not eligible.
We explicitly abort if we can't find any "Primary" unit since that's probably a situation where we should have manual intervention.
Otherwise yes, we should probably consider the case when there is only one eligible unit, but the role we want is non-primary to be "degraded" since that either means all replicas are non-eligible, or there's only one unit and the user should have used a different role in the first place.
didrocks
left a comment
There was a problem hiding this comment.
Good work again and nice that there are tests for the backup itself too! This looks really solid to me.
|
|
||
| - name: Syntax check all Python files | ||
| run: | | ||
| while IFS= read -r file; do |
There was a problem hiding this comment.
I always have to double check that scripts are running with set -e by default, but it does :)
matthew-hagemann
left a comment
There was a problem hiding this comment.
I don't think we need a generalized backup solution as Ubuntu Desktop. Everyone else working on greenfield projects in the team should default to PostgreSQL and request managed DB's from IS, who need to be in charge of this operations.
For us, I think the only exception is the mediawiki operator, as we are tied to MySQL and need a solution for that. I feel that what would be better is a much smaller action, similar to PE's backup action for PostgreSQL (see comments on #133 re jaas auth).
Because we know it's a MySQL DB, we don't need to discover which actions are available or reconstruct topology by parsing juju status
There is the constraint of if its a multi-node setup or not. I'd still keep it simple and in bash, or as a flag. A rough sketch based on PE's action and some idea's re not targeting the leading unit from landscape
inputs:
application:
description: "Juju application to back up"
default: "mysql-k8s"
no-primary:
description: "Target a non-primary unit (required for multi-unit MySQL)"
default: "true"#!/usr/bin/env bash
set -euo pipefail
APP="${APPLICATION:-mysql-k8s}"
NO_PRIMARY="${NO_PRIMARY:-true}"
TIMEOUT="${TIMEOUT:-4h}"
# get-cluster-status must run on the leader (per charm docs).
# results.status is the InnoDB status encoded as a JSON *string* -> decode twice.
topology="$(
juju run -m "$MODEL" "${APP}/leader" get-cluster-status --format=json \
| jq -r '.[].results.status' \
| jq '.defaultReplicaSet.topology'
)"
count="$(jq 'length' <<<"$topology")"
if [[ "$count" -eq 0 ]]; then
echo "Empty cluster topology" >&2; exit 1
elif [[ "$count" -eq 1 || "$NO_PRIMARY" != "true" ]]; then
# Single-unit cluster (primary may back up itself), or caller opted out.
label="$(jq -r 'keys[0]' <<<"$topology")"
else
# Multi-unit: MUST target a healthy non-primary (ONLINE + R/O).
label="$(jq -r '
to_entries
| map(select(.value.status == "ONLINE" and .value.mode == "R/O"))
| (.[0].key // empty)' <<<"$topology")"
[[ -n "$label" ]] || { echo "No ONLINE non-primary unit available; refusing to back up the primary." >&2; exit 1; }
fi
unit="$(sed -E 's/-([0-9]+)$/\/\1/' <<<"$label")" # mysql-k8s-0 -> mysql-k8s/0
echo "Backing up $unit (cluster member $label)"
# The charm validates health and refuses the primary in multi-unit clusters.
juju run -m "$MODEL" "$unit" create-backup --wait="$TIMEOUT"I'd keep validation as dumb as possible. We would know our deployment is multi-node or not, and I'd let the MySQL operator perform validation if we are targeting the wrong unit and complain to us, instead of having too much code trying to pick the right unit to calll to.
As far as I'm aware, part of the issue is that the charm doesn't do any validation about if you are targeting the primary or not and if you should. The implications being that it backs up against an out of sync unit or broken unit, potentially causing a silently bad or corrupted backup, or we target the primary and cause excess load on the writing primary unit. Once you have all that unit selection logic in place, it doesn't take that much more to genericisize it for different solutions. |
lib does. Seems to check you are not targeting the primary when you shouldn't. There are also plenty of checks that you are targeting an online unit, on a cluster in a valid state etc. |
Ah, I didn't see that before. Either way though, even with the checks to make it fail-safe, you still need to target the correct unit for the backup to run correctly. Plus, with something like this, I'd rather not risk anything when we don't need to. The alternative would be to loop through all the units until something works, and that's honestly kind of silly. On using |
|
On simplifying to use something like a bash script, there's admittedly a good amount of logic not needed for the core operations here. But they're mostly for ease of user use, debugging, and summary generation. Once stuff for that is added to the bash script, the size and complexity isn't that different and it's just easier to use Python, also taking advantage of unit testing with Python. Plus, a lot of the abstractions are made in mind with the credentials backup handling done in the other PR. |
Here, we introduce a new action to handle database backups on our infra.
The general flow is selecting a healthy unit of a Juju application and then running a database charm backup action on it. We assume that Juju is already authenticated.
Note that special consideration needs to be made when deciding which unit to run the backup action against. In multi-unit deployments, we typically want to target non-primary units. The exception is with non-TLS deployments, where we must target the primary. With this in mind, we need to have different modes depending on what we would like to do.
However, the only standardized way between the common DB charms, PostgreSQL and MySQL, of determining which unit is the primary is by using
juju status. Actions likeget-cluster-statusandget-primaryare not always available.Additionally, in cases where there is a broken unit, we want to avoid it and not include it for consideration.
For the
dry-runcase, we only runlist-backups, making no writes.Note that this action does not handle backup expiration. For PostgreSQL, this should be handled by configuring
experimental-delete-older-than-dayswith thes3-integratorcharm. For MySQL, this needs to be handled carefully with S3 lifecycle rules in such a way thatgroup_replication_id.txtdoes not expire.Refs:
https://canonical.com/data/mysql/docs/8.0/how-to/back-up-and-restore/create-a-backup/
https://canonical.com/data/postgresql/docs/14/how-to/back-up-and-restore/create-a-backup/
UDENG-11019