Skip to content

Mgdapi 3472 3 #723

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.

Sign up for GitHub

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

Jump to bottom
Merged
MStokluska merged 11 commits into from
Mar 23, 2022
Merged

Mgdapi 3472 3 #723

MStokluska merged 11 commits into from
Mar 23, 2022

Conversation

@MStokluska
Copy link
Contributor

@MStokluska MStokluska commented Mar 3, 2022 •
edited by eguzki
Loading

  • Enables support of backend && product deletion (product deletion done by Patryk Stefanski)
  • Enables cascading deletion of product and backend CRs when a tenant CR is deleted

Jira issues:

eguzki
eguzki requested changes Mar 7, 2022
View reviewed changes
Copy link
Member

@eguzki eguzki left a comment

Choose a reason for hiding this comment

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

The removeBackend is the key here.

What I would do is to list all the products (in the same namespace as the backend) and remove the current backend from the spec.BackendUsage map where it exists.

I find quite confusing the current implementation. Some clarification would help. Comparing to how I would implement, what is the benefit from this implementation?

I would never update the remote 3scale product entity. The 3scale will do when deleting the backend.

If the 3scale API call to delete the backend fails, the operator should create a openshift event for visibility.

The backend usages is not the only reference from the product to the backend being deleted. Application plan limits and pricing rules also have references to backends. Check out my description in the issue. You are here implementing option a

@MStokluska MStokluska force-pushed the MGDAPI-3472-3 branch 2 times, most recently from 3e57de6 to a1c9f5c Compare March 9, 2022 08:42
@MStokluska
Copy link
Contributor Author

MStokluska commented Mar 9, 2022 •
edited
Loading

Hey @eguzki
So following our conversation here's the list of updates I did:

  1. Imports should be in correct order now
  2. Product reconciler - removed the ownerReference bits - it will be done in another PR
  3. Backend reconciler:
  • removed the ownerRef logic - will be done in another PR
  • removed calls to get the products from 3scale
  • fetching product CRs now works as follows:
    a) we first get all the product CRs from the ns where the backend is, as proven during our conversation, they will have to be under the same ns
    b) we then loop through products CR to find only the ones that match the providerAcc URL with backend providerAcc URL
    c) return only the ones that have the backendUsage mentioned
  • update productCR (removal of backendUsage)
  • wait for product reconciler to update product in 3scale -this means error out on the initial backend deletion run
  • come back to backend reconciler to remove the backend from 3scale

The issue here however, was that the reconcile finazilers had to be moved only after backend is removed, this means that
backend CR will not be removed if backend fails to remove from 3scale, although to me, this is not an issue and makes sense, but I might be missing something.

eguzki
eguzki requested changes Mar 14, 2022
View reviewed changes
eguzki
eguzki reviewed Mar 14, 2022
View reviewed changes
eguzki
eguzki requested changes Mar 21, 2022
View reviewed changes
controllers/capabilities/product_controller.go Outdated
return ctrl.Result{}, err
}
} else {
return ctrl.Result{}, fmt.Errorf("product %s .status.ID is missing, cannot remove product", product.Spec.Name)
Copy link
Member

@eguzki eguzki Mar 21, 2022

Choose a reason for hiding this comment

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

same comment as for the backend controller

Copy link
Contributor Author

@MStokluska MStokluska Mar 22, 2022

Choose a reason for hiding this comment

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

Thanks

controllers/capabilities/product_controller.go Outdated
// https://kubernetes.io/docs/concepts/workloads/controllers/garbage-collection/#foreground-cascading-deletion
if product.DeletionTimestamp != nil {
if product.GetDeletionTimestamp() != nil && controllerutil.ContainsFinalizer(product, productFinalizer) {
controllerutil.RemoveFinalizer(product, productFinalizer)
Copy link
Member

@eguzki eguzki Mar 21, 2022

Choose a reason for hiding this comment

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

To be consistent with the backend controller, the finalizer should be removed after removing the product in 3scale

Copy link
Contributor Author

@MStokluska MStokluska Mar 22, 2022

Choose a reason for hiding this comment

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

done, thanks

controllers/capabilities/product_controller.go Outdated
}

//Confirm that product has been removed
products, err := threescaleAPIClient.ListProducts()
Copy link
Member

@eguzki eguzki Mar 21, 2022

Choose a reason for hiding this comment

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

no need to check. If the 3scale API does not return an error, the controller can assume it has been deleted.

Copy link
Contributor Author

@MStokluska MStokluska Mar 22, 2022

Choose a reason for hiding this comment

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

true, thanks Eguzki

@MStokluska
Copy link
Contributor Author

MStokluska commented Mar 22, 2022

/test test-e2e

1 similar comment
@MStokluska
Copy link
Contributor Author

MStokluska commented Mar 22, 2022

/test test-e2e

@qlty-cloud-legacy
Copy link

qlty-cloud-legacy bot commented Mar 22, 2022

Code Climate has analyzed commit d82cc0b and detected 3 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 3

View more on Code Climate.

@MStokluska
Copy link
Contributor Author

MStokluska commented Mar 22, 2022

/test test-e2e

@eguzki
Copy link
Member

eguzki commented Mar 23, 2022

good job 🎖️

@eguzki
Copy link
Member

eguzki commented Mar 23, 2022

/retest

@MStokluska MStokluska merged commit 6e2c211 into 3scale:master Mar 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eguzki eguzki eguzki approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@MStokluska @eguzki @Patryk-Stefanski