-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Use docker compose exec to create additional kafka topics #2904
Conversation
I think you need a |
@BYK There's this note:
But it'd be a good thing to include that, whether we're reusing |
Lol, I'm pretty sure I'm the one who left that note and looks like it is not very effective 😅 Also, this was pre Compose 2.0 time where we did not have the $dc up --no-build --no-recreate --wait kafka |
Btw the previous script just does |
@BYK Think you're right, CI will fail as kafka isn't up yet. Let's see what happens |
CI didn't fail. Hubert is flabbergasted. |
Ok, running snuba-api will bring up zookeeper, redis, clickhouse, and kafka. It's from https://github.com/getsentry/self-hosted/blob/master/docker-compose.yml#L81 |
@saz can you add a line before executing everything else on the setup-kafka-topics as suggested by Burak here: #2904 (comment) That should be all |
@aldy505 Done. I've also removed the comment |
Looks like we cannot use |
GAH :( |
Any other things I can do to solve this issue? Sticking to the implicit way isn't an option? I've seen a different issue upgrading |
Is the issue with |
@azaslavsky it's about adding |
We could just remove the while [ true ]
do
$kafka_healthy=$(docker compose ps kafka | grep 'healthy')
if [ ! -z "$kafka_healthy" ]; then
break
fi
echo "Kafka container is not healthy, waiting for 30 seconds. If this took too long, abort the installation process, and check your Kafka configuration"
sleep 30s
done One alternative would be using the wait-for-it script. |
e4adaba
to
744609c
Compare
@aldy505 Changed as requested |
0973104
to
ccec185
Compare
@aldy505 Changed. Thanks for spotting it! |
Gentle ping @hubertdeng123 @BYK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks! :)
install/create-kafka-topics.sh
Outdated
while [ true ] | ||
do | ||
$kafka_healthy=$($dc ps kafka | grep 'healthy') | ||
if [ ! -z "$kafka_healthy" ]; then | ||
break | ||
fi | ||
|
||
echo "Kafka container is not healthy, waiting for 30 seconds. If this took too long, abort the installation process, and check your Kafka configuration" | ||
sleep 30s | ||
done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Protip, you can do something like this:
self-hosted/_integration-test/run.sh
Line 47 in 7143d88
timeout 90 bash -c 'until $(curl -Isf -o /dev/null $SENTRY_TEST_HOST); do printf '.'; sleep 0.5; done' |
while [ true ] | |
do | |
$kafka_healthy=$($dc ps kafka | grep 'healthy') | |
if [ ! -z "$kafka_healthy" ]; then | |
break | |
fi | |
echo "Kafka container is not healthy, waiting for 30 seconds. If this took too long, abort the installation process, and check your Kafka configuration" | |
sleep 30s | |
done | |
echo "Waiting for kafka container to be healthy for up to 30 seconds." | |
timeout 30 bash -c 'until $($dc ps kafka | grep -q 'healthy'); do printf '.'; sleep 0.5; done' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But sometimes Kafka took an entire 5 minutes to boot up :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, then let's make it 300s? :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So... Applying the change with at least 300s? Maybe it's better to even raise it a bit further, just in case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, I'm not 100% sure. I don't use Kafka anymore on my Sentry instance, but for a minimum 4 core CPU, Kafka is rather problematic to boot up. I think it's okay to stay as it is (rather than changing to Burak's recommendation).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BYK What's your opinionen? Would be nice to get this merged
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with whatever to be honest. I think 300s is not too bad. Maybe settle at 150? 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay let's settle this at 150s 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the reason CI is failing is because you may need to remove the $ in front of $kafka_healthy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hubertdeng123 can you just directly commit it?
Self-Hosted Version CPU Architecture Docker Version Docker Compose Version Centos Version .install.sh error
|
Hey @azaslavsky @hubertdeng123 can we merge this before tomorrow's release? |
Running the tests now, if it passes, I'll make sure it lands before 24.4.0 |
It looks like the tests are failing - can y'all see the action output? Ex: https://github.com/getsentry/self-hosted/actions/runs/8695624967/job/23847053353?pr=2904 |
@azaslavsky Please approve workflows again |
Looks like it is still failing :( |
@azaslavsky I don't understand, why it's not able to attach to the container. I'm not able to reproduce this on my system :( |
I've found the issue. Would be great, if someone can approve the workflows again |
🤞 |
* feat: provide csrf settings information for sentry config (getsentry#2762) * feat: provide csrf settings information for sentry config * chore: trim trailing whitespace * build(deps): bump actions/setup-python from 4 to 5 (getsentry#2644) Bumps [actions/setup-python](https://github.com/actions/setup-python) from 4 to 5. - [Release notes](https://github.com/actions/setup-python/releases) - [Commits](actions/setup-python@v4...v5) --- updated-dependencies: - dependency-name: actions/setup-python dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Hubert Deng <[email protected]> * Fix groupedmessage indexing error (getsentry#2777) * fix groupedmessage indexing error * Check memcached backend in Django (getsentry#2778) Bail if using old memcached backend * release: 24.1.2 * build: Set master version to nightly #skip-changelog * fix: DB migration script (getsentry#2779) * use different approach to wait for postgres server * Tweak postgres indexing fix (getsentry#2792) * tweak postgres indexing fix * add exists constraint * build(deps): bump pre-commit/action from 3.0.0 to 3.0.1 (getsentry#2788) Bumps [pre-commit/action](https://github.com/pre-commit/action) from 3.0.0 to 3.0.1. - [Release notes](https://github.com/pre-commit/action/releases) - [Commits](pre-commit/action@v3.0.0...v3.0.1) --- updated-dependencies: - dependency-name: pre-commit/action dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump nginx version (getsentry#2797) bump nginx version * release: 24.2.0 * build: Set master version to nightly #skip-changelog * use rust consumers in self-hosted * revert changes in 3067683 * chore: Use django ORM to perform sql commands (getsentry#2827) * use django orm * chore: provide clearer csrf url example (getsentry#2833) * Upgrade to FSL-1.1 (getsentry#2835) * simplify if for open-ai-suggestion (getsentry#2732) * simplify if for open-ai-suggestion * Add snuba rust consumers (getsentry#2831) * add snuba rust consumers * Enable proxy buffering in nginx (getsentry#2844) Enable proxy buffering Setting the proxy_buffering directive to “off” is a common mistake because it can cause performance issues and unexpected behavior in NGINX. When proxy buffering is disabled, NGINX receives a response from the proxied server and immediately sends it to the client without storing it in a buffer. This can cause problems if the response is large or if the connection between NGINX and the client is slow, because it can result in increased memory usage and potentially lead to request timeouts. To avoid this mistake, it is recommended to always enable proxy buffering in NGINX. * deps: bump maxmind/geoipupdate to 6.1.0 (getsentry#2859) https://github.com/maxmind/geoipupdate/releases/tag/v6.1.0 * increase postgres max_connections above 100 connections (getsentry#2740) * use default value and env for postgres max_connections * Integration test improvements (getsentry#2858) * integration test improvements * feat(spans): Ingest spans (getsentry#2861) * release: 24.3.0 * build: Set master version to nightly #skip-changelog * Remove duplicate feature flags (getsentry#2899) * feat: run outcomes-billing consumer (getsentry#2909) * Integration tests in python (getsentry#2892) * integration tests in python * Fix defunct java processes (getsentry#2914) revert kafka healthcheck change * Port backup tests to python (getsentry#2907) * port backup tests to python * feat(clickhouse): Added max_suspicious_broken_parts to the config.xml (getsentry#2853) * feat(clickhouse): Added max_suspicious_broken_parts to the config.xml * refactor(clickhouse): Set default max_suspicious_bronken_parts and Issue reference --------- Co-authored-by: Hubert Deng <[email protected]> * Write Customization tests in python (getsentry#2918) * port everything integration test related to python * Bump ubuntu version for tests (getsentry#2923) * bump ubuntu version used for testing * get rid of codecov cli dependency * fix(spans): Adds organizations:standalone-span-ingestion flag to default config (getsentry#2936) Adds organizations:standalone-span-ingestion flag to default config * feat: adds group attributes consumer (getsentry#2927) adds group attributes consumer * Use python for e2e tests (getsentry#2953) * bump e2e action commit sha * release: 24.4.0 * build: Set master version to nightly #skip-changelog * Port last integration tests to python (getsentry#2966) * port custom ca cert test to python * Add example to docker compose version in problem report (getsentry#2959) * Use docker compose exec to create additional kafka topics (getsentry#2904) * chore(deps): bump memcached and redis to latest patch versions (getsentry#2973) * release: 24.4.1 * build: Set master version to nightly #skip-changelog * Add workstation configuration (getsentry#2968) * Add workstation configuration These are prebuilt docker images for spinning up a local self-hosted image on the Google Cloud Workstation project. While primarily intended for internal development at Sentry, in theory these can be used by anyone with GCWS project to create a fresh workstation for developing self-hosted via a remote VSCode connection. Users who have GCWS properly configured will be able to use the forthcoming `workstations ...` command in the `sentry` dev CLI to create, manage, and destroy one-off or long-lived workstations in either the pre-install or post-install configuration. Note that the `sentry workstations ...` CLI has not yet landed in the `sentry` repo - those changes are coming soon! Issue: getsentry/team-ospo#240 * Fix shfmt complaints * Upgrade postgres to 14.11 (getsentry#2975) chore(deps): bump postgres to latest 14 alpine version * Bump docker compose version in CI (getsentry#2980) * only rerun tests on v2.0.1 * change from http error to request error * use 3 retries like before * fix: backport changes --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: Reinaldy Rafli <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Hubert Deng <[email protected]> Co-authored-by: Chad Whitacre <[email protected]> Co-authored-by: getsentry-bot <[email protected]> Co-authored-by: getsentry-bot <[email protected]> Co-authored-by: hubertdeng123 <[email protected]> Co-authored-by: Iven Schlenther <[email protected]> Co-authored-by: RexTim <[email protected]> Co-authored-by: Victor <[email protected]> Co-authored-by: Erfan <[email protected]> Co-authored-by: Pierre Massat <[email protected]> Co-authored-by: Jann Kleen <[email protected]> Co-authored-by: Lyn Nagara <[email protected]> Co-authored-by: edwardgou-sentry <[email protected]> Co-authored-by: Stephen Cefali <[email protected]> Co-authored-by: Edgar Sanchez <[email protected]> Co-authored-by: Steffen Zieger <[email protected]> Co-authored-by: Matthew T <[email protected]> Co-authored-by: Alex Zaslavsky <[email protected]>
fixes #2903
fixes #2754
docker compose run
will start a second kafka container, instead of reusing the existing one.install/create-kafka-topics.sh
is failing, askafka-topics
is trying to connect within the same container.Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.