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.

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

Only delete service from devfile #4761

Merged

Conversation

feloy
Copy link
Contributor

@feloy feloy commented May 31, 2021

What type of PR is this?

/kind feature

What does this PR do / why we need it:

  • odo service delete removes a service from the Devfile only, it does not undeploy it from the cluster.
    • Operator services
    • [n/c] Catalog services
  • odo service list lists services in Devfile and/or deployed into the cluster
    • Operator services
    • [n/c] Catalog services
  • odo push undeploys services managed by the current devfile not present in this devfile
    • Operator services
    • [n/c] Catalog services

Which issue(s) this PR fixes:

Fixes #4160

PR acceptance criteria:

  • Unit test

  • Integration test

  • Documentation

  • Update changelog

  • I have read the test guidelines

How to test changes / Special notes to the reviewer:

From a cluster with etcdoperator installed:

$ odo create nodejs
$ odo push
$ odo service create etcdoperator.v0.9.4-clusterwide/EtcdCluster myetcd

# at this point, the service should be declared in the devfile, but not deployed

$ odo service list
NAME                  MANAGED BY ODO               STATE        AGE
EtcdCluster/myetcd   Yes (nodejs-prj1-api-ixhx)   Not pushed   

$ odo push

# the service should be deployed

$ odo service list
NAME                  MANAGED BY ODO               STATE     AGE
EtcdCluster/myetcd   Yes (nodejs-prj1-api-ixhx)   Pushed    14s

$ odo service delete EtcdCluster/myetcd

# at this point, the service should be removed from the devfile, but not undeployed

$ odo service list
NAME                  MANAGED BY ODO               STATE             AGE
EtcdCluster/myetcd   Yes (nodejs-prj1-api-ixhx)   Deleted locally   1m7s

$ odo push

# the service should be undeployed

$ odo service list
 ✗  no operator backed services found in namespace: prj1

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label May 31, 2021
@openshift-ci openshift-ci bot requested review from dharmit and mik-dass May 31, 2021 13:36
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Jun 1, 2021
@feloy feloy force-pushed the feature-4160/service-delete branch 2 times, most recently from 248fb70 to 09ca0a7 Compare June 2, 2021 13:33
@openshift-ci openshift-ci bot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Jun 3, 2021
@feloy
Copy link
Contributor Author

feloy commented Jun 3, 2021

/test psi-kubernetes-integration-e2e

2 similar comments
@feloy
Copy link
Contributor Author

feloy commented Jun 4, 2021

/test psi-kubernetes-integration-e2e

@feloy
Copy link
Contributor Author

feloy commented Jun 4, 2021

/test psi-kubernetes-integration-e2e

@feloy feloy force-pushed the feature-4160/service-delete branch from 90a6ea9 to 13be133 Compare June 4, 2021 06:52
@feloy
Copy link
Contributor Author

feloy commented Jun 4, 2021

/test v4.7-integration-e2e

@feloy
Copy link
Contributor Author

feloy commented Jun 4, 2021

/test psi-kubernetes-integration-e2e

4 similar comments
@feloy
Copy link
Contributor Author

feloy commented Jun 4, 2021

/test psi-kubernetes-integration-e2e

@feloy
Copy link
Contributor Author

feloy commented Jun 4, 2021

/test psi-kubernetes-integration-e2e

@feloy
Copy link
Contributor Author

feloy commented Jun 4, 2021

/test psi-kubernetes-integration-e2e

@feloy
Copy link
Contributor Author

feloy commented Jun 4, 2021

/test psi-kubernetes-integration-e2e

@feloy feloy force-pushed the feature-4160/service-delete branch from 85d741c to b31b52b Compare June 4, 2021 14:56
@feloy
Copy link
Contributor Author

feloy commented Jun 7, 2021

/test psi-kubernetes-integration-e2e

@feloy feloy closed this Jun 7, 2021
@feloy feloy reopened this Jun 7, 2021
@feloy
Copy link
Contributor Author

feloy commented Jun 7, 2021

I can see a difference in the content of the label app.kubernetes.io/instance for Service catalog service and Operator backed service.

For an Operator backed service, this is the name of the component from which the service is deployed:

apiVersion: etcd.database.coreos.com/v1beta2
kind: EtcdCluster
metadata:
  annotations:
    etcd.database.coreos.com/scope: clusterwide
  creationTimestamp: "2021-06-07T09:36:33Z"
  generation: 5
  labels:
    app: app
    app.kubernetes.io/instance: nodejs-prj1-api-xaeo
    app.kubernetes.io/managed-by: odo
    app.kubernetes.io/managed-by-version: v2.2.1
    app.kubernetes.io/part-of: app
  name: myetcd

For a service catalog service instance, this is the name of the service itself:

apiVersion: servicecatalog.k8s.io/v1beta1
kind: ServiceInstance
metadata:
  creationTimestamp: "2021-06-07T09:50:03Z"
  finalizers:
  - kubernetes-incubator/service-catalog
  generation: 1
  labels:
    app: app
    app.kubernetes.io/instance: my-dh-postgresql-apb
    app.kubernetes.io/managed-by: odo
    app.kubernetes.io/managed-by-version: v2.2.1
    app.kubernetes.io/name: dh-postgresql-apb
    app.kubernetes.io/part-of: app
  name: my-dh-postgresql-apb

@feloy
Copy link
Contributor Author

feloy commented Jun 8, 2021

/test unit

@feloy feloy force-pushed the feature-4160/service-delete branch from a0e35b8 to f8761c0 Compare June 8, 2021 07:28
@feloy
Copy link
Contributor Author

feloy commented Jun 8, 2021

/test psi-unit-test-windows
/test v4.7-integration-e2e

@@ -44,53 +44,13 @@ $ kubectl create -f https://operatorhub.io/install/postgresql-operator-dev4devs-

**NOTE**: The `my-postgresql-operator-dev4devs-com` Operator will be installed in the `my-postgresql-operator-dev4devs-com` namespace and will be usable from this namespace only.

==== Creating a database to be used by the sample application
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved this section after odo create as it is now (from a previous PR) necessary to have a component before to create a service.

@feloy feloy force-pushed the feature-4160/service-delete branch from cdbd0b9 to 70c07c8 Compare June 9, 2021 09:20
@feloy
Copy link
Contributor Author

feloy commented Jun 9, 2021

/test psi-kubernetes-integration-e2e

@feloy feloy force-pushed the feature-4160/service-delete branch from 709782a to c41f8d5 Compare June 9, 2021 14:16
@feloy
Copy link
Contributor Author

feloy commented Jun 9, 2021

/retest

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Jun 15, 2021
@feloy feloy force-pushed the feature-4160/service-delete branch from fb5b38c to bcb021e Compare June 15, 2021 10:36
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Jun 15, 2021
@feloy feloy force-pushed the feature-4160/service-delete branch from 71300f4 to ca5cd36 Compare June 15, 2021 10:41
Copy link
Contributor

@mik-dass mik-dass left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Jun 15, 2021
@openshift-ci
Copy link

openshift-ci bot commented Jun 15, 2021

@feloy: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/psi-unit-test-mac ca5cd36 link /test psi-unit-test-mac
ci/prow/psi-unit-test-windows ca5cd36 link /test psi-unit-test-windows

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@feloy
Copy link
Contributor Author

feloy commented Jun 15, 2021

/test v4.7-integration-e2e

@dharmit
Copy link
Member

dharmit commented Jun 15, 2021

Timeout while spinning up the cluster - link to logs:

level=error
level=error msg=Error: timeout while waiting for state to become 'INSYNC' (timeout: 30m0s)
level=error
level=error msg=  on ../tmp/openshift-install-772034745/route53/base.tf line 56, in resource "aws_route53_record" "api_internal_alias":
level=error msg=  56: resource "aws_route53_record" "api_internal_alias" {
level=error
level=error
level=fatal msg=failed to fetch Cluster: failed to generate asset "Cluster": failed to create cluster: failed to apply Terraform: failed to complete the change 

@openshift-merge-robot openshift-merge-robot merged commit 9188588 into redhat-developer:main Jun 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. lgtm Indicates that a PR is ready to be merged. Required by Prow.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

odo service create should add operator CR to the devfile
4 participants