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.

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

Infoblox, heavy load fixed #312

Merged
merged 2 commits into from Feb 23, 2021
Merged

Infoblox, heavy load fixed #312

merged 2 commits into from Feb 23, 2021

Conversation

kuritka
Copy link
Collaborator

@kuritka kuritka commented Feb 22, 2021 •
edited

Rancher includes annotation into Ingress object.

  "field.cattle.io/publicEndpoints": "[{\"addresses\":[\"22.240.40.188\"],\"port\":80,\"protocol\":\"HTTP\",\"serviceName\":\"test-gslb:frontend-podinfo\",\"ingressName\":\"test-gslb:test-gslb-failover\",\"hostname\":\"failover.k8gb-test-nonprod.bcp.absa.co.za\",\"path\":\"/\",\"allNodes\":true}]",

Original code modified ingress object back into desired state, but when reconciliation ended, rancher included new annotation again.
As result was annotation included, removed, included, removed, etc...
Such behavior led to executing reconciliation loop immediately instead of specified delay (RECONCILE_REQUEUE_SECONDS).

  • fix reconciliation issue (see ingress.go changes)

  • rename function ensureIngres -> saveIngress

  • ReconcileResultHandler

    • instead of return ctrl.Result{},err we call return r.RequeueError(err),return r.Requeue() etc..

  • Some functions like saveIngress, SaveEndpoint returned (*ctrl.Result, error) as result, but they shouldn't be responsible for reconciliation status.
    Now they return only error and only gslb_controller handles Reconciliation.

ytsarev
ytsarev reviewed Feb 22, 2021
View reviewed changes
controllers/gslb_controller.go
result, err = r.ensureIngress(gslb, ingress)
if result != nil {
return *result, err
err = r.saveIngress(gslb, ingress)
Copy link
Member

Choose a reason for hiding this comment

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

Just in case the ensureIngress naming was inherited from official operator-sdk quickstart tutorial. Obviously no strong opinion here :)

controllers/ingress.go Outdated Show resolved Hide resolved
somaritane
somaritane suggested changes Feb 22, 2021
View reviewed changes
Copy link
Contributor

@somaritane somaritane left a comment

Choose a reason for hiding this comment

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

Nice work! It should help to reduce load on edgeDNS providers in general, not only Infoblox.
lgtm, just few comments from me.

controllers/internal/utils/result.go Outdated Show resolved Hide resolved
controllers/internal/utils/annotations_test.go Outdated Show resolved Hide resolved
@kuritka kuritka changed the title Infoblox heavy load fixed Infoblox, heavy load fixed Feb 22, 2021
somaritane
somaritane suggested changes Feb 22, 2021
View reviewed changes
controllers/internal/utils/annotations.go Outdated Show resolved Hide resolved
controllers/ingress.go Outdated
return nil
}

func ingressCompare(found *v1beta1.Ingress, i *v1beta1.Ingress) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider renaming

Suggested change
func ingressCompare(found *v1beta1.Ingress, i *v1beta1.Ingress) bool {
func ingressEqual(ing1 *v1beta1.Ingress, ing2 *v1beta1.Ingress) bool {

as we're testing for equality, not comparing entities.

Copy link
Collaborator Author

@kuritka kuritka Feb 22, 2021 •
edited

Choose a reason for hiding this comment

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

Just thinking about new package k8gb/metav1 next to the k8gb/utils package and reuse ApiMachinery
https://pkg.go.dev/k8s.io/apimachinery/pkg/apis/meta/v1#SetMetaDataAnnotation but in k8gb

Having metav1.AnnotationsEqual(obj1 *ObjectMeta, obj2 *ObjectMeta), metav1.MergeAnnotations(obj1 *ObjectMeta, obj2 *ObjectMeta)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Then we can reuse for any kind. (Maybe move to AbsaOSS/gopkg)

Copy link
Contributor

Choose a reason for hiding this comment

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

Like here https://github.com/AbsaOSS/k8gb/pull/314 ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@somaritane yes

Copy link
Contributor

Choose a reason for hiding this comment

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

Might be good in general along with reusing helpers from https://pkg.go.dev/k8s.io/apimachinery/pkg/apis/meta/v1.
I don't have strong opinion here, perhaps we can extract it later when we find more use in other projects.
@ytsarev, @k0da, wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

Good idea, though I wouldn't block this specific PR with it. We should log the Issue for that though

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok, will provide in later.

@somaritane somaritane mentioned this pull request Feb 22, 2021
Closed
@kuritka
Infoblox heavy load fixed
7ec35c0 
Rancher includes annotation into Ingress object.
```json
  "field.cattle.io/publicEndpoints": "[{\"addresses\":[\"22.240.40.188\"],\"port\":80,\"protocol\":\"HTTP\",\"serviceName\":\"test-gslb:frontend-podinfo\",\"ingressName\":\"test-gslb:test-gslb-failover\",\"hostname\":\"failover.k8gb-test-nonprod.bcp.absa.co.za\",\"path\":\"/\",\"allNodes\":true}]",
```
Original code modified ingress object back into desired state, but when reconciliation ended, rancher included new annotation again.
As result was annotation included, removed, included , removed etc...
Such behavior leds to executing reconciliation loop immediatelly instead of specified delay (RECONCILE_REQUEUE_SECONDS).

 - fix reconciliation issue
 - rename function ensureIngres -> saveIngress
 - ReconcileResultHandler
   - instead of `return ctrl.Result{},err` we call `r.RequeueError(err)`,`r.Requeue()` etc..

 - Some functions returned `(*ctrl.Result, error)` but they shouldn't be responsible for reconciliation results.
   Now they return only `error` and only `gslb_controller` is responsible for Reconciliation (decide on the form of `ctrl.Reconciliation` object).
@kuritka
MergeAnnotations + Test
9c56098
somaritane
somaritane approved these changes Feb 23, 2021
View reviewed changes
Copy link
Contributor

@somaritane somaritane left a comment

Choose a reason for hiding this comment

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

lgtm!

@kuritka kuritka merged commit 4113ee4 into master Feb 23, 2021
@somaritane somaritane deleted the rate-limiter-fix branch February 23, 2021 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@somaritane somaritane somaritane approved these changes

@donovanmuller donovanmuller Awaiting requested review from donovanmuller donovanmuller is a code owner

@k0da k0da Awaiting requested review from k0da k0da is a code owner

@ytsarev ytsarev Awaiting requested review from ytsarev ytsarev is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@kuritka @somaritane @ytsarev