-
Notifications
You must be signed in to change notification settings - Fork 244
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.
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
Delete services belonging to an application during 'odo app delete' #2491
Delete services belonging to an application during 'odo app delete' #2491
Conversation
This error might show up when there are permission issue, but it also shows up when Service Catalog is not enabled in the cluster. I tried to delete the app on a minishift cluster that didn't have Service Catalog setup: # Before enabling the Service Catalog
$ odo app delete app -f
This application has following components that will be deleted
component named node
No services / could not get services
✗ serviceinstances.servicecatalog.k8s.io is forbidden: User "developer" cannot list serviceinstances.servicecatalog.k8s.io in the namespace "myproject": no RBAC policy matched
$ odo service list --project myproject
✗ Service catalog is not enabled within your cluster: unable to list services: unable to list ServiceInstances: serviceinstances.servicecatalog.k8s.io is forbidden: User "developer" cannot list serviceinstances.servicecatalog.k8s.io in the namespace "myproject": no RBAC policy matched
# After enabling the Service Catalog
$ odo app delete app -f
This application has following components that will be deleted
component named node
Deleted application: app from project: myproject I remember having a conversation where the reason for not enabling Service Catalog in the tests was to keep the test cluster less beefy. Let me know what you recommend in this situation. @kadel @amitkrout |
we shouldn't fail the deletion of app if the service catalog doesn't exist. |
svcList, err := service.List(client, name) | ||
if err != nil { | ||
// error is returned when there's no Service Catalog enabled in the service | ||
glog.V(4).Infof("Service catalog is not enabled in the cluster, skipping service deletion") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming this based on https://github.com/openshift/odo/blob/974de1fd1285ce8680e7597ba09cf17345e0504d/pkg/odo/cli/service/list.go#L59-L62 in which List
function is one among more functions which are generalized by this log message
Of course we shouldn't. And that's not what were doing. The problem was only in the CI. If you tried in your dev environment with an OpenShift cluster that doesn't have Service Catalog, it wouldn't have failed. It would have just exited silently and successfully! Anyway, I get your point and I have made the changes by referring to other parts of the code base. Most CI checks look happy now. 🕺 |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: girishramnani The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@@ -1398,20 +1398,22 @@ func TestCreateServiceBinding(t *testing.T) { | |||
name string | |||
bindingNS string | |||
bindingName string | |||
labels map[string]string | |||
wantErr bool | |||
}{ | |||
{ | |||
name: "Case: Valid request for creating a secret", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should have some more test case for this unit test like we should have the test case for invalid request
also. What will happen if we pass empty labels
or different binding name
other than foo
. I think we should consider these test case also. It's just a thought, WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at it as you suggested. But we're getting these values from the function that calls CreateServiceBinding
. About the invalid request point, it's valid but CreateServiceBinding
uses function in the OpenShift API to create the Service Binding. So adding a test for it would be futile (assuming the tests already exist in OpenShift source code.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO we should also add some invalid test case to be safe. Depending on Openshift source code tests won't be good idea i guess. :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should have some more test case for this unit test like we should have the test case for
invalid request
also. What will happen if we passempty labels
ordifferent binding name
other thanfoo
. I think we should consider these test case also. It's just a thought, WDYT?
The function we are testing here just calls the openshift api server with the arguments passed to it. The cases that you have mentioned would have been already taken care of by openshift.
It's not about scope but actually who is responsible of the test. once the occlient handed over the data all the validation and functionality come under the scope of openshift. and so does testing those pieces.
Breaks the test at:
|
typo 🙂 @dharmit |
😂 I'm anyway considering taking back the check for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/retest Please review the full test history for this PR and help us cut down flakes. |
/test v4.1-integration-e2e-benchmark |
/hold do-not-merge Adding integration test for this. I was under an impression that we don't have service catalog tests in the CI. |
/retest |
Travis is failing due to: Created dir: /tmp/863945455
Creating a new project: ltwieroyye
Running odo with args [odo project create ltwieroyye -w -v4]
[odo] • Waiting for project to come up ...
[odo] I0109 14:34:54.285469 31887 occlient.go:537] Project ltwieroyye now exists
[odo] ✓ Waiting for project to come up [844ms]
[odo] ✓ Project 'ltwieroyye' is ready for use
[odo] ✓ New project created and now using project: ltwieroyye
[odo] I0109 14:34:54.293428 31887 odo.go:77] Could not get the latest release information in time. Never mind, exiting gracefully :)
Created dir: /tmp/526972802
Created dir: /tmp/454825721
Current working dir: /home/travis/gopath/src/github.com/openshift/odo/tests/integration/servicecatalog
Running odo with args [odo service create --app oahjjh -w dh-postgresql-apb --project ltwieroyye --plan dev -p postgresql_user=luke -p postgresql_password=secret -p postgresql_database=my_data -p postgresql_version=9.6]
[odo] Deploying service dh-postgresql-apb of type: dh-postgresql-apb
[odo] • Deploying service ...
[odo] ✓ Deploying service [34ms]
[odo] • Waiting for service to come up ...
[odo] ✓ Waiting for service to come up [1m]
[odo] ✓ Service 'dh-postgresql-apb' is ready for use
[odo] Optionally, link dh-postgresql-apb to your component by running: 'odo link <component-name>'
Running oc with args [oc get serviceinstance -o name -n ltwieroyye]
[oc] serviceinstance.servicecatalog.k8s.io/dh-postgresql-apb
Running odo with args [odo app delete oahjjh -f]
[odo] ✗ oahjjh app does not exists
Setting current dir to: /home/travis/gopath/src/github.com/openshift/odo/tests/integration/servicecatalog
Deleting project: ltwieroyye
Running odo with args [odo project delete ltwieroyye -f]
[odo] This project contains the following applications, which will be deleted
[odo] Application oahjjh
[odo] This application has following service that will be deleted
[odo] service named dh-postgresql-apb of type dh-postgresql-apb
[odo] • Deleting project ltwieroyye ...
[odo] ✓ Deleting project ltwieroyye [6s]
[odo] ✓ Deleted project : ltwieroyye
Deleting dir: /tmp/863945455
Deleting dir: /tmp/526972802
Deleting dir: /tmp/454825721
• Failure [71.751 seconds]
odo service command tests
/home/travis/gopath/src/github.com/openshift/odo/tests/integration/servicecatalog/cmd_service_test.go:16
When the application is deleted
/home/travis/gopath/src/github.com/openshift/odo/tests/integration/servicecatalog/cmd_service_test.go:421
should delete the service(s) in the application as well [It]
/home/travis/gopath/src/github.com/openshift/odo/tests/integration/servicecatalog/cmd_service_test.go:429
No future change is possible. Bailing out early after 0.505s.
Running odo with args [odo app delete oahjjh -f]
Expected
<int>: 1
to match exit code:
<int>: 0
/home/travis/gopath/src/github.com/openshift/odo/tests/helper/helper_run.go:33 which indicates that the app we're trying to delete doesn't exist on the cluster. However, on my local minishift setup, it fails with: NAME TYPE PLAN STATUS
dh-postgresql-apb dh-postgresql-apb dev Deprovisioning
Running odo with args [odo service list --app ogrssx]
[odo] NAME TYPE PLAN STATUS
[odo] dh-postgresql-apb dh-postgresql-apb dev Deprovisioning
NAME TYPE PLAN STATUS
dh-postgresql-apb dh-postgresql-apb dev Deprovisioning
Running odo with args [odo service list --app ogrssx]
[odo] ✗ There are no services deployed for this application
Setting current dir to: /home/dshah/go/src/github.com/openshift/odo/tests/integration/servicecatalog
Deleting project: yxavlqckrg
Running odo with args [odo project delete yxavlqckrg -f]
[odo] • Deleting project yxavlqckrg ...
[odo] ✓ Deleting project yxavlqckrg [6s]
[odo] ✓ Deleted project : yxavlqckrg
Deleting dir: /tmp/190614664
Deleting dir: /tmp/516509255
Deleting dir: /tmp/798073594
• Failure [116.506 seconds]
odo service command tests
/home/dshah/go/src/github.com/openshift/odo/tests/integration/servicecatalog/cmd_service_test.go:16
When the application is deleted
/home/dshah/go/src/github.com/openshift/odo/tests/integration/servicecatalog/cmd_service_test.go:421
should delete the service(s) in the application as well [It]
/home/dshah/go/src/github.com/openshift/odo/tests/integration/servicecatalog/cmd_service_test.go:429
No future change is possible. Bailing out early after 0.344s.
Running odo with args [odo service list --app ogrssx]
Expected
<int>: 1
to match exit code:
<int>: 0
/home/dshah/go/src/github.com/openshift/odo/tests/helper/helper_run.go:33
------------------------------
Summarizing 1 Failure:
[Fail] odo service command tests When the application is deleted [It] should delete the service(s) in the application as well
/home/dshah/go/src/github.com/openshift/odo/tests/helper/helper_run.go:33
Ran 1 of 22 Specs in 116.506 seconds
FAIL! -- 0 Passed | 1 Failed | 0 Pending | 21 Skipped
--- FAIL: TestServicecatalog (116.51s)
FAIL In the tests I've called @amitkrout can you help me understand what's going wrong here? 😞 |
Never mind. I was wrong in expecting things to work if the underlying command fails. Hopefully the tests will pass this time. 😄 |
@kadel @amitkrout the travis check is failing with same error as it fails for me in my local setup and I don't understand why that's the case or what I'm doing wrong. https://travis-ci.com/github/openshift/odo/jobs/298915517#L733-L909 |
|
||
ocArgs = []string{"get", "serviceinstances"} | ||
helper.WaitForCmdOut("oc", ocArgs, 1, true, func(output string) bool { | ||
return strings.Contains(output, "No resources found.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dharmit can you please print the output
content here? I think No resources found.
might be printed on stderr stream which WaitForCmdout
doesn't check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, let me check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
confirmed
Running oc with args [oc get serviceinstances]
[oc] No resources found.
GIRISH output= # its empty here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
confirming that No resources found.
is showing in the err stream
Travis failure:
4.2 integration failure:
I'm guessing these are flake. /test v4.2-integration-e2e-benchmark |
/hold cancel ready for another review @mik-dass @adisky @cdrage @kanchwala-yusuf |
|
||
ocArgs = []string{"get", "serviceinstances"} | ||
helper.WaitForCmdOut("oc", ocArgs, 1, true, func(output string) bool { | ||
return strings.Contains(output, "Deprovisioning") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deprovisioning
literally seems the process is still going on. Mab be Deprovisioned
would be the best fit for this. But it seems we don't have control over the oc
output. So @dharmit Can you please confirm does the substring Deprovisioning
means all the services link to the application are deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deprovisioning
indeed means that the process of deletion of service is still going on. Once the service is deleted, the output of oc get serviceinstances
becomes something like No resources found...
since there are no service instances in the project. It won't print Deprovisioned
for deleted service.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to see if doing ocArgs = []string{"get", "serviceinstances", "2>&1"}
helps. So far, it doesn't seem to be working on my test setup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, trying to redirect stderr to stdout is not working:
Running oc with args [oc get serviceinstances 2>&1]
[oc] Error from server (NotFound): serviceinstances.servicecatalog.k8s.io "2>&1" not found
------------------------------
Failure [20.698 seconds]
odo service command tests
/home/dshah/go/src/github.com/openshift/odo/tests/integration/servicecatalog/cmd_service_test.go:15
When the application is deleted
/home/dshah/go/src/github.com/openshift/odo/tests/integration/servicecatalog/cmd_service_test.go:439
should delete the service(s) in the application as well [It]
/home/dshah/go/src/github.com/openshift/odo/tests/integration/servicecatalog/cmd_service_test.go:444
No future change is possible. Bailing out early after 0.111s.
Running oc with args [oc get serviceinstances 2>&1]
Expected
<int>: 1
to match exit code:
<int>: 0
/home/dshah/go/src/github.com/openshift/odo/tests/helper/helper_run.go:34
Doesn't work when I use odo
command either:
[odo] ✗ unknown command "2>&1" for "odo service list"
------------------------------
• Failure [19.730 seconds]
odo service command tests
/home/dshah/go/src/github.com/openshift/odo/tests/integration/servicecatalog/cmd_service_test.go:15
When the application is deleted
/home/dshah/go/src/github.com/openshift/odo/tests/integration/servicecatalog/cmd_service_test.go:439
should delete the service(s) in the application as well [It]
/home/dshah/go/src/github.com/openshift/odo/tests/integration/servicecatalog/cmd_service_test.go:444
No future change is possible. Bailing out early after 0.040s.
Running odo with args [odo service list --app kushnf 2>&1]
Expected
<int>: 1
to match exit code:
<int>: 0
/home/dshah/go/src/github.com/openshift/odo/tests/helper/helper_run.go:34
Looks like using shell way of redirecting stderr to stdout doesn't work well from Go code. 😞
one quick fix here would be to augment the
so now the WaitForCmdOut would be like
I know this might not be the most documentation friendly approach but soon we need to redo the helpers ( all of them ) to something nice which covers all our cases and are clean to use. Until that day this looks ok. |
If there are no components in the application but the only some service then `odo app delete <app-name>` won't show the list of services that will be deleted as a result of deleting the application. This commit fixes that behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested, works, and code LGTM! :)
/lgtm
svcList, err := service.List(client, name) | ||
if err != nil { | ||
// error is returned when there's no Service Catalog enabled in the service | ||
glog.V(4).Infof("Service catalog is not enabled in the cluster, skipping service deletion") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick, it's Service Catalog
|
||
helper.CmdShouldPass("odo", "app", "delete", app, "-f") | ||
|
||
ocArgs = []string{"get", "serviceinstances"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM!
/retest Please review the full test history for this PR and help us cut down flakes. |
6 similar comments
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
What type of PR is this?
/kind bug
What does does this PR do / why we need it:
This PR deletes the service (ServiceInstance) created from Service Catalog when user tries to delete an application (
odo app delete <app-name>
).When there are no components in the application but only service(s), it also prints the list of services that would be deleted when doing
odo app delete <app-name>
. This list of services was not getting printed unless there was a component in<app-name>
marked for deletion.Which issue(s) this PR fixes:
Fixes #2113
How to test changes / Special notes to the reviewer:
odo service create
odo service list --app app
odo app delete app -f