Fix timeout caused by nginx #4336

aronasorman · 2023-11-10T22:51:20Z

Summary

Description of the change(s) you made

Fixes https://github.com/learningequality/infrastructure/issues/452. Nginx is the source of the timeout. Extends this to 300 seconds (5 minutes).

First commit is to update and cleanup Sigil, the templating engine we use for nginx.

Docker is having trouble parsing complex env var substitution, so we move that logic to a full fledged script.

DXCanas

Definitely looks like some yakshaving for that one liner lol

DXCanas · 2023-11-13T18:51:23Z

k8s/images/nginx/download_sigil.sh

+#!/bin/sh
+export SIGIL_VERSION=0.10.1
+export OS=`sh -c "uname -s | tr '[:upper:]' '[:lower:]'"`
+export ARCH=`sh -c "uname -m | tr '[:upper:]' '[:lower:]' | sed 's/aarch64/arm64/'"`
+
+
+curl -L "https://github.com/gliderlabs/sigil/releases/download/v${SIGIL_VERSION}/gliderlabs-sigil_${SIGIL_VERSION}_${OS}_${ARCH}.tgz" | tar -zxC /tmp
+mv /tmp/gliderlabs-sigil-${ARCH} /tmp/sigil


I see a refactor in our future lol

I actually didn't carry out the refactor we talked about, where we can simply dispense with Sigil. Should I do that in this PR @DXCanas ?

Nah. Too big a load, and this should be fixed ASAP. We should also think about the various paths forward. Hence my asking @bjester for his thoughts on moving some of this to infra:
#4336 (comment)

DXCanas · 2023-11-13T18:51:45Z

k8s/images/nginx/Dockerfile

+COPY /k8s/images/nginx/download_sigil.sh /tmp/download_sigil.sh
+RUN chmod +x /tmp/download_sigil.sh
+RUN /tmp/download_sigil.sh


I moved it to a script, since complex env vas substitution doesn't work in a Dockerfile.

DXCanas · 2023-11-13T18:51:58Z

deploy/nginx.conf.jinja2

@@ -82,6 +82,7 @@ http {
            proxy_pass         http://studio;
            proxy_redirect     off;
            proxy_set_header   Host $host;
+            proxy_read_timeout 300s;


what was the reasoning in choosing 300s?

tbh, I'd be surprised if the reasoning was anything deeper than "add a 0 at the end"/"it's longer than 30s" and it made for easy testing.

Are you thinking we should have it match the Cloudflare at 100s? Or does the Studio API have its own value we want to match?

Yeah I mean if Cloudflare is less than this at 100s, I'm not sure there's any use in having it longer? Just means more resources are tied up longer?

Ok I've pushed a commit limiting it at 100s. let me know if it looks good and I'll squash the fixup commit.

DXCanas · 2023-11-13T18:53:58Z

@bjester this looks alright to me, but I think it'd be a good idea for you to be the one to review.

Would you have any objections to us bringing more of this stuff into the infra repo rather than studio?

bjester · 2023-11-13T18:55:28Z

k8s/images/nginx/Dockerfile

-RUN curl -L "https://github.com/gliderlabs/sigil/releases/download/v0.4.0/sigil_0.4.0_$(uname -sm|tr \  _).tgz" \ | tar -zxC /usr/bin
+COPY /k8s/images/nginx/download_sigil.sh /tmp/download_sigil.sh
+RUN chmod +x /tmp/download_sigil.sh
+RUN /tmp/download_sigil.sh

 FROM nginx:1.11


Any reason to not update nginx while we're at it? I think 1.11 was EOL in 2017...

Don't want to speak for Aron, but just to hold you over till he's out: I think he mentioned that he was running into some build issues after an upgrade? But I could be conflating nginx and the sigil work during our verbal updates.

I can upgrade the version number, but I think it's best to do that in another PR. Keep the PRs small!

That's the Sigil issue @DXCanas

bjester · 2023-11-14T18:35:04Z

@bjester this looks alright to me, but I think it'd be a good idea for you to be the one to review.

Would you have any objections to us bringing more of this stuff into the infra repo rather than studio?

If our plan is to remove nginx at some point, then I don't think it matters. If we don't remove it, I think it is helpful in this repo for replicating production, if we actually had the dev setup to do so (something which I moved us towards in an unmerged PR IIRC). For example, when BCK bugs come up, there's some tension regarding whether the bug is BCK or Kolibri related, which we don't have a good developer setup to help us navigate that through. I hope and envision that for Studio though.

DXCanas · 2023-11-14T18:48:16Z

when BCK bugs come up, there's some tension regarding whether the bug is BCK or Kolibri related

For Kolibri specifically, this was intentional — at the time of design, everything lived in Kolibri. But this was getting increasingly bloated and difficult to manage. Any little infra-ish fix would necessitate an entirely new tagged Kolibri release, and going through the Kolibri testing just to get an image to deploy. With a Kolibri PR that required a Kolibri dev to review code they didn't understand. Only for the fix not to work and for us to start the process over again.

Having things in the infra repo allows for rapid iteration. And, because there's such a deep integration with values from Kubernetes, postgres, etc, a lot of the code is templated and populated using infra tooling, like Terraform. This is actually the reason Aron had to add Sigil here — he had no templating library to work with.

Processes have changed, and I see your point. But having that permeable membrane between the 2 teams has its benefits. KDP having its production docker image in the KDP repo can get cumbersome as well — @anguyen1234 is starting to see this for herself as she iterates on the Frontend/Worker division. Some things about the KDP image don't match up with industry best practices. But we don't want to go in and break the dev workflow by iterating on it there. I have a hard time even adding a requirement to requirements.txt because I have to go through requirements.in which just straight up won't compile on my machine. Gets yakshavey really quick.

I think do think there's a middle ground here. But that we should be careful not to optimize too much for the workflow of either an app dev or an infra dev. Might be a good talking point for a dev meeting one of these days?

bjester · 2023-11-14T18:56:16Z

@DXCanas I think the point is that there is pain on both sides. You bring up the infra pain, but don't forget there's pain on our end too trying to debug production issues. It isn't one-sided

DXCanas · 2023-11-14T20:50:26Z

@bjester totally agree. I just think it's important we get input from both sides as we come up with a solution.

I think do think there's a middle ground here. But that we should be careful not to optimize too much for the workflow of either an app dev or an infra dev

Fixes https://github.com/learningequality/infrastructure/issues/452

That's the latest stable version as of this writing

bjester · 2023-11-20T16:52:10Z

@aronasorman Looks like the Cloud Build failed:

mv: can't rename '/tmp/gliderlabs-sigil-x86_64': No such file or directory
The command '/bin/sh -c /tmp/download_sigil.sh' returned a non-zero code: 1

aronasorman · 2023-11-20T16:59:58Z

Can you revert that PR for now to unblock everyone? I’ll fix it later

On November 20, 2023, "José L. Redrejo Rodríguez" ***@***.***> wrote: @aronasorman <https://github.com/aronasorman> Looks like the Cloud Build failed: mv: can't rename '/tmp/gliderlabs-sigil-x86_64': No such file or directory The command '/bin/sh -c /tmp/download_sigil.sh' returned a non-zero code: 1

— Reply to this email directly, view it on GitHub <#4336 (comment)- 1819449133>, or unsubscribe <https://github.com/notifications/unsubscribe- auth/AABO3UZFAS2PKGFEUR5AWNTYFODELAVCNFSM6AAAAAA7GZKOVCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMJZGQ2DSMJTGM>. You are receiving this because you were mentioned.Message ID: ***@***.***>

jredrejo · 2023-11-27T19:56:49Z

when BCK bugs come up, there's some tension regarding whether the bug is BCK or Kolibri related

For Kolibri specifically, this was intentional — at the time of design, everything lived in Kolibri. But this was getting increasingly bloated and difficult to manage. Any little infra-ish fix would necessitate an entirely new tagged Kolibri release, and going through the Kolibri testing just to get an image to deploy. With a Kolibri PR that required a Kolibri dev to review code they didn't understand. Only for the fix not to work and for us to start the process over again.

Having things in the infra repo allows for rapid iteration. And, because there's such a deep integration with values from Kubernetes, postgres, etc, a lot of the code is templated and populated using infra tooling, like Terraform. This is actually the reason Aron had to add Sigil here — he had no templating library to work with.

Processes have changed, and I see your point. But having that permeable membrane between the 2 teams has its benefits. KDP having its production docker image in the KDP repo can get cumbersome as well — @anguyen1234 is starting to see this for herself as she iterates on the Frontend/Worker division. Some things about the KDP image don't match up with industry best practices. But we don't want to go in and break the dev workflow by iterating on it there. I have a hard time even adding a requirement to requirements.txt because I have to go through requirements.in which just straight up won't compile on my machine. Gets yakshavey really quick.

I think do think there's a middle ground here. But that we should be careful not to optimize too much for the workflow of either an app dev or an infra dev. Might be a good talking point for a dev meeting one of these days?

As I mentioned in https://github.com/learningequality/kolibri-data-portal/pull/335#issuecomment-1828500394 I don't see the point in maintaining a Dockerfile in the KDP repo with a cloud deployment target. Dockerfile in KDP repo, if needed, only makes sense for local developers installation, from my particular point of view. Infra must do production deployments based on their own needs/limitations/architecture decissions.

refactor: move sigil download logic to a separate script

09ea961

Docker is having trouble parsing complex env var substitution, so we move that logic to a full fledged script.

aronasorman changed the title ~~Fix timeout~~ Fix timeout caused by nginx Nov 10, 2023

aronasorman requested review from DXCanas and bjester November 13, 2023 18:16

DXCanas approved these changes Nov 13, 2023

View reviewed changes

bjester reviewed Nov 13, 2023

View reviewed changes

bjester approved these changes Nov 17, 2023

View reviewed changes

Aron Asor added 2 commits November 19, 2023 10:46

fix: increase timeout on reading upstream studio server responses

f61442e

Fixes https://github.com/learningequality/infrastructure/issues/452

chore: upgrade nginx to 1.25

6ccb9a9

That's the latest stable version as of this writing

aronasorman force-pushed the fix-timeout branch from 0f13b45 to 6ccb9a9 Compare November 19, 2023 18:48

aronasorman merged commit 35321d9 into learningequality:unstable Nov 19, 2023
9 checks passed

aronasorman deleted the fix-timeout branch November 19, 2023 18:49

aronasorman mentioned this pull request Nov 22, 2023

Fix issue created by download_sigil.sh #4343

Merged

bjester mentioned this pull request Feb 27, 2024

Copies nginx proxy timeout configuration update #4458

Merged

24 tasks

akolson mentioned this pull request Aug 13, 2024

Bulk Edit Release #4635

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix timeout caused by nginx #4336

Fix timeout caused by nginx #4336

aronasorman commented Nov 10, 2023

DXCanas left a comment

DXCanas Nov 13, 2023

aronasorman Nov 13, 2023

DXCanas Nov 13, 2023

DXCanas Nov 13, 2023

aronasorman Nov 13, 2023

DXCanas Nov 13, 2023

bjester Nov 14, 2023

DXCanas Nov 14, 2023

bjester Nov 14, 2023

aronasorman Nov 17, 2023

DXCanas commented Nov 13, 2023

bjester Nov 13, 2023

DXCanas Nov 13, 2023

aronasorman Nov 13, 2023

aronasorman Nov 13, 2023

bjester commented Nov 14, 2023

DXCanas commented Nov 14, 2023

bjester commented Nov 14, 2023

DXCanas commented Nov 14, 2023

bjester commented Nov 20, 2023

aronasorman commented Nov 20, 2023 via email

jredrejo commented Nov 27, 2023

Fix timeout caused by nginx #4336

Fix timeout caused by nginx #4336

Conversation

aronasorman commented Nov 10, 2023

Summary

Description of the change(s) you made

DXCanas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DXCanas commented Nov 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjester commented Nov 14, 2023

DXCanas commented Nov 14, 2023

bjester commented Nov 14, 2023

DXCanas commented Nov 14, 2023

bjester commented Nov 20, 2023

aronasorman commented Nov 20, 2023 via email

jredrejo commented Nov 27, 2023