Specifying a name does not matter when using odo delete #3892

cdrage · 2020-09-03T16:02:21Z

/kind bug

What versions of software are you using?

Operating System: Linux

Output of odo version: master 👍

How did you run odo exactly?

~/openshift/foobar                                                                                                                                                                                                                                                                                                                                                         ⍉
▶ odo list
Experimental mode is enabled, use at your own risk
Devfile Components: 
APP     NAME                PROJECT     TYPE                STATE
app     java-springboot     foo         java-springboot     Pushed
app     nodejs              foo         nodejs              Pushed
~/openshift/foobar                                                                                                                                                                                                                                                                                                                                                          
▶ odo delete kalsdjalksdjlakdjlasljkd -f
Gathering information for component nodejs
 ✓  Checking status for component [47ms]
Deleting component nodejs
 ✓  Deleting Kubernetes resources for component [46ms]
 ✓  Successfully deleted component
~/openshift/foobar

Actual behavior

It does not matter what you specify to odo delete it will delete whatever is in your current folder...

Expected behavior

To fail when passing in something random such as odo delete alksjdalksjdklasd -f

Any logs, error output, etc?

See above

The text was updated successfully, but these errors were encountered:

johnmcollier · 2020-09-03T19:24:07Z

This one's gonna be a little messy to fix 😕 - looking over the delete code, there's lots of assumptions that we have access to both the devfile and envinfo file when deleting a component

maysunfaisal · 2020-09-11T18:24:21Z

@johnmcollier whats wrong with that? if devfile context is always defined by the current dir or --context flag

Maysuns-MacBook-Pro:springboot-ex maysun$ odo create java-springboot myspr
Validation
 ✓  Checking devfile existence [35097ns]
 ✓  Checking devfile compatibility [394465ns]
 ✓  Creating a devfile component from registry: DefaultDevfileRegistry [386173ns]
 ✓  Validating devfile component [233052ns]

Please use `odo push` command to create the component with source deployed
Maysuns-MacBook-Pro:springboot-ex maysun$ 
Maysuns-MacBook-Pro:springboot-ex maysun$ 
Maysuns-MacBook-Pro:springboot-ex maysun$ odo delete
? Are you sure you want to delete the devfile component: myspr? (y/N) 
Maysuns-MacBook-Pro:springboot-ex maysun$ cd ..
Maysuns-MacBook-Pro:resources maysun$ odo delete --context ./springboot-ex/
? Are you sure you want to delete the devfile component: myspr? (y/N)

johnmcollier · 2020-09-11T18:28:39Z

@maysunfaisal It's moreso that if we want to be able to delete a devfile component by name, we need to be able to delete it without being in / pointing to that component's context folder.

E.g. odo delete mycomp should delete mycomp without requiring that we point to the folder for the mycomp component

maysunfaisal · 2020-09-11T18:35:14Z

But

isnt that the point of having the --context flag? so that we can delete from anywhere using this flag
all odo commands require you to point to the component dir, so why should delete be any different?
I just tried your example for s2i, I cant do that from any dir. It requires a component in the current dir or requires the --context flag:

Maysuns-MacBook-Pro:resources maysun$ odo delete mys2ijava --s2i
 ✗  The current directory does not represent an odo component. Use 'odo create' to create component here or switch to directory with a component

Maysuns-MacBook-Pro:resources maysun$ odo delete mys2ijava --context ./springboot-ex/ --s2i
 ✗  Component mys2ijava does not exist on the cluster

There can be only one component defined in an odo config be it, config.yaml or env.yaml, so why do we need to specify a name if i cant just do odo delete <mycomp> from anywhere? 🤔

Maysuns-MacBook-Pro:springboot-ex maysun$ cat .odo/config.yaml 
kind: LocalConfig
apiversion: odo.dev/v1alpha1
ComponentSettings:
  Type: java
  SourceLocation: ./
  SourceType: local
  Ports:
  - 8080/TCP
  - 8443/TCP
  - 8778/TCP
  Application: app
  Project: mjf
  Name: mys2ijava

johnmcollier · 2020-09-11T19:16:18Z

Yes and no. But if you've got multiple components deployed on a cluster, across multiple apps (once odo app support for devfile components #3402 is in), having to find the context folder for the app you want to delete can be a real nuisance.
odo delete's help text does allow you to specify a name for a component:

Johns-MacBook-Pro-3:odo johncollier$ ./odo delete --help
Delete component.

Usage:
  odo delete <component_name> [flags]

Examples:
  # Delete component named 'frontend'.
  odo delete frontend
  odo delete frontend --all

So, I just tried on master, and had no issues deleting a s2i component from a non-component directory. I just had to specify --app on odo delete if not inside a component directory:

Johns-MacBook-Pro-3:nodejs-ex johncollier$ odo create nodejs --s2i
Validation
 ✓  Validating component [250ms]

Please use `odo push` command to create the component with source deployed
Johns-MacBook-Pro-3:nodejs-ex johncollier$ odo push
Validation
 ✓  Checking component [388ms]

Configuration changes
 ✓  Initializing component
 ✓  Creating component [2s]

Applying URL changes
 ✓  URLs are synced with the cluster, no changes are required.

Pushing to component nodejs-nodejs-ex-ibmz of type local
 ✓  Checking files for pushing [1ms]
 ✓  Waiting for component to start [49s]
 ✓  Syncing files to the component [3s]
 ✓  Building component [22s]
 ✓  Changes successfully pushed to component
Johns-MacBook-Pro-3:nodejs-ex johncollier$ cd ~/
Johns-MacBook-Pro-3:~ johncollier$ mkdir testdir
Johns-MacBook-Pro-3:~ johncollier$ cd testdir
Johns-MacBook-Pro-3:testdir johncollier$ odo list --app app
Openshift Components: 
APP     NAME                      PROJECT     TYPE       SOURCETYPE     STATE
app     nodejs-nodejs-ex-ibmz     default     nodejs     local          Pushed
Johns-MacBook-Pro-3:testdir johncollier$ odo delete nodejs-nodejs-ex-ibmz --app app
? Are you sure you want to delete nodejs-nodejs-ex-ibmz from app? Yes
 ✓  Deleting component nodejs-nodejs-ex-ibmz [404ms]
 ✓  Component nodejs-nodejs-ex-ibmz from application app has been deleted

It may not make much sense without support for odo app with devfiles in, but being able to do odo delete <comp> --app <app> is a whole lot nicer than having to find the context folder for each component.

maysunfaisal · 2020-09-11T19:44:57Z

@johnmcollier ok i see your point. Yeah you needed the -app flag. However, how is odo delete different from any other commands. Shouldn't other odo component commands also have this benefit?

johnmcollier · 2020-09-11T19:48:54Z

@maysunfaisal That's a good question. Since we don't need to point to the context / component folder for odo delete (everything it does is server-side), it probably wasn't made required. Since most of the other odo commands do require access to the component folder (like odo url create), that's why it's not really consistent.

elsony · 2020-09-11T19:53:56Z

Are we going to have problems on supporting odo delete with --all on a random directory given that we don't have access to the component folder to clean up the files, e.g. .odo/env/env.yaml

johnmcollier · 2020-09-11T20:00:12Z

@elsony I went and checked to see what odo delete --all did for s2i when outside of the component. I thought it would throw an error, but it surprisingly didn't:

Johns-MacBook-Pro-3:testdir johncollier$ odo delete nodejs-nodejs-ex-ibmz --app app --all
? Are you sure you want to delete nodejs-nodejs-ex-ibmz from app? Yes
 ✓  Deleting component nodejs-nodejs-ex-ibmz [546ms]
 ✓  Component nodejs-nodejs-ex-ibmz from application app has been deleted
? Are you sure you want to delete local config for nodejs-nodejs-ex-ibmz? Yes
 ✓  Config for the Component nodejs-nodejs-ex-ibmz has been deleted

However, the config files were not deleted so it really just silently failed.

I think if there isn't a way to get --all to work with odo delete <name> --app <app>, then we should throw a validation error when --all is used with a component name, outside of the component directory (i.e. odo delete <component> --app app --all in some random folder)

elsony · 2020-09-11T20:09:07Z

I have the same concern as @maysunfaisal to make odo delete behave differently than other commands. All other commands needs to either set the context or run under the component directory. If we block the --all, we are yet introducing another inconsistent behaviour that --all only support when you are running under the component directory.

For the --app, we may be still be able to get that to work if we keep track on the component folders in the metadata. If we happens to introduce a single odo push command with -app in the future to push all components under a given app, we may need that information anyway. But it is a bigger design discussion to make if we want to store the location.

Is the only reason why we wanted to make it to work is to get a consistent behaviour as the s2i scenario? We have got other cases where the command parameters, etc. are different between devfile and s2i support. Given that devfile support is our future, I'd rather make it clear on the command help/behaviour on what work on s2i vs devfile than introducing inconsistent behaviour within the devfile support.

johnmcollier · 2020-09-11T20:15:16Z

So I have two reasons for wanting to be able to delete a component by name, and not relying solely on the component folder:

If I've got multiple components in multiple apps deployed on a cluster, and I'm not sure which folder belongs to the component folder I have to delete, I don't have to find the component folder first to delete the component. I can just do odo delete <comp> --app <app> and I'm good to go.
Similar to Use oc binary instead client-go for initial implementation #1, if I'm running odo on another system (pointing to the same cluster), or I've lost the folder belonging to a component I want to delete, odo delete <component> --app app will work anywhere. Without this, I would have to run kubectl delete ... to manually clean up all of the resources.

There's already some commands that don't require the component folder to work, such as odo describe, so odo delete wouldn't be the odd command out:

Johns-MacBook-Pro-3:testdir johncollier$ odo describe nodejs-nodejs-ex-ibmz --app app
Component Name: nodejs-nodejs-ex-ibmz
Type: nodejs
Environment Variables:
 · DEBUG_PORT=5858

elsony · 2020-09-11T20:34:53Z

If I've got multiple components in multiple apps deployed on a cluster, and I'm not sure which folder belongs to the component folder I have to delete, I don't have to find the component folder first to delete the component. I can just do odo delete <comp> --app <app> and I'm good to go.

Similar to Use oc binary instead client-go for initial implementation #1, if I'm running odo on another system (pointing to the same cluster), or I've lost the folder belonging to a component I want to delete, odo delete <component> --app app will work anywhere. Without this, I would have to run kubectl delete ... to manually clean up all of the resources.

#1 I don't think multiple app/component makes a difference. A single component has the same problem as the multiple case.

ok, I can see user case for #2 where I lost my original folder and wanted to delete the component. I am fine to support that for devfile type components. However, the described scenario is restricted to --app which I don't think we should have that limitation. We should implement it in a way that we don't require to have --app to be specified and still allow user to delete a given component by specifying the component name only. --app should be optional.

johnmcollier · 2020-09-11T20:38:34Z

Sorry, for #1 let me clarify - I meant scenarios where we may have multiple components on your machine (and even more if there's multiple components in multiple apps), and each component has its own folder. So being able to delete the component without having to find the component folder would save time / avoid a nuisance for the user.

But I agree, the better use case is actually #2 where the folder is lost or not accessible. I'm surprised actually that --app is required, since I believe the past functionality with odo was to use the default app if one isn't specified, so yup, --app should be optional.

**What type of PR is this?** > Uncomment only one ` /kind` line, and delete the rest. > For example, `> /kind bug` would simply become: `/kind bug` /kind bug **What does does this PR do / why we need it**: This is a temporary fix by removing the current feature of being able to pass in a component name to `odo delete`. For example. `odo delete foobar` does not work. We remove that functionality so you may only specify `odo delete` in the current directory. **Which issue(s) this PR fixes**: Does not fix any issues, but please see: redhat-developer#3892 **PR acceptance criteria**: - [X] Unit test - [X] Integration test - [X] Documentation - [X] I have read the [test guidelines](https://github.com/openshift/odo/blob/master/docs/dev/test-architecture.adoc) **How to test changes / Special notes to the reviewer**: ```sh odo create nodejs --starter odo push odo delete ``` Signed-off-by: Charlie Drage <charlie@charliedrage.com>

cdrage · 2020-10-07T18:24:14Z

So need some insight on this...

When deploying a nodejs example (via odo create nodejs --starter && odo push).

odo list works well, and lists all components.

The issue is running these two commands OUTSIDE of the directory.

odo describe nodejs does not WORK. But odo describe nodejs --app app does
odo delete does not work regardless of --app passing in or --project.

My thoughts:

The user runs odo delete nodejs outside of the directory
No --project has been supplied, so we use the default namespace that's currently being used by Kubernetes context
No --app has been supplied, so we iterate through each "app" in the namespace until the component is found, and use the first one that's found.

One problem:

What if there are multiple components of the same name in two different app's?

My solution:

Disclaimer to the user that it will delete component X from app X that it has found.
If there are multiple components found in different app's, we output to the user that component X was found in app X and Y and to choose one by passing in --app.

What do you guys think @johnmcollier @elsony

elsony · 2020-10-07T21:50:10Z

No --project has been supplied, so we use the default namespace that's currently being used by Kubernetes context

sounds good

No --app has been supplied, so we iterate through each "app" in the namespace until the component is found, and use the first one that's found.

Along the line of your multiple hit with same name question, I think we should iterate through all of them to make sure there is one and only one match before we execute the deletion.

When multiple components with the same name under different apps are found, we need to either prompt them to choose or give them the list of matched components with the app name and ask them to specify --app on the delete command. We should avoid deleting the first found since we may ending up deleting a component that the user wants to keep.

cdrage · 2020-10-08T13:04:28Z

No --project has been supplied, so we use the default namespace that's currently being used by Kubernetes context

sounds good

No --app has been supplied, so we iterate through each "app" in the namespace until the component is found, and use the first one that's found.

Along the line of your multiple hit with same name question, I think we should iterate through all of them to make sure there is one and only one match before we execute the deletion.

When multiple components with the same name under different apps are found, we need to either prompt them to choose or give them the list of matched components with the app name and ask them to specify --app on the delete command. We should avoid deleting the first found since we may ending up deleting a component that the user wants to keep.

Sounds good! Glad we're on the same page. I'll start on the implementation.

cdrage · 2020-10-09T19:38:30Z

Worked the past two days on this implementation and unfortunately there's an another issue I face.

Specifically, it's PreStop events as per:

https://github.com/openshift/odo/blob/master/pkg/devfile/adapters/kubernetes/component/adapter.go#L517

Specifically:

	preStopEvents := a.Devfile.Data.GetEvents().PreStop
	if len(preStopEvents) > 0 {
		if pod.Status.Phase != corev1.PodRunning {
			return fmt.Errorf("unable to execute preStop events, pod for component %s is not running", a.ComponentName)
		}

		err = a.ExecDevfileEvent(preStopEvents, common.PreStop, show)
		if err != nil {
			return err
		}
	}

So when using odo delete out of a directory containing PreStop, it will be unable to execute whatever "PreStop" command it is, since it will be unable to access the original devfile.yaml.

Should we just warn / skip PreStop? @johnmcollier @elsony

mohammedzee1000 · 2020-11-05T12:58:41Z

Ok I have kind of got the gist of what @cdrage was trying to do. I have now pulled pr into my own branch and will push it as a WIP shortly

dharmit · 2020-12-04T04:59:20Z

Ok I have kind of got the gist of what @cdrage was trying to do. I have now pulled pr into my own branch and will push it as a WIP shortly

How is this coming along?

openshift-bot · 2021-03-09T20:04:43Z

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

openshift-bot · 2021-04-08T21:58:38Z

Stale issues rot after 30d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle rotten
/remove-lifecycle stale

openshift-bot · 2021-05-09T01:01:57Z

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

openshift-ci · 2021-05-09T01:02:11Z

@openshift-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Sep 3, 2020

cdrage added the priority/High Important issue; should be worked on before any other issues (except priority/Critical issue(s)). label Sep 3, 2020

elsony added the area/devfile-spec Issues or PRs related to the Devfile specification and how odo handles and interprets it. label Sep 3, 2020

girishramnani assigned cdrage Sep 14, 2020

cdrage mentioned this issue Sep 21, 2020

Remove specifying a component when deleting #3998

Closed

4 tasks

cdrage mentioned this issue Oct 28, 2020

WIP Add being able to delete outside of the devfile dir #4170

Closed

4 tasks

girishramnani assigned mohammedzee1000 and unassigned cdrage Oct 29, 2020

dharmit unassigned mohammedzee1000 Dec 9, 2020

dharmit self-assigned this Dec 9, 2020

openshift-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 9, 2021

openshift-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Apr 8, 2021

openshift-ci bot closed this as completed May 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Specifying a name does not matter when using odo delete #3892

Specifying a name does not matter when using odo delete #3892

cdrage commented Sep 3, 2020

johnmcollier commented Sep 3, 2020

maysunfaisal commented Sep 11, 2020 •

edited

Loading

johnmcollier commented Sep 11, 2020 •

edited

Loading

maysunfaisal commented Sep 11, 2020 •

edited

Loading

johnmcollier commented Sep 11, 2020 •

edited

Loading

maysunfaisal commented Sep 11, 2020 •

edited

Loading

johnmcollier commented Sep 11, 2020

elsony commented Sep 11, 2020

johnmcollier commented Sep 11, 2020 •

edited

Loading

elsony commented Sep 11, 2020

johnmcollier commented Sep 11, 2020 •

edited

Loading

elsony commented Sep 11, 2020 •

edited

Loading

johnmcollier commented Sep 11, 2020 •

edited

Loading

cdrage commented Oct 7, 2020

elsony commented Oct 7, 2020 •

edited

Loading

cdrage commented Oct 8, 2020

cdrage commented Oct 9, 2020

mohammedzee1000 commented Nov 5, 2020

dharmit commented Dec 4, 2020

openshift-bot commented Mar 9, 2021

openshift-bot commented Apr 8, 2021

openshift-bot commented May 9, 2021

openshift-ci bot commented May 9, 2021

Specifying a name does not matter when using odo delete #3892

Specifying a name does not matter when using odo delete #3892

Comments

cdrage commented Sep 3, 2020

What versions of software are you using?

How did you run odo exactly?

Actual behavior

Expected behavior

Any logs, error output, etc?

johnmcollier commented Sep 3, 2020

maysunfaisal commented Sep 11, 2020 • edited Loading

johnmcollier commented Sep 11, 2020 • edited Loading

maysunfaisal commented Sep 11, 2020 • edited Loading

johnmcollier commented Sep 11, 2020 • edited Loading

maysunfaisal commented Sep 11, 2020 • edited Loading

johnmcollier commented Sep 11, 2020

elsony commented Sep 11, 2020

johnmcollier commented Sep 11, 2020 • edited Loading

elsony commented Sep 11, 2020

johnmcollier commented Sep 11, 2020 • edited Loading

elsony commented Sep 11, 2020 • edited Loading

johnmcollier commented Sep 11, 2020 • edited Loading

cdrage commented Oct 7, 2020

elsony commented Oct 7, 2020 • edited Loading

cdrage commented Oct 8, 2020

cdrage commented Oct 9, 2020

mohammedzee1000 commented Nov 5, 2020

dharmit commented Dec 4, 2020

openshift-bot commented Mar 9, 2021

openshift-bot commented Apr 8, 2021

openshift-bot commented May 9, 2021

openshift-ci bot commented May 9, 2021

maysunfaisal commented Sep 11, 2020 •

edited

Loading

johnmcollier commented Sep 11, 2020 •

edited

Loading

maysunfaisal commented Sep 11, 2020 •

edited

Loading

johnmcollier commented Sep 11, 2020 •

edited

Loading

maysunfaisal commented Sep 11, 2020 •

edited

Loading

johnmcollier commented Sep 11, 2020 •

edited

Loading

johnmcollier commented Sep 11, 2020 •

edited

Loading

elsony commented Sep 11, 2020 •

edited

Loading

johnmcollier commented Sep 11, 2020 •

edited

Loading

elsony commented Oct 7, 2020 •

edited

Loading