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

Support for snuba additional arguments, livenessProbe & readinessProbe #1078

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

artemistomaras
Copy link

This pull request references issue #1076

@adonskoy
Copy link
Contributor

I think it’s worth adding default probes for consumers, which will be enabled by default and leave it possible to overwrite them in the values file

@artemistomaras
Copy link
Author

@adonskoy in order to have a valid probe for consumers the arroyo HealthCheck strategy needs to be instantiated. So therefore both of the values that I provided (.additionalArgs, .livenessProbe) need to be set

Copy link
Contributor

@pyhp2017 pyhp2017 left a comment

Choose a reason for hiding this comment

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

Please change additionArgs as i mentioned

@@ -655,6 +656,20 @@ snuba:
# maxBatchTimeMs: ""
# queuedMaxMessagesKbytes: ""
# queuedMinMessages: ""
additionalArgs: []
# additionalArgs:
# - --health-check-file=/tmp/health.txt
Copy link
Contributor

Choose a reason for hiding this comment

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

should be changed to: "--healthcheck-file-path"

Copy link
Author

Choose a reason for hiding this comment

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

@pyhp2017 if I understand correctly you want me to remove the additionalArgs array and instead have a plain string value like:

healthCheckFilePath: "/tmp/health.txt"

Copy link
Contributor

Choose a reason for hiding this comment

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

in the commented area you have this: "--health-check-file=/tmp/health.txt"
which is not correct:

--healthcheck-file-path TEXT A file to touch roughly every second to indicate that the
consumer is still alive. See
https://getsentry.github.io/arroyo/strategies/healthcheck.html
for more information.

Copy link
Author

@artemistomaras artemistomaras Dec 16, 2023

Choose a reason for hiding this comment

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

@pyhp2017 I am invoking the snuba consumer & snuba subscriptions-scheduler-executor cli commands.

Both commands accept an argument named --health-check-file:

  1. snuba consumer command
  2. snuba subscriptions-scheduler-executor command

which maps the underlying arroyo healthcheck strategy.

I have also tested the above in my personal kubernetes cluster and it works as --health-check-file.


additionalArgs: []
# additionalArgs:
# - --health-check-file=/tmp/health.txt
Copy link
Contributor

Choose a reason for hiding this comment

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

should be changed to: "--healthcheck-file-path"


additionalArgs: []
# additionalArgs:
# - --health-check-file=/tmp/health.txt
Copy link
Contributor

Choose a reason for hiding this comment

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

should be changed to: "--healthcheck-file-path"

@@ -736,6 +777,20 @@ snuba:
autoOffsetReset: "earliest"
# volumes: []
# volumeMounts: []
additionalArgs: []
# additionalArgs:
# - --health-check-file=/tmp/health.txt
Copy link
Contributor

Choose a reason for hiding this comment

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

should be changed to: "--healthcheck-file-path"

@@ -750,6 +805,21 @@ snuba:
autoOffsetReset: "earliest"
# volumes: []
# volumeMounts: []
additionalArgs: []
# additionalArgs:
# - --health-check-file=/tmp/health.txt
Copy link
Contributor

Choose a reason for hiding this comment

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

should be changed to: "--healthcheck-file-path"


additionalArgs: []
# additionalArgs:
# - --health-check-file=/tmp/health.txt
Copy link
Contributor

Choose a reason for hiding this comment

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

should be changed to: "--healthcheck-file-path"

@@ -900,6 +1039,21 @@ snuba:
# queuedMaxMessagesKbytes: ""
# queuedMinMessages: ""

additionalArgs: []
# additionalArgs:
# - --health-check-file=/tmp/health.txt
Copy link
Contributor

Choose a reason for hiding this comment

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

should be changed to: "--healthcheck-file-path"


additionalArgs: []
# additionalArgs:
# - --health-check-file=/tmp/health.txt
Copy link
Contributor

Choose a reason for hiding this comment

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

should be changed to: "--healthcheck-file-path"


additionalArgs: []
# additionalArgs:
# - --health-check-file=/tmp/health.txt
Copy link
Contributor

Choose a reason for hiding this comment

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

should be changed to: "--healthcheck-file-path"


additionalArgs: []
# additionalArgs:
# - --health-check-file=/tmp/health.txt
Copy link
Contributor

Choose a reason for hiding this comment

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

should be changed to: "--healthcheck-file-path"

@artemistomaras
Copy link
Author

@pyhp2017 any update on this?

@Mokto
Copy link
Contributor

Mokto commented Oct 25, 2024

👋 Hi, @artemistomaras,
I detected conflicts against the base branch 🙊
You'll want to sync 🔄 your branch with upstream!

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

Successfully merging this pull request may close these issues.

4 participants