-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
fix: adjust file healthcheck durations #3874
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
Conversation
What is the necessity for adding swap? AFAICS it already has 16GB memory which is above self-hosted limit (14GB)
|
Sorry, I've just seen #3859 (comment) I think there is another problem here if it wants to use more than 16GB. |
There are 72 containers, when they start (almost all) at once, the RAM requirements are much higher than 16GB, it settles later, but during startup, swap is required. |
Adding healthcheck just exposed that problem, before |
We should increase memory requirement if that's the case, there are many scenarios which swap cannot be added, my environments are one of them:) |
That is not a bad idea; however, we cannot do much about GitHub-hosted runners, having the amount of resources they have. There are large runners, but those are not free to use. |
Oops! Has there been any other complaint about this in issues or discord? @aldy505
Yes, you're right. We have no other option aside from adding swap in GHA. |
No, please don't do that. I already recommend people to add some swapfile to battle with the insane amount of RAM usage. I'd prefer adding more info regarding swap. |
I was the one who complained 😆. SH e2e tests are failing everywhere. And I don't want to merge the revert PR. |
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.
Your knowledge is insane!!
@aminvakil Burak is on vacation and I don't wanna wait until Hubert is awake. If you're ok with this, I'll go merge this. |
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 adding swap on GHA.
I still cannot understand how integration tests passed before, it only needs more than 16GB upon starting up? |
However, with healthchecks defined, those services not only have to report a healthy state, but also stay in that state for the entire duration of the wait. It was still possible for the test to fail before, but I suspect the odds were quite small for that to happen. I also did not check what exactly the integration tests are testing, it might be that they would pass even with some snuba or sentry consumers not running ;) |
I think most probably this was the case. |
Some of it test user creation, login, and some basic interaction with the API. Other than that, it sends events through the Python SDK: error, transaction, and profile. Then, query to the Sentry API to see if there are any submitted event from the SDK. |
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.