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

change Lister to DynamicClient for subscription clean up in Teardown #2616

Conversation

akihikokuroda
Copy link
Member

Signed-off-by: akihikokuroda akihikokuroda2020@gmail.com

Description of the change:
This line

return client.DeleteAllOf(clientCtx, &operatorsv1alpha1.Subscription{}, inNamespace)
in the Teardown should delete all subscriptions in the namespace but it sometime doesn't (I don't know why so far, I 'll investigate, later).

This line

err = client.List(clientCtx, list, inNamespace)
is used to wait until the all subscriptions are deleted but it always gets no instance because the DeleteAllOf function deletes the subscriptions in the cache and the LIst function gets the instance from the cache.

This PR changes the way to get the subscriptions from the API server directly instead of the cache.

Motivation for the change:
Closes #2615

Reviewer Checklist

  • Implementation matches the proposed design, or proposal is updated to match implementation
  • Sufficient unit test coverage
  • Sufficient end-to-end test coverage
  • Docs updated or added to /doc
  • Commit messages sensible and descriptive

@openshift-ci openshift-ci bot requested review from awgreene and exdx February 4, 2022 14:36
test/e2e/util.go Outdated
if list != nil {
remaining = list.Items
Eventually(func() ([]unstructured.Unstructured, error) {
var subscriptiongvr = schema.GroupVersionResource{Group: "operators.coreos.com", Version: "v1alpha1", Resource: "subscriptions"}
Copy link
Member

Choose a reason for hiding this comment

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

nit: could we move this assignment out of the Eventually() into the var block above since it only needs to be done once (and is not part of the core polling logic)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. I'll move it. Thanks!

remaining = list.Items
Eventually(func() ([]unstructured.Unstructured, error) {
var subscriptiongvr = schema.GroupVersionResource{Group: "operators.coreos.com", Version: "v1alpha1", Resource: "subscriptions"}
list, err := dynamic.Resource(subscriptiongvr).Namespace(namespace).List(context.Background(), metav1.ListOptions{})
Copy link
Member

Choose a reason for hiding this comment

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

I think this logic makes sense, moving the call from the cache to the api-server. Maybe if this works well we can expand the change to the other types in TearDown() as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good to me. Thanks!

@akihikokuroda akihikokuroda force-pushed the subscriptioncleanup branch 2 times, most recently from 0baceb8 to 4a0ccde Compare February 4, 2022 18:09
Signed-off-by: akihikokuroda <akihikokuroda2020@gmail.com>
@exdx
Copy link
Member

exdx commented Feb 4, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 4, 2022
@openshift-ci
Copy link

openshift-ci bot commented Feb 4, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akihikokuroda, timflannagan

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 4, 2022
@openshift-merge-robot openshift-merge-robot merged commit aa20d1c into operator-framework:master Feb 4, 2022
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. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

e2e - subscription is left after tests intermittently
4 participants