support backup preservation [BF-2219] #171

kathia-barahona · 2023-09-11T13:44:49Z

About this change: What it does, why it matters

Supports request for preserving/holding a backup till a particular date, this way we can guarantee that the backup is not deleted during a restoration request.

If the oldest backup is being preserved, myhoard will not purge any backup but will keep generating new backups. Purging is resumed till the preservation date is met.

How to request a backup preservation?

For preserving a backup, a request to the HTTP API needs to be performed. For example:

PUT /backup/valid_stream_id_1234/preserve
{
    "preserve_until": "2023-09-12T13:30:00"
}

This will trigger the controller on fetching the respective backup stream and storing preserved_info.json on the object storage, it will contain the request preserve_until. After storing the file, all backups metadata are immediately refreshed and myhoard considers updated preserve_until values before trying to purge old backups.

How to cancel a backup preservation?

PUT /backup/valid_stream_id_1234/preserve
{
    "preserve_until": null,
}

This will trigger the controller on fetching the respective backup stream and storing preserved_info.json on the object storage, but will store an empty "preseve_until" value.

codecov-commenter · 2023-09-12T13:07:21Z

Codecov Report

Merging #171 (707c475) into master (9654217) will increase coverage by 0.03%.
The diff coverage is 70.47%.

@@            Coverage Diff             @@
##           master     #171      +/-   ##
==========================================
+ Coverage   78.77%   78.81%   +0.03%     
==========================================
  Files          17       17              
  Lines        4400     4493      +93     
  Branches      995     1016      +21     
==========================================
+ Hits         3466     3541      +75     
- Misses        696      716      +20     
+ Partials      238      236       -2

Files	Coverage Δ
myhoard/controller.py	`82.43% <89.79%> (+0.69%)`	⬆️
myhoard/backup_stream.py	`79.00% <68.42%> (-0.01%)`	⬇️
myhoard/web_server.py	`73.93% <45.94%> (-3.08%)`	⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Mulugruntz

Looks good and functional. The tests are also great!
However, there are 2 main points that might need refinement:

the http endpoints could be better
the named tuple PreservationRequest brings no value and obfuscates the code a bit.

On less important points (not blocking blocking):

the wasteful iteration over state["backups"] to find the one we want with stream_id looks like it's not the right data structure. I'm not sure if there's a reason for it being a list.
type hints missing here and there
some boolean expressions

myhoard/backup_stream.py

Mulugruntz · 2023-09-12T14:04:36Z

myhoard/controller.py

+    def mark_backup_preservation(self, preservation_request: PreservationRequest) -> None:
+        backup_to_preserve = self._get_backup_by_stream_id(preservation_request.stream_id)
+        if not backup_to_preserve:
+            raise Exception(f"Stream {preservation_request.stream_id} was not found in completed backups.")


Let's not raise base Exception. We can also create our own. It'll be easier to handle them.

Why? This is the pattern used in the entire code base.

Oh, you're right. Then this becomes out of scope. I'll resolve this. Maybe we can create a ticket for new-developer, WDYT?

myhoard/controller.py

Mulugruntz · 2023-09-12T14:21:18Z

myhoard/controller.py

        for backup in self.state["backups"]:
            if backup["stream_id"] == stream_id:
-                return backup["site"]
-        raise KeyError(f"Stream {stream_id} not found in backups")
+                return backup


Do we often call that? If we always want to access backups by their stream_id, it might be worth considering changing self.state["backups"] from a list[Backup] to a dict[stream_id, Backup].

I don't think this should be in the scope of the ticket though. But depending on your opinion and knowledge, it might be worth creating a ticket for this (if it makes sense), good for new-developer.

We do filter backups by stream_id lots of times... but that sounds as a major change and multiple places on the code will be affected, not sure if it will be worth it at the end of the day.

Okay, out of scope then. 👍
At the end of the review, I'll create a few tickets about all the points mentioned, to keep track of them and they can be prioritized by Amichai at a later date.

#175 handles this

Mulugruntz · 2023-09-12T14:28:14Z

myhoard/controller.py

@@ -1408,11 +1434,12 @@ def _purge_old_binlogs(self, *, mysql_maybe_not_running=False):
            self.stats.gauge_float("myhoard.binlog.time_since_any_purged", current_time - last_purge)
            self.stats.gauge_float("myhoard.binlog.time_since_could_have_purged", current_time - last_could_have_purged)

-    def _refresh_backups_list(self):
+    def _refresh_backups_list(self, force_refresh: bool = False):


Suggested change

def _refresh_backups_list(self, force_refresh: bool = False):

def _refresh_backups_list(self, force_refresh: bool = False) -> List[Backup]:

this requires a lot of changes since Controller.get_backup_list is not returning List[Backup], is returning List[Dict[.....]]. I rather keep this PR small and not involve any refactoring work that is not required. I'll create an issue for this.

Opened #175 for this

Mulugruntz · 2023-09-12T14:28:24Z

myhoard/controller.py

        interval = self.backup_refresh_interval_base
        if self.mode == self.Mode.active:
            interval *= self.BACKUP_REFRESH_ACTIVE_MULTIPLIER
-        if time.time() - self.state["backups_fetched_at"] < interval:
+
+        if force_refresh is False and time.time() - self.state["backups_fetched_at"] < interval:


Suggested change

if force_refresh is False and time.time() - self.state["backups_fetched_at"] < interval:

if not force_refresh and time.time() - self.state["backups_fetched_at"] < interval:

myhoard/web_server.py

test/test_controller.py

myhoard/web_server.py

README.md

myhoard/backup_stream.py

myhoard/web_server.py

alexole

Clicked a wrong button

myhoard/controller.py

dismissed, some suggestions are not related to this PR

myhoard/controller.py

applied changes

myhoard/web_server.py

rikonen

This has quite a lot of commits that look like internal development history, didn't check if there are some that make sense as individual ones but probably they should mostly be squashed.

myhoard/controller.py

myhoard/web_server.py

myhoard/controller.py

Sometimes, it becomes necessary to retain a backup, as there may be instances where a backup is in the process of being restored, but it suddenly gets removed due to its age and the current amount of backups is surpassing the maximum allowed to be retained. To prevent such scenario, one can request the preservation of a specific backup until a particular date. To initiate this process, a PUT request to /backup/{stream_id}/preserve must be executed. This action will prompt the controller to add the preservation request under 'pending_preservation_requests,' signifying that it will subsequently update the backup's preservation status while in 'active' mode. The preservation of a backup involves the storage of a 'preserve.json' file within the site's object storage, which contains the designated 'preserve until' date." [BF-2219]

applied suggestions and squashed all commits into a final one

rikonen

Leaving still open for the team working on this component nowadays to review but as far as I'm concerned looks good now.

One gotcha this will introduce is that if a backup is preserved for so long that we end up purging a backup that was newer than the one we preserved, we end up with a hole in the stream of binlogs and the mechanism that automatically restores an older backup plus binlogs of all newer backups if we encounter a broken backup will not work. I suspect that won't be an issue in practice as this is presumably just a temporary measure we can use to keep backups for some extra minutes or hours.

kathia-barahona marked this pull request as ready for review September 12, 2023 13:27

kathia-barahona requested a review from a team September 12, 2023 13:27

kathia-barahona force-pushed the preserve_until_backup branch from d6c91c0 to 12be115 Compare September 12, 2023 13:44

kathia-barahona changed the title ~~support backup preservation~~ support backup preservation [BF-2219] Sep 12, 2023

Mulugruntz previously requested changes Sep 12, 2023

View reviewed changes

rikonen previously requested changes Sep 13, 2023

View reviewed changes

myhoard/web_server.py Outdated Show resolved Hide resolved

alexole approved these changes Sep 13, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

myhoard/backup_stream.py Outdated Show resolved Hide resolved

myhoard/backup_stream.py Show resolved Hide resolved

myhoard/backup_stream.py Outdated Show resolved Hide resolved

myhoard/web_server.py Outdated Show resolved Hide resolved

alexole previously requested changes Sep 13, 2023

View reviewed changes

kathia-barahona marked this pull request as draft September 21, 2023 12:31

kathia-barahona commented Sep 22, 2023

View reviewed changes

myhoard/controller.py Outdated Show resolved Hide resolved

kathia-barahona force-pushed the preserve_until_backup branch from aabe449 to 0efe31c Compare September 22, 2023 14:09

kathia-barahona marked this pull request as ready for review September 22, 2023 14:25

kathia-barahona requested review from alexole and rikonen September 22, 2023 14:26

rikonen reviewed Sep 25, 2023

View reviewed changes

myhoard/controller.py Outdated Show resolved Hide resolved

kathia-barahona force-pushed the preserve_until_backup branch from 0efe31c to a67d291 Compare September 25, 2023 07:57

kathia-barahona requested a review from rikonen September 25, 2023 08:29

rikonen reviewed Sep 25, 2023

View reviewed changes

myhoard/web_server.py Show resolved Hide resolved

kathia-barahona force-pushed the preserve_until_backup branch from b468e5c to 5464b20 Compare September 27, 2023 12:32

kathia-barahona requested a review from rikonen September 27, 2023 12:33

rikonen previously requested changes Sep 28, 2023

View reviewed changes

kathia-barahona force-pushed the preserve_until_backup branch 3 times, most recently from 180c599 to 02605fb Compare September 28, 2023 09:45

kathia-barahona force-pushed the preserve_until_backup branch from 02605fb to 707c475 Compare September 28, 2023 11:43

kathia-barahona requested a review from rikonen September 28, 2023 14:55

rikonen approved these changes Sep 29, 2023

View reviewed changes

alexole approved these changes Oct 4, 2023

View reviewed changes

alexole merged commit 9fb9bcc into master Oct 4, 2023
6 checks passed

alexole deleted the preserve_until_backup branch October 4, 2023 07:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support backup preservation [BF-2219] #171

support backup preservation [BF-2219] #171

kathia-barahona commented Sep 11, 2023 •

edited

Loading

codecov-commenter commented Sep 12, 2023 •

edited

Loading

Mulugruntz left a comment •

edited

Loading

Mulugruntz Sep 12, 2023

kathia-barahona Sep 13, 2023

Mulugruntz Sep 13, 2023

Mulugruntz Sep 12, 2023

kathia-barahona Sep 13, 2023

Mulugruntz Sep 13, 2023

kathia-barahona Sep 28, 2023

Mulugruntz Sep 12, 2023

kathia-barahona Sep 21, 2023

kathia-barahona Sep 28, 2023

Mulugruntz Sep 12, 2023

alexole left a comment

rikonen left a comment

rikonen left a comment

	def _refresh_backups_list(self, force_refresh: bool = False):
	def _refresh_backups_list(self, force_refresh: bool = False) -> List[Backup]:

	if force_refresh is False and time.time() - self.state["backups_fetched_at"] < interval:
	if not force_refresh and time.time() - self.state["backups_fetched_at"] < interval:

support backup preservation [BF-2219] #171

support backup preservation [BF-2219] #171

Conversation

kathia-barahona commented Sep 11, 2023 • edited Loading

About this change: What it does, why it matters

How to request a backup preservation?

How to cancel a backup preservation?

codecov-commenter commented Sep 12, 2023 • edited Loading

Codecov Report

Mulugruntz left a comment • 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

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

alexole left a comment

Choose a reason for hiding this comment

rikonen left a comment

Choose a reason for hiding this comment

rikonen left a comment

Choose a reason for hiding this comment

kathia-barahona commented Sep 11, 2023 •

edited

Loading

codecov-commenter commented Sep 12, 2023 •

edited

Loading

Mulugruntz left a comment •

edited

Loading