clients: Implement storage fallback for recordings #1303

victorges · 2024-06-20T20:19:58Z

This implements the storage fallback logic following the design doc.

This builds on top of livepeer/go-tools#121 and #1302

The trickiest bits were the error handling for both manifest and segment file reading. We need to handle not found
errors explicitly from primary and backup to return appropriate errors to the caller.

This is incomplete as we still need to fix the probing made on the manifest itself instead of just on each segment. Rabbit hole.

…liable storage Just proxies to Minio with a random error chance

mjh1 · 2024-07-04T11:49:28Z

@victorges I've done a refactor of this now so that we only check the backup location at the beginning of the job, working out which manifest to use and writing out a new manifest with absolute URLs if necessary. Then probing, clipping etc works as before (in theory). Does it look ok to you so far?

victorges

LGTM! (can't approve it myself)

clients/manifest.go

errors/errors.go

main.go

pipeline/ffmpeg.go

mjh1 · 2024-07-11T09:50:35Z

@victorges please could you give this one more sign off?

leszko

Added some comments, other than that LGTM

config/storage_backup_url.go

main.go

test/steps/ffmpeg.go

leszko · 2024-07-15T07:59:44Z

clients/manifest.go

+	if errPrimary != nil && !primaryNotFound {
+		return nil, 0, errPrimary
+	}
+	if errBackup != nil && !backupNotFound {


This is weird, if the primary works fine but the backup is broken, we should not return an error I guess. We should return the playlist from the primary bucket, isn't that the case?

If it's not found in the backup then we return the primary, but unfortunately we need to successfully check for the backups existence because we need to pick the largest of the two manifests lower down. If we failed to check the backup exists then it could be it does exist and is the more complete, larger of the two but we've missed that and gone ahead with the primary.

So, we want to fail completely if there is an error from the backup?

Yes, the backup should always be working but just giving us a 404 most of the time

leszko · 2024-07-15T08:05:50Z

clients/manifest.go

+	err = backoff.Retry(func() error {
+		rc, err := GetFile(context.Background(), requestID, sourceManifestOSURL, dStorage)
+		if err != nil {
+			if time.Since(start) > 10*time.Second && errors.IsObjectNotFound(err) {


What is this one? We anyway have the deadline in DownloadRetryBackOff() set to 10 * 5s = 50s, why do also need this check?

I'll add a comment for this, we annoyingly had to include this because there's a chance that we will get a not found due to eventual consistency, so we still want to retry not found errors. However we don't want to wait quite as long as normal errors because it'll be quite a common case where the backup doesn't exist. So we don't to add a whole 50s of delay to every recording job essentially.

Might be worth a lil constant for the 10s too. maybe const MANIFEST_NOT_FOUND_INCONSISTENCY_TOLERANCE = 10s?

Yeah, I'm fine with with extracting a constant plus adding a comment.

clients/manifest_test.go

victorges

LGTM!

victorges · 2024-07-16T20:22:39Z

clients/manifest.go

+	err = backoff.Retry(func() error {
+		rc, err := GetFile(context.Background(), requestID, sourceManifestOSURL, dStorage)
+		if err != nil {
+			if time.Since(start) > 10*time.Second && errors.IsObjectNotFound(err) {


Might be worth a lil constant for the 10s too. maybe const MANIFEST_NOT_FOUND_INCONSISTENCY_TOLERANCE = 10s?

victorges · 2024-07-16T20:24:07Z

config/storage_backup_url_test.go

+import "testing"
+
+func TestGetStorageBackupURL(t *testing.T) {
+	StorageFallbackURLs = map[string]string{"https://storj.livepeer.com/catalyst-recordings-com/hls": "https://google.livepeer.com/catalyst-recordings-com/hls"}


do we need to restore the default value after tests?

Suggested change

StorageFallbackURLs = map[string]string{"https://storj.livepeer.com/catalyst-recordings-com/hls": "https://google.livepeer.com/catalyst-recordings-com/hls"}

StorageFallbackURLs = map[string]string{"https://storj.livepeer.com/catalyst-recordings-com/hls": "https://google.livepeer.com/catalyst-recordings-com/hls"}

defer func() { StorageFallbackURLs = nil }()

victorges force-pushed the vg/feat/storage-fallback branch 5 times, most recently from c90b2dc to 54d5168 Compare June 21, 2024 12:53

victorges force-pushed the vg/fix/not-found-err-handling branch from 3bbdb4a to bbf1581 Compare June 21, 2024 13:07

victorges force-pushed the vg/feat/storage-fallback branch from 54d5168 to de8eaad Compare June 21, 2024 13:08

victorges force-pushed the vg/fix/not-found-err-handling branch from bbf1581 to 63c4373 Compare June 21, 2024 14:50

victorges force-pushed the vg/feat/storage-fallback branch from de8eaad to 91d01c4 Compare June 21, 2024 14:51

victorges changed the title ~~Vg/feat/storage fallback~~ clients: Implement storage fallback for recordings Jun 21, 2024

victorges marked this pull request as ready for review June 21, 2024 22:38

victorges force-pushed the vg/fix/not-found-err-handling branch from 63c4373 to e386c88 Compare June 24, 2024 17:34

victorges force-pushed the vg/feat/storage-fallback branch from ce59e24 to 75789c3 Compare June 24, 2024 17:35

mjh1 force-pushed the vg/feat/storage-fallback branch 2 times, most recently from 068ed63 to 101c949 Compare July 3, 2024 16:53

Base automatically changed from vg/fix/not-found-err-handling to main July 3, 2024 17:33

victorges and others added 7 commits July 3, 2024 18:39

config: Create storage-fallback-urls flag

35c9df9

config: Make the StorageFallbackURLs a global

346b28a

clients: Download manifest from backup OS

613a4b0

clients: Download segments from backup URL

63e8ee6

[WIP] clients: Use storage fallback for probing segments too

0f8d91b

This is incomplete as we still need to fix the probing made on the manifest itself instead of just on each segment. Rabbit hole.

[DEV-revert] box-only: Start a local proxy server to simulate an unre…

a051c8f

…liable storage Just proxies to Minio with a random error chance

Add cucumber test

3328231

mjh1 force-pushed the vg/feat/storage-fallback branch from 101c949 to 3328231 Compare July 3, 2024 17:41

mjh1 added 2 commits July 4, 2024 12:43

Refactor

f83502e

remove comment

8623feb

mjh1 added 3 commits July 4, 2024 12:52

fix unit test. (cukes still need fixing)

bc71d54

Fix cukes

557ae90

add unit tests

2e73ea3

victorges commented Jul 9, 2024

View reviewed changes

mjh1 added 3 commits July 10, 2024 21:27

Address TODOs and review comments

a87b5cc

Merge branch 'main' into vg/feat/storage-fallback

90d6964

return variable no longer needed

0655802

mjh1 requested review from thomshutt and leszko and removed request for thomshutt July 11, 2024 09:50

leszko approved these changes Jul 15, 2024

View reviewed changes

mjh1 added 2 commits July 16, 2024 17:24

Merge branch 'main' into vg/feat/storage-fallback

d5122a8

review comments

cac0c5f

victorges commented Jul 16, 2024

View reviewed changes

mjh1 added 2 commits July 25, 2024 10:34

Merge branch 'main' into vg/feat/storage-fallback

391709e

Review comments

44e0fd8

mjh1 merged commit ac85dfd into main Jul 25, 2024
11 checks passed

mjh1 deleted the vg/feat/storage-fallback branch July 25, 2024 10:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clients: Implement storage fallback for recordings #1303

clients: Implement storage fallback for recordings #1303

victorges commented Jun 20, 2024 •

edited

Loading

mjh1 commented Jul 4, 2024

victorges left a comment

mjh1 commented Jul 11, 2024

leszko left a comment

leszko Jul 15, 2024

mjh1 Jul 16, 2024

leszko Jul 17, 2024

mjh1 Jul 25, 2024

leszko Jul 15, 2024

mjh1 Jul 16, 2024 •

edited

Loading

victorges Jul 16, 2024

leszko Jul 17, 2024 •

edited

Loading

victorges left a comment

victorges Jul 16, 2024

victorges Jul 16, 2024

	StorageFallbackURLs = map[string]string{"https://storj.livepeer.com/catalyst-recordings-com/hls": "https://google.livepeer.com/catalyst-recordings-com/hls"}
	StorageFallbackURLs = map[string]string{"https://storj.livepeer.com/catalyst-recordings-com/hls": "https://google.livepeer.com/catalyst-recordings-com/hls"}
	defer func() { StorageFallbackURLs = nil }()

clients: Implement storage fallback for recordings #1303

clients: Implement storage fallback for recordings #1303

Conversation

victorges commented Jun 20, 2024 • edited Loading

mjh1 commented Jul 4, 2024

victorges left a comment

Choose a reason for hiding this comment

mjh1 commented Jul 11, 2024

leszko 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

mjh1 Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leszko Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

victorges left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victorges commented Jun 20, 2024 •

edited

Loading

mjh1 Jul 16, 2024 •

edited

Loading

leszko Jul 17, 2024 •

edited

Loading