squashme: resolve conflicts, bring up to date

git-https-proxy/main.go

@@ -21,14 +21,18 @@ import (
 
 func main() {
 	config := parseEnv()
-	// INFO: Make a channel that will receive the SIGTERM
+	// INFO: Make a channel that will receive the SIGTERM on shutdown
 	sigTerm := make(chan os.Signal, 1)
-	signal.Notify(sigTerm, syscall.SIGINT, syscall.SIGTERM)
+	signal.Notify(sigTerm, syscall.SIGTERM, syscall.SIGINT)
 	ctx := context.Background()
+	// INFO: Used to coordinate shutdown between git-proxy and the session when the user
+	// is not anonymous and there may be an autosave branch that needs to be created
 	shutdownFlags := shutdownFlagsStruct{
 		sigtermReceived: false,
 		shutdownAllowed: false,
 	}
+
+	// INFO: Setup servers
 	proxyHandler := getProxyHandler(config)
 	proxyServer := http.Server{
 		Addr:    fmt.Sprintf(":%s", config.ProxyPort),
@@ -39,34 +43,55 @@ func main() {
 		Addr:    fmt.Sprintf(":%s", config.HealthPort),
 		Handler: healthHandler,
 	}
+
+	// INFO: Run servers in the background
 	go func() {
-		// INFO: Run the health server in the "background"
 		log.Printf("Health server active on port %s\n", config.HealthPort)
 		log.Fatalln(healthServer.ListenAndServe())
 	}()
 	go func() {
-		// INFO: Run the proxy server in the "background"
 		log.Printf("Git proxy active on port %s\n", config.ProxyPort)
 		log.Printf("Repo Url: %v, anonymous session: %v\n", config.RepoUrl, config.AnonymousSession)
 		log.Fatalln(proxyServer.ListenAndServe())
 	}()
-	// INFO: Block until you receive sitTerm
+
+	// INFO: Block until you receive sigTerm to shutdown. All of this is necessary
+	// because the proxy has to shut down only after all the other containers do so in case 
+	// any other containers (i.e. session or sidecar) need git right before shutting down, 
+	// and this is the case exactly for creating autosave branches.
 	<- sigTerm
-	log.Printf(
-		"SIGTERM received. Waiting for /shutdown to be called or timing out in %v\n", 
-		config.SessionTerminationGracePeriod,
-	)
-	// INFO: After sigterm is received update flags and wait for shutdown flag to show up
-	sigTermTime := time.Now()
-	shutdownFlags.lock.Lock()
-	shutdownFlags.sigtermReceived = true
-	shutdownFlags.lock.Unlock()
-	for {
-		if shutdownFlags.shutdownAllowed || (time.Now().Sub(sigTermTime) > config.SessionTerminationGracePeriod) {
-			healthServer.Shutdown(ctx)
-			proxyServer.Shutdown(ctx)
+	if config.AnonymousSession {
+		log.Print("SIGTERM received. Shutting down servers.\n")
+		healthServer.Shutdown(ctx)
+		proxyServer.Shutdown(ctx)
+	} else {
+		log.Printf(
+			"SIGTERM received. Waiting for /shutdown to be called or timing out in %v\n", 
+			config.SessionTerminationGracePeriod,
+		)
+		sigTermTime := time.Now()
+		shutdownFlags.lock.Lock()
+		shutdownFlags.sigtermReceived = true
+		shutdownFlags.lock.Unlock()
+		for {
+			if shutdownFlags.shutdownAllowed || (time.Now().Sub(sigTermTime) > config.SessionTerminationGracePeriod) {
+				log.Printf(
+					"Shutting down servers. SIGTERM received: %v, Shutdown allowed: %v.\n",
+					shutdownFlags.sigtermReceived,
+					shutdownFlags.shutdownAllowed,
+				)
+				err := healthServer.Shutdown(ctx)
+				if err != nil {
+					log.Fatalln(err)
+				}
+				err = proxyServer.Shutdown(ctx)
+				if err != nil {
+					log.Fatalln(err)
+				}
+				break
+			}
+			time.Sleep(time.Second * 5)
 		}
-		time.Sleep(time.Second * 5)
 	}
 }
 
@@ -93,10 +118,10 @@ func parseEnv() *gitProxyConfig {
 	var repoUrl *url.URL
 	var err error
 	var SessionTerminationGracePeriod time.Duration
-	if proxyPort, ok = os.LookupEnv("MITM_PROXY_PORT"); !ok {
+	if proxyPort, ok = os.LookupEnv("GIT_PROXY_PORT"); !ok {
 		proxyPort = "8080"
 	}
-	if healthPort, ok = os.LookupEnv("HEALTH_PORT"); !ok {
+	if healthPort, ok = os.LookupEnv("GIT_PROXY_HEALTH_PORT"); !ok {
 		healthPort = "8081"
 	}
 	if anonymousSessionStr, ok = os.LookupEnv("ANONYMOUS_SESSION"); !ok {

git_services/Dockerfile.init

@@ -3,7 +3,7 @@ FROM python:3.9-slim
 LABEL maintainer="Swiss Data Science Center <info@datascience.ch>"
 
 RUN apt-get update && \
-    apt-get install -y --no-install-recommends git=1:2.30.2-1 git-lfs=2.13.2-1+b5 curl && \
+    apt-get install -y --no-install-recommends git=1:2.30.2-1 git-lfs=2.13.2-1+b5 curl tini && \
     apt-get purge -y --auto-remove && \
     rm -rf /var/lib/apt/lists/* && \
     useradd jovyan -u1000 -g100 --create-home
@@ -20,4 +20,5 @@ ENV PATH "/home/jovyan/.local/bin:$PATH"
 RUN curl -sSL https://install.python-poetry.org | python3 - && \
     poetry install --no-dev
 
-ENTRYPOINT ["poetry", "run", "python", "-m", "git_services.init.clone"]
+ENTRYPOINT ["tini", "-g", "--"]
+CMD ["poetry", "run", "python", "-m", "git_services.init.clone"]

git_services/Dockerfile.sidecar

@@ -3,7 +3,7 @@ FROM python:3.9-slim
 LABEL maintainer="Swiss Data Science Center <info@datascience.ch>"
 
 RUN apt-get update && \
-    apt-get install -y --no-install-recommends git=1:2.30.2-1 git-lfs=2.13.2-1+b5 curl && \
+    apt-get install -y --no-install-recommends git=1:2.30.2-1 git-lfs=2.13.2-1+b5 curl tini && \
     apt-get purge -y --auto-remove && \
     rm -rf /var/lib/apt/lists/* && \
     useradd jovyan -u1000 -g100 --create-home
@@ -25,4 +25,5 @@ ENV HOST="0.0.0.0"
 #       even if this is an invalid rpc request.
 HEALTHCHECK CMD curl http://$HOST:4000
 
-ENTRYPOINT ["poetry", "run", "python", "-m", "git_services.sidecar.rpc_server"]
+ENTRYPOINT ["tini", "-g", "--"]
+CMD ["poetry", "run", "python", "-m", "git_services.sidecar.rpc_server"]

git_services/git_services/cli/sentry.py

@@ -12,7 +12,7 @@ class SentryConfig:
     def __post_init__(self):
         if type(self.enabled) is str:
             # NOTE: Required because bool("False") == True and environment vars are always strings
-            self.enabled = (self.enabled.lower() == "true")
+            self.enabled = self.enabled.lower() == "true"
         # INFO: Convert empty strings to None
         for attr_name in ["dsn", "environment"]:
             attr_val = getattr(self, attr_name)

git_services/git_services/init/cloner.py

@@ -43,9 +43,7 @@ def _wait_for_server(self, timeout_mins=None):
         start = datetime.now()
 
         while True:
-            logging.info(
-                f"Waiting for git to become available with timeout mins {timeout_mins}..."
-            )
+            logging.info(f"Waiting for git to become available with timeout mins {timeout_mins}...")
             res = requests.get(self.git_url)
             if res.status_code >= 200 and res.status_code < 400:
                 logging.info("Git is available")

git_services/git_services/sidecar/rpc_server.py

@@ -1,11 +1,15 @@
-from git_services.sidecar.config import config_from_env
 from jsonrpc import JSONRPCResponseManager, dispatcher
 import os
+import requests
 from werkzeug.wrappers import Request, Response
 from werkzeug.serving import run_simple
 from pathlib import Path
+from subprocess import PIPE, Popen
+import shlex
+
 from git_services.cli import GitCLI
 from git_services.cli.sentry import setup_sentry
+from git_services.sidecar.config import config_from_env
 
 
 @dispatcher.add_method
@@ -53,40 +57,60 @@ def status(path: str = ".", **kwargs):
 @dispatcher.add_method
 def autosave(**kwargs):
     """Create an autosave branch with uncommitted work."""
-    repo_path = os.environ.get("MOUNT_PATH")
-    status_result = status(path=repo_path)
-    should_commit = not status_result["clean"]
-    should_push = status_result["ahead"] > 0
-
-    if not should_commit and not should_push:
-        return
+    try:
+        git_proxy_health_port = os.getenv("GIT_PROXY_HEALTH_PORT", "8081")
+        repo_path = os.environ.get("MOUNT_PATH")
+        status_result = status(path=repo_path)
+        should_commit = not status_result["clean"]
+        should_push = status_result["ahead"] > 0
 
-    initial_commit = os.environ["CI_COMMIT_SHA"][0:7]
-    current_commit = status_result["commit"][0:7]
-    current_branch = status_result["branch"]
+        if not should_commit and not should_push:
+            return
 
-    user = os.environ["RENKU_USERNAME"]
+        initial_commit = os.environ["CI_COMMIT_SHA"][0:7]
+        current_commit = status_result["commit"][0:7]
+        current_branch = status_result["branch"]
 
-    autosave_branch_name = (
-        f"renku/autosave/{user}/{current_branch}/{initial_commit}/{current_commit}"
-    )
+        user = os.environ["RENKU_USERNAME"]
 
-    cli = GitCLI(Path(repo_path))
-
-    cli.git_checkout(f"-b {autosave_branch_name}")
-
-    if should_commit:
-        cli.git_add("-A")
-        cli.git_commit(
-            "--no-verify "
-            f"-m 'Auto-saving for {user} on branch {current_branch} from commit {initial_commit}'"
+        autosave_branch_name = (
+            f"renku/autosave/{user}/{current_branch}/{initial_commit}/{current_commit}"
         )
 
-    cli.git_push(f"origin {autosave_branch_name}")
-
-    cli.git_reset(f"--soft {current_branch}")
-    cli.git_checkout(f"{current_branch}")
-    cli.git_branch(f"-D {autosave_branch_name}")
+        cli = GitCLI(Path(repo_path))
+
+        cli.git_checkout(f"-b {autosave_branch_name}")
+
+        if should_commit:
+            # INFO: Find large files that should be checked in git LFS
+            autosave_min_file_size = os.getenv("AUTOSAVE_MINIMUM_LFS_FILE_SIZE_BYTES", "1000000")
+            cmd_res = Popen(
+                shlex.split(f"find . -type f -size +{autosave_min_file_size}c"),
+                cwd=Path(repo_path),
+                stdout=PIPE,
+                stderr=PIPE,
+            )
+            stdout, _ = cmd_res.communicate()
+            lfs_files = stdout.decode("utf-8").split()
+            if len(lfs_files) > 0:
+                cli.git_lfs("track " + " ".join(lfs_files))
+            cli.git_add("-A")
+            cli.git_commit(
+                "--no-verify "
+                f"-m 'Auto-saving for {user} on branch "
+                f"{current_branch} from commit {initial_commit}'"
+            )
+
+        cli.git_push(f"origin {autosave_branch_name}")
+
+        cli.git_reset(f"--soft {current_branch}")
+        cli.git_checkout(f"{current_branch}")
+        cli.git_branch(f"-D {autosave_branch_name}")
+    finally:
+        # INFO: Inform the proxy it can shut down
+        # NOTE: Do not place return, break or continue here, otherwise
+        # the exception from try will be completely discarded.
+        requests.get(f"http://localhost:{git_proxy_health_port}/shutdown")
 
 
 @Request.application

git_services/tests/test_autosave.py

@@ -32,13 +32,15 @@ def _mocked_cli(path):
         "MOUNT_PATH": ".",
         "CI_COMMIT_SHA": "75d22e14c12b5c70957ef73fcfdb12c03aef21bf",
         "RENKU_USERNAME": "Renku-user",
+        "GIT_PROXY_HEALTH_PORT": "8081",
     },
     clear=True,
 )
-def test_autosave_clean(mock_rpc_server_cli):
+@mock.patch("git_services.sidecar.rpc_server.requests.get")
+def test_autosave_clean(mock_requests_get, mock_rpc_server_cli):
     """Test no autosave branch is created on clean repo."""
     rpc_server.autosave()
-
+    mock_requests_get.assert_called_once_with("http://localhost:8081/shutdown")
     mock_rpc_server_cli.git_status.assert_called_once()
     mock_rpc_server_cli.git_commit.assert_not_called()
     mock_rpc_server_cli.git_push.assert_not_called()
@@ -50,17 +52,20 @@ def test_autosave_clean(mock_rpc_server_cli):
         "MOUNT_PATH": ".",
         "CI_COMMIT_SHA": "75d22e14c12b5c70957ef73fcfdb12c03aef21bf",
         "RENKU_USERNAME": "Renkuuser",
+        "GIT_PROXY_HEALTH_PORT": "8081",
     },
     clear=True,
 )
-def test_autosave_unpushed_changes(mock_rpc_server_cli):
+@mock.patch("git_services.sidecar.rpc_server.requests.get")
+def test_autosave_unpushed_changes(mock_requests_get, mock_rpc_server_cli):
     """Test no autosave branch is created on clean repo."""
     mock_rpc_server_cli.git_status.return_value = """# branch.head master
 # branch.oid 03c909db8dfdbb5ef411c086824aacd13fbad9d5
 # branch.ab +1 -0"""
 
     rpc_server.autosave()
 
+    mock_requests_get.assert_called_once_with("http://localhost:8081/shutdown")
     mock_rpc_server_cli.git_status.assert_called_once()
     mock_rpc_server_cli.git_commit.assert_not_called()
     mock_rpc_server_cli.git_push.assert_called_once_with(
@@ -92,10 +97,12 @@ def test_autosave_unpushed_changes(mock_rpc_server_cli):
         "MOUNT_PATH": ".",
         "CI_COMMIT_SHA": "75d22e14c12b5c70957ef73fcfdb12c03aef21bf",
         "RENKU_USERNAME": "Renku-user",
+        "GIT_PROXY_HEALTH_PORT": "8081",
     },
     clear=True,
 )
-def test_autosave_dirty_changes(status_file_line, mock_rpc_server_cli):
+@mock.patch("git_services.sidecar.rpc_server.requests.get")
+def test_autosave_dirty_changes(mock_requests_get, status_file_line, mock_rpc_server_cli):
     """Test no autosave branch is created on clean repo."""
     mock_rpc_server_cli.git_status.return_value = f"""# branch.head master
 # branch.oid 03c909db8dfdbb5ef411c086824aacd13fbad9d5
@@ -104,7 +111,7 @@ def test_autosave_dirty_changes(status_file_line, mock_rpc_server_cli):
 
     rpc_server.autosave()
 
-    mock_rpc_server_cli.git_status.assert_called_once()
+    mock_requests_get.assert_called_once_with("http://localhost:8081/shutdown")
     mock_rpc_server_cli.git_commit.assert_called_once_with(
         "--no-verify -m 'Auto-saving for Renku-user on branch master from commit 75d22e1'"
     )

helm-chart/renku-notebooks/templates/statefulset.yaml

@@ -154,6 +154,8 @@ spec:
               value: {{ .Values.cloudstorage.s3.enabled | quote }}
             - name: SESSION_TERMINATION_GRACE_PERIOD_SECONDS
               value: {{ .Values.sessionAutosave.terminationGracePeriodSeconds | quote }}
+            - name: AUTOSAVE_MINIMUM_LFS_FILE_SIZE_BYTES
+              value: {{ .Values.sessionAutosave.minimumLFSFileSizeBytes | quote }}
             - name: NOTEBOOKS_SERVICE_VERSION
               value: {{ .Values.image.tag | quote }}
           ports:

helm-chart/renku-notebooks/templates/test.yaml

@@ -86,6 +86,8 @@ spec:
           value: {{ $.Values.cloudstorage.s3.enabled | quote }}
         - name: SESSION_TERMINATION_GRACE_PERIOD_SECONDS
           value: {{ $.Values.sessionAutosave.terminationGracePeriodSeconds | quote }}
+        - name: AUTOSAVE_MINIMUM_LFS_FILE_SIZE_BYTES
+          value: {{ $.Values.sessionAutosave.minimumLFSFileSizeBytes | quote }}
         - name: NOTEBOOKS_SERVICE_VERSION
           value: {{ $.Values.image.tag | quote }}
       command:

helm-chart/renku-notebooks/values.yaml

@@ -135,7 +135,7 @@ sessionIngress:
 
 sessionAutosave:
   ## Any file higher than the minimum will be added to LFS
-  minimumLFSFileSize: 5M
+  minimumLFSFileSizeBytes: 1000000
   ## How long should k8s wait for an autosave branch to be created before
   ## fully and forcefully removing a user session
   terminationGracePeriodSeconds: 600