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

[synthetic-monitoring-agent] fix deployment not starting on update/auto-scaling #3070

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

Iridias
Copy link

@Iridias Iridias commented Apr 10, 2024

Retry #2994

The implementation of the agent does not seem to allow for concurrency.
Thus scaling the deployment - either on helm-upgrade or auto-scaling - will result in the new PODs to never become ready!

So any helm-upgrade will run into a timeout and then abort.

In the logs you'll find the following messages (if debug is enabled):

{"level":"info","program":"synthetic-monitoring-agent","subsystem":"updater","error":"registering probe with synthetic-monitoring-api, response: probe already exists","was_connected":false,"connection_state":"READY","time":1708611775289,"caller":"github.com/grafana/synthetic-monitoring-agent/internal/checks/checks.go:259","message":"broke out of loop"}
{"level":"warn","program":"synthetic-monitoring-agent","subsystem":"updater","error":"registering probe with synthetic-monitoring-api, response: probe already exists","connection_state":"READY","time":1708611775289,"caller":"github.com/grafana/synthetic-monitoring-agent/internal/checks/checks.go:309","message":"handling check changes"}

With emphasis on: response: probe already exists

To fix that, I changed the Deployment to a StatefulSet, as k8s ensures, that the old POD is killed/deleted before spawning the new one.
I also removed all the autoscaling-resources, as they're not useful anyway.

And of course, I also successfully tested the changes on one of our clusters.

Signed-off-by: Iridias <iridias@tiphares.de>
Signed-off-by: Iridias <iridias@tiphares.de>
Copy link
Collaborator

@Sheikh-Abubaker Sheikh-Abubaker left a comment

Choose a reason for hiding this comment

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

@Iridias Could you please resolve the Conflicts

@CLAassistant
Copy link

CLAassistant commented Sep 27, 2024

CLA assistant check
All committers have signed the CLA.

Sheikh-Abubaker and others added 3 commits September 27, 2024 21:03
Signed-off-by: MH <zanhsieh@gmail.com>
Signed-off-by: MH <zanhsieh@gmail.com>
@Sheikh-Abubaker
Copy link
Collaborator

Sheikh-Abubaker commented Oct 1, 2024

@zanhsieh what do you think about this ? I mean there are other charts as well which are using hpa, how are they resolving this ?

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.

4 participants