ovn-org / ovn-heater

Mega script to deploy/configure/run OVN scale tests.
Apache License 2.0
12 stars 12 forks source link

Introduce initial support for multiple distributions. #157

Closed fnordahl closed 1 year ago

fnordahl commented 1 year ago

Please review commit by commit.

Signed-off-by: Frode Nordahl frode.nordahl@canonical.com

igsilya commented 1 year ago

Hi @fnordahl . Thanks for the PR! I didn't go too deep into review, but one thing that is needed here, I think, is CI. I spent some time with cirrus and the following change seems to work (ubuntu images have less available disk space for some reason):

diff --git a/.cirrus.yml b/.cirrus.yml
index f24475b..69602d7 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -1,14 +1,19 @@
 low_scale_task:

   compute_engine_instance:
-    image_project: fedora-cloud
-    image: family/fedora-cloud-35
+    matrix:
+      - image_project: fedora-cloud
+        image: family/fedora-cloud-35
+      - image_project: ubuntu-os-cloud
+        image: family/ubuntu-2204-lts
     platform: linux
     memory: 8G
+    disk: 20

   env:
-    DEPENDENCIES: git ansible ansible-collection-ansible-posix
-                  podman podman-docker
+    DEPENDENCIES: git ansible
+    FEDORA_DEP: podman podman-docker ansible-collection-ansible-posix
+    UBUNTU_DEP: docker.io
     PHYS_DEPLOYMENT: ${CIRRUS_WORKING_DIR}/physical-deployments/ci.yml

   runtime_cache:
@@ -23,7 +28,12 @@ low_scale_task:
     - ssh root@$(hostname) -v echo Hello

   install_dependencies_script:
-    - dnf install -y ${DEPENDENCIES}
+    - 'if [ $(which dnf) ]; then
+        dnf install -y ${DEPENDENCIES} ${FEDORA_DEP};
+       fi'
+    - 'if [ $(which apt) ]; then
+        apt update && apt install -y ${DEPENDENCIES} ${UBUNTU_DEP};
+       fi'

   unpack_caches_script:
     - mkdir -p runtime runtime-cache

Would be great if you can include something like that, as missing dependencies are hard to track in ovn-heater.

On the other note, your current implementation seem to require docker, while for rpm-based distros we allow using podman as well. Do you think it makes sense to allow podman for debian-based systems as well? We also had a lot of issues with docker in the past (mainly a requirement to be logged in and some unfortunate deamon hangs), so the question of supporting only podman and run s/docker/podman/ on a codebase was brought up several times in the past. Do you think it makes sense to support only podman? Most of major distributions seem to package it (I didn't check debian).

Also, if I install podman in the .cirrus.yml for ubuntu I'm getting errors later from ./do.sh install, likely because podman is getting replaced by docker.io in the process and something goes wrong:

TASK [Pull latest containers] **************************************************
fatal: [cirrus-task-6259513473368064]: FAILED! => {"changed": true, "cmd": "set -e\ndocker pull cirrus-task-6259513473368064:5000/ovn/ovn-multi-node\ndocker tag cirrus-task-6259513473368064:5000/ovn/ovn-multi-node ovn/ovn-multi-node\n", "delta": "0:00:00.050686", "end": "2023-03-10 11:56:42.841776", "msg": "non-zero return code", "rc": 1, "start": "2023-03-10 11:56:42.791090", "stderr": "Error response from daemon: Get \"https://cirrus-task-6259513473368064:5000/v2/\": http: server gave HTTP response to HTTPS client", "stderr_lines": ["Error response from daemon: Get \"https://cirrus-task-6259513473368064:5000/v2/\": http: server gave HTTP response to HTTPS client"], "stdout": "Using default tag: latest", "stdout_lines": ["Using default tag: latest"]}
fnordahl commented 1 year ago

Thank you for the review @igsilya and thanks a lot for contributing the Cirrus CI recipe, I included it verbatim in the PR with appropriate attribution etc. Making use of the podman and podman-docker packages appear to work fine, so happy to go with that, change incorporated in the updated version of the PR.

Cheers!

dceara commented 1 year ago

Thank you for the review @igsilya and thanks a lot for contributing the Cirrus CI recipe, I included it verbatim in the PR with appropriate attribution etc.

@fnordahl according to the contributing guidelines (which I just pushed last week :D) all co-authors must sign off (like in ovs/ovn). @igsilya are you ok with me or @fnordahl adding your Signed-off-by to the last commit?

igsilya commented 1 year ago

Thank you for the review @igsilya and thanks a lot for contributing the Cirrus CI recipe, I included it verbatim in the PR with appropriate attribution etc.

@fnordahl according to the contributing guidelines (which I just pushed last week :D) all co-authors must sign off (like in ovs/ovn). @igsilya are you ok with me or @fnordahl adding your Signed-off-by to the last commit?

Sure. Though, I'd move the podman and podman-docker to DEPENDENCIES and removed the UBUNTU_DEP for a time being since we don't really need it.

dceara commented 1 year ago

@igsilya Do you want to have another look at these changes before I merge the PR?

igsilya commented 1 year ago

@igsilya Do you want to have another look at these changes before I merge the PR?

Sure. I can take a look. I won't test though. :)

igsilya commented 1 year ago

I added 2 small comments. Otherwise LGTM.

dceara commented 1 year ago

Thanks a lot for the contribution @fnordahl and @igsilya for reviews! I also pushed a commit to add you to the authors list:

https://github.com/dceara/ovn-heater/commit/2e7eacb61c5f18e901a5e1b0f6bde2efc15c1100