Add missing architecture arm64 support to the test suits.

antoniodimariano commented 1 month ago

Description

We noted that tests failed while running on a MacBookPro (M1/M2/M3) because the required Clickhouse server did not have arm64 support.

Our changes are simple. We changed the docker image choosing the first one in the official repository (clickhouse/clickhouse-server:22.8 ) with an arm64 support. It is different from the one currently used in production : https://github.com/hystax/optscale/blob/2add5929bc340f178b97dc1f5123f73e66f651cd/optscale-deploy/optscale/templates/clickhouse/statefulset.yaml#L31 which doesn't support arm64.

Also, we have removed the bin files from the folder rest_api/rest_api_server/tests/binaries because there is no need to compile binaries using multi-arch images as we did.

Related issue number

Special notes

Checklist

[ X] The pull request title is a good summary of the changes
[ X] Unit tests for the changes exist
[ X] New and existing unit tests pass locally

maxb-hystax commented 1 month ago

Thanks for the PR! Checking this change against our workflows.

sd-hystax commented 1 month ago

Greetings! I carefully looked (and tested) these changes and I want to help finish them. There are still problems with running tests locally (without docker) and running on older macs So I suggest: 

we are keeping the clickhouse from image only for tests in docker
Remove docker volume and symlinks (we dont need it anymore)
prepare_clickhouse_local need to call once to get local clickhouse client for any architecture (for example, the clickhouse-server does not work with older mac os architectures)

You can apply this diff on your commit and try it P.S. Please update this MR after!

diff --git a/rest_api/Dockerfile_tests b/rest_api/Dockerfile_tests
index 7826b88f..31dc700b 100644
--- a/rest_api/Dockerfile_tests
+++ b/rest_api/Dockerfile_tests
@@ -10,4 +10,4 @@ COPY optscale_client/rest_api_client optscale_client/rest_api_client
 RUN pip install --no-cache-dir -r rest_api/test-requirements.txt
 COPY rest_api/rest_api_server/tests rest_api/rest_api_server/tests
 COPY rest_api/prepare_clickhouse_local.sh rest_api/prepare_clickhouse_local.sh
-COPY --from=clickhouse /usr/bin/clickhouse /usr/bin/clickhouse
\ No newline at end of file
+COPY --from=clickhouse /usr/bin/clickhouse rest_api/.clickhouse/clickhouse
diff --git a/rest_api/prepare_clickhouse_local.sh b/rest_api/prepare_clickhouse_local.sh
index 3b4fe81a..e474eb71 100755
--- a/rest_api/prepare_clickhouse_local.sh
+++ b/rest_api/prepare_clickhouse_local.sh
@@ -1,5 +1,14 @@
 #!/bin/bash
 set -e
-CLICKHOUSE_ROOT_DIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" &> /dev/null && pwd)
-mkdir -p $CLICKHOUSE_ROOT_DIR/.clickhouse
-ln -sf /usr/bin/clickhouse $CLICKHOUSE_ROOT_DIR/.clickhouse/clickhouse
\ No newline at end of file
+CLICKHOUSE_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" &> /dev/null && pwd)/.clickhouse"
+CLICKHOUSE_BIN="$CLICKHOUSE_DIR/clickhouse"
+
+if [ ! -f $CLICKHOUSE_BIN ]; then
+    echo "Preparing clickhouse for local use"
+    mkdir -p $CLICKHOUSE_DIR
+    cd $CLICKHOUSE_DIR && curl https://clickhouse.com/ | sh
+    chmod +x $CLICKHOUSE_BIN
+    echo "Complete"
+else
+  echo "Сlickhouse binary ($CLICKHOUSE_BIN) already exists"
+fi
diff --git a/rest_api/run_test.sh b/rest_api/run_test.sh
index bde1a1e8..1fba8533 100755
--- a/rest_api/run_test.sh
+++ b/rest_api/run_test.sh
@@ -22,7 +22,7 @@ docker run -i --rm ${TEST_IMAGE} bash -c \
 echo "<<Alembic down revision tests"

 echo "Unit tests>>>"
-docker run -i --rm -v $PWD/rest_api/.clickhouse:/usr/src/app/rest_api/.clickhouse ${TEST_IMAGE} \
-    bash -c "./rest_api/prepare_clickhouse_local.sh docker && PYTHONPATH=. pytest -n auto rest_api --maxprocesses 3 --dist no --disable-warnings"
+docker run -i --rm ${TEST_IMAGE} \
+    bash -c "PYTHONPATH=. pytest -n auto rest_api --maxprocesses 3 --dist no --disable-warnings"
 echo "<<Unit tests"
 docker rmi ${TEST_IMAGE}

sd-hystax commented 1 month ago

@antoniodimariano Greetings! I see that diff was applied to only one file, instead of 3. My diff also contains changes in Dockerfile_tests and prepare_clickhouse_local.sh Did something confuse you about the changes? (or maybe the diff was applied incorrectly)

antoniodimariano commented 1 month ago

@antoniodimariano Greetings! I see that diff was applied to only one file, instead of 3. My diff also contains changes in Dockerfile_tests and prepare_clickhouse_local.sh Did something confuse you about the changes? (or maybe the diff was applied incorrectly) Hi @sd-hystax ! I got an error patching the first and the last diff.
|diff --git a/rest_api/run_test.sh b/rest_api/run_test.sh
|index bde1a1e8..1fba8533 100755
|--- a/rest_api/run_test.sh
|+++ b/rest_api/run_test.sh
--------------------------
File to patch: rest_api/run_test.sh
patching file rest_api/run_test.sh
patch unexpectedly ends in middle of line

sd-hystax commented 1 month ago

@antoniodimariano I just checked it out and everything is working well. There are already changes in the files so please revert the last commit and apply the diff as one, without splitting it into several 1) revert last commit 2) save diff into file fix.diff 3) git apply fix.diff

antoniodimariano commented 1 month ago

@sd-hystax, I still got the same error when applying your dff for some reason. I have double-checked for extra chars, but I didn't find anything wrong. By the way, since the changes are minimal, I applied them manually. I hope it's ok.

hystax / optscale