move most backup controller logic into pkg/backup

[skriss]

Partially addresses #131

This PR moves most of the logic related to validation, defaulting and execution of backups out of the backup controller and into pkg/backup under a new type, Processor (we can bikeshed on the name, I just picked something generic for now). For the most part, this was a copy-paste effort.

If you look at the backup controller now, you'll see that it now only grabs a queue key, tries to fetch a backup with that namespace/name from the lister, and then immediately calls into pkg/backup via the backupProcessor to handle the rest of the logic. (ref https://github.com/skriss/velero/blob/move-backup-validation/pkg/controller/backup_controller.go#L91-L112)

I've done basic testing here and things seem to run fine but I'd like to do a little more before being ready for merge.

In the mean time, let me know if you have any thoughts on approach! I'm working on the same for restore but I'll hold off on pushing a PR until everyone's happy with how this one looks.

cc @nrb @ashish-amarnath

[nrb]

I'm 👍 on the approach. I like keeping a struct around that gets added as a field on the controller; for the CSI support, I was going to add another client onto the backup controller, but it makes far more sense to be a client on the Processor (or whatever it's ultimately named), and the controller just manages the loop.

[nrb]

I like the Processor name, especially combined with the package name (backup.Processor, restore.Processor for the next one). The one thing I would nitpick about the name is the stutter in Processor.Process. Maybe Processor.Run or Processor.Do?

As far as alternatives for this type, just to strawman:

Backupper is already taken, so that doesn't fit here unless we want revisit renaming most of the package's high level types, which I'm not really wanting to do.

To get more specific. maybe RequestProcessor, since it only processes requests for new backups?

Or maybe shortening that, Requester and Requester.Process?

[skriss]

Thanks for the input. I think I prefer Processor.Run, RequestProcessor.Run, or something along those lines. Agree re: Backupper - it might've made sense to name this type Backupper, and change the existing one to something else, but I don't really want to go down that path right now.

[skriss]

@carlisia @ashish-amarnath any inputs on naming here?

[ashish-amarnath]

Processor.Run() gets my vote

[carlisia]

I like making it more specific and explicit, so RequestProcessor is my favorite. Processor is not as clear imo. Requester is even less clear, in this context.

As for RequestProcessor.Run, the .Run() method does : "validates, defaults, runs and persists". It'd be nice to have a name that wasn't one of the actual thing it does, but I'm coming up empty. I'm not a fan of .Do(). Maybe .Execute()? I'd be good with .Run() tho, nbd.

[skriss]

I'm likely gonna have to address some conflicts in this before it's ready to go, putting on hold for the moment.

[ashish-amarnath]

should this just be 2020?

[skriss]

maybe, I'm actually not sure - I copied it over as-is from backup_controller.go since most of the code in this file came from there but I don't know what the right policy is here.

[carlisia]

I "think" it being a new file, it should be 2020. Only if it was a new file with the entirety of the code from the controller being moved (in essence, renaming and moving the entire file) it would make sense "to me".

[skriss]

This is now out of date so I'm going to close it; I think we'll need to end up tackling this as part of the worker pods effort.

[nrb]

I can also see this as a precursor to moving to controller-runtime/kubebuilder.

[carlisia]

Whichever is first? 😄