Refactor plugin management #495

ncdc · 2018-05-16T16:56:40Z

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)

To-dos:

Add max # of restarts and/or restart backoff

Fixes #481

Things to verify:

all plugins work as expected
when a plugin process terminates, all Init()able plugins are reinitialized
we can run a backup & a restore at the same time and the plugins work ok (no races)
log messages from plugins are delivered to the server log appropriately
per-backup/restore logs continue to work

Signed-off-by: Andy Goldstein andy.goldstein@gmail.com

ncdc · 2018-05-16T16:58:47Z

One thing I want to do is some benchmarking of running plugins such as backup item actions. It seemed "slow" to me watching the backup in real time, but maybe that's just because I never paid attention before.

skriss

Initial comments from everything except pkg/plugin

skriss · 2018-05-22T17:12:56Z

examples/gcp/00-ark-config.yaml

@@ -22,8 +22,8 @@ persistentVolumeProvider:
  name: gcp
 backupStorageProvider:
  name: gcp
-  bucket: <YOUR_BUCKET>
-backupSyncPeriod: 30m
+  bucket: andy-heptio-ark-data


revert changes to this file

skriss · 2018-05-22T17:14:26Z

pkg/cloudprovider/backup_service.go

-func (br *backupService) UploadBackup(bucket, backupName string, metadata, backup, log io.Reader) error {
-	// Uploading the log file is best-effort; if it fails, we log the error but it doesn't impact the
-	// backup's status.
+func UploadBackupLog(objectStore ObjectStore, bucket, backupName string, log io.Reader) error {


skriss · 2018-05-22T17:17:23Z

pkg/cloudprovider/backup_service.go

 	key := getRestoreResultsKey(backup, restore)
-	return br.objectStore.PutObject(bucket, key, results)
+	return objectStore.PutObject(bucket, key, results)
 }

 // cachedBackupService wraps a real backup service with a cache for getting cloud backups.


is this going to stick around?

skriss · 2018-05-22T17:19:30Z

pkg/cmd/server/server.go

-	if err := s.initBackupService(config); err != nil {
-		return err
-	}
+	// if err := s.initBackupService(config); err != nil {


skriss · 2018-05-22T17:19:38Z

pkg/cmd/server/server.go

-	if err != nil {
-		return err
-	}
+// func (s *server) initBackupService(config *api.Config) error {


skriss · 2018-05-22T17:24:37Z

pkg/controller/backup_controller.go

@@ -377,6 +384,48 @@ func (controller *backupController) runBackup(backup *api.Backup, bucket string)
 	return kerrors.NewAggregate(errs)
 }

+func (controller *backupController) UploadBackup(logger logrus.FieldLogger, objectStore cloudprovider.ObjectStore, bucket, backupName string, metadata, backup, log io.Reader) error {


I wonder if it would make sense to extract this to pkg/backup so that it's part of the reusable lib?

skriss · 2018-05-22T17:28:59Z

pkg/restore/restore.go

-	backupService      cloudprovider.BackupService
+	discoveryHelper discovery.Helper
+	dynamicFactory  client.DynamicFactory
+	// backupService      cloudprovider.BackupService


skriss · 2018-05-22T17:29:07Z

pkg/restore/restore.go

@@ -137,7 +137,7 @@ func prioritizeResources(helper discovery.Helper, priorities []string, includedR
 func NewKubernetesRestorer(
 	discoveryHelper discovery.Helper,
 	dynamicFactory client.DynamicFactory,
-	backupService cloudprovider.BackupService,
+	// backupService cloudprovider.BackupService,


skriss

Once I got the model I found it fairly easy to follow, but it was confusing the first time through. I think some package-level godoc that explains in some detail would be helpful (how plugins and processes relate, how restartIfNeeded flows, how multiplexing works and what the different structs on the client and server do to implement it). Also added some thoughts on possible alternate names. I'd probably err on the side of longer & clearer names here since there is a lot of overloading of terms like plugin and client which makes it hard to understand initially.

skriss · 2018-05-31T21:34:51Z

pkg/cloudprovider/backup_service.go

@@ -17,7 +17,6 @@ limitations under the License.
 package cloudprovider


we probably want to rename this file

skriss · 2018-05-31T21:40:53Z

pkg/cmd/server/plugin/plugin.go

@@ -36,90 +32,89 @@ func NewCommand(f client.Factory) *cobra.Command {
 	logger := arkplugin.NewLogger()

 	c := &cobra.Command{
-		Use:    "run-plugin [KIND] [NAME]",
+		Use:    "run-plugin",


probably want to pluralize this now (run-plugins)

skriss · 2018-05-31T22:05:20Z

pkg/plugin/client_builder.go


 	return b
 }

-func (b *clientBuilder) withCommand(name string, args ...string) *clientBuilder {
-	b.config.Cmd = exec.Command(name, args...)
+// withCommand sets the clientBuilder's commandName to name and commandArgs to args.


should this still have a variadic args arg? I see they're being used in client() to set the Cmd. If not, edit the comment to remove the ref and probably remove the field from the clientBuilder struct.

We don't need it as an arg. Plugin authors must expect their plugin executable file to be invoked directly, with no arguments. We do still need it in the struct, as it's used for the special ark run-plugin case.

skriss · 2018-06-01T03:45:56Z

pkg/plugin/client_mux.go

+
+// clientMux supports the initialization and retrieval of multiple implementations for a single plugin kind, such as
+// "aws" and "azure" implementations of the object store plugin.
+type clientMux struct {


I wonder if something like clientFactory would be clearer here. I think mux is slightly confusing because this struct is actually just handing out different clients as opposed to actually doing any multiplexing itself.

Per previous - what about clientDispenser?

skriss · 2018-06-01T03:50:43Z

pkg/plugin/manager.go

-	arkCommand := os.Args[0]
+// getWrapper returns a wrapper for a plugin identified by kind and name, creating a wrapper if it is the first time it
+// has been requested.
+func (m *manager) getWrapper(kind PluginKind, name string) (*restartablePluginProcess, error) {


Trying to think of a name that is a bit more descriptive than getWrapper -- getRestartablePluginProc, getPluginProcessHandle?

This missed getting renamed when I changed wrapper to restartablePluginProcess. It should be called getRestartablePluginProcess.

skriss · 2018-06-01T03:51:27Z

pkg/plugin/manager.go

 }

-// GetObjectStore returns the plugin implementation of the cloudprovider.ObjectStore
-// interface with the specified name.
+// GetObjectStore returns a resumableObjectStore for name.


s/resumable/restartable

skriss · 2018-06-01T03:51:41Z

pkg/plugin/manager.go

 }

-// GetBlockStore returns the plugin implementation of the cloudprovider.BlockStore
-// interface with the specified name.
+// GetBlockStore returns a resumableBlockStore for name.


s/resumable/restartable

skriss · 2018-06-01T03:56:34Z

pkg/plugin/restartable_plugin_process.go

+	}
+
+	// Currently all plugins except for PluginLister dispense clientMux instances.
+	if mux, ok := dispensed.(*clientMux); ok {


I don't love that the restartablePluginProcess struct is the one that knows about clientMux and what to do with it to get an actual client - feels like a completely separate concerrn to restarting plugin processes. I'm not exactly sure how I'd do this differently, but wonder if there's a generic interface that all GRPCClient funcs could return that enables callers to get an actual client/proxy from the result.

Is it the "restartable" part of the name that's tripping you up? Other than the registry that calls Dispense() to list plugins, this is the only other place in the code where we call Dispense(). We could call this something other than restartablePluginProcess.

Or if it's less the name and more that restarting & dispensing seem like 2 different things, maybe we could find something. We need the protocolClient to be able to dispense, so we could introduce some other struct that solely focuses on dispensing using that. And then have the restartablePluginProcess hold on to those.

wonder if there's a generic interface that all GRPCClient funcs could return that enables callers to get an actual client/proxy from the result

Different from the type assertion to *clientMux?

I have some local changes where I extracted the dispensing into a pluginDispenser struct. It's a small change but it separates that logic, at least.

Just pushed up the changes. I quickly replaced pluginDispenser with process, responsible for managing a single execution of a plugin process.

skriss · 2018-06-01T03:59:26Z

pkg/plugin/server_mux.go

+type ServerInitializer func() (interface{}, error)
+
+// serverMux manages multiple implementations of a single plugin kind, such as pod and pvc BackupItemActions.
+type serverMux struct {


What do you think about referring to server-side implementations of pluggable interfaces as handlers, analagous to net/http nomenclature (e.g. the aws blockstore handler)? Trying not to overload plugin too much, and impl/implementation is kind of generic.

skriss · 2018-06-01T04:04:41Z

pkg/plugin/backup_item_action.go

-}
-
-// GRPCClient returns a BackupItemAction gRPC client.
+// GRPCClient returns a clientMux for BackupItemAction gRPC clients.
 func (p *BackupItemActionPlugin) GRPCClient(c *grpc.ClientConn) (interface{}, error) {


The naming that we're locked into here (GRPCClient(...)) due to go-plugin is unfortunate, since what we're returning is not actually a gRPC client but something that makes clients. Might be helpful to expand the godoc to explain that this is called by go-plugin, and can return any type but it should be a client or something that can be used to create a client

skriss · 2018-06-06T19:31:27Z

The updates LGTM, I like the additional types and revised names!

skriss · 2018-06-12T02:51:49Z

Makefile

@@ -24,7 +24,7 @@ PKG := github.com/heptio/ark
 REGISTRY ?= gcr.io/heptio-images

 # Which architecture to build - see $(ALL_ARCH) for options.
-ARCH ?= linux-amd64
+ARCH ?= $(shell go env GOOS)-$(shell go env GOARCH)


could we do this only for make local?

I realized that this will hurt make container if you're running on macOS. I'll see what I can do. We don't have to include the Makefile changes in this PR if it's too much of a pain.

skriss · 2018-06-12T02:56:53Z

pkg/cloudprovider/backup_service.go

 // BackupGetter knows how to list backups in object storage.
 type BackupGetter interface {
 	// GetAllBackups lists all the api.Backups in object storage for the given bucket.
 	GetAllBackups(bucket string) ([]*api.Backup, error)
 }

+type XXXBackupGetter interface {


Maybe this should just be BackupGetter and the current BackupGetter should be BackupLister

You read my mind :-) haven't done the rename yet.

skriss · 2018-06-12T02:58:29Z

pkg/cloudprovider/backup_service.go

@@ -203,10 +189,27 @@ func (br *backupService) GetAllBackups(bucket string) ([]*api.Backup, error) {
 	return output, nil
 }

-func (br *backupService) GetBackup(bucket, backupName string) (*api.Backup, error) {
+type liveXXXBackupGetter struct {


obv needs a rename - if we rename the interfaces per my prev comment this could just be liveBackupGetter

skriss · 2018-06-12T03:10:33Z

pkg/controller/backup_deletion_controller.go

+	clock                clock.Clock
+}
+
+type backupStorageDeleter interface {


the need to do this kind of thing is an annoying downside to having package-level funcs vs. methods on a struct :/

I'll see if I can find something simpler

skriss · 2018-06-12T03:12:31Z

pkg/controller/restore_controller.go

@@ -211,7 +278,9 @@ func (controller *restoreController) processRestore(key string) error {
 	logContext.Debug("Running processRestore")
 	ns, name, err := cache.SplitMetaNamespaceKey(key)
 	if err != nil {
-		return errors.Wrap(err, "error splitting queue key")
+		logContext.WithError(err).Error("unable to process restore: error splitting queue key")


I think we have this returning an error in most/all of our existing controllers, we probably want to add an issue to go change that across the board

skriss · 2018-06-12T03:15:22Z

pkg/plugin/backup_item_action.go

 func (p *BackupItemActionPlugin) GRPCClient(c *grpc.ClientConn) (interface{}, error) {
-	return &BackupItemActionGRPCClient{grpcClient: proto.NewBackupItemActionClient(c), log: p.log}, nil
+	return newClientMux(c, newBackupItemActionGRPCClient), nil


what did you think about renaming clientMux -> clientDispenser (and this func as well)?

Yes, we talked about that, but I overlooked it. I'll make the change.

skriss · 2018-06-12T03:16:58Z

pkg/plugin/client_mux.go

+
+// clientMux supports the initialization and retrieval of multiple implementations for a single plugin kind, such as
+// "aws" and "azure" implementations of the object store plugin.
+type clientMux struct {


Per previous - what about clientDispenser?

skriss · 2018-06-12T03:19:15Z

pkg/plugin/server_mux.go

+type serverMux struct {
+	kind         PluginKind
+	initializers map[string]ServerInitializer
+	instances    map[string]interface{}


WDYT about using handler in place of instance?

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 <andy.goldstein@gmail.com>

Signed-off-by: Andy Goldstein <andy.goldstein@gmail.com>

skriss · 2018-06-20T17:21:40Z

pkg/plugin/server_mux.go

@@ -42,10 +42,10 @@ func (m *serverMux) setServerLog(log logrus.FieldLogger) {
 	m.serverLog = log
 }

-// getInstance returns the instance for a plugin with the given name. If an instance has already been initialized,
+// getHandler returns the instance for a plugin with the given name. If an instance has already been initialized,


nit s/instance/handler

skriss · 2018-06-20T20:20:00Z

pkg/plugin/restartable_process.go

@@ -80,8 +81,13 @@ func (p *restartableProcess) reset() error {
 //
 // Callers of resetLH *must* acquire the lock before calling it.
 func (p *restartableProcess) resetLH() error {
+	if p.resetFailures > 10 {


are you thinking about including any sort of backoff for this?

I haven't fully fleshed this out yet, but we can/should discuss. Ideally we can handle the following scenarios:

process dies for whatever reason (e.g. oom kill), and restarting succeeds the first try

process dies for whatever reason, attempt(s) to restart fail early and quickly -> we surface the issue somehow (ark server shuts itself down, or we create events, or ?)

I would like to try to avoid the situation where the process dies, and it takes minutes or more to finish all the attempts to restart it.

skriss · 2018-06-20T20:30:03Z

haven't gone through the tests themselves yet, but went through the rest of the rename & refactor stuff and LGTM.

Signed-off-by: Andy Goldstein <andy.goldstein@gmail.com>

ncdc · 2018-07-31T16:36:38Z

Replaced by #710

ncdc added Bug enhancement kind/tech-debt labels May 16, 2018

ncdc assigned skriss May 16, 2018

ncdc force-pushed the resumable-plugins branch from 30642a8 to fe2fb39 Compare May 18, 2018 21:18

skriss reviewed May 22, 2018

View reviewed changes

skriss suggested changes Jun 1, 2018

View reviewed changes

ncdc force-pushed the resumable-plugins branch 2 times, most recently from a071d82 to f5d5cec Compare June 11, 2018 16:21

skriss reviewed Jun 12, 2018

View reviewed changes

ncdc force-pushed the resumable-plugins branch from 9749519 to f6a3dee Compare June 14, 2018 20:37

ncdc added 8 commits June 20, 2018 12:06

Bring back 'make local'

f2cc096

Signed-off-by: Andy Goldstein <andy.goldstein@gmail.com>

clientMux->clientDispenser

3c2a7b3

Signed-off-by: Andy Goldstein <andy.goldstein@gmail.com>

serverMux renames

a87cd9c

Signed-off-by: Andy Goldstein <andy.goldstein@gmail.com>

add license headers

4be1afd

Signed-off-by: Andy Goldstein <andy.goldstein@gmail.com>

file rename

a065aa9

Signed-off-by: Andy Goldstein <andy.goldstein@gmail.com>

wip cap restartable process reset failures

9846f39

Signed-off-by: Andy Goldstein <andy.goldstein@gmail.com>

wip more unit tests

5dea4e5

Signed-off-by: Andy Goldstein <andy.goldstein@gmail.com>

ncdc force-pushed the resumable-plugins branch from ee1599d to 5dea4e5 Compare June 20, 2018 17:10

ncdc added 3 commits June 20, 2018 15:20

add restartable backup item action test

de9babc

Signed-off-by: Andy Goldstein <andy.goldstein@gmail.com>

add restartable block store test

c77c17c

Signed-off-by: Andy Goldstein <andy.goldstein@gmail.com>

vendor: bump stretchr/testify

c34d004

Signed-off-by: Andy Goldstein <andy.goldstein@gmail.com>

skriss suggested changes Jun 20, 2018

View reviewed changes

ncdc added 2 commits June 21, 2018 11:53

restartable block store test

dfe41dc

Signed-off-by: Andy Goldstein <andy.goldstein@gmail.com>

test restartable object store

17bdca2

Signed-off-by: Andy Goldstein <andy.goldstein@gmail.com>

ncdc added 2 commits June 21, 2018 16:53

extract restartable delegate test

f73c006

Signed-off-by: Andy Goldstein <andy.goldstein@gmail.com>

test restartable restore action item

ed02077

Signed-off-by: Andy Goldstein <andy.goldstein@gmail.com>

rosskukulinski mentioned this pull request Jun 24, 2018

Ark server needs to handle crashed/stopped plugin processes #481

Closed

ncdc mentioned this pull request Jun 25, 2018

Decide if we want to have 1 plugin manager per scope (server, backup, restore) or 1 overall #601

Closed

rosskukulinski added Enhancement/Dev Internal or Developer-focused Enhancement to Velero and removed Enhancement labels Jun 25, 2018

This was referenced Jul 27, 2018

Resumable plugins rebased #710

Merged

Bring back 'make local' #711

Merged

ncdc closed this Jul 31, 2018

		@@ -17,7 +17,6 @@ limitations under the License.
		package cloudprovider

Refactor plugin management #495

Refactor plugin management #495

Conversation

ncdc commented May 16, 2018 • edited Loading

ncdc commented May 16, 2018

skriss 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skriss 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

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

skriss Jun 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skriss commented Jun 6, 2018

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

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

skriss commented Jun 20, 2018

ncdc commented Jul 31, 2018

ncdc commented May 16, 2018 •

edited

Loading

skriss Jun 1, 2018 •

edited

Loading