Supported running cleanup job on GCP #719
Conversation
WalkthroughThis change updates the Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
jobs/cleanup-expired-key-value-records/Dockerfile (1)
3-4: Consider verifying the cloud-sql-proxy binary integrity.The code correctly downloads the cloud-sql-proxy binary from Google's official storage, but it doesn't verify the binary's integrity using checksums or signatures. For production environments, it's recommended to verify downloaded binaries to prevent supply chain attacks.
You could add a checksum verification step:
+RUN curl -Lo /bin/cloud-sql-proxy.sha256 https://storage.googleapis.com/cloud-sql-connectors/cloud-sql-proxy/v2.10.1/cloud-sql-proxy.linux.amd64.sha256 RUN curl -Lo /bin/cloud-sql-proxy https://storage.googleapis.com/cloud-sql-connectors/cloud-sql-proxy/v2.10.1/cloud-sql-proxy.linux.amd64 +RUN echo "$(cat /bin/cloud-sql-proxy.sha256) /bin/cloud-sql-proxy" | sha256sum -c RUN chmod +x /bin/cloud-sql-proxy +RUN rm /bin/cloud-sql-proxy.sha256jobs/cleanup-expired-key-value-records/cleanup-expired-key-value-records (1)
8-9: Replace static sleep with connection verification.Using a static 2-second sleep assumes the proxy will be ready in that time. Under high load or network latency, this might not be sufficient.
Consider replacing the static sleep with a polling mechanism to verify the proxy is ready:
cloud-sql-proxy -instances="$INSTANCE_CONNECTION_NAME"=tcp:$PORT & PROXY_PID=$! -sleep 2 +# Wait for proxy to be ready with timeout +TIMEOUT=30 +COUNTER=0 +while ! nc -z 127.0.0.1 $PORT && [ $COUNTER -lt $TIMEOUT ]; do + sleep 1 + COUNTER=$((COUNTER+1)) +done +if [ $COUNTER -eq $TIMEOUT ]; then + echo "Error: Timed out waiting for Cloud SQL Auth Proxy to be ready" + kill $PROXY_PID + exit 1 +fiNote: This requires adding
netcatto your Docker image:RUN apt-get update && apt-get install -y netcat-openbsd && rm -rf /var/lib/apt/lists/*
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
jobs/cleanup-expired-key-value-records/Dockerfile(1 hunks)jobs/cleanup-expired-key-value-records/cleanup-expired-key-value-records(1 hunks)
🔇 Additional comments (1)
jobs/cleanup-expired-key-value-records/cleanup-expired-key-value-records (1)
16-18: LGTM: Appropriate use of dynamic HOST and PORT variables.The script correctly uses the dynamically assigned HOST and PORT variables in the MySQL connection command, allowing seamless switching between direct connection and proxy connection modes.
| if [ -n "$INSTANCE_CONNECTION_NAME" ]; then | ||
| echo "Using Cloud SQL Auth Proxy for $INSTANCE_CONNECTION_NAME" | ||
| cloud-sql-proxy -instances="$INSTANCE_CONNECTION_NAME"=tcp:$PORT & | ||
| sleep 2 | ||
| HOST=127.0.0.1 | ||
| else | ||
| echo "Using direct TCP host $MYSQL_HOST" | ||
| HOST="$MYSQL_HOST" | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve Cloud SQL Proxy process management.
The current implementation starts the proxy as a background process but doesn't handle its termination when the script exits. Also, there's no error handling if the proxy fails to start.
Consider adding proper process management and error handling:
if [ -n "$INSTANCE_CONNECTION_NAME" ]; then
echo "Using Cloud SQL Auth Proxy for $INSTANCE_CONNECTION_NAME"
- cloud-sql-proxy -instances="$INSTANCE_CONNECTION_NAME"=tcp:$PORT &
+ cloud-sql-proxy -instances="$INSTANCE_CONNECTION_NAME"=tcp:$PORT &
+ PROXY_PID=$!
sleep 2
+ # Check if proxy is running
+ if ! kill -0 $PROXY_PID 2>/dev/null; then
+ echo "Error: Cloud SQL Auth Proxy failed to start"
+ exit 1
+ fi
HOST=127.0.0.1
+ # Set trap to kill proxy on script exit
+ trap 'kill $PROXY_PID' EXIT
else
echo "Using direct TCP host $MYSQL_HOST"
HOST="$MYSQL_HOST"
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [ -n "$INSTANCE_CONNECTION_NAME" ]; then | |
| echo "Using Cloud SQL Auth Proxy for $INSTANCE_CONNECTION_NAME" | |
| cloud-sql-proxy -instances="$INSTANCE_CONNECTION_NAME"=tcp:$PORT & | |
| sleep 2 | |
| HOST=127.0.0.1 | |
| else | |
| echo "Using direct TCP host $MYSQL_HOST" | |
| HOST="$MYSQL_HOST" | |
| fi | |
| if [ -n "$INSTANCE_CONNECTION_NAME" ]; then | |
| echo "Using Cloud SQL Auth Proxy for $INSTANCE_CONNECTION_NAME" | |
| cloud-sql-proxy -instances="$INSTANCE_CONNECTION_NAME"=tcp:$PORT & | |
| PROXY_PID=$! | |
| sleep 2 | |
| # Check if proxy is running | |
| if ! kill -0 $PROXY_PID 2>/dev/null; then | |
| echo "Error: Cloud SQL Auth Proxy failed to start" | |
| exit 1 | |
| fi | |
| HOST=127.0.0.1 | |
| # Set trap to kill proxy on script exit | |
| trap 'kill $PROXY_PID' EXIT | |
| else | |
| echo "Using direct TCP host $MYSQL_HOST" | |
| HOST="$MYSQL_HOST" | |
| fi |
🤖 Prompt for AI Agents
In jobs/cleanup-expired-key-value-records/cleanup-expired-key-value-records
around lines 6 to 14, the Cloud SQL Proxy is started as a background process
without managing its lifecycle or handling startup errors. Modify the script to
capture the proxy process ID after starting it, add a trap to ensure the proxy
process is terminated when the script exits, and include checks to verify the
proxy started successfully, exiting with an error message if it fails.
|
|
||
| PORT=${MYSQL_PORT:-3306} | ||
|
|
||
| if [ -n "$INSTANCE_CONNECTION_NAME" ]; then |
There was a problem hiding this comment.
if MYSQL_SOCKET_PATH is set:
mysql \
--socket="$MYSQL_SOCKET_PATH" \
--user="$MYSQL_USER" \
--password="$MYSQL_PASSWORD" \
--database="$MYSQL_DATABASE"
...
else:
mysql \
--host="$MYSQL_SOCKET_PATH" \
--port="$MYSQL_PORT" \
--user="$MYSQL_USER" \
--password="$MYSQL_PASSWORD" \
--database="$MYSQL_DATABASE"
...
ref https://linear.app/ghost/issue/PROD-685 Connecting to CloudSQL isn't done in the usual way, we need to use the unix socket which is mounted automatically for Cloud Run Jobs at /cloudsql/<instance-name>
allouis
force-pushed
the
support-gcp-for-cleanup-job
branch
from
ba2cdb6 to
ecb3387
Compare
| @@ -1,5 +1,8 @@ | |||
| FROM mysql:8.3 | |||
|
|
|||
| RUN curl -Lo /bin/cloud-sql-proxy https://storage.googleapis.com/cloud-sql-connectors/cloud-sql-proxy/v2.10.1/cloud-sql-proxy.linux.amd64 | |||
ref https://linear.app/ghost/issue/PROD-685
Connecting to CloudSQL isn't done in the usual way, we need to use the cloud-sql-proxy, or a socket, or some other way of connecting. Using this proxy seemed like the easiest way to have a shared image for both local and cloud use