Bug 1536629 - send job state and credentials from job #610

maleck13 · 2018-01-04T16:08:32Z

Note for Reviewer
There are two commits here

making the changes required
adding a set of test for the provision subscriber and job. If happy with the approach here I will expand the tests to cover the other subscribers and jobs. Making a number of changes like this, I felt a little disconcerted that I had no unit test to run so would love to add more to help protect against regressions in future changes.

These commits are independent of each other so if the new tests are not wanted I can remove.

I am currently trying this out in my local cluster but wanted to create the PR to let the integration tests run.

Describe what this PR does and why we need it:
The subscribers were deciding on the state of the Job, rather than the job informing the subscriber of the job state. This pr follows on from issue #608
Also the Job was Marshalling the extracted credentials and sending them as a string on the Msg field, then the subscriber would Unmarshal it and save it. Instead of doing this we just send the credentials through

Changes proposed in this pull request

simplify subscriber logic
send state from the individual job
send credentials directly via the Job rather than turning them into a JSON string and then back to credentials again in the subscriber (avoid unneeded marshalling and unmarshalling)
Make an attempt at adding test cases for the ProvisionJob as an example for how it might be done and to get feedback on the approach

Which issue this PR fixes (This will close that issue when PR gets merged)
fixes #608

maleck13 · 2018-01-04T16:09:32Z

@eriknelson PR from conversation on IRC yesterday about subscribers setting state.

coveralls · 2018-01-04T16:46:02Z

Changes Unknown when pulling 98aa415 on maleck13:608-send-job-state-from-job into ** on openshift:master**.

maleck13 · 2018-01-04T18:47:03Z

This pr is also part of the larger work being done for #475

shawn-hurley · 2018-01-04T16:17:42Z

pkg/broker/provision_job.go

 }

+// Provisioner defines a function that knows how to provision an apb
+type Provisioner func(si *apb.ServiceInstance) (string, *apb.ExtractedCredentials, error)


What are you thoughts on this being defined in the apb package?

yeah no issue with that. The flow of dependencies seems to go from apb to broker.

shawn-hurley · 2018-01-04T16:23:24Z

pkg/broker/provision_job.go

 	return &ProvisionJob{
 		serviceInstance: serviceInstance,
+		provision:       provision,


👍 I like this approach

shawn-hurley · 2018-01-04T19:02:26Z

This looks really good to me, want to discuss the provisioner type being defined in the apb package.

I like the way you have made it testable by making the provisioner function. That looks really correct to me, and I think that we should make a not to update the deprovision and update jobs with this style.

rthallisey

minor change

rthallisey · 2018-01-04T19:43:05Z

pkg/broker/provision_subscriber.go

 type ProvisionWorkSubscriber struct {
-	dao       *dao.Dao
-	msgBuffer <-chan JobMsg
+	dao SubcriberDAO


s/SubcriberDAO/SubscriberDAO/

rthallisey · 2018-01-04T19:55:03Z

@mhrivnak if you change your initial commit to say fixes #608 instead of 608 it will automatically close the git issue after this PR merges.

jmrodri · 2018-01-04T20:23:21Z

I'll review this and see what the impact is on my current outstanding async work. I'm currently knee deep in this area.

mhrivnak · 2018-01-04T21:12:37Z

@mhrivnak if you change your initial commit to say fixes #608 instead of 608 it will automatically close the git issue after this PR merges.

I assume that comment is for @maleck13

maleck13 · 2018-01-05T09:23:41Z

@shawn-hurley Will wait for @jmrodri review before changing the location of the provision type, in case there are further changes required.
I think if all is good after the reviews perhaps we should merge at that point and I can follow up with the tests for the remaining subscribers and jobs? Just trying to avoid to big of a rebase and merge for others. If not I can do it on this PR not a big deal either way

coveralls · 2018-01-05T09:56:37Z

Changes Unknown when pulling dd08f15 on maleck13:608-send-job-state-from-job into ** on openshift:master**.

shawn-hurley · 2018-01-05T13:46:37Z

@maleck13 👍 for waiting for @jmrodri. If you want to follow up with the changes you can take it, but I wouldn't mind taking it off your plate if you want to work on your other PR? Completely up to you just offering my help if you want it.

maleck13 · 2018-01-05T14:49:44Z

@shawn-hurley thanks for the offer. I think though I will naturally end up creating the additional tests as part of the PR work as I want to be able to run them when iterating on the feature pr and a good bit of the work for the last operation feature is around the Jobs and subscribers.

Funny enough almost all of my PRs are part of the feature :) its like building a feature in layers

rthallisey · 2018-01-09T16:50:12Z

Partially-fixes #542

jmrodri

Overall I like the approach. Some questions about why some things were changed. Also, the stuttering of State.State is slightly annoying so I'd prefer we address that.

jmrodri · 2018-01-12T04:39:52Z

pkg/broker/deprovision_job.go

@@ -43,42 +43,44 @@ func NewDeprovisionJob(serviceInstance *apb.ServiceInstance,
 // Run - will run the deprovision job.
 func (p *DeprovisionJob) Run(token string, msgBuffer chan<- JobMsg) {
 	metrics.DeprovisionJobStarted()
+	defer metrics.DeprovisionJobFinished()
+	var jobMsg = JobMsg{


any reason we do var jobMsg = ... vs jobMsg :=?

Nothing specific, just a habit I have as it is accessed from several blocks within the function. Happy to change it

jmrodri · 2018-01-12T04:44:23Z

pkg/broker/deprovision_job.go

+		State: apb.JobState{
+			State:  apb.StateInProgress,
+			Method: apb.JobMethodDeprovision,
+			Token:  token,


something about this setup bothers me, but I don't see a cleaner way.

jmrodri · 2018-01-12T04:45:52Z

pkg/broker/deprovision_job.go


 	if p.skipApbExecution {
 		log.Debug("skipping deprovision and sending complete msg to channel")
-		msgBuffer <- JobMsg{InstanceUUID: p.serviceInstance.ID.String(), PodName: "",
-			JobToken: token, SpecID: p.serviceInstance.Spec.ID, Error: ""}
+		jobMsg.State.State = apb.StateSucceeded


I'd rather see us add a SetState(JobState) to the JobMsg struct. I don't like the stuttering of State.State. So it would look more like jobMsg.SetState(apb.StateSucceeded).

Agree on stuttering struggled with it also, but not a big fan of using setters either though. I wondered if perhaps we could remove JobMsg entirely and just have JobState as their is some duplication between them?

On a side note, I think we could simplify things by removing the subscribers and either have a set of observers registered for each job that act on JobState changes or simply give the Job access to the DAO to set the state itself.
Maybe better to discuss this outside of this PR though.

I think it's worth a discussion @maleck13. I don't remember the issue where you first mentioned this idea, but it's worth bringing it up again.

I will follow up with a clear separate issue to allow for that discussion

jmrodri · 2018-01-12T04:47:57Z

pkg/broker/deprovision_job.go

+		// an error type in a struct you want marshalled
+		// https://github.com/golang/go/issues/5161
+		jobMsg.State.State = apb.StateFailed
+		jobMsg.State.Error = errMsg


In addition to the SetState I'd add a SetError or SetErrorMsg to the JobMsg struct. That way it hides the implementation detail there there's a JobState hidden in there.

jmrodri · 2018-01-12T04:56:39Z

pkg/broker/deprovision_subscriber.go

-				)
-				setFailedDeprovisionJob(d.dao, msg)
-				continue
+		for msg := range msgBuffer {


any particular reason for change from the for { to the for msg := range msgBuffer {?

2 reasons:

When you close a channel and you use the for range method, the loop also closes, where as if you just use a for loop and receive from the channel, once the channel is closed you will continue to receive the default value for that channel. If you don't want that behaviour your code would need to check the second value on a channel receive:

msg,closed <- chan if closed{ break }

https://dave.cheney.net/2014/03/19/channel-axioms

I am ok with checking if the channel is closed, but in my opinion it also reads a little better with the range statement.

jmrodri · 2018-01-12T05:05:27Z

pkg/broker/deprovision_subscriber.go

-				continue
+		for msg := range msgBuffer {
+			log.Debug("received deprovision message from buffer")
+			if msg.State.State == apb.StateSucceeded {


I would flip the check to look for the error conditions first and have escape valves, then do the success case afterwards without being in an if block.

func (d *DeprovisionWorkSubscriber) Subscribe(msgBuffer <-chan JobMsg) { d.msgBuffer = msgBuffer go func() { log.Info("Listening for deprovision messages") for msg := range msgBuffer { log.Debug("received deprovision message from buffer") if msg.State.State != apb.StateSucceeded { if err := d.dao.SetState(msg.InstanceUUID, msg.State); err != nil { log.Errorf("failed to set state after deprovision %v", err) } continue } instance, err := d.dao.GetServiceInstance(msg.InstanceUUID) if err != nil { log.Errorf( "Error occurred getting service instance [ %s ] after deprovision job:", msg.InstanceUUID, ) setFailedDeprovisionJob(d.dao, msg) continue } if err := cleanupDeprovision(instance, d.dao); err != nil { log.Errorf("Failed cleaning up deprovision after job, error: %v", err) // Cleanup is reporting something has gone wrong. Deprovision overall // has not completed. Mark the job as failed. setFailedDeprovisionJob(d.dao, msg) continue } } }() }

removed the if entirely as we always want the state to be set

jmrodri · 2018-01-12T05:06:23Z

pkg/broker/deprovision_subscriber.go

-	}); err != nil {
-		log.Errorf("failed to set state after deprovision %#v", err)
+	// have to set the state here manually as the logic that triggers this is in the subscriber
+	dmsg.State.State = apb.StateFailed


The stutter is definitely weird. If we don't want to add the method, then dmsg.JobState.State might work.

Yeah thought about doing that also, but see above comment on perhaps removing the JobMsg and just having JobState.State

jmrodri · 2018-01-12T05:08:11Z

pkg/broker/deprovision_subscriber.go

@@ -102,12 +77,12 @@ func cleanupDeprovision(instance *apb.ServiceInstance, dao *dao.Dao) error {
 	id := instance.ID.String()

 	if err = dao.DeleteExtractedCredentials(id); err != nil {
-		log.Error("failed to delete extracted credentials - %#v", err)
+		log.Error("failed to delete extracted credentials - %v", err)


any reason you changed from %#v to %v? @shawn-hurley I think I've seen you use the %#v variant, thoughts?

There seemed to be different approaches to this across the code base with %v seeming to be the most common. In my opinion, the %v is good for error messages as it will just print the error string, where as perhaps the %#v is good for debug statements as it prints the struct

&errors.errorString{s:"test error"}

see example https://play.golang.org/p/nkVaxztAG4_I

jmrodri · 2018-01-12T05:09:11Z

pkg/broker/provision_subscriber.go

 )

-// ProvisionWorkSubscriber - Lissten for provision messages
+// ProvisionWorkSubscriber - Listen for provision messages


👍 to fixing typos

maleck13 · 2018-01-16T09:36:40Z

Will rebase and bring the new Bind and Unbind jobs into line with these changes

maleck13 · 2018-01-16T22:18:59Z

updated with refactored bind and unbind
added new tests for those jobs
changed the agreed on comments

@jmrodri Any further thoughts based on the comments?

I think the only outstanding things are the stutter, which I think could be resolved by removing the JobMsg and just sending through JobState (ie come up with a single type that suits the purposes). Maybe this could be a follow up issue and PR?
The other issues were the for loop ranging over the channel and the log format.

coveralls · 2018-01-17T17:15:12Z

Changes Unknown when pulling 5229147 on maleck13:608-send-job-state-from-job into ** on openshift:master**.

coveralls · 2018-01-18T11:55:19Z

Changes Unknown when pulling 658379f on maleck13:608-send-job-state-from-job into ** on openshift:master**.

coveralls · 2018-01-18T14:05:47Z

Changes Unknown when pulling 434f7ed on maleck13:608-send-job-state-from-job into ** on openshift:master**.

rthallisey · 2018-01-18T15:54:01Z

Looks like we may need to add github.com/apex/log to glide. Travis turned up with this error:

pkg/broker/binding_job.go:24:2: cannot find package "github.com/apex/log" in any of:
	/home/travis/gopath/src/github.com/openshift/ansible-service-broker/vendor/github.com/apex/log (vendor tree)
	/home/travis/.gimme/versions/go1.8.5.linux.amd64/src/github.com/apex/log (from $GOROOT)
	/home/travis/gopath/src/github.com/apex/log (from $GOPATH)

maleck13 · 2018-01-18T16:35:05Z

I think this was an error on my part. My editor may have imported it accidentally. I have resolved that issue and rebased on master. Seems a ci job is timing out now Craig

…

On 18 Jan 2018, at 15:54, Ryan Hallisey ***@***.***> wrote: Looks like we may need to add github.com/apex/log to glide. Travis turned up with this error: pkg/broker/binding_job.go:24:2: cannot find package "github.com/apex/log" in any of: /home/travis/gopath/src/github.com/openshift/ansible-service-broker/vendor/github.com/apex/log (vendor tree) /home/travis/.gimme/versions/go1.8.5.linux.amd64/src/github.com/apex/log (from $GOROOT) /home/travis/gopath/src/github.com/apex/log (from $GOPATH) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

rthallisey · 2018-01-18T17:01:58Z

Thanks Craig. I just restarted it. We'll see if it happens again.

eriknelson · 2018-01-19T01:49:12Z

@maleck13 Sorry it's taken me a bit to review this. Trying to make my way through a review backlog. I'm guessing this still needs a rebase?

maleck13 · 2018-01-19T08:38:02Z

rebased

coveralls · 2018-01-19T09:18:33Z

Changes Unknown when pulling fd5304a on maleck13:608-send-job-state-from-job into ** on openshift:master**.

maleck13 · 2018-01-19T09:58:02Z

The CI build is failing, but it is not clear to me how best to find the issue. The log is quite large https://api.travis-ci.org/v3/job/330711368/log.txt

rthallisey · 2018-01-19T14:54:06Z

It's failing on lint. The fix was merged an hour ago: #662 . Another rebase should fix it.

=================================
              Lint               
=================================
pkg/apb/types.go:318:2: struct field HttpProxy should be HTTPProxy
pkg/apb/types.go:319:2: struct field HttpsProxy should be HTTPSProxy
Found 2 lint suggestions; failing.
Makefile:27: recipe for target 'lint' failed
make: *** [lint] Error 1

eriknelson · 2018-01-19T15:47:16Z

Apologies. I broke the build :) Will take a closer look at this today.

jmrodri · 2018-01-19T16:37:22Z

I'll take another look at this one now that bind and unbind are in

…tests. Address pr feedback on decided comments remove bad import. refactor job test to accept binding instance

maleck13 · 2018-01-19T17:30:42Z

@rthallisey Thanks for pointer! have rebased again now

jmrodri · 2018-01-19T18:48:05Z

My async bind scripts work fine with this deployed. I like the Binder interface func.

coveralls · 2018-01-19T18:49:05Z

Changes Unknown when pulling fe0fd97 on maleck13:608-send-job-state-from-job into ** on openshift:master**.

Bug 1536629 - send job state and credentials from job fixes openshift#608

openshift-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jan 4, 2018

shawn-hurley added feature needs-review labels Jan 4, 2018

shawn-hurley reviewed Jan 4, 2018

View reviewed changes

rthallisey suggested changes Jan 4, 2018

View reviewed changes

maleck13 force-pushed the 608-send-job-state-from-job branch from 98aa415 to dd08f15 Compare January 5, 2018 09:15

rthallisey approved these changes Jan 5, 2018

View reviewed changes

maleck13 mentioned this pull request Jan 8, 2018

475 last operation description #619

Merged

rthallisey added the 3.9 | release-1.1 Kubernetes 1.9 | Openshift 3.9 | Broker release-1.1 label Jan 9, 2018

jmrodri self-requested a review January 12, 2018 04:21

jmrodri suggested changes Jan 12, 2018

View reviewed changes

shawn-hurley added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 12, 2018

maleck13 force-pushed the 608-send-job-state-from-job branch from dd08f15 to 5229147 Compare January 16, 2018 22:14

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 16, 2018

maleck13 force-pushed the 608-send-job-state-from-job branch from 5229147 to 658379f Compare January 18, 2018 11:06

maleck13 force-pushed the 608-send-job-state-from-job branch from 658379f to 434f7ed Compare January 18, 2018 13:15

maleck13 force-pushed the 608-send-job-state-from-job branch from 434f7ed to fd5304a Compare January 19, 2018 08:37

maleck13 mentioned this pull request Jan 19, 2018

SetState (JobState) from within StartNewJob? #504

Closed

maleck13 added 2 commits January 19, 2018 17:29

send through the JobState object from the Job itself.

fe78721

refactor bind and unbind jobs to send state and credentials. Add job …

fe0fd97

…tests. Address pr feedback on decided comments remove bad import. refactor job test to accept binding instance

maleck13 force-pushed the 608-send-job-state-from-job branch from fd5304a to fe0fd97 Compare January 19, 2018 17:30

jmrodri approved these changes Jan 19, 2018

View reviewed changes

jmrodri changed the title ~~608 send job state and credentials from job~~ Bug 1536629 - send job state and credentials from job Jan 19, 2018

jmrodri removed the needs-review label Jan 19, 2018

jmrodri merged commit 1526119 into openshift:master Jan 19, 2018

jianzhangbjz pushed a commit to jianzhangbjz/ansible-service-broker that referenced this pull request May 17, 2018

Bug 1536629 - send job state and credentials from job (openshift#610)

262c3a1

Bug 1536629 - send job state and credentials from job fixes openshift#608

Bug 1536629 - send job state and credentials from job #610

Bug 1536629 - send job state and credentials from job #610

Conversation

maleck13 commented Jan 4, 2018 • edited

maleck13 commented Jan 4, 2018

coveralls commented Jan 4, 2018

maleck13 commented Jan 4, 2018

Choose a reason for hiding this comment

maleck13 Jan 4, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shawn-hurley commented Jan 4, 2018

rthallisey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rthallisey commented Jan 4, 2018

jmrodri commented Jan 4, 2018

mhrivnak commented Jan 4, 2018

maleck13 commented Jan 5, 2018

coveralls commented Jan 5, 2018

shawn-hurley commented Jan 5, 2018

maleck13 commented Jan 5, 2018 • edited

rthallisey commented Jan 9, 2018 • edited

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

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 Jan 16, 2018

maleck13 commented Jan 16, 2018

coveralls commented Jan 17, 2018

coveralls commented Jan 18, 2018

coveralls commented Jan 18, 2018 • edited

rthallisey commented Jan 18, 2018

maleck13 commented Jan 18, 2018 via email

rthallisey commented Jan 18, 2018

eriknelson commented Jan 19, 2018

maleck13 commented Jan 19, 2018

coveralls commented Jan 19, 2018 • edited

maleck13 commented Jan 19, 2018

rthallisey commented Jan 19, 2018 • edited

eriknelson commented Jan 19, 2018 • edited

jmrodri commented Jan 19, 2018

maleck13 commented Jan 19, 2018

jmrodri commented Jan 19, 2018

coveralls commented Jan 19, 2018

maleck13 commented Jan 4, 2018 •

edited

maleck13 Jan 4, 2018 •

edited

maleck13 commented Jan 5, 2018 •

edited

rthallisey commented Jan 9, 2018 •

edited

coveralls commented Jan 18, 2018 •

edited

coveralls commented Jan 19, 2018 •

edited

rthallisey commented Jan 19, 2018 •

edited

eriknelson commented Jan 19, 2018 •

edited