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

Remove SBR finalizers in the test namespace before deleting the namespace #639

Merged

Conversation

pmacik
Copy link
Contributor

@pmacik pmacik commented Aug 26, 2020

Motivation

Currently the acceptance tests setup deletes the test namespace and creates a new one. But if there are finalizers set on SBRs in that test namespace (for whatever reason) the deletion of the namespace is stuck in Terminating state.

Like in #476

Changes

This PR removes all the finalizers from all the SBRs in the test namespace just before it attempts the namespace deletion.

Testing

make e2e-cleanup

@pmacik
Copy link
Contributor Author

pmacik commented Aug 26, 2020

/retest

@pmacik
Copy link
Contributor Author

pmacik commented Aug 26, 2020

/retest

@Avni-Sharma
Copy link
Contributor

Should #626 be considered in this regard? I think we should give that a look

@pmacik
Copy link
Contributor Author

pmacik commented Aug 27, 2020

/test 4.5-unit

Copy link
Contributor

@pedjak pedjak left a comment

Choose a reason for hiding this comment

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

deploy-clean make rule suffers from the same issue, because SBR CRD cannot be removed if SBR CRs contain finalizers.

Makefile Outdated Show resolved Hide resolved
hack/remove-sbr-finalizers.sh Outdated Show resolved Hide resolved
@pedjak
Copy link
Contributor

pedjak commented Aug 27, 2020

this PR tend to fix the issue that impacts local dev workflow. Hence, @DhritiShikhar @Avni-Sharma @akashshinde @baijum @isutton please test it locally. Right now, the following workflow is impacted and does not work on master:

1 start the operator with make local
2. follow up any of examples
3. stop the operator
4. try to start the operator with make local, the process does not move forward because SBR CRD cannot be removed

but it should work on this PR.

@Avni-Sharma
Copy link
Contributor

Should #626 be considered in this regard? I think we should give that a look

cc @pedjak @pmacik

@DhritiShikhar
Copy link
Contributor

I tried this PR locally.

➜  service-binding-operator git:(remove-finalizer) make test-acceptance VERBOSE=3
Requirement already up-to-date: setuptools in ./out/venv3/lib/python3.8/site-packages (50.0.0)
Requirement already up-to-date: pip in ./out/venv3/lib/python3.8/site-packages (20.2.2)
servicebindingrequest.apps.openshift.io/binding-request-1 patched
servicebindingrequest.apps.openshift.io/binding-request-2 patched
servicebindingrequest.apps.openshift.io/binding-request-a-d-s patched
servicebindingrequest.apps.openshift.io/binding-request-a-s-d patched
servicebindingrequest.apps.openshift.io/binding-request-d-a-s patched
servicebindingrequest.apps.openshift.io/binding-request-empty-app patched
servicebindingrequest.apps.openshift.io/binding-request-knative patched
servicebindingrequest.apps.openshift.io/binding-request-missing-app patched (no change)
namespace "test-namespace-6ec92571" deleted

SBR finalizers are removed.

@pedjak
Copy link
Contributor

pedjak commented Sep 4, 2020

/retest

Copy link
Contributor

@pedjak pedjak left a comment

Choose a reason for hiding this comment

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

#639 (review) is still not addressed

hack/e2e-cleanup.sh Outdated Show resolved Hide resolved
@pmacik
Copy link
Contributor Author

pmacik commented Sep 4, 2020

I've rebased and squashed the commits.

Copy link
Contributor

@pedjak pedjak left a comment

Choose a reason for hiding this comment

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

looks good - with an additional question

@pmacik
Copy link
Contributor Author

pmacik commented Sep 7, 2020

/retest

@Avni-Sharma
Copy link
Contributor

Should #626 be considered in this regard? I think we should give that a look

cc @pedjak @pmacik

Bumping my question up too...icymi :)

@pedjak
Copy link
Contributor

pedjak commented Sep 7, 2020

/approve

@pmacik please squash the commits before we merge it.

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pedjak

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

…espace.

* Remove SBR finalizers also for deploy-clean target.
@pedjak
Copy link
Contributor

pedjak commented Sep 8, 2020

/lgtm

@pmacik pmacik deleted the pre-clean-namespace branch November 27, 2020 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants