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

fix(app): keep git-proxy alive on session shutdown #951

Merged
merged 18 commits into from
Apr 11, 2022

Conversation

olevski
Copy link
Member

@olevski olevski commented Feb 24, 2022

We noticed that in many cases the autosave branch does not get created. This was especially true if the unsaved commits in the session has large data files.

The main reason for this is that k8s waits only 30 seconds after it has sent the termination signal (SIGTERM) to a pod before it forcefully shuts it down. So if the autosave takes more than 30 seconds to be created then it will be interrupted before it is fully pushed/created.

K8s has the terminationGracePeriodSeconds for this purpose. But for this to work properly all containers have to properly handle the initial SIGTERM signal that k8s sent. If not then the pod will always take terminationGracePeriodSeconds to shutdown.

By adding tini to containers now the SIGTERM signal is properly handled. But now the containers shut down so quickly that the autosave branch still cannot be created.

This is solved by the following:

  • the git proxy catches the SIGTERM event and waits until it receives a GET request on the health port at /shutdown
  • the git proxy therefore shuts down only if it gets such a /shutdown request or the terminationGracePeriod expires
  • the preStop step that creates the autosave branch now sends a GET request to /shutdown when it is done to indicate to the git proxy it can shutdown
  • to avoid confusion and edge cases the git proxy does not always shut down when it receives a request at /shutdown. It will only properly act on the /shutdown url once it receives a SIGTERM, if this hasn't happened then calling /shutdown has no effect
  • in addition to the above the code that creates the autosave finds any large files and checks them in LFS - prior to this change all files were simply checked in git
  • in the case of anonymous sessions the git proxy will shutdown as soon as it receives the SIGTERM and will not wait for /shutdown request, this is because the anonymous sessions cannot create autosaves

closes #920

@olevski olevski temporarily deployed to renku-ci-nb-951 February 24, 2022 20:38 Inactive
@RenkuBot
Copy link
Contributor

You can access the deployment of this PR at https://renku-ci-nb-951.dev.renku.ch

@olevski olevski temporarily deployed to renku-ci-nb-951 March 13, 2022 20:58 Inactive
@olevski olevski temporarily deployed to renku-ci-nb-951 March 13, 2022 21:17 Inactive
@olevski olevski temporarily deployed to renku-ci-nb-951 March 13, 2022 21:31 Inactive
@olevski olevski temporarily deployed to renku-ci-nb-951 March 14, 2022 14:38 Inactive
@olevski olevski temporarily deployed to renku-ci-nb-951 March 14, 2022 15:34 Inactive
@olevski olevski temporarily deployed to renku-ci-nb-951 March 14, 2022 16:57 Inactive
@olevski olevski temporarily deployed to renku-ci-nb-951 March 14, 2022 17:20 Inactive
@olevski olevski temporarily deployed to renku-ci-nb-951 March 14, 2022 18:34 Inactive
@olevski olevski deployed to renku-ci-nb-951 March 14, 2022 20:25 Active
vigsterkr
vigsterkr previously approved these changes Apr 6, 2022
Copy link
Contributor

@vigsterkr vigsterkr left a comment

Choose a reason for hiding this comment

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

🚀

git_services/git_services/sidecar/rpc_server.py Outdated Show resolved Hide resolved
@olevski olevski requested a review from vigsterkr April 11, 2022 08:45
vigsterkr
vigsterkr previously approved these changes Apr 11, 2022
Copy link
Contributor

@vigsterkr vigsterkr left a comment

Choose a reason for hiding this comment

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

:shipit:

@olevski olevski merged commit 7589230 into master Apr 11, 2022
@olevski olevski deleted the use-tini-in-sidecars branch April 11, 2022 11:42
@olevski olevski restored the use-tini-in-sidecars branch November 30, 2022 21:43
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.

Properly handle LFS files and long-stopping sessions when creating autosaves
3 participants