fix: [DENA-602] Wait for cockroach cluster to be ready

[MarcinGinszt]

Without those, scripts were entering "CrashLoopBackoff" on cluster start

[linear]

DENA-602 cockroachdb-backup- wait for cluster to be ready

[matthewhughes-uw]

could we move this to a lifecycle hook on the container, and let k8s handle the retrying/back-off etc. behaviour?

Why doesn't the main cockcorach container in the STS suffer from the same issues, can we just re-use whatever that uses to figure out 'readiness'?

I'm also not a fan of this just being copied across the two scripts, it feels like it would be very easy to update one and forget to update the other

[MarcinGinszt]

Lifecycle hooks execute synchronously with container entrypoint. The same about readiness probes.

Trying to make it nicer, I put it into separate script that runs before main scripts. Not sure if it's much better, but I can't find a better way to do it.

[matthewhughes-uw]

do we really want to remove this? It could mean the script exiting with 0 even if e.g. an SQL command failed

[MarcinGinszt]

unremoved

[github-actions]

Post kustomize build diff:

diff --git a/root-manifests/cockroachdb/manifests-cert-manager/db-init/manifests.yaml b/built-manifests/cockroachdb/manifests-cert-manager/db-init/manifests.yaml
index 21999bc..0b0d8cc 100644
--- a/root-manifests/cockroachdb/manifests-cert-manager/db-init/manifests.yaml
+++ b/built-manifests/cockroachdb/manifests-cert-manager/db-init/manifests.yaml
@@ -9,9 +9,11 @@ spec:
   template:
     spec:
       containers:
-      - command:
+      - args:
+        - -c
+        - /opt/scripts/wait-for-cluster-to-be-ready.sh && /opt/scripts/user-schema-bootstrap.sh
+        command:
         - /bin/bash
-        - /opt/scripts/user-schema-bootstrap.sh
         env:
         - name: COCKROACH_HOST
           valueFrom:
@@ -61,6 +63,7 @@ spec:
                 path: client.root.key
               name: cockroachdb.client.root
       - configMap:
+          defaultMode: 511
           name: cockroachdb-scripts
         name: cockroachdb-scripts
 ---
diff --git a/root-manifests/cockroachdb/manifests-cert-manager/manifests.yaml b/built-manifests/cockroachdb/manifests-cert-manager/manifests.yaml
index d0e7ea6..2cf6859 100644
--- a/root-manifests/cockroachdb/manifests-cert-manager/manifests.yaml
+++ b/built-manifests/cockroachdb/manifests-cert-manager/manifests.yaml
@@ -14,7 +14,7 @@ data:
         exit 1
     fi
 
-    # todo: wait for the cockroachdb cluster to be ready
+    # execute command
     SQL_CMD="/cockroach/cockroach sql" > /dev/null
 
     $SQL_CMD << EOF
@@ -29,14 +29,20 @@ data:
     EOF
   user-schema-bootstrap.sh: "#!/bin/bash\nset -e\n\nif [ -z \"$DB_USER\" ]; then\n
     \   echo \"DB_USER envvar is not set\"\n    exit 1\nfi\n\nif [ -z \"$DB_NAME\"
-    ]; then\n    echo \"DB_NAME envvar not set\"\n    exit 1\nfi\n\nSQL_CMD=\"/cockroach/cockroach
-    -d system sql\" > /dev/null\n\n# try creating user, database, grant database permissions
-    to user\n$SQL_CMD << EOF\n  CREATE USER IF NOT EXISTS $DB_USER;\n  CREATE DATABASE
-    IF NOT EXISTS $DB_NAME;\n  GRANT ALL ON DATABASE $DB_NAME TO $DB_USER;\nEOF\n\n#
-    If database already exists and contains tables, grant access to them for user.
-    \n# This command will fail for freshly initialized databases.\n$SQL_CMD --execute
-    \"GRANT ALL ON TABLE $DB_NAME.* TO $DB_USER;\" 2>1 || echo \"no tables in database
-    yet\"\n"
+    ]; then\n    echo \"DB_NAME envvar not set\"\n    exit 1\nfi\n\n# execute command
+    \nSQL_CMD=\"/cockroach/cockroach -d system sql\" > /dev/null\n\n# try creating
+    user, database, grant database permissions to user\n$SQL_CMD << EOF\n  CREATE
+    USER IF NOT EXISTS $DB_USER;\n  CREATE DATABASE IF NOT EXISTS $DB_NAME;\n  GRANT
+    ALL ON DATABASE $DB_NAME TO $DB_USER;\nEOF\n\n# If database already exists and
+    contains tables, grant access to them for user. \n# This command will fail for
+    freshly initialized databases.\n$SQL_CMD --execute \"GRANT ALL ON TABLE $DB_NAME.*
+    TO $DB_USER;\" 2>1 || echo \"no tables in database yet\"\n"
+  wait-for-cluster-to-be-ready.sh: "#!/bin/bash\n\nMAX_RETRIES=40\nRETRY_INTERVAL=3\n\nis_cluster_ready()
+    {\n  cockroach sql --execute=\"SELECT 1;\" &> /dev/null\n}\n\nuntil is_cluster_ready;
+    do\n  ((retry_count++))\n\n  if [ $retry_count -ge $MAX_RETRIES ]; then\n      echo
+    \"Reached maximum retries. Cluster is not ready.\"\n      exit 1\n  fi\n\n  echo
+    \"Cluster is not ready yet. Retry $retry_count/$MAX_RETRIES. Waiting for $RETRY_INTERVAL
+    seconds...\"\n  sleep $RETRY_INTERVAL\ndone \n"
 kind: ConfigMap
 metadata:
   name: cockroachdb-scripts
@@ -303,9 +309,11 @@ spec:
   template:
     spec:
       containers:
-      - command:
+      - args:
+        - -c
+        - /opt/scripts/wait-for-cluster-to-be-ready.sh && /opt/scripts/backup-bootstrap.sh
+        command:
         - /bin/bash
-        - /opt/scripts/backup-bootstrap.sh
         env:
         - name: COCKROACH_HOST
           valueFrom:
@@ -361,6 +369,7 @@ spec:
                 path: client.root.key
               name: cockroachdb.client.root
       - configMap:
+          defaultMode: 511
           name: cockroachdb-scripts
         name: cockroachdb-scripts
   ttlSecondsAfterFinished: 600

=============================================
k8s objects: 0 to add, 0 to destroy
6 changed hunks

[sbuliarca]

I think this is nicer than what we had before and would result in fewer job pod failures.
In the past I think we were relying on k8s retrying within the backoffLimit