delete objects based on label #483

procrypt · 2017-03-10T09:23:11Z

@kadel @containscafeine @surajssd @cdrage

This PR uses the SelectorFromSet() function which takes alabel or selector as an argument returns all the object that uses that label or selector. Once we get the object that uses a particular label we can further do the delete operation on them. This is similar to kubectl delete <object> --selector=<key>=<value>. Also the label has been modified from service to io.kompose.service.
Thanks @kadel for all the help :)
Review please.
Fixes #255

cdrage · 2017-03-10T14:59:24Z

Hi @procrypt

Just two things before reviewing!

Please split the commits, one for updating vendoring, and the other the actual code you're modifying. Otherwise, it's really hard to review on GitHub / gif dif
Please update your Git commit messages outlining what's changed / more descriptive rather than Fixes #255. You can do this with git commit --amend

procrypt · 2017-03-10T16:16:17Z

Hey @cdrage I have split the commit into two different commit one for the code and other for vender.
Also I have elaborated what this PR is doing.
Ready for review :)

cdrage · 2017-03-10T16:27:18Z

@procrypt

Commit titles shouldn't have underscores in them, and it looks like you haven't put your updated message into your commit, you need to git commit --amend, add then git push -f it. And it's vendor :)

procrypt · 2017-03-10T16:46:10Z

@cdrage Done :D

cdrage · 2017-03-13T19:48:45Z

I wasn't excepting the labels to appear when using kompose convert, I've typed up my concerns in the issue #255 !

Great code however, I don't see any problems with it and I've tested the PR 👍

cdrage · 2017-03-13T19:49:52Z

I think we should get in the habit of commenting on our code, mind adding a few lines @procrypt to your implementation?

kadel · 2017-03-14T17:01:05Z

pkg/transformer/kubernetes/kubernetes.go

@@ -633,37 +635,67 @@ func (k *Kubernetes) Undeploy(komposeObject kobject.KomposeObject, opt kobject.C
 		switch t := v.(type) {
 		case *extensions.Deployment:
 			//delete deployment
-			rpDeployment, err := kubectl.ReaperFor(extensions.Kind("Deployment"), client)
+			label := labels.SelectorFromSet(labels.Set(map[string]string{"io.kompose.service": t.Name}))


io.kompose.service is used in a lot of places. maybe we could use transformer.ConfigLabels to handle all the labels.
Or other option would be to create new function or constant that will hold that string

procrypt · 2017-03-14T17:56:05Z

@kadel Updated PR.

kadel · 2017-03-16T16:34:55Z

this will require new vendor update :-(

I think that easiest thing to do, will be remove 'update vendor' commit from your branch, do rebase, and than update vendor again ;-)

procrypt · 2017-03-16T20:43:54Z

@kadel Done all green now :D

kadel · 2017-03-17T12:31:13Z

pkg/transformer/kubernetes/kubernetes.go

@@ -705,37 +708,67 @@ func (k *Kubernetes) Undeploy(komposeObject kobject.KomposeObject, opt kobject.C
 		switch t := v.(type) {
 		case *extensions.Deployment:
 			//delete deployment
-			rpDeployment, err := kubectl.ReaperFor(extensions.Kind("Deployment"), client)
+			label := labels.SelectorFromSet(labels.Set(map[string]string{selector: t.Name}))


This and next line is same for every object. You can safely move them outside of this switch and for, than you won't have to repeat same code multiple times 😉

@kadel If we put

label := labels.SelectorFromSet(labels.Set(map[string]string{selector: t.Name}))

outside switch and for how do we access the name of the object we want to delete created, here we are accessing it using t.Name.

Than you can do it like this:
Keep it in for, but move it outside switch.

You can access object metadata fields by casting it to meta.Object.

// import "k8s.io/kubernetes/pkg/api/meta" v.(meta.Object).GetName()

kadel · 2017-03-17T12:34:04Z

pkg/transformer/openshift/openshift.go

@@ -509,11 +514,20 @@ func (o *OpenShift) Undeploy(komposeObject kobject.KomposeObject, opt kobject.Co
 		switch t := v.(type) {
 		case *imageapi.ImageStream:
 			//delete imageStream
-			err = oclient.ImageStreams(namespace).Delete(t.Name)
+			label := labels.SelectorFromSet(labels.Set(map[string]string{selector: t.Name}))


same think as in kubernetes, you are repeating same lines

kadel · 2017-03-17T13:01:07Z

pkg/transformer/kubernetes/kubernetes.go

+					return err
+				}
+				log.Infof("Successfully deleted PersistentVolumeClaim: %s", t.Name)
+			}

 		case *extensions.Ingress:


I think we should do same thing for Ingress and Pods also.

kadel · 2017-03-17T13:01:55Z

pkg/transformer/openshift/openshift.go

+					return err
+				}
+				log.Infof("Successfully deleted PersistentVolumeClaim: %s", t.Name)
+			}

 		case *routeapi.Route:


Don't forget about Pods and Routes ;-)

cdrage · 2017-03-17T14:40:35Z

pkg/transformer/kubernetes/kubernetes.go

@@ -695,6 +697,7 @@ func (k *Kubernetes) Undeploy(komposeObject kobject.KomposeObject, opt kobject.C
 	if err != nil {
 		return errors.Wrap(err, "k.Transform failed")
 	}
+	selector := "io.kompose.service"


not to nitpick, but should this instead be a constant and at the top of the file? (outside the scope) ping @kadel

@cdrage you are right. I was thinking about it, but forgot to commend on it :-)

This should be even somewhere else (maybe const in transformer pkg?), so ConfigLabels function can use it, and we don't have same magic string in two places.

kadel · 2017-03-20T11:19:46Z

if nothing is deployed and call kompose down is called:

./kompose down
panic: runtime error: index out of range

goroutine 1 [running]:
github.com/kubernetes-incubator/kompose/pkg/transformer/kubernetes.(*Kubernetes).Undeploy(0xc4202cc180, 0xc42033e8a0, 0x1c625c5, 0x7, 0x100, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
	/home/tomas/dev/go/src/github.com/kubernetes-incubator/kompose/pkg/transformer/kubernetes/kubernetes.go:740 +0x14d6
github.com/kubernetes-incubator/kompose/pkg/app.Down(0x100, 0x0, 0x0, 0x0, 0x0, 0x0, 0x1, 0xc42025fe50, 0x1, 0x1, ...)
	/home/tomas/dev/go/src/github.com/kubernetes-incubator/kompose/pkg/app/app.go:288 +0x2be
github.com/kubernetes-incubator/kompose/cmd.glob..func5(0x2a3b200, 0x2a69368, 0x0, 0x0)
	/home/tomas/dev/go/src/github.com/kubernetes-incubator/kompose/cmd/down.go:52 +0x4f
github.com/kubernetes-incubator/kompose/vendor/github.com/spf13/cobra.(*Command).execute(0x2a3b200, 0x2a69368, 0x0, 0x0, 0x2a3b200, 0x2a69368)
	/home/tomas/dev/go/src/github.com/kubernetes-incubator/kompose/vendor/github.com/spf13/cobra/command.go:603 +0x22b
github.com/kubernetes-incubator/kompose/vendor/github.com/spf13/cobra.(*Command).ExecuteC(0x2a3b420, 0xc420070058, 0x0, 0xc4204b9f48)
	/home/tomas/dev/go/src/github.com/kubernetes-incubator/kompose/vendor/github.com/spf13/cobra/command.go:689 +0x339
github.com/kubernetes-incubator/kompose/vendor/github.com/spf13/cobra.(*Command).Execute(0x2a3b420, 0x1, 0x1)
	/home/tomas/dev/go/src/github.com/kubernetes-incubator/kompose/vendor/github.com/spf13/cobra/command.go:648 +0x2b
github.com/kubernetes-incubator/kompose/cmd.Execute()
	/home/tomas/dev/go/src/github.com/kubernetes-incubator/kompose/cmd/root.go:92 +0x31
main.main()
	/home/tomas/dev/go/src/github.com/kubernetes-incubator/kompose/main.go:22 +0x20

procrypt · 2017-03-20T12:31:42Z

@kadel Ah this is bad, I guess I know what the problem is, when we call kompose down the

client.Deployments(namespace).List()            returns  *extensions.DeploymentList
client.Services(namespace).List()               returns  *api.ServiceList
client.PersistentVolumeClaims(namespace).List() returns  *api.PersistentVolumeClaimList

and the struct

DeploymentList            contains  Items []Deployment
ServiceList               contains  Items []Service
PersistentVolumeClaimList contains  Items []PersistentVolumeClaim

and to get the Label we are accessing the first index of the each of slice of

Deployment              using  deployment.Items[0].Labels
Service                 using  svc.Items[0].Labels
PersistentVolumeClaim   using  pvc.Items[0].Labels

So this works when we do kompose up and then do kompose down because the application has been already deployed and kompose down get what needed to find the Label, but if we do kompose down and there is no application deployed the there is are objects, kompose tries to access the first element of the

DeploymentList 
ServiceList
PersistentVolumeClaimList

and since there is nothing like

deployment.Items[0].Labels
svc.Items[0].Labels
pvc.Items[0].Labels

It panics with runtime error: index out of range
Working to fix this.

procrypt · 2017-03-20T13:29:53Z

@kadel Fixed kompose down

$ kompose down
FATA[0000] Error while deleting application: service `redis` not found

procrypt · 2017-03-21T10:57:12Z

@kadel @cdrage Updated the PR Please Review

kadel

two last small changes, that it will be perfect ;-)

kadel · 2017-03-22T11:52:36Z

pkg/transformer/kubernetes/kubernetes.go

-			err = rpDeployment.Stop(namespace, t.Name, TIMEOUT*time.Second, nil)
-			if err != nil {
-				return err
+			komposeLabel := map[string]string{transformer.Selector: t.Name}


You can move this outside of this switch. That you wont have to repeat same line in every case.

kadel · 2017-03-22T11:53:15Z

pkg/transformer/kubernetes/kubernetes.go

-			if err != nil {
-				return err
+			komposeLabel := map[string]string{transformer.Selector: t.Name}
+			if len(deployment.Items) == 0 {


You can completely remove this this now.

@kadel If we remove this line, there will be no output of kompose down is there is nothing deployed and you mentioned earlier that we want at least want to give an error in this situation.

That is OK. It is not an error if nothing is found.

But we should warn the user about it.

If there are no objects, that its fine. The were not created or already have been deleted. No need to inform user about that. Nothing happened.

This PR uses the "SelectorFromSet()" function which takes alabel or selector as an argument returns all the object that uses that label or selector. Once we get the object that uses a particular label we can further do the delete operation on them. This is similar to "kubectl delete <object> --selector=<key>=<value>". Also the label has been modified from service to io.kompose.service.

procrypt · 2017-03-23T19:06:45Z

@kadel Updated and removed the check 😉

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

procrypt force-pushed the empty_vols branch 3 times, most recently from 9bf74fd to 9233c70 Compare March 10, 2017 10:24

procrypt force-pushed the empty_vols branch 2 times, most recently from 6bf4a0f to 0aafb02 Compare March 10, 2017 16:01

procrypt changed the title ~~[WIP]delete_objects_based_on_label~~ [WIP]delete objects based on label Mar 10, 2017

procrypt force-pushed the empty_vols branch 3 times, most recently from 4fbb0d1 to 2fa5292 Compare March 10, 2017 16:44

surajssd mentioned this pull request Mar 13, 2017

kompose down with openshift doesn't delete BuildConfig #382

Closed

procrypt force-pushed the empty_vols branch 2 times, most recently from 118f8cd to 3082ddd Compare March 13, 2017 20:30

procrypt changed the title ~~[WIP]delete objects based on label~~ delete objects based on label Mar 13, 2017

procrypt force-pushed the empty_vols branch from 3082ddd to 527c942 Compare March 13, 2017 22:01

kadel reviewed Mar 14, 2017

View reviewed changes

procrypt force-pushed the empty_vols branch 2 times, most recently from d9ab7c9 to 2683c84 Compare March 14, 2017 17:54

surajssd added the rebase needed label Mar 16, 2017

surajssd mentioned this pull request Mar 16, 2017

Kompose will keep trying its job #477

Merged

procrypt force-pushed the empty_vols branch 2 times, most recently from aa95ee2 to e9cdb8b Compare March 16, 2017 18:07

procrypt force-pushed the empty_vols branch 2 times, most recently from 5173b97 to 0e6df93 Compare March 16, 2017 20:34

kadel removed the rebase needed label Mar 17, 2017

kadel requested changes Mar 17, 2017

View reviewed changes

cdrage reviewed Mar 17, 2017

View reviewed changes

procrypt force-pushed the empty_vols branch 2 times, most recently from c767d3b to a92dee9 Compare March 20, 2017 04:36

procrypt force-pushed the empty_vols branch from a92dee9 to 60cf7c1 Compare March 20, 2017 11:38

procrypt force-pushed the empty_vols branch from 60cf7c1 to 17134f1 Compare March 20, 2017 13:27

procrypt force-pushed the empty_vols branch 4 times, most recently from 0985d02 to f51e41f Compare March 21, 2017 10:55

kadel requested changes Mar 22, 2017

View reviewed changes

procrypt force-pushed the empty_vols branch from f51e41f to 1e05eff Compare March 23, 2017 04:01

procrypt added 2 commits March 24, 2017 00:31

vendor update

d212758

procrypt force-pushed the empty_vols branch from 1e05eff to d212758 Compare March 23, 2017 19:05

kadel approved these changes Mar 24, 2017

View reviewed changes

kadel merged commit 3c3ae98 into kubernetes:master Mar 24, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

delete objects based on label #483

delete objects based on label #483

procrypt commented Mar 10, 2017 •

edited

Loading

cdrage commented Mar 10, 2017

procrypt commented Mar 10, 2017

cdrage commented Mar 10, 2017 •

edited

Loading

procrypt commented Mar 10, 2017

cdrage commented Mar 13, 2017

cdrage commented Mar 13, 2017

kadel Mar 14, 2017

procrypt commented Mar 14, 2017

kadel commented Mar 16, 2017

procrypt commented Mar 16, 2017

kadel Mar 17, 2017

procrypt Mar 20, 2017 •

edited

Loading

kadel Mar 20, 2017 •

edited

Loading

kadel Mar 17, 2017

kadel Mar 17, 2017 •

edited

Loading

kadel Mar 17, 2017

cdrage Mar 17, 2017

kadel Mar 17, 2017

kadel commented Mar 20, 2017

procrypt commented Mar 20, 2017 •

edited

Loading

procrypt commented Mar 20, 2017 •

edited

Loading

procrypt commented Mar 21, 2017

kadel left a comment

kadel Mar 22, 2017

kadel Mar 22, 2017

procrypt Mar 23, 2017

kadel Mar 23, 2017

procrypt Mar 23, 2017

kadel Mar 23, 2017

procrypt commented Mar 23, 2017

delete objects based on label #483

delete objects based on label #483

Conversation

procrypt commented Mar 10, 2017 • edited Loading

cdrage commented Mar 10, 2017

procrypt commented Mar 10, 2017

cdrage commented Mar 10, 2017 • edited Loading

procrypt commented Mar 10, 2017

cdrage commented Mar 13, 2017

cdrage commented Mar 13, 2017

Choose a reason for hiding this comment

procrypt commented Mar 14, 2017

kadel commented Mar 16, 2017

procrypt commented Mar 16, 2017

Choose a reason for hiding this comment

procrypt Mar 20, 2017 • edited Loading

Choose a reason for hiding this comment

kadel Mar 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kadel Mar 17, 2017 • edited Loading

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 20, 2017

procrypt commented Mar 20, 2017 • edited Loading

procrypt commented Mar 20, 2017 • edited Loading

procrypt commented Mar 21, 2017

kadel 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

procrypt commented Mar 23, 2017

procrypt commented Mar 10, 2017 •

edited

Loading

cdrage commented Mar 10, 2017 •

edited

Loading

procrypt Mar 20, 2017 •

edited

Loading

kadel Mar 20, 2017 •

edited

Loading

kadel Mar 17, 2017 •

edited

Loading

procrypt commented Mar 20, 2017 •

edited

Loading

procrypt commented Mar 20, 2017 •

edited

Loading