Mrc 5146 container gh actions #7

M-Kusumgar · 2024-08-07T14:55:20Z

This turned into a longer PR than I had initially hoped for. This PR builds docker and pushes it dockerhub and github container registry. Main changes include:

workflow for pushing
upgrading R version in the package because R versions less than 4.1.0 have problems with decoding url params
added host option in server main entrypoint
added worker main entrypoint that also creates their worker directory in .packit
REDIS_CONTAINER_NAME env var so that we can connect to redis running in another container
various scripts for testing this out fully in a docker setup, these are outlined in the README

M-Kusumgar · 2024-08-08T18:51:33Z

docker/Dockerfile

+RUN git config --global --add safe.directory /orderly-root
+RUN echo ".packit" > /.gitignore
+RUN git config --global core.excludesFile "/.gitignore"


same issue as in outpack_server PR, gotta add this as a safe directory as it is not owned by us in the container, also the global gitignore means that the .packit where the workers are isnt tracked by git

absternator

LGTM!!!! just some minor commnets..

Maybe the scripts can be one level higher above test? in a scripts folder for consitency

Also just a question... im gussiing in packet we just spin up the runner and worker right?

absternator · 2024-08-09T12:50:17Z

docker/test/run-test

+
+docker volume create orderly-root-volume
+
+docker run --rm -d --pull=missing \


Q: whats this one for?

that one is for a couple things:

you need a volume attached to a container to do anything with it so for us modifying the orderly repo, we could use this as the place to change things in the volume for testing

to view how things change in the working directory (since workers will be running things and producing result files and metadata of the run) theres a helper script docker/test/copy-orderly-root that will copy the folder so we can see it easily - that script uses this container too

absternator · 2024-08-09T12:51:46Z

docker/test/run-test

+
+docker network create orderly.runner_network
+
+docker run --rm -d \


also wondering if redis is needed here? or maybe another script that dosent... as this will all be brought up with packet (which already brings up redis), thus will get conflict

we do need redis here, but that's fine for testing this stuff. You will get a conflict if two containers bring up redis on the same port, but in practice it's not hard to deal with

absternator · 2024-08-09T12:59:34Z

docker/Dockerfile

@@ -17,3 +19,15 @@ RUN install2.r --error remotes &&  \

 COPY . /src
 RUN R CMD INSTALL --install-tests /src && rm -rf /src
+
+ENV ORDERLY_RUNNER_QUEUE_ID=orderly.runner
+ENV REDIS_CONTAINER_NAME=orderly.runner_redis


will this come from packet? or will this have another redis container?

packit does not need a redis container, after addressing rich's comments, this will come from env args in docker run and so these variables will come from the packit deploy tool which makes sense

absternator · 2024-08-09T13:00:25Z

docker/Dockerfile

@@ -17,3 +19,15 @@ RUN install2.r --error remotes &&  \

 COPY . /src
 RUN R CMD INSTALL --install-tests /src && rm -rf /src
+
+ENV ORDERLY_RUNNER_QUEUE_ID=orderly.runner


a more informative name just to be safe ahha eg. orderly.runner.queueId

gone with orderly.runner.queue for casing consistency

absternator

LGTM!!!! just some minor commnets..

Maybe the scripts can be one level higher above test? in a scripts folder for consitency

Also just a question... im gussiing in packet we just spin up the runner and worker right?

richfitz · 2024-08-09T19:31:30Z

docker/Dockerfile


 RUN apt-get update &&  apt-get install -y --no-install-recommends \
        libcurl4-openssl-dev \
        libssl-dev \
        zlib1g-dev \
+        libhiredis-dev \
+        git \


can yo sort these alphabetically please?

richfitz · 2024-08-09T19:31:55Z

docker/Dockerfile

@@ -17,3 +19,15 @@ RUN install2.r --error remotes &&  \

 COPY . /src
 RUN R CMD INSTALL --install-tests /src && rm -rf /src
+
+ENV ORDERLY_RUNNER_QUEUE_ID=orderly.runner
+ENV REDIS_CONTAINER_NAME=orderly.runner_redis


I don't think that this should be a default envvar in the image

richfitz · 2024-08-09T19:32:57Z

docker/test/clear-test

+set -x
+HERE=$(dirname $0)
+
+docker kill \


Use docker rm -f and you can avoid the repetition here

richfitz · 2024-08-09T19:33:53Z

docker/test/run-test

+
+docker network create orderly.runner_network
+
+docker run --rm -d \


we do need redis here, but that's fine for testing this stuff. You will get a conflict if two containers bring up redis on the same port, but in practice it's not hard to deal with

richfitz · 2024-08-09T19:35:14Z

docker/test/run-test

+docker run --rm -d --pull=always \
+       --net=orderly.runner_network \
+       --name=orderly.runner_server \
+       --entrypoint="/usr/local/bin/orderly.runner.server" \


Suggested change

--entrypoint="/usr/local/bin/orderly.runner.server" \

--entrypoint="/usr/local/bin/orderly.runner.server" \

--env=REDIS_CONTAINER_NAME=orderly.runner_redis \

etc

richfitz · 2024-08-09T19:35:34Z

docker/test/run-test

@@ -0,0 +1,41 @@
+#!/usr/bin/env bash
+set -ex


Suggested change

set -ex

set -exu

Everywhere you want -e you usually want -u

M-Kusumgar · 2024-08-12T13:38:49Z

I have addressed the comments but also done a bit of cleanup, all ive done that is extra is extract out certain variables you may want to change like container names, after approval I will also change it to pull image from main and merge it in so that we arent always pulling from this random branch. I will also remove triggers to the build and push workflow from this branch.

Also I remember this ticket that I had to do involving replacing _ with - in docker container names, ive forgotten why I had to do it but to be on the safe side, the naming convention is now orderly.runner-[container] instead of orderly.runner_[container]

richfitz · 2024-08-16T11:18:31Z

R/main.R

+main_worker <- function(args = commandArgs(TRUE)) {
+  dat <- parse_main_worker(args)
+
+  # assumes ORDERLY_RUNNER_QUEUE_ID is set


Does this error informatively if not set?

richfitz · 2024-08-16T11:20:51Z

R/queue.R

      # Create queue
      self$controller <- rrq::rrq_controller(
-        queue_id %||% orderly_queue_id()
+        queue_id %||% orderly_queue_id(),


Suggest erroring here or orderly_queue_id() here if not set? but we could explore that in another PR if you prefer?

i think we should error in the main_worker function because this is okay if we havent set the queue id, in fact we use that behaviour a lot in the tests, have made a ticket: https://mrc-ide.myjetbrains.com/youtrack/issue/mrc-5663/orderly.runner-worker-error-if-ORDERLYRUNNERQUEUEID-not-set

add entrypoints + build and push gh actions workflow

9012df6

M-Kusumgar marked this pull request as draft August 7, 2024 14:55

actually reference dockerfile

2e09553

M-Kusumgar marked this pull request as ready for review August 7, 2024 15:00

M-Kusumgar changed the base branch from main to mrc-5585-submit-endpoint August 7, 2024 15:01

M-Kusumgar added 20 commits August 7, 2024 16:16

install rrq, orderly2 and porcelain

07a403a

add run on push to this branch

c549df9

try install hiredis too

8742205

install libhiredis-dev

cfef943

clean up + login to ghcr

7428c9c

chmod entrypoint scripts

5d8789c

install git in dockerfile

131e8a5

install redis for debugging on image

ed80e47

redis name via env var for containers

ca53041

use worker loop to keep docker container alive

558345a

remove unnecessary scripts and setup worker dir

8228277

include test with containers

3572288

better clear script

e492630

have a minimum version for R and update docker build version

8624c64

update description file to contain stringr

975a562

try r 4.2

85ca014

try r 4.1

fe662f7

use volume for orderly root + git config

99a382a

more scripts

e96804b

tests

27a99ce

M-Kusumgar changed the base branch from mrc-5585-submit-endpoint to main August 8, 2024 18:44

add readme section on testing

bfb02ef

M-Kusumgar changed the base branch from main to mrc-5585-submit-endpoint August 8, 2024 18:49

M-Kusumgar commented Aug 8, 2024

View reviewed changes

M-Kusumgar requested a review from richfitz August 8, 2024 18:58

M-Kusumgar requested a review from absternator August 8, 2024 18:58

absternator approved these changes Aug 9, 2024

View reviewed changes

absternator reviewed Aug 9, 2024

View reviewed changes

richfitz requested changes Aug 9, 2024

View reviewed changes

review comments + general cleanup

ef01fa0

M-Kusumgar requested a review from richfitz August 12, 2024 13:35

consistent variable

812037a

richfitz approved these changes Aug 16, 2024

View reviewed changes

M-Kusumgar changed the base branch from mrc-5585-submit-endpoint to main August 16, 2024 12:17

M-Kusumgar added 2 commits August 16, 2024 13:17

empty for ci

e08697a

replace branchname with main for ci

59856ad

M-Kusumgar merged commit 9985f51 into main Aug 16, 2024
8 checks passed

richfitz mentioned this pull request Aug 22, 2024

Jidea-60 Daedalus deploy tool jameel-institute/daedalus-deploy#1

Closed

M-Kusumgar mentioned this pull request Jan 15, 2025

Fix the reported modification time of reports. #16

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mrc 5146 container gh actions #7

Mrc 5146 container gh actions #7

M-Kusumgar commented Aug 7, 2024 •

edited

Loading

M-Kusumgar Aug 8, 2024

absternator left a comment

absternator Aug 9, 2024

M-Kusumgar Aug 12, 2024 •

edited

Loading

absternator Aug 9, 2024

richfitz Aug 9, 2024

absternator Aug 9, 2024

M-Kusumgar Aug 12, 2024

absternator Aug 9, 2024

M-Kusumgar Aug 12, 2024

absternator left a comment

richfitz Aug 9, 2024

M-Kusumgar Aug 12, 2024

richfitz Aug 9, 2024

M-Kusumgar Aug 12, 2024

richfitz Aug 9, 2024

richfitz Aug 9, 2024

richfitz Aug 9, 2024

M-Kusumgar Aug 12, 2024

richfitz Aug 9, 2024

M-Kusumgar Aug 12, 2024

M-Kusumgar commented Aug 12, 2024 •

edited

Loading

richfitz Aug 16, 2024

richfitz Aug 16, 2024

M-Kusumgar Aug 16, 2024


		docker volume create orderly-root-volume

		docker run --rm -d --pull=missing \


		docker network create orderly.runner_network

		docker run --rm -d \

	--entrypoint="/usr/local/bin/orderly.runner.server" \
	--entrypoint="/usr/local/bin/orderly.runner.server" \
	--env=REDIS_CONTAINER_NAME=orderly.runner_redis \

Mrc 5146 container gh actions #7

Mrc 5146 container gh actions #7

Conversation

M-Kusumgar commented Aug 7, 2024 • edited Loading

Choose a reason for hiding this comment

absternator left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

M-Kusumgar Aug 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

absternator left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

M-Kusumgar commented Aug 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

M-Kusumgar commented Aug 7, 2024 •

edited

Loading

M-Kusumgar Aug 12, 2024 •

edited

Loading

M-Kusumgar commented Aug 12, 2024 •

edited

Loading