coop-cloud/abra
coop-cloud
/
abra
5
2
Fork 15
Code Issues Pull Requests 4 Packages Releases 36 Activity

feat: backup revolution #361

Merged
decentral1se merged 7 commits from backup-revolution into main 2024-01-12 20:52:34 +00:00
Conversation 5 Commits 7 Files Changed 10 +401 -559
decentral1se commented 2023-10-01 06:24:12 +00:00
Owner
Copy Link

See coop-cloud/organising#485.

Building on work in coop-cloud/backup-bot-two#23.

Agreed on in R011: https://docs.coopcloud.tech/federation/resolutions/passed/011

See https://git.coopcloud.tech/coop-cloud/organising/issues/485. Building on work in https://git.coopcloud.tech/coop-cloud/backup-bot-two/pulls/23. Agreed on in R011: https://docs.coopcloud.tech/federation/resolutions/passed/011
decentral1se added 1 commit 2023-10-01 06:24:13 +00:00
continuous-integration/drone/push Build is passing Details
continuous-integration/drone/pr Build is passing Details
cf37076865
WIP: backup revolution 
See coop-cloud/organising#485
decentral1se force-pushed backup-revolution from cf37076865 to ed0ab7fb90 2023-10-04 20:52:23 +00:00 Compare
decentral1se force-pushed backup-revolution from ed0ab7fb90 to c4c73d2ba8 2023-10-10 06:43:41 +00:00 Compare
decentral1se force-pushed backup-revolution from c4c73d2ba8 to 17513967de 2023-10-10 06:50:22 +00:00 Compare
decentral1se force-pushed backup-revolution from 17513967de to 02a83057fc 2023-10-11 07:21:27 +00:00 Compare
moritz requested changes 2023-10-11 13:12:20 +00:00
moritz left a comment
Member
Copy Link

Thank you for the work! So existing, we're getting very close to abra backupbot integration!

Thank you for the work! So existing, we're getting very close to `abra` backupbot integration!
🎉 1
cli/app/backup.go Outdated
@ -414,0 +392,4 @@
}
if targetPath != "" {
logrus.Debugf("including TARGET=%s in backupbot exec invocation", targetPath)
execEnv = append(execEnv, fmt.Sprintf("TARGET=%s", targetPath))
moritz commented 2023-10-11 13:09:52 +00:00
Member
Copy Link

The TARGET env need to be part of restore. For listing snapshots it's not relevant.

The `TARGET` env need to be part of restore. For listing snapshots it's not relevant.
decentral1se commented 2023-10-11 15:12:47 +00:00
Author
Owner
Copy Link

Ah yes, nice one. Removed and added to restore logic. Force pushed but unsure if it's showing up yet. Thanks for the review!

Ah yes, nice one. Removed and added to `restore` logic. Force pushed but unsure if it's showing up yet. Thanks for the review!
decentral1se marked this conversation as resolved
decentral1se force-pushed backup-revolution from 02a83057fc to 5127966438 2023-10-11 14:52:28 +00:00 Compare
decentral1se commented 2023-10-11 15:13:16 +00:00
Author
Owner
Copy Link

I'm gonna try put out a point release before merging this.

Commences integration test suite engines...

I'm gonna try put out a point release before merging this. *Commences integration test suite engines...*
moritz reviewed 2023-10-12 10:52:33 +00:00
cli/app/restore.go Outdated
@ -135,0 +98,4 @@
AttachStderr: true,
AttachStdin: true,
AttachStdout: true,
Cmd: []string{"/usr/bin/backup", "--", "snapshots"},
moritz commented 2023-10-12 10:52:33 +00:00
Member
Copy Link

snapshots need to be replaced with restore

`snapshots` need to be replaced with `restore`
👍 1
decentral1se marked this conversation as resolved
decentral1se force-pushed backup-revolution from 5127966438 to 63387a011a 2023-10-12 13:09:21 +00:00 Compare
decentral1se force-pushed backup-revolution from 63387a011a to 7ee89d3a8f 2024-01-09 10:47:10 +00:00 Compare
decentral1se commented 2024-01-09 11:25:42 +00:00
Author
Owner
Copy Link

Trying to finish the last step of this to get it off my plate...

Final steps for abra app backup download ... implementation...

Trying to finish the last step of this to get it off my plate... Final steps for `abra app backup download ...` implementation...
decentral1se force-pushed backup-revolution from 7ee89d3a8f to 529cb9bd12 2024-01-11 14:38:26 +00:00 Compare
decentral1se changed title from WIP: backup revolution to feat: backup revolution 2024-01-11 14:39:00 +00:00
decentral1se commented 2024-01-11 14:40:11 +00:00
Author
Owner
Copy Link

Ready For Review 🎉

Ready For Review 🎉
decentral1se requested review from moritz 2024-01-11 15:04:07 +00:00
decentral1se requested review from p4u1 2024-01-11 15:04:16 +00:00
p4u1 reviewed 2024-01-11 15:38:42 +00:00
cli/app/backup.go
@ -135,3 +83,1 @@
if !ok {
logrus.Fatalf("no backup config for %s? does %s exist?", serviceName, serviceName)
}
chosenService, err := service.GetServiceByLabel(context.Background(), cl, config.BackupbotLabel, internal.NoInput)
p4u1 commented 2024-01-11 15:37:27 +00:00
Member
Copy Link

Could add a helper function to retrieve the backup bot container

Could add a helper function to retrieve the backup bot container
p4u1 marked this conversation as resolved
cli/app/backup.go
@ -153,3 +112,1 @@
logrus.Fatal(err)
}
}
execBackupListOpts := types.ExecConfig{
p4u1 commented 2024-01-11 15:31:20 +00:00
Member
Copy Link

Maybe add a helper functions to exec the backup script on the remote container?

Maybe add a helper functions to exec the backup script on the remote container?
p4u1 marked this conversation as resolved
cli/app/restore.go Outdated
@ -40,4 +38,3 @@
},
Before: internal.SubCommandBefore,
BashComplete: autocomplete.AppNameComplete,
Description: `
p4u1 commented 2024-01-11 15:21:04 +00:00
Member
Copy Link

Why remove the description? :)

Why remove the description? :)
decentral1se commented 2024-01-12 15:51:06 +00:00
Author
Owner
Copy Link

Think I was waiting for design and/or direction. See #361 (comment). Will see how that discussion pans out and re-instate something.

Think I was waiting for design and/or direction. See https://git.coopcloud.tech/coop-cloud/abra/pulls/361#issuecomment-19436. Will see how that discussion pans out and re-instate something.
👍 1
p4u1 marked this conversation as resolved
cli/app/restore.go Outdated
@ -32,4 +30,2 @@
Name: "restore",
Aliases: []string{"rs"},
Usage: "Run app restore",
ArgsUsage: "<domain> <service> <file>",
p4u1 commented 2024-01-11 15:28:43 +00:00
Member
Copy Link

Is it possible to set a backup file (which was previously downloaded) here?

Is it possible to set a backup file (which was previously downloaded) here?
moritz commented 2024-01-11 17:28:30 +00:00
Member
Copy Link

At the moment the backupbot only support to restore from restic snapshots directly. I'm not sure if this must be implemented in the backupbot or if abra could do the restore completely by itself.

At the moment the backupbot only support to restore from restic snapshots directly. I'm not sure if this must be implemented in the backupbot or if abra could do the restore completely by itself.
p4u1 commented 2024-01-12 11:18:34 +00:00
Member
Copy Link

Yes, I guess the backupbot does not necessarily need to do a manual backup. This can also be implemented in abra. @decentral1se Maybe the code path for restoring from a local backup should be kept.

Yes, I guess the backupbot does not necessarily need to do a manual backup. This can also be implemented in abra. @decentral1se Maybe the code path for restoring from a local backup should be kept.
decentral1se commented 2024-01-12 15:50:28 +00:00
Author
Owner
Copy Link

@p4u1 @moritz looking at this again, I'm not sure how to proceed. There is still some outstanding discussion re: design of restore? See coop-cloud/backup-bot-two#26 and coop-cloud/backup-bot-two#42? We're already wildly over budget for this work, so not sure what can be done beyond what has been implemented now. Let me know what you think.

@p4u1 @moritz looking at this again, I'm not sure how to proceed. There is still some outstanding discussion re: design of restore? See https://git.coopcloud.tech/coop-cloud/backup-bot-two/issues/26 and https://git.coopcloud.tech/coop-cloud/backup-bot-two/issues/42? We're already wildly over budget for this work, so not sure what can be done beyond what has been implemented now. Let me know what you think.
p4u1 commented 2024-01-12 16:40:05 +00:00
Member
Copy Link

Maybe we need a meeting to discuss this? I think the feature to restore from a local backup is pretty important. And I guess creating a backup to the local machine should also still be possible.

Maybe we need a meeting to discuss this? I think the feature to restore from a local backup is pretty important. And I guess creating a backup to the local machine should also still be possible.
👍 1
decentral1se marked this conversation as resolved
pkg/service/service.go Outdated
@ -16,1 +16,4 @@
// GetService retrieves a service container based on a label. If prompt is true
// and the retrievd count of service containers does not match 1, then a prompt
// is presented to let the user choose. A count of 0 is handled gracefully.
p4u1 commented 2024-01-11 15:19:25 +00:00
Member
Copy Link

" A count of 0 is handled gracefully." What does this mean? If I understood the code, an error gets returned, when no service was found.

" A count of 0 is handled gracefully." What does this mean? If I understood the code, an error gets returned, when no service was found.
p4u1 marked this conversation as resolved
pkg/service/service.go
@ -17,0 +27,4 @@
return swarm.Service{}, fmt.Errorf("no services deployed?")
}
var backupServices []swarm.Service
p4u1 commented 2024-01-11 15:17:42 +00:00
Member
Copy Link

The variable name should not contain "backup", since this is function is not specific to backup right?

The variable name should not contain "backup", since this is function is not specific to backup right?
p4u1 marked this conversation as resolved
pkg/upstream/container/exec.go Outdated
@ -15,2 +15,3 @@
func RunExec(dockerCli command.Cli, client *apiclient.Client, containerID string, execConfig *types.ExecConfig) error {
func RunExec(dockerCli command.Cli, client *apiclient.Client, containerID string,
execConfig *types.ExecConfig) (io.Writer, error) {
p4u1 commented 2024-01-11 15:16:12 +00:00
Member
Copy Link

Can you add a doc comment for this function and explain what the io.Writer is that gets returned?

Can you add a doc comment for this function and explain what the io.Writer is that gets returned?
p4u1 commented 2024-01-11 15:30:02 +00:00
Member
Copy Link

Also is the returned io.Writer used anywhere? I could not find any usage of the first return value

Also is the returned io.Writer used anywhere? I could not find any usage of the first return value
decentral1se commented 2024-01-12 16:15:59 +00:00
Author
Owner
Copy Link

So far the out is ignored but I think we'll eventually want it. It's a pretty involved refactor (simple, but changes a few things in a few places), so I'd rather leave it as is for now.

So far the `out` is ignored but I think we'll eventually want it. It's a pretty involved refactor (simple, but changes a few things in a few places), so I'd rather leave it as is for now.
👍 1
p4u1 marked this conversation as resolved
p4u1 commented 2024-01-11 15:39:39 +00:00
Member
Copy Link

I'm really looking forward to the backupbot integration in abra! I didn't test it localy yet. Just added a few comments to the code :)

I'm really looking forward to the backupbot integration in abra! I didn't test it localy yet. Just added a few comments to the code :)
🚀 1
decentral1se added 6 commits 2024-01-12 16:15:09 +00:00
continuous-integration/drone/pr Build is passing Details
continuous-integration/drone/push Build is passing Details
f7ded7ef87
fixup! feat: backup revolution
p4u1 reviewed 2024-01-12 16:35:40 +00:00
cli/app/backup.go
@ -135,3 +75,1 @@
if !ok {
logrus.Fatalf("no backup config for %s? does %s exist?", serviceName, serviceName)
}
targetContainer, err := internal.RetrieveBackupBotContainer(cl)
p4u1 commented 2024-01-12 16:35:40 +00:00
Member
Copy Link

I think backupContainer is a better variable name here and at the other occurrences

I think `backupContainer` is a better variable name here and at the other occurrences
decentral1se marked this conversation as resolved
decentral1se commented 2024-01-12 20:52:24 +00:00
Author
Owner
Copy Link

Restore functionality needs to be worked out, a meeting is in the works... for another PR!

Restore functionality needs to be worked out, a meeting is in the works... for another PR!
decentral1se merged commit f7ded7ef87 into main 2024-01-12 20:52:34 +00:00
decentral1se deleted branch backup-revolution 2024-01-12 20:52:34 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
moritz
p4u1
Labels
Clear labels   
bug

Something is not working

  
build

go build related issues

  
ci/cd

Building things with CI/CD

  
contributing

Dealing with contributing

  
design

UI/UX

  
documentation

Documenting all the things

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
help wanted

Need some help

  
invalid

Something is wrong

  
meta

relating to the project or repo itself

  
question

More information is needed

  
security

Security related

  
wontfix

This won't be fixed

No Label
bug
build
ci/cd
contributing
design
documentation
duplicate
enhancement
help wanted
invalid
meta
question
security
wontfix
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: coop-cloud/abra#361
No description provided.
Delete Branch "backup-revolution"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?