State support for Bundles #60

maleck13 · 2018-04-23T15:56:06Z

Apologies on delay with this. Here is a pass at the state support in the bundle lib. Please be brutal 😀 I have changes to the broker also but will wait until this is nailed down.

@shawn-hurley You mentioned making the PR against 0.1-release. Is that needed for this work as it is targeting the next release?

maleck13 · 2018-04-23T15:57:02Z

bundle/executor.go

 }

 // NewExecutor - Creates a new Executor for running an APB.
-func NewExecutor() Executor {
+func NewExecutor(k8sClient k8.Interface, stateNS string) Executor {


not sure if this is the right approach. Feedback welcome

I would prefer to use the clients package to get the k8s client that way. That is how most things are behaving and I have examples of how to test that.

Is stateNS different than bundle.serviceInstance.Context.Namespace https://github.com/automationbroker/bundle-lib/blob/master/bundle/types.go#L140 ?

maleck13 · 2018-04-23T16:00:28Z

Not sure how to handle version checks here as state support wont be present until the following is present ansibleplaybookbundle/ansible-asb-modules#15 Does it mean a bump of the bundle / apb spec version and adding checks to the code for this version?

jmrodri · 2018-04-24T05:31:14Z

Can someone point me to the genesis of this PR? IRC logs, email, or issue? Trying to get an idea what it is we're trying to solve. I think I missed or probably just forgot :) Thanks.

maleck13 · 2018-04-24T07:37:14Z

@jmrodri it is the bundle-lib part of https://github.com/openshift/ansible-service-broker/blob/master/docs/proposals/apb-state-support.md the apb state support proposal

Notes This is the first real thing I have done in the lib so struggled a little with understanding where the boundaries lay (ie what should be in the broker and what should be in the lib) If I am off on where the implementation of this should be, I'd be keen to know and understand more.

maleck13 · 2018-04-24T07:42:48Z

runtime/state.go

+func (s *State) PrepareMount(name string) (*v1.VolumeMount, *v1.Volume, error) {
+	mount := &v1.VolumeMount{
+		Name:      name,
+		MountPath: "/etc/apb/state",


set this value in PodEnv also so that it can be read by the module

shawn-hurley

this is really LGTM!

I like the use of volume mounts.

shawn-hurley · 2018-04-25T13:05:57Z

bundle/bind.go

@@ -38,7 +38,7 @@ func (e *executor) Bind(
 	go func() {
 		e.actionStarted()
 		executionContext, err := e.executeApb(
-			"bind", instance.Spec, instance.Context, parameters)
+			"bind", instance, parameters)


I'm okay with it, I prefer only to pass in what is absolutely necessary but that is just a nitpick. I also understand this makes it easier to keep the signature stable as you require more things on the instance.

shawn-hurley · 2018-04-25T13:07:36Z

bundle/executor.go

 }

 // NewExecutor - Creates a new Executor for running an APB.
-func NewExecutor() Executor {
+func NewExecutor(k8sClient k8.Interface, stateNS string) Executor {


I would prefer to use the clients package to get the k8s client that way. That is how most things are behaving and I have examples of how to test that.

shawn-hurley · 2018-04-25T13:25:03Z

runtime/state.go

+
+// State handles the state for service bundles
+type State struct {
+	Client k8.Interface


I would prefer this to use the clients package.

shawn-hurley · 2018-04-25T13:25:07Z

bundle/executor.go

@@ -141,8 +155,10 @@ func (e *executor) updateDescription(newDescription string) {

 // executeApb - Runs an APB Action with a provided set of inputs
 func (e *executor) executeApb(
-	action string, spec *Spec, context *Context, p *Parameters,
+	action string, instance *ServiceInstance, p *Parameters,


jmrodri

Overall decent job. Minor change requests. Thanks for the PR.

jmrodri · 2018-04-25T14:33:14Z

bundle/executor.go

+	PrepareMount(name string) (*v1.VolumeMount, *v1.Volume, error)
+	StateIsPresent(name string) (bool, error)
+	Name(instanceID string) string
+	MasterNS() string


If NS is Namespace, I'd call the method MasterNamespace, using ns as a variable is okay, but for the method feels better to spell it out.

jmrodri · 2018-04-25T14:33:46Z

bundle/bind.go

@@ -38,7 +38,7 @@ func (e *executor) Bind(
 	go func() {
 		e.actionStarted()
 		executionContext, err := e.executeApb(
-			"bind", instance.Spec, instance.Context, parameters)
+			"bind", instance, parameters)


I'm okay with it, I prefer only to pass in what is absolutely necessary but that is just a nitpick. I also understand this makes it easier to keep the signature stable as you require more things on the instance.

jmrodri · 2018-04-25T14:39:12Z

bundle/bind.go

@@ -62,6 +62,12 @@ func (e *executor) Bind(
 			}
 		}

+		// pod execution is complete so transfer state back
+		if err := e.stateManager.CopyState(executionContext.PodName, e.stateManager.Name(instance.ID.String()), executionContext.Namespace, e.stateManager.MasterNS()); err != nil {


This is one really really long line :) can we just break it on one of the parameters?

if err := e.stateManager.CopyState(executionContext.PodName, e.stateManager.Name(instance.ID.String()), executionContext.Namespace, e.stateManager.MasterNS()); err != nil { e.actionFinishedWithError(err) return }

OR

if err := e.stateManager.CopyState(executionContext.PodName, e.stateManager.Name(instance.ID.String()), executionContext.Namespace, e.stateManager.MasterNS()); err != nil { e.actionFinishedWithError(err) return }

jmrodri · 2018-04-25T14:40:44Z

bundle/deprovision.go

+		executionContext, err := e.executeApb("deprovision", instance, instance.Parameters)
+		defer func() {
+			if err := e.stateManager.DeleteState(e.stateManager.Name(instance.ID.String())); err != nil {
+				log.Error("failed to delete state for instance "+instance.ID.String(), err)


I'd use the formatted version:

log.Errorf("failed to delete state for instance %s - %v", instance.ID.String(), err)

jmrodri · 2018-04-25T14:41:32Z

bundle/executor.go

@@ -31,6 +31,7 @@ import (
 	"github.com/pborman/uuid"
 	log "github.com/sirupsen/logrus"
 	"k8s.io/api/core/v1"
+	k8 "k8s.io/client-go/kubernetes"


k8 feels weird, haven't we been doing k8s? or k8scli

jmrodri · 2018-04-25T15:02:32Z

runtime/state.go

+
+// Name provides a consistent name for the state object
+func (s *State) Name(id string) string {
+	return id + "-state"


this works, usually we do the return fmt.Sprintf("%s-state", id)

jmrodri · 2018-04-25T15:03:20Z

runtime/state.go

+	log.Info("state: checking is present")
+	if _, err := s.Client.CoreV1().ConfigMaps(s.targetNs).Get(stateName, metav1.GetOptions{}); err != nil {
+		if kerror.IsNotFound(err) {
+			log.Info("state: is not found ** ")


How often will this occur? Feels like this should be a debug statement not info.

yep left in by accident there

jmrodri · 2018-04-25T15:03:50Z

runtime/state.go

+	return true, nil
+}
+
+// PrepareMount will prepare the voume definitions and mounts


typo in comment: voume -> volume

jmrodri · 2018-04-25T15:04:48Z

runtime/state.go

+
+// DeleteState will remove the state object from the broker namespace
+func (s *State) DeleteState(name string) error {
+	log.Info("state: deleting master state " + name + " in ns " + s.targetNs)


use the formatted version of info:

log.Infof("state: deleting master state %s in ns %s", name, s.targetNs)

jmrodri · 2018-04-25T15:06:14Z

runtime/state.go

+type State struct {
+	Client k8.Interface
+	// this is the namespace where the master configs will be stored
+	targetNs string


I would prefer targetns or targetNS or even better nsTarget

maleck13 · 2018-04-25T15:25:24Z

@jmrodri @shawn-hurley ok thanks for feedback looks like we are happy with the approach. I will make the required changes and flesh out unit tests for the state module (didn't do this first as wanted to ensure I was heading in the right direction)

rthallisey

Nice job.

rthallisey · 2018-04-25T19:10:40Z

bundle/executor.go

 }

 // NewExecutor - Creates a new Executor for running an APB.
-func NewExecutor() Executor {
+func NewExecutor(k8sClient k8.Interface, stateNS string) Executor {


Is stateNS different than bundle.serviceInstance.Context.Namespace https://github.com/automationbroker/bundle-lib/blob/master/bundle/types.go#L140 ?

maleck13 · 2018-04-26T07:59:44Z

@rthallisey stateNS is the where the master copy of the state will be stored. In our case the broker namespace. So it is a different namespace

maleck13 · 2018-05-02T15:13:12Z

addressed all requested changes. I will add a set of tests for the state manager. Note I also added a new env var to set the BUNDLE_STATE_LOCATION for use by the module

shawn-hurley · 2018-05-02T17:13:26Z

@maleck13 So I was re-looking through this after the changes and I actually see 1 thing that should be changed.

We are trying to move all of the stuff that uses k8scli to the runtime, and therefore I think that State Manager should be moved to the runtime package, its interface embedded into the Runtime interface and the provider struct should embed a default runtime. I also think that allowing an override in the configuration should be added. Please ping me if you have any questions.

Sorry I didn't catch this the first time around.

maleck13 · 2018-05-03T10:41:35Z

Is the idea here that a different StateManager could be provided externally when using the lib outside of the broker but we will provide a default implementation? I am ok making the change just trying to understand the reasoning. The actual concrete implementation of the interface is already in the runtime package limiting the k8scli to the runtime package.

This makes me wonder should the state type in the runtime package be exported, seems it should be internal right?

shawn-hurley · 2018-05-03T13:27:11Z

@maleck13

The idea is that the only way to interact with the runtime package is through the Provider. This Provider is just an implementation of the Runtime interface. We should not be adding a new way to interact with runtime than through the provider.

The idea is that if we have a bunch of sane defaults, but then if someone wants to change something they can. This will allow users of bundle lib who want to implement a new runtime can. I don't envision anyone doing this right at the moment but I do see future use cases that are not too far into the future.

maleck13 · 2018-05-08T13:49:49Z

@shawn-hurley I believe I have made the changes you requested. Let me know if this what your were expecting.

@jmrodri I believe I have addressed your feedback

shawn-hurley

This a nit, but I would like to keep the same structure/layout that we have been using

shawn-hurley · 2018-05-08T14:35:54Z

runtime/runtime.go

+}
+
+// StateManager defines an interface for managing state created by service bundles
+type StateManager interface {


I would move this interface into the state file. this would follow the pattern that we have used for ExtractedCredential

jmrodri · 2018-05-09T21:51:12Z

runtime/state.go

+import (
+	"fmt"
+
+	"github.com/apex/log"


I think this is a mistaken import. I believe it should be log "github.com/sirupsen/logrus"

Remove the apex log, and add the logrus UNDER the clients, like it looks below. This makes goimports happy and uses the correct logger.

import ( "fmt" "github.com/automationbroker/bundle-lib/clients" log "github.com/sirupsen/logrus" "k8s.io/api/core/v1" kerror "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" )

jmrodri

@maleck13 Wrong logger used in state.go, see the other comment above with the suggested fix. Then I will merge it tomorrow (Thursday). I validated it today. Great job.

fix import for log

maleck13 · 2018-05-10T08:09:53Z

@jmrodri Thanks. Good spot on the import. Once merged will we tag / release I will then make the needed updates to the broker

dymurray

Looks great. This approach is going to make it much easier for APB developers to pass data between calls even outside of the cluster approach since it's just mounted files. Great job!

maleck13 commented Apr 23, 2018

View reviewed changes

maleck13 commented Apr 24, 2018

View reviewed changes

jmrodri self-requested a review April 24, 2018 14:00

rthallisey self-requested a review April 24, 2018 14:01

shawn-hurley requested changes Apr 25, 2018

View reviewed changes

jmrodri requested changes Apr 25, 2018

View reviewed changes

rthallisey reviewed Apr 25, 2018

View reviewed changes

maleck13 force-pushed the bundle-state-support branch 2 times, most recently from c6f0bc9 to a31958b Compare May 2, 2018 15:12

shawn-hurley mentioned this pull request May 8, 2018

removing the bundle execution to runtime. #65

Merged

maleck13 force-pushed the bundle-state-support branch 3 times, most recently from c1f2d72 to aa349d2 Compare May 8, 2018 13:48

maleck13 force-pushed the bundle-state-support branch from aa349d2 to baa1351 Compare May 8, 2018 13:51

shawn-hurley requested changes May 8, 2018

View reviewed changes

shawn-hurley approved these changes May 8, 2018

View reviewed changes

shawn-hurley changed the title ~~wip state support for Bundles~~ State support for Bundles May 8, 2018

jmrodri reviewed May 9, 2018

View reviewed changes

jmrodri requested changes May 9, 2018

View reviewed changes

Add state support for Bundles

6fbe98e

fix import for log

maleck13 force-pushed the bundle-state-support branch from 84f1338 to 6fbe98e Compare May 10, 2018 08:00

dymurray approved these changes May 10, 2018

View reviewed changes

jmrodri approved these changes May 10, 2018

View reviewed changes

jmrodri merged commit 697a231 into automationbroker:master May 10, 2018

State support for Bundles #60

State support for Bundles #60

Conversation

maleck13 commented Apr 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maleck13 commented Apr 23, 2018

jmrodri commented Apr 24, 2018

maleck13 commented Apr 24, 2018 • edited

Choose a reason for hiding this comment

shawn-hurley 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

jmrodri 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

maleck13 commented Apr 25, 2018

rthallisey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maleck13 commented Apr 26, 2018

maleck13 commented May 2, 2018 • edited

shawn-hurley commented May 2, 2018

maleck13 commented May 3, 2018 • edited

shawn-hurley commented May 3, 2018 • edited

maleck13 commented May 8, 2018

shawn-hurley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmrodri left a comment

Choose a reason for hiding this comment

maleck13 commented May 10, 2018

dymurray left a comment

Choose a reason for hiding this comment

maleck13 commented Apr 24, 2018 •

edited

maleck13 commented May 2, 2018 •

edited

maleck13 commented May 3, 2018 •

edited

shawn-hurley commented May 3, 2018 •

edited