From a7037a4f7a3735da2cc7975ea5e76f6a22e62982 Mon Sep 17 00:00:00 2001 From: Charlie Drage Date: Wed, 20 May 2020 09:41:53 -0400 Subject: [PATCH] Actually wait for the project to be deleted.. Actually wait.. Signed-off-by: Charlie Drage --- pkg/occlient/occlient.go | 140 +++++++++++------- pkg/odo/cli/project/delete.go | 18 ++- pkg/project/project.go | 13 +- pkg/project/project_test.go | 23 ++- tests/integration/project/cmd_project_test.go | 36 +++++ 5 files changed, 162 insertions(+), 68 deletions(-) diff --git a/pkg/occlient/occlient.go b/pkg/occlient/occlient.go index 9d25b663adf..82e29b7b76e 100644 --- a/pkg/occlient/occlient.go +++ b/pkg/occlient/occlient.go @@ -103,6 +103,9 @@ const ( // timeout for getting the default service account getDefaultServiceAccTimeout = 1 * time.Minute + // timeout for waiting for project deletion + waitForProjectDeletionTimeOut = 3 * time.Minute + // The length of the string to be generated for names of resources nameLength = 5 @@ -2214,67 +2217,102 @@ func (c *Client) DeleteServiceInstance(labels map[string]string) error { } // DeleteProject deletes given project -func (c *Client) DeleteProject(name string) error { - err := c.projectClient.Projects().Delete(name, &metav1.DeleteOptions{}) - if err != nil { - return errors.Wrap(err, "unable to delete project") +// +// NOTE: +// There is a very specific edge case that may happen during project deletion when deleting a project and then immediately creating another. +// Unfortunately, despite the watch interface, we cannot safely determine if the project is 100% deleted. See this link: +// https://stackoverflow.com/questions/48208001/deleted-openshift-online-pro-project-has-left-a-trace-so-cannot-create-project-o +// Will Gordon (Engineer @ Red Hat) describes the issue: +// +// "Projects are deleted asynchronously after you send the delete command. So it's possible that the deletion just hasn't been reconciled yet. It should happen within a minute or so, so try again. +// Also, please be aware that in a multitenant environment, like OpenShift Online, you are prevented from creating a project with the same name as any other project in the cluster, even if it's not your own. So if you can't create the project, it's possible that someone has already created a project with the same name." +func (c *Client) DeleteProject(name string, wait bool) error { + + // Instantiate watcher for our "wait" function + var watcher watch.Interface + var err error + + // If --wait has been passed, we will wait for the project to fully be deleted + if wait { + watcher, err = c.projectClient.Projects().Watch(metav1.ListOptions{ + FieldSelector: fields.Set{"metadata.name": name}.AsSelector().String(), + }) + if err != nil { + return errors.Wrapf(err, "unable to watch project") + } + defer watcher.Stop() } - // wait for delete to complete - w, err := c.projectClient.Projects().Watch(metav1.ListOptions{ - FieldSelector: fields.Set{"metadata.name": name}.AsSelector().String(), - }) + // Delete the project + err = c.projectClient.Projects().Delete(name, &metav1.DeleteOptions{}) if err != nil { - return errors.Wrapf(err, "unable to watch project") + return errors.Wrap(err, "unable to delete project") } - defer w.Stop() - for { - val, ok := <-w.ResultChan() - // When marked for deletion... val looks like: - /* - val: { - Type:MODIFIED - Object:&Project{ - ObjectMeta:k8s_io_apimachinery_pkg_apis_meta_v1.ObjectMeta{...}, - Spec:ProjectSpec{...}, - Status:ProjectStatus{ - Phase:Terminating, - }, - } - } - */ - // Post deletion val will look like: - /* - val: { - Type:DELETED - Object:&Project{ - ObjectMeta:k8s_io_apimachinery_pkg_apis_meta_v1.ObjectMeta{...}, - Spec:ProjectSpec{...}, - Status:ProjectStatus{ - Phase:, - }, - } - } - */ - if !ok { - return fmt.Errorf("received unexpected signal %+v on project watch channel", val) - } - // So we depend on val.Type as val.Object.Status.Phase is just empty string and not a mapped value constant - if prj, ok := val.Object.(*projectv1.Project); ok { - klog.V(4).Infof("Status of delete of project %s is %s", name, prj.Status.Phase) - switch prj.Status.Phase { - //prj.Status.Phase can only be "Terminating" or "Active" or "" - case "": - if val.Type == watch.Deleted { - return nil + // If watcher has been created (wait was passed) we will create a go routine and actually **wait** + // until *EVERYTHING* is successfully deleted. + if watcher != nil { + + // Project channel + // Watch error channel + projectChannel := make(chan *projectv1.Project) + watchErrorChannel := make(chan error) + + // Create a go routine to run in the background + go func() { + + for { + + // If watch unexpected has been closed.. + val, ok := <-watcher.ResultChan() + if !ok { + //return fmt.Errorf("received unexpected signal %+v on project watch channel", val) + watchErrorChannel <- errors.Errorf("watch channel was closed unexpectedly: %+v", val) + break } - if val.Type == watch.Error { - return fmt.Errorf("failed watching the deletion of project %s", name) + + // So we depend on val.Type as val.Object.Status.Phase is just empty string and not a mapped value constant + if projectStatus, ok := val.Object.(*projectv1.Project); ok { + + klog.V(4).Infof("Status of delete of project %s is '%s'", name, projectStatus.Status.Phase) + + switch projectStatus.Status.Phase { + //projectStatus.Status.Phase can only be "Terminating" or "Active" or "" + case "": + if val.Type == watch.Deleted { + projectChannel <- projectStatus + break + } + if val.Type == watch.Error { + watchErrorChannel <- errors.Errorf("failed watching the deletion of project %s", name) + break + } + } + + } else { + watchErrorChannel <- errors.New("unable to convert event object to Project") + break } + } + close(projectChannel) + close(watchErrorChannel) + }() + + select { + case val := <-projectChannel: + klog.V(4).Infof("Deletion information for project: %+v", val) + return nil + case err := <-watchErrorChannel: + return err + case <-time.After(waitForProjectDeletionTimeOut): + return errors.Errorf("waited %s but couldn't delete project %s in time", waitForProjectDeletionTimeOut, name) } + } + + // Return nil since we don't bother checking for the watcher.. + return nil } // GetDeploymentConfigLabelValues get label values of given label from objects in project that are matching selector diff --git a/pkg/odo/cli/project/delete.go b/pkg/odo/cli/project/delete.go index a9d25e7a77b..2a8506d1f09 100644 --- a/pkg/odo/cli/project/delete.go +++ b/pkg/odo/cli/project/delete.go @@ -35,6 +35,10 @@ type ProjectDeleteOptions struct { // generic context options common to all commands *genericclioptions.Context + + // wait is a boolean value to choose if we wait or not for + // our project to be deleted + wait bool } // NewProjectDeleteOptions creates a ProjectDeleteOptions instance @@ -62,6 +66,9 @@ func (pdo *ProjectDeleteOptions) Validate() (err error) { // Run runs the project delete command func (pdo *ProjectDeleteOptions) Run() (err error) { + // Create the "spinner" + s := &log.Status{} + // This to set the project in the file and runtime err = project.SetCurrent(pdo.Context.Client, pdo.projectName) if err != nil { @@ -77,15 +84,23 @@ func (pdo *ProjectDeleteOptions) Run() (err error) { if log.IsJSON() || (pdo.projectForceDeleteFlag || ui.Proceed(fmt.Sprintf("Are you sure you want to delete project %v", pdo.projectName))) { successMessage := fmt.Sprintf("Deleted project : %v", pdo.projectName) - err := project.Delete(pdo.Context.Client, pdo.projectName) + // If the --wait parameter has been passed, we add a spinner.. + if pdo.wait { + s = log.Spinner("Waiting for project to be deleted") + defer s.End(false) + } + + err := project.Delete(pdo.Context.Client, pdo.projectName, pdo.wait) if err != nil { return err } + s.End(true) if log.IsJSON() { project.MachineReadableSuccessOutput(pdo.projectName, successMessage) } else { log.Success(successMessage) + log.Warning("Warning! Projects are deleted from the cluster asynchronously. Odo does its best to delete the project. Due to multi-tenant clusters, the project may still exist on a different node.") } return nil } @@ -109,6 +124,7 @@ func NewCmdProjectDelete(name, fullName string) *cobra.Command { }, } + projectDeleteCmd.Flags().BoolVarP(&o.wait, "wait", "w", false, "Wait until the project has been completely deleted") projectDeleteCmd.Flags().BoolVarP(&o.projectForceDeleteFlag, "force", "f", false, "Delete project without prompting") return projectDeleteCmd diff --git a/pkg/project/project.go b/pkg/project/project.go index a12a740563e..6a0d510f2c1 100644 --- a/pkg/project/project.go +++ b/pkg/project/project.go @@ -4,7 +4,6 @@ import ( "github.com/openshift/odo/pkg/machineoutput" "github.com/pkg/errors" - "github.com/openshift/odo/pkg/log" "github.com/openshift/odo/pkg/occlient" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -45,18 +44,16 @@ func Create(client *occlient.Client, projectName string, wait bool) error { } // Delete deletes the project with name projectName and returns errors if any -func Delete(client *occlient.Client, projectName string) error { - // Loading spinner - s := log.Spinnerf("Deleting project %s", projectName) - defer s.End(false) +func Delete(client *occlient.Client, projectName string, wait bool) error { + if projectName == "" { + return errors.Errorf("no project name given") + } // Delete the requested project - err := client.DeleteProject(projectName) + err := client.DeleteProject(projectName, wait) if err != nil { return errors.Wrap(err, "unable to delete project") } - - s.End(true) return nil } diff --git a/pkg/project/project_test.go b/pkg/project/project_test.go index 895dbc398b2..9116db57b6d 100644 --- a/pkg/project/project_test.go +++ b/pkg/project/project_test.go @@ -6,6 +6,7 @@ import ( "reflect" "testing" + projectv1 "github.com/openshift/api/project/v1" v1 "github.com/openshift/api/project/v1" "github.com/openshift/odo/pkg/occlient" @@ -108,16 +109,19 @@ func TestDelete(t *testing.T) { tests := []struct { name string wantErr bool + wait bool projectName string }{ { - name: "Test project delete for multiple projects", + name: "Case 1: Test project delete for multiple projects", wantErr: false, + wait: false, projectName: "prj2", }, { - name: "Test delete the only remaining project", + name: "Case 2: Test delete the only remaining project", wantErr: false, + wait: false, projectName: "testing", }, } @@ -162,19 +166,22 @@ func TestDelete(t *testing.T) { return true, nil, nil }) - go func() { - fkWatch.Delete(testingutil.FakeProjectStatus(corev1.NamespacePhase(""), tt.projectName)) - }() + // We pass in the fakeProject in order to avoid race conditions with multiple go routines + fakeProject := testingutil.FakeProjectStatus(corev1.NamespacePhase(""), tt.projectName) + go func(project *projectv1.Project) { + fkWatch.Delete(project) + }(fakeProject) + fakeClientSet.ProjClientset.PrependWatchReactor("projects", func(action ktesting.Action) (handled bool, ret watch.Interface, err error) { return true, fkWatch, nil }) // The function we are testing - err := Delete(client, tt.projectName) + err := Delete(client, tt.projectName, tt.wait) if err == nil && !tt.wantErr { - if len(fakeClientSet.ProjClientset.Actions()) != 2 { - t.Errorf("expected 2 ProjClientSet.Actions() in Project Delete, got: %v", len(fakeClientSet.ProjClientset.Actions())) + if len(fakeClientSet.ProjClientset.Actions()) != 1 { + t.Errorf("expected 1 ProjClientSet.Actions() in Project Delete, got: %v", len(fakeClientSet.ProjClientset.Actions())) } } diff --git a/tests/integration/project/cmd_project_test.go b/tests/integration/project/cmd_project_test.go index 8cd650492fb..d438f4400a0 100644 --- a/tests/integration/project/cmd_project_test.go +++ b/tests/integration/project/cmd_project_test.go @@ -1,6 +1,7 @@ package project import ( + "fmt" "os" "path/filepath" "strings" @@ -48,6 +49,41 @@ var _ = Describe("odo project command tests", func() { }) }) + Context("Should be able to delete a project with --wait", func() { + var projectName string + JustBeforeEach(func() { + projectName = helper.RandString(6) + }) + + It("--wait should work with deleting a project", func() { + + // Create the project + helper.CmdShouldPass("odo", "project", "create", projectName) + + // Delete with --wait + output := helper.CmdShouldPass("odo", "project", "delete", projectName, "-f", "--wait") + Expect(output).To(ContainSubstring("Waiting for project to be deleted")) + + }) + + }) + + Context("Delete the project with flag -o json", func() { + var projectName string + JustBeforeEach(func() { + projectName = helper.RandString(6) + }) + + // odo project delete foobar -o json + It("should be able to delete project and show output in json format", func() { + helper.CmdShouldPass("odo", "project", "create", projectName, "-o", "json") + + actual := helper.CmdShouldPass("odo", "project", "delete", projectName, "-o", "json") + desired := fmt.Sprintf(`{"kind":"Project","apiVersion":"odo.dev/v1alpha1","metadata":{"name":"%s","namespace":"%s","creationTimestamp":null},"message":"Deleted project : %s"}`, projectName, projectName, projectName) + Expect(desired).Should(MatchJSON(actual)) + }) + }) + Context("when running project command app parameter in directory that doesn't contain .odo config directory", func() { It("should successfully execute list along with machine readable output", func() {