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

Avoid unnecessary object updates #438

Merged
merged 4 commits into from
Nov 3, 2020
Merged

Avoid unnecessary object updates #438

merged 4 commits into from
Nov 3, 2020

Conversation

ansd
Copy link
Member

@ansd ansd commented Nov 2, 2020

Before this PR, the reconciler was unnecessarily updating the headless service, client service, and stateful set in every reconcile loop.

To reproduce, deploy a RabbitMQ cluster and trigger a reconcile loop (e.g. by adding an annotation to kubectl edit serviceaccounts <crd name>-rabbitmq-server). In the operator logs, you will see:

2020-11-02T14:40:30.615Z	INFO	rabbitmqcluster-controller	updated resource example-rabbitmq-headless of Type *v1.Service
2020-11-02T14:40:30.615Z	DEBUG	controller-runtime.manager.events	Normal	{"object": {"kind":"RabbitmqCluster","namespace":"rabbitmq-system","name":"example","uid":"392f8b3f-d466-460e-8cfa-e75385d1f431","apiVersion":"rabbitmq.com/v1beta1","resourceVersion":"883"}, "reason": "SuccessfulUpdate", "message": "updated resource example-rabbitmq-headless of Type *v1.Service"}
2020-11-02T14:40:30.619Z	INFO	rabbitmqcluster-controller	updated resource example-rabbitmq-client of Type *v1.Service
2020-11-02T14:40:30.620Z	DEBUG	controller-runtime.manager.events	Normal	{"object": {"kind":"RabbitmqCluster","namespace":"rabbitmq-system","name":"example","uid":"392f8b3f-d466-460e-8cfa-e75385d1f431","apiVersion":"rabbitmq.com/v1beta1","resourceVersion":"883"}, "reason": "SuccessfulUpdate", "message": "updated resource example-rabbitmq-client of Type *v1.Service"}
2020-11-02T14:40:30.626Z	INFO	rabbitmqcluster-controller	updated resource example-rabbitmq-server of Type *v1.StatefulSet
2020-11-02T14:40:30.626Z	DEBUG	controller-runtime.manager.events	Normal	{"object": {"kind":"RabbitmqCluster","namespace":"rabbitmq-system","name":"example","uid":"392f8b3f-d466-460e-8cfa-e75385d1f431","apiVersion":"rabbitmq.com/v1beta1","resourceVersion":"883"}, "reason": "SuccessfulUpdate", "message": "updated resource example-rabbitmq-server of Type *v1.StatefulSet"

This is fixed by having the controller set default fields which are also set when fetching the object.

Before this commit the headless service was updated unnecessarily in
every reconile loop.
Before this commit the client service was updated unnecessarily in
every reconile loop.
Before this commit the stateful set was updated unnecessarily in
every reconile loop.
Copy link
Collaborator

@Zerpet Zerpet left a comment

Choose a reason for hiding this comment

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

We are making some of the StatefulSet "default" opinions our own by putting them in code in statefulset.go. Most of them are probably harmless today e.g. DNSPolicy or SchedulerName, however we are acquiring a commitment of keeping those defaults up to date, if they ever change in the StatefulSet controller; otherwise we would be setting some strong-ish opinions here, as those would only be "overridable" by using the override feature.

Some other changes are not so harmless, specifically, the image pull policy. The StatefulSet controller has a logic to set this value to Always if the tag in the image is latest, or set it to IfNotPresent otherwise. We should not have an opinion on this matter and just rely on the StatefulSet controller to make this decision for us. I don't think we should replica the logic of the STS controller because we would acquire a similar commitment as in the above paragraph. By always setting the image pull policy to "if not present", we might surprise some users using the tag latest.

In summary, I think the price we have to pay to not see this message in the log is too high and we should not make this change to the statefulset.go, the changes to the services are fine and welcome.

Alternatively, we could Get() the STS object and set the spec as we want it to be, modifying the fields we care about and leaving everything else as it came from Kubernetes API. This will also resolve the issue; however this approach is a lot more work and it conflicts with our current approach of "the resource A should look like this and I don't consider what the current state is".

internal/resource/statefulset.go Outdated Show resolved Hide resolved
@ansd
Copy link
Member Author

ansd commented Nov 3, 2020

Thanks @Zerpet. I reverted that commit leaving the 2 commits for the service objects.

@ansd ansd requested a review from Zerpet November 3, 2020 15:24
@ansd ansd merged commit d4b6129 into main Nov 3, 2020
@ansd ansd deleted the fix-updates branch November 3, 2020 16:59
ChunyiLyu pushed a commit that referenced this pull request Nov 6, 2020
* Avoid unnecessary update for headless service

Before this commit the headless service was updated unnecessarily in
every reconile loop.

* Avoid unnecessary update for client service

Before this commit the client service was updated unnecessarily in
every reconile loop.

* Avoid unnecessary update for StatefulSet

Before this commit the stateful set was updated unnecessarily in
every reconile loop.

* Revert "Avoid unnecessary update for StatefulSet"

This reverts commit 3c28bc5.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants