Kompose will keep trying its job #477

surajnarwade · 2017-03-08T13:07:27Z

fixes #270

surajnarwade · 2017-03-08T13:20:05Z

cdrage · 2017-03-09T13:27:00Z

pkg/transformer/kubernetes/kubernetes.go

@@ -640,9 +640,10 @@ func (k *Kubernetes) Undeploy(komposeObject kobject.KomposeObject, opt kobject.C
 			//FIXME: gracePeriod is nil
 			err = rpDeployment.Stop(namespace, t.Name, TIMEOUT*time.Second, nil)
 			if err != nil {
-				return err


These REALLY shouldn't be removed... If it errors out during deployment Kompose should return an err. It's not good Go practice to simply log.Error(err).

If we're retrying for a job.... this should be more complicated and use a Go channel (maybe) + retry maybe 3 times + timeout then return err.

Thoughts @kadel ?

@cdrage , if we returned the error, kompose will not be able to keep doing its job (as discussed in issue, its there only in the case of kompose down)

Whole point of the issue is that we error out (return error) and that is not good. We need at least try delete the rest of the objects. Error in deleting one of the objects shouldn't be show stopper for deleting the rest.

On the other hand, Undeploy function should probably return error if we can't be sure that all the objects were deleted. Maybe those error should be aggregated so it can be returned at the end.

@containscafeine was looking into error handling lately. He might know what would be best here.

How about we defer the return err statements? This way, the errors will get returned at the end of Undeploy() and by that time the other resources would also have been deleted?

I don't know if defer is going to help here.
Maybe just collecting all the errors to array and then returning it should be enough.

Just create a list of errors and then return that maybe and the caller of undeploy should print it in right way or whatever!

cdrage · 2017-03-10T15:18:26Z

Please use git commit --amend as well to update your git message so it's more descriptive!

surajssd · 2017-03-16T16:05:45Z

Anyway the code this PR is editing is gonna go away because @procrypt is working on new approach of deleting things! #483

kadel · 2017-03-16T16:18:20Z

It is not going to go away. This logic will be still needed, #483 is not solving this problem.
But it will create lot of annoying conflicts :-(

surajnarwade · 2017-03-28T10:25:49Z

cc @kadel @surajssd @cdrage

cdrage · 2017-03-28T11:36:30Z

pkg/transformer/openshift/openshift.go

 				}
 			}
 		}
 	}
-	return nil
+	return ErrorList


This seems a bit unconventional for Go, why an ErrorList? We should be returning an error when it actually happens and not continue throughout the code, should this be nil? @kadel

Honestly, it seems awkward that in order to "keep trying" for Kompose, it means going against Go conventions and continuing through the code brute-force. Having logic throughout the code would be more viable instead.

@cdrage , as @kadel said, create list of errors and return it after deleting all components.

Okay, shouldn't we use https://golang.org/pkg/go/scanner/#ErrorList instead? @kadel

This seems a bit unconventional for Go, why an ErrorList? We should be returning an error when it actually happens and not continue throughout the code, should this be nil? @kadel

I don't think that this is against Go conventions. It quite common to aggregate errors into slice. If you look at Kubernetes code base you can see it there. Only difference is that they are doing it little bit smarter. They don't return slice of errors, but Aggregate. Aggregate is special type that implements error interface and can contain multiple errors. (see https://github.com/kubernetes/kubernetes/blob/55bee3ad21f025b1416a4e1f10de753f484b66d3/staging/src/k8s.io/apimachinery/pkg/util/errors/errors.go#L36).
I don't think we need to do that like that right now, just returning []error should be enough.

Something went wrong, even if we tried to continue but we need to inform user that something wasn't deleted.

This is simplification, because ideally deletion should happen in parallel. Every delete should have run its own goroutine. But I don't think we should worry about that right now. Doing it serially on-by-one is ok for now.

This makes sense. Instead of Go routines (maybe implement them later if API calls start becoming lengthy), just iterating through each-step would be the best-case-scenario. I totally agree!

kadel · 2017-03-28T19:54:46Z

Okay, shouldn't we use https://golang.org/pkg/go/scanner/#ErrorList instead? @kadel

It looks like that ErrorList is specially made for scanner package.

kadel · 2017-03-28T19:55:41Z

pkg/transformer/kubernetes/kubernetes.go

@@ -691,17 +691,19 @@ func (k *Kubernetes) Deploy(komposeObject kobject.KomposeObject, opt kobject.Con
 }

 // Undeploy deletes deployed objects from Kubernetes cluster
-func (k *Kubernetes) Undeploy(komposeObject kobject.KomposeObject, opt kobject.ConvertOptions) error {
+func (k *Kubernetes) Undeploy(komposeObject kobject.KomposeObject, opt kobject.ConvertOptions) []error {
+	var ErrorList []error


There shouldn't be capital 'E'. it should be lower-case.
Uppercase in go means public variable, we don't want that here.

kadel · 2017-03-28T19:56:26Z

pkg/transformer/openshift/openshift.go

@@ -495,20 +495,22 @@ func (o *OpenShift) Deploy(komposeObject kobject.KomposeObject, opt kobject.Conv
 }

 //Undeploy removes deployed artifacts from OpenShift cluster
-func (o *OpenShift) Undeploy(komposeObject kobject.KomposeObject, opt kobject.ConvertOptions) error {
+func (o *OpenShift) Undeploy(komposeObject kobject.KomposeObject, opt kobject.ConvertOptions) []error {
+	var ErrorList []error


same here. this should be errorList

cdrage

Other than the changes requests (removing comments), LGTM.

cdrage · 2017-03-30T12:55:28Z

pkg/transformer/kubernetes/kubernetes.go

 	//Convert komposeObject
 	objects, err := k.Transform(komposeObject, opt)

 	if err != nil {
-		return errors.Wrap(err, "k.Transform failed")
+		errorList = append(errorList, err)
+		//return errors.Wrap(err, "k.Transform failed")


need to remove this comment

cdrage · 2017-03-30T12:55:59Z

pkg/transformer/openshift/openshift.go

 	//Convert komposeObject
 	objects, err := o.Transform(komposeObject, opt)

 	if err != nil {
-		return errors.Wrap(err, "o.Transform failed")
+		errorList = append(errorList, err)
+		//return errors.Wrap(err, "o.Transform failed")


this needs to be removed

fixes kubernetes#270 all errors in undeploy method(both kubernetes and openshift) are appended in a slice of errors, and then it will be returned after successful deletion of all components.

surajnarwade · 2017-03-31T08:58:03Z

@cdrage done with changes

cdrage · 2017-04-03T15:25:09Z

LGTM

kadel · 2017-04-03T15:46:31Z

I just found that there is a BIG mistake in there.

If it can't connect to cluster it panics.

Problem is that we continue after every error.
There should be return after some errors, and after every error in switch should be break

cdrage · 2017-04-03T15:53:40Z

@kadel Yeah. I thought we were brute-forcing the force down since we're iterating right through each-one of them.

I've opened up a PR to revert this so we can at least push a release today.

kadel · 2017-04-03T16:00:01Z

yeh, that was a bit too much brute-forcing :-)
For example we can't continue and call Stop if client creation failed. In that case it should continue and try to create client again for next object.

new take on "Kompose will keep trying its job #477"

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 8, 2017

cdrage requested changes Mar 9, 2017

View reviewed changes

kadel added the rebase needed label Mar 24, 2017

surajnarwade force-pushed the keep_trying_job branch from c03dadf to 27b37ca Compare March 27, 2017 08:39

cdrage requested changes Mar 28, 2017

View reviewed changes

kadel reviewed Mar 28, 2017

View reviewed changes

surajnarwade force-pushed the keep_trying_job branch from 27b37ca to ccad3d4 Compare March 30, 2017 04:33

cdrage requested changes Mar 30, 2017

View reviewed changes

Kompose will keep trying its job

80b15c1

fixes kubernetes#270 all errors in undeploy method(both kubernetes and openshift) are appended in a slice of errors, and then it will be returned after successful deletion of all components.

surajnarwade force-pushed the keep_trying_job branch from ccad3d4 to 80b15c1 Compare March 31, 2017 08:44

kadel approved these changes Apr 3, 2017

View reviewed changes

cdrage approved these changes Apr 3, 2017

View reviewed changes

cdrage merged commit 14a5ed3 into kubernetes:master Apr 3, 2017

cdrage mentioned this pull request Apr 3, 2017

Revert "Kompose will keep trying its job" #535

Merged

kadel mentioned this pull request Apr 3, 2017

new take on "Kompose will keep trying its job #477" #536

Merged

cdrage added a commit that referenced this pull request Apr 10, 2017

Merge pull request #536 from kadel/pr477-angain-and-better

27fc70a

new take on "Kompose will keep trying its job #477"

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kompose will keep trying its job #477

Kompose will keep trying its job #477

surajnarwade commented Mar 8, 2017

surajnarwade commented Mar 8, 2017

cdrage Mar 9, 2017 •

edited

Loading

surajnarwade Mar 9, 2017

kadel Mar 9, 2017

concaf Mar 13, 2017 •

edited

Loading

kadel Mar 14, 2017

surajssd Mar 16, 2017

cdrage commented Mar 10, 2017

surajssd commented Mar 16, 2017

kadel commented Mar 16, 2017

surajnarwade commented Mar 28, 2017

cdrage Mar 28, 2017

surajnarwade Mar 28, 2017

cdrage Mar 28, 2017

kadel Mar 28, 2017

cdrage Mar 29, 2017

kadel commented Mar 28, 2017

kadel Mar 28, 2017

kadel Mar 28, 2017

cdrage left a comment

cdrage Mar 30, 2017

cdrage Mar 30, 2017

surajnarwade commented Mar 31, 2017

cdrage commented Apr 3, 2017

kadel commented Apr 3, 2017

cdrage commented Apr 3, 2017

kadel commented Apr 3, 2017

Kompose will keep trying its job #477

Kompose will keep trying its job #477

Conversation

surajnarwade commented Mar 8, 2017

surajnarwade commented Mar 8, 2017

cdrage Mar 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

concaf Mar 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdrage commented Mar 10, 2017

surajssd commented Mar 16, 2017

kadel commented Mar 16, 2017

surajnarwade commented Mar 28, 2017

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

kadel commented Mar 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdrage left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

surajnarwade commented Mar 31, 2017

cdrage commented Apr 3, 2017

kadel commented Apr 3, 2017

cdrage commented Apr 3, 2017

kadel commented Apr 3, 2017

cdrage Mar 9, 2017 •

edited

Loading

concaf Mar 13, 2017 •

edited

Loading