Skip to content
This repository has been archived by the owner on Nov 30, 2023. It is now read-only.

avoid saving the azuremachine CR too often #1367

Merged
merged 2 commits into from Feb 19, 2021
Merged

Conversation

whites11
Copy link
Contributor

This handler was calling Update on the AzureMachine CR at every reconciliation loop, regardless of any changes being made to the CR.
This triggered a loop in the handler that was running in an infinite sequence.
This PR check for changes in the CR and avoid saving if there isn't any

@whites11 whites11 self-assigned this Feb 18, 2021
@whites11 whites11 requested a review from a team February 18, 2021 16:44
@nprokopic
Copy link
Contributor

Are you sure this is triggering new reconciliation loops? We were checking a case like this before, and it should not happen, as update should basically be a noop if there were no changes.

If it is triggering new reconciliation loops without CR being actually changed, it might be a bug in operatorkit cc @giantswarm/sig-operator

@nprokopic
Copy link
Contributor

If it is triggering new reconciliation loops without CR being actually changed, it might be a bug in operatorkit cc @giantswarm/sig-operator

I might be wrong about operatorkit bug, but we did have few discussions about the topic and it came down to these updates without actual changes not having any effect.

@whites11
Copy link
Contributor Author

Are you sure this is triggering new reconciliation loops? We were checking a case like this before, and it should not happen, as update should basically be a noop if there were no changes.

If it is triggering new reconciliation loops without CR being actually changed, it might be a bug in operatorkit cc @giantswarm/sig-operator

100% sure I can provide CRs and logs

@nprokopic
Copy link
Contributor

There might be some issue in the func called few lines above then:

err = helpers.InitAzureMachineAnnotations(ctx, r.ctrlClient, r.logger, &azureMachine)

https://github.com/giantswarm/azure-operator/blob/master/pkg/helpers/azuremachine.go#L16

@whites11
Copy link
Contributor Author

There might be some issue in the func called few lines above then:

err = helpers.InitAzureMachineAnnotations(ctx, r.ctrlClient, r.logger, &azureMachine)

https://github.com/giantswarm/azure-operator/blob/master/pkg/helpers/azuremachine.go#L16

BTW I copied what you did in another place where you use the same function

https://github.com/giantswarm/azure-operator/blob/master/service/controller/azurecluster/handler/azureclusterupgrade/resource.go#L69

@nprokopic
Copy link
Contributor

Touche :) Forgot now why I did that, it wasn't for avoiding reconciliation loops though.

My only concern here is that there might be an issue/bug somewhere in code that is causing infinite reconciliation loops (maybe caused by me in that func 😬), and this might hide it temporarily.

@nprokopic
Copy link
Contributor

and this might hide it temporarily.

Like I possibly did with that other "changed" check.

@whites11
Copy link
Contributor Author

and this might hide it temporarily.

Like I possibly did with that other "changed" check.

I'm not sure right now but I think that's the only piece of code where we actually edit the Azure machine cr. If that's true it has to be it causing the reconciliation madness

Copy link
Contributor

@tuommaki tuommaki left a comment

Choose a reason for hiding this comment

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

Nice 👍

@whites11 whites11 merged commit 9e12f92 into master Feb 19, 2021
@whites11 whites11 deleted the avoid-too-many-saves branch February 19, 2021 07:12
if changed {
err = r.ctrlClient.Update(ctx, &azureMachine)
if errors.IsConflict(err) {
r.logger.Debugf(ctx, "conflict trying to save object in k8s API concurrently", "stack", microerror.JSON(microerror.Mask(err)))
Copy link
Member

Choose a reason for hiding this comment

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants