Bug 1512042 - Allowing error messages to make it from apb to user. #607

shawn-hurley · 2018-01-02T16:52:34Z

Make it so that job state holds errors from apb package
Make it so de/provision/update/_job can get the error and handle correctly.
make sure that the broker/handler handles the error and returns
Warn when about permissions for specs that may not be pullable.

Describe what this PR does and why we need it:
Fixes bug 1512042 by adding more explicit warning logs and handing errors back to the user.

Which issue this PR fixes (This will close that issue when PR gets merged)
Bug 1512042

coveralls · 2018-01-02T17:30:41Z

Changes Unknown when pulling 17b3518 on shawn-hurley:bug-1512042 into ** on openshift:master**.

* Make it so that job state holds errors from apb package * Make it so de/provision/update/_job can get the error and handle correctly. * make sure that he broker/handler handles the error and returns * Warn when about permissions for specs that may not be pullable.

djzager

This all looks good to me. Once I understand the reason for the conditional in the job error handling I'll approve.

djzager · 2018-01-02T17:57:52Z

pkg/broker/broker.go

 		ser, err := SpecToService(spec)
 		if err != nil {
 			log.Errorf("not adding spec %v to list of services due to error transforming to service - %v", spec.FQName, err)
 		} else {
-			services[i] = ser
+			services = append(services, ser)


djzager · 2018-01-02T18:23:03Z

pkg/apb/watch_pod.go

@@ -47,6 +52,9 @@ func watchPod(podName string, namespace string) error {

 		switch podStatus.Phase {
 		case apiv1.PodFailed:
+			if errorPullingImage(podStatus.Conditions) {


djzager · 2018-01-02T18:24:31Z

pkg/broker/deprovision_job.go

@@ -55,8 +55,26 @@ func (p *DeprovisionJob) Run(token string, msgBuffer chan<- JobMsg) {
 	if err != nil {
 		log.Error("broker::Deprovision error occurred.")
 		log.Errorf("%s", err.Error())
-		msgBuffer <- JobMsg{InstanceUUID: p.serviceInstance.ID.String(), PodName: podName,
-			JobToken: token, SpecID: p.serviceInstance.Spec.ID, Error: err.Error()}
+		// Because we know the error we should return that error.


I'm not certain I understand why we are returning the specific error message only if apb.ErrorPodPullErr. What's the reason for adding this logic?

Want to give a specific error about not being able to get the image.

If some other error occurs, I don't want to display that message to the user, that is why I want to do this check here.

Thinking more about this. Why not just log the error and return a generic error message in watch_pod like:

log.Errorf("Pod [ %s ] failed - %v", podName, podStatus.Message) return fmt.Error("Error occurred during APB execution. Please contact administrator if it presists.")

Because it is not just watch_pod that we are worried about, it is the error that apb.(provision/update/deprovision) returns that we are dealing with here.

Let's hope that is the biggest oversight I have this year.

djzager · 2018-01-02T18:24:53Z

pkg/registries/adapters/local_openshift_adapter.go

+			// logging to warn users about the potential bug if
+			// the svc-acct does not have access to the namespace.
+			if ns != "openshift" {
+				r.Log.Warningf("You may not be able to load provision images from the namespace: %v.\n"+


rthallisey · 2018-01-02T18:34:54Z

pkg/apb/watch_pod.go

+
+func errorPullingImage(conds []apiv1.PodCondition) bool {
+	for _, cond := range conds {
+		if cond.Reason == "ErrImgPull" {


I didn't find "ErrImgPull" anywhere in vendor, only "ErrImagePull". Is "ErrImgPull" the error we want or is it coming from something we don't have vendored? https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/images/types.go#L33

We are using the pod's status field. The conditions slice is where I am looking for this value. It appears that the reason is a string and this is set to ErrImgPull which you can see when you get the pod w/ yaml when it can not pull the image.

Is there a better way to get this? I would prefer a better way.

I think we can do something like https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/kubelet_pods.go#L1114-L1147

I will re-do this section based on that. It looks much more correct!

coveralls · 2018-01-02T18:37:09Z

Changes Unknown when pulling a88a8a3 on shawn-hurley:bug-1512042 into ** on openshift:master**.

rthallisey · 2018-01-02T19:30:00Z

pkg/apb/watch_pod.go

+	// https://github.com/kubernetes/kubernetes/blob/886e04f1fffbb04faf8a9f9ee141143b2684ae68/pkg/kubelet/images/types.go#L27
+	status := conds[0].State.Waiting
+
+	if status.Reason == "ErrImagePull" {


Might as well use images.ErrImagePull.Error() and images.ErrImagePullBackOff.Error() since it's where the strings come from.

I would prefer not to use something that is not in the kubernetes client-go or kubernetes api.

The images errors that are referenced in the core kuberentes package, I think that eventually, we could remove this dependency.

rthallisey · 2018-01-02T19:49:54Z

pkg/apb/watch_pod.go

+	// https://github.com/kubernetes/kubernetes/blob/886e04f1fffbb04faf8a9f9ee141143b2684ae68/pkg/kubelet/images/types.go#L27
+	status := conds[0].State.Waiting
+
+	if status.Reason == "ErrImagePull" {


coveralls · 2018-01-02T19:58:04Z

Changes Unknown when pulling 68b8b23 on shawn-hurley:bug-1512042 into ** on openshift:master**.

djzager

ACK

djzager · 2018-01-02T20:27:48Z

pkg/broker/deprovision_job.go

@@ -55,8 +55,26 @@ func (p *DeprovisionJob) Run(token string, msgBuffer chan<- JobMsg) {
 	if err != nil {
 		log.Error("broker::Deprovision error occurred.")
 		log.Errorf("%s", err.Error())
-		msgBuffer <- JobMsg{InstanceUUID: p.serviceInstance.ID.String(), PodName: podName,
-			JobToken: token, SpecID: p.serviceInstance.Spec.ID, Error: err.Error()}
+		// Because we know the error we should return that error.


Let's hope that is the biggest oversight I have this year.

…penshift#607) * Bug 1512042 - Allowing error messages to make it from apb to user. * Make it so that job state holds errors from apb package * Make it so de/provision/update/_job can get the error and handle correctly. * make sure that he broker/handler handles the error and returns * Warn when about permissions for specs that may not be pullable. * Addressing issues with determining if image pull error.

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 2, 2018

shawn-hurley force-pushed the bug-1512042 branch from 17b3518 to a88a8a3 Compare January 2, 2018 17:57

djzager requested changes Jan 2, 2018

View reviewed changes

rthallisey reviewed Jan 2, 2018

View reviewed changes

Addressing issues with determining if image pull error.

68b8b23

rthallisey reviewed Jan 2, 2018

View reviewed changes

rthallisey approved these changes Jan 2, 2018

View reviewed changes

djzager approved these changes Jan 2, 2018

View reviewed changes

rthallisey merged commit 0bade96 into openshift:master Jan 2, 2018

shawn-hurley mentioned this pull request Jan 11, 2018

Async bind feature #625

Merged

shawn-hurley mentioned this pull request Jan 22, 2018

custom error messaging #675

Closed

Bug 1512042 - Allowing error messages to make it from apb to user. #607

Bug 1512042 - Allowing error messages to make it from apb to user. #607

shawn-hurley commented Jan 2, 2018

coveralls commented Jan 2, 2018

djzager left a comment

djzager Jan 2, 2018

djzager Jan 2, 2018

djzager Jan 2, 2018

shawn-hurley Jan 2, 2018

djzager Jan 2, 2018 •

edited

shawn-hurley Jan 2, 2018

djzager Jan 2, 2018

djzager Jan 2, 2018

rthallisey Jan 2, 2018

shawn-hurley Jan 2, 2018

rthallisey Jan 2, 2018

shawn-hurley Jan 2, 2018

coveralls commented Jan 2, 2018

rthallisey Jan 2, 2018 •

edited

shawn-hurley Jan 2, 2018

rthallisey Jan 2, 2018

rthallisey Jan 2, 2018

coveralls commented Jan 2, 2018

djzager left a comment

djzager Jan 2, 2018

Bug 1512042 - Allowing error messages to make it from apb to user. #607

Bug 1512042 - Allowing error messages to make it from apb to user. #607

Conversation

shawn-hurley commented Jan 2, 2018

coveralls commented Jan 2, 2018

djzager 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

djzager Jan 2, 2018 • edited

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

coveralls commented Jan 2, 2018

rthallisey Jan 2, 2018 • edited

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

coveralls commented Jan 2, 2018

djzager left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djzager Jan 2, 2018 •

edited

rthallisey Jan 2, 2018 •

edited