Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Wait for component deletion, Add --wait flag to odo delete #2732

Merged
merged 2 commits into from
Apr 14, 2020
Merged

Wait for component deletion, Add --wait flag to odo delete #2732

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2b94652
Added --wait flag to odo delete
Mar 18, 2020
9f3ed5b
Added Integration test
Mar 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/application/application.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ func Delete(client *occlient.Client, name string) error {
}
}
// delete application from cluster
err = client.Delete(labels)
err = client.Delete(labels, false)
if err != nil {
return errors.Wrapf(err, "unable to delete application %s", name)
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/component/component.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,14 +270,14 @@ func CreateFromPath(client *occlient.Client, params occlient.CreateArgs) error {
}

// Delete whole component
func Delete(client *occlient.Client, componentName string, applicationName string) error {
func Delete(client *occlient.Client, wait bool, componentName, applicationName string) error {

// Loading spinner
s := log.Spinnerf("Deleting component %s", componentName)
defer s.End(false)

labels := componentlabels.GetLabels(componentName, applicationName, false)
err := client.Delete(labels)
err := client.Delete(labels, wait)
if err != nil {
return errors.Wrapf(err, "error deleting component %s", componentName)
}
Expand Down
63 changes: 57 additions & 6 deletions pkg/occlient/occlient.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,10 +93,11 @@ type CreateArgs struct {
}

const (
failedEventCount = 5
OcUpdateTimeout = 5 * time.Minute
OcBuildTimeout = 5 * time.Minute
OpenShiftNameSpace = "openshift"
failedEventCount = 5
OcUpdateTimeout = 5 * time.Minute
OcBuildTimeout = 5 * time.Minute
OpenShiftNameSpace = "openshift"
waitForComponentDeletionTimeout = 120 * time.Second

// timeout for getting the default service account
getDefaultServiceAccTimeout = 1 * time.Minute
Expand Down Expand Up @@ -2089,15 +2090,22 @@ func (c *Client) DisplayDeploymentConfigLog(deploymentConfigName string, followL
}

// Delete takes labels as a input and based on it, deletes respective resource
func (c *Client) Delete(labels map[string]string) error {
func (c *Client) Delete(labels map[string]string, wait bool) error {

// convert labels to selector
selector := util.ConvertLabelsToSelector(labels)
glog.V(4).Infof("Selectors used for deletion: %s", selector)

var errorList []string
var deletionPolicy = metav1.DeletePropagationBackground

// for --wait flag, it deletes component dependents first and then delete component
if wait {
deletionPolicy = metav1.DeletePropagationForeground
}
// Delete DeploymentConfig
glog.V(4).Info("Deleting DeploymentConfigs")
err := c.appsClient.DeploymentConfigs(c.Namespace).DeleteCollection(&metav1.DeleteOptions{}, metav1.ListOptions{LabelSelector: selector})
err := c.appsClient.DeploymentConfigs(c.Namespace).DeleteCollection(&metav1.DeleteOptions{PropagationPolicy: &deletionPolicy}, metav1.ListOptions{LabelSelector: selector})
if err != nil {
errorList = append(errorList, "unable to delete deploymentconfig")
}
Expand All @@ -2114,6 +2122,16 @@ func (c *Client) Delete(labels map[string]string) error {
errorList = append(errorList, "unable to delete imagestream")
}

// for --wait it waits for component to be deleted
// TODO: Need to modify for `odo app delete`, currently wait flag is added only in `odo component delete`
// so only one component gets passed in selector
if wait {
err = c.WaitForComponentDeletion(selector)
if err != nil {
errorList = append(errorList, err.Error())
}
}

// Error string
errString := strings.Join(errorList, ",")
if len(errString) != 0 {
Expand All @@ -2123,6 +2141,39 @@ func (c *Client) Delete(labels map[string]string) error {

}

// WaitForComponentDeletion waits for component to be deleted
func (c *Client) WaitForComponentDeletion(selector string) error {

glog.V(4).Infof("Waiting for component to get deleted")

watcher, err := c.appsClient.DeploymentConfigs(c.Namespace).Watch(metav1.ListOptions{LabelSelector: selector})
if err != nil {
return err
}
defer watcher.Stop()
eventCh := watcher.ResultChan()

for {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems that all the case statements in the select are terminating which means we might not need the for loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes even i thought so, and removed the for loop but it did not work, since we get events on eventCh other than error deleted, case events gets selected and executes none of the if conditions, returns from the function.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, maybe an idea would be to just v4 log those events for debugging purposes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup, sure will update for that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe lets do that in a follow up?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

created a follow up #2879

select {
case event, ok := <-eventCh:
_, typeOk := event.Object.(*appsv1.DeploymentConfig)
if !ok || !typeOk {
return errors.New("Unable to watch deployment config")
}
if event.Type == watch.Deleted {
glog.V(4).Infof("WaitForComponentDeletion, Event Recieved:Deleted")
return nil
} else if event.Type == watch.Error {
glog.V(4).Infof("WaitForComponentDeletion, Event Recieved:Deleted ")
return errors.New("Unable to watch deployment config")
}
case <-time.After(waitForComponentDeletionTimeout):
glog.V(4).Infof("WaitForComponentDeletion, Timeout")
return errors.New("Time out waiting for component to get deleted")
}
}
}

// DeleteServiceInstance takes labels as a input and based on it, deletes respective service instance
func (c *Client) DeleteServiceInstance(labels map[string]string) error {
glog.V(4).Infof("Deleting Service Instance")
Expand Down
1 change: 1 addition & 0 deletions pkg/odo/cli/application/delete.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package application

import (
"fmt"

odoUtil "github.com/openshift/odo/pkg/odo/util"

"github.com/openshift/odo/pkg/application"
Expand Down
9 changes: 6 additions & 3 deletions pkg/odo/cli/component/delete.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,10 @@ package component

import (
"fmt"
"path/filepath"

"github.com/openshift/odo/pkg/envinfo"
"github.com/openshift/odo/pkg/odo/util/experimental"
"path/filepath"

"github.com/openshift/odo/pkg/util"

Expand Down Expand Up @@ -36,6 +37,7 @@ var deleteExample = ktemplates.Examples(` # Delete component named 'frontend'.
type DeleteOptions struct {
componentForceDeleteFlag bool
componentDeleteAllFlag bool
componentDeleteWaitFlag bool
componentContext string
isCmpExists bool
*ComponentOptions
Expand All @@ -48,7 +50,7 @@ type DeleteOptions struct {

// NewDeleteOptions returns new instance of DeleteOptions
func NewDeleteOptions() *DeleteOptions {
return &DeleteOptions{false, false, "", false, &ComponentOptions{}, "", "", nil}
return &DeleteOptions{false, false, false, "", false, &ComponentOptions{}, "", "", nil}
}

// Complete completes log args
Expand Down Expand Up @@ -171,7 +173,7 @@ func (do *DeleteOptions) Run() (err error) {
log.Successf(fmt.Sprintf("Unlinked component %q from component %q for secret %q", parentComponent.Name, component, secretName))
}
}
err = component.Delete(do.Client, do.componentName, do.Application)
err = component.Delete(do.Client, do.componentDeleteWaitFlag, do.componentName, do.Application)
if err != nil {
return err
}
Expand Down Expand Up @@ -235,6 +237,7 @@ func NewCmdDelete(name, fullName string) *cobra.Command {

componentDeleteCmd.Flags().BoolVarP(&do.componentForceDeleteFlag, "force", "f", false, "Delete component without prompting")
componentDeleteCmd.Flags().BoolVarP(&do.componentDeleteAllFlag, "all", "a", false, "Delete component and local config")
componentDeleteCmd.Flags().BoolVarP(&do.componentDeleteWaitFlag, "wait", "w", false, "Wait for complete deletion of component and its dependent")

componentDeleteCmd.SetUsageTemplate(odoutil.CmdUsageTemplate)
completion.RegisterCommandHandler(componentDeleteCmd, completion.ComponentNameCompletionHandler)
Expand Down
8 changes: 8 additions & 0 deletions tests/helper/helper_oc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -481,3 +481,11 @@ func (oc *OcRunner) GetServices(namespace string) string {
output := string(session.Wait().Out.Contents())
return output
}

// VerifyResourceDeleted verifies if the given resource is deleted from cluster
func (oc *OcRunner) VerifyResourceDeleted(resourceType, resourceName, namespace string) {
session := CmdRunner(oc.path, "get", resourceType, "--namespace", namespace)
Eventually(session).Should(gexec.Exit(0))
output := string(session.Wait().Out.Contents())
mik-dass marked this conversation as resolved.
Show resolved Hide resolved
adisky marked this conversation as resolved.
Show resolved Hide resolved
Expect(output).NotTo(ContainSubstring(resourceName))
}
36 changes: 31 additions & 5 deletions tests/integration/component.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -835,13 +835,14 @@ func componentTests(args ...string) {
It("should delete the component and the owned resources", func() {
helper.CopyExample(filepath.Join("source", "nodejs"), context)
helper.CmdShouldPass("odo", append(args, "create", "nodejs", cmpName, "--app", appName, "--project", project, "--context", context)...)
helper.CmdShouldPass("odo", "url", "create", "example", "--context", context)
helper.CmdShouldPass("odo", "storage", "create", "storage-name", "--size", "1Gi", "--path", "/data", "--context", context)
helper.ValidateLocalCmpExist(context, "Type,nodejs", "Name,"+cmpName, "Application,"+appName, "URL,0,Name,example")
helper.CmdShouldPass("odo", "url", "create", "example-1", "--context", context)

helper.CmdShouldPass("odo", "storage", "create", "storage-1", "--size", "1Gi", "--path", "/data1", "--context", context)
helper.ValidateLocalCmpExist(context, "Type,nodejs", "Name,"+cmpName, "Application,"+appName, "URL,0,Name,example-1")
helper.CmdShouldPass("odo", append(args, "push", "--context", context)...)

helper.CmdShouldPass("odo", "url", "create", "example-1", "--context", context)
helper.CmdShouldPass("odo", "storage", "create", "storage-name-1", "--size", "1Gi", "--path", "/data-1", "--context", context)
helper.CmdShouldPass("odo", "url", "create", "example-2", "--context", context)
helper.CmdShouldPass("odo", "storage", "create", "storage-2", "--size", "1Gi", "--path", "/data2", "--context", context)
helper.CmdShouldPass("odo", append(args, "push", "--context", context)...)

helper.CmdShouldPass("odo", append(args, "delete", "-f", "--context", context)...)
Expand All @@ -853,5 +854,30 @@ func componentTests(args ...string) {
oc.WaitAndCheckForExistence("is", project, 1)
oc.WaitAndCheckForExistence("service", project, 1)
})

It("should delete the component and the owned resources with wait flag", func() {
helper.CopyExample(filepath.Join("source", "nodejs"), context)
helper.CmdShouldPass("odo", append(args, "create", "nodejs", cmpName, "--app", appName, "--project", project, "--context", context)...)
helper.CmdShouldPass("odo", "url", "create", "example-1", "--context", context)

helper.CmdShouldPass("odo", "storage", "create", "storage-1", "--size", "1Gi", "--path", "/data1", "--context", context)
helper.ValidateLocalCmpExist(context, "Type,nodejs", "Name,"+cmpName, "Application,"+appName, "URL,0,Name,example-1")
helper.CmdShouldPass("odo", append(args, "push", "--context", context)...)

helper.CmdShouldPass("odo", "url", "create", "example-2", "--context", context)
helper.CmdShouldPass("odo", "storage", "create", "storage-2", "--size", "1Gi", "--path", "/data2", "--context", context)
helper.CmdShouldPass("odo", append(args, "push", "--context", context)...)

// delete with --wait flag
helper.CmdShouldPass("odo", append(args, "delete", "-f", "-w", "--context", context)...)

oc.VerifyResourceDeleted("routes", "example", project)
oc.VerifyResourceDeleted("service", cmpName, project)
// verify s2i pvc is delete
oc.VerifyResourceDeleted("pvc", "s2idata", project)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are three PVCs, we need to check if all of them are deleted

Running oc with args [oc get pvc --namespace twmwqqprru]
[oc] NAME                  STATUS    VOLUME                                     CAPACITY   ACCESS MODES   STORAGECLASS    AGE
[oc] mvfzn-hgfxb-s2idata   Bound     pvc-jdslkaj-9f92-ljajlds          1Gi        RWO            gp2-encrypted   1m
[oc] storage-1-hgfxb-pvc   Bound     pvc-dasjldj-9f92-dhfhd           1Gi        RWO            gp2-encrypted   1m
[oc] storage-2-hgfxb-pvc   Bound     pvc-jsalkjd-9f92-lksala           1Gi        RWO            gp2-encrypted   44s

Copy link
Contributor Author

@adisky adisky Apr 9, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for reminding, added

oc.VerifyResourceDeleted("pvc", "storage-1", project)
oc.VerifyResourceDeleted("pvc", "storage-2", project)
oc.VerifyResourceDeleted("dc", cmpName, project)
})
})
}