Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resumable plugins rebased #710

Merged
merged 1 commit into from
Jul 31, 2018
Merged

Conversation

skriss
Copy link
Contributor

@skriss skriss commented Jul 27, 2018

This is a rebase & some cleanup to #495. Will squash commits down before merge.

I'd like to do some more manual testing before merge, but my main feedback points have been addressed and we need to get this in now-ish, so any further issues that come up can be addressed in subsequent PRs.

FYI @nrb @carlisia. Don't expect full reviews since it's a lot of code.

@skriss skriss added this to the v0.10.0 milestone Jul 27, 2018
@skriss
Copy link
Contributor Author

skriss commented Jul 27, 2018

Recording a couple of potential action items:

  • this includes breaking changes to the protobuf for plugins. We should reach out to all of our known plugin developers to make sure they're not surprised by this.
  • backup/restore controller - this PR logs the per-backup & restore logs to both a file in object storage AND to stdout, so there's info available if the obj storage upload fails. Do we want to log to stdout in all cases? Can we do this only if the upload fails?
  • eliminating the backup service and moving to pkg-level functions resulted in some annoying wrapper structs being created in order to enable mocking in unit tests. Can we use a monkey-patching-like approach here to simplify?
  • the sample plugins repo will need to be updated

@ncdc
Copy link
Contributor

ncdc commented Jul 27, 2018

backup/restore controller - this PR logs the per-backup & restore logs to both a file in object storage AND to stdout, so there's info available if the obj storage upload fails. Do we want to log to stdout in all cases? Can we do this only if the upload fails?

I can't think of an easy way to do that. Is there any harm in always uploading to the ark server log too?

eliminating the backup service and moving to pkg-level functions resulted in some annoying wrapper structs being created in order to enable mocking in unit tests. Can we use a monkey-patching-like approach here to simplify?

We can bring it back

@skriss
Copy link
Contributor Author

skriss commented Jul 27, 2018

Re: backup service, interested in opinions on the last commit that I just pushed as an approach that allows us not to have the backup service and also doesn't require a bunch of interface/struct definitions for testing purposes. I am a fan of it.

cc @nrb, appreciate input when you get a chance. if you're ok with it i'll do the same for the other functions.

@skriss
Copy link
Contributor Author

skriss commented Jul 27, 2018

@ncdc re: logs, I was just concerned about the server logs getting super-verbose. Couldn't we just copy the contents of the log temp file into the server log if/only if the call to upload it to obj storage fails?

@ncdc
Copy link
Contributor

ncdc commented Jul 27, 2018

I guess it's worth a try

@ncdc
Copy link
Contributor

ncdc commented Jul 27, 2018

Or you could revert that part of the PR

@skriss
Copy link
Contributor Author

skriss commented Jul 27, 2018

eh, i'm gonna leave the logs as-is. we can always change if it's an issue down the road.

Copy link
Contributor

@nrb nrb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally +1 on the approach. For the rebase, I'd suggest at least keeping the message from the first commit.

A question - does this require any changes to the https://github.com/heptio/ark-plugin-example/ repo for 3rd party contributors?

@ncdc
Copy link
Contributor

ncdc commented Jul 27, 2018

A question - does this require any changes to the https://github.com/heptio/ark-plugin-example/ repo for 3rd party contributors?

Yes, and we should start tagging and/or releasing versions of the example repo to match tagged/released versions of ark (and master=master)

@nrb
Copy link
Contributor

nrb commented Jul 27, 2018

@skriss Specifically addressing the backupDownloader replacement, +1 there. Makes sense to me.

@skriss skriss force-pushed the resumable-plugins-rebased branch from f5c18d3 to 9833412 Compare July 27, 2018 19:14
@skriss
Copy link
Contributor Author

skriss commented Jul 27, 2018

@nrb did the rest of the refactoring work, commit per type. at this point I think i'm ready to merge (once I squash everything down)

@skriss skriss force-pushed the resumable-plugins-rebased branch from 9833412 to 5802afe Compare July 27, 2018 19:49
@nrb
Copy link
Contributor

nrb commented Jul 27, 2018

The refactored work LGTM. Looks like the rebase work isn't terrible, either.

@skriss
Copy link
Contributor Author

skriss commented Jul 30, 2018

FYI pushed one more commit on this. @carlisia @nrb if you have free time today, would be awesome to do some testing. I'll plan to squash later today and then we can get it merged.

@nrb
Copy link
Contributor

nrb commented Jul 31, 2018

Testing looked good to me. The plugin process restarted and I never saw more than one.

@nrb nrb requested a review from carlisia July 31, 2018 14:00
Refactor plugin management:
- support multiple plugins per executable
- support restarting a plugin process in the event it terminates
- simplify plugin lifecycle management by using separate managers for
  each scope (server vs backup vs restore)

Signed-off-by: Andy Goldstein <[email protected]>
@skriss skriss force-pushed the resumable-plugins-rebased branch from 1f36ed4 to 1305121 Compare July 31, 2018 15:39
@skriss
Copy link
Contributor Author

skriss commented Jul 31, 2018

squashed and ready to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants