fix(ci): resolve systest failures on arm #8503

joshua-goldstein · 2022-12-11T20:29:54Z

Problem

Systest/export backup to minio was failing on arm64.

Solution

The minio image that we require was published before multi-architecture docker images were introduced. As such, there are two tags, namely RELEASE.2020-11-13T20-10-18Z (which we currently use for systest/export) and RELEASE.2020-11-13T20-10-18Z-arm64 (which is required for arm64). See the Dockerhub page here.

Unfortunately docker-compose files do not offer some sort of conditional logic. As such the easiest way to use this image is to create a new docker-compose-arm64.yml file and a flag that specifies when it should be uses at the t.go level. This is not so elegant but is the easiest solution currently since we lack a way to dynamically generate docker-compose files.

This PR also improves some testing functionality. In brief, all that is required to run tests now is to run make test from the root dgraph directory. If one wants to run specific package tests (e.g. systest/export) one can use make test args="--pkg=systest/export". What this does is pass the --pkg=systest/export flag to the t.go binary when it is run. This has the benefit that any number of arguments can be passed to t.go. E.g., in the process of testing this PR, I ran

make test args="--pkg=systest/export --arch=arm64"

which (after building the dgraph binary and making a dgraph docker image with local tag) builds t.go and executes ./t --pkg=systest/export --arch-arm64

Finally, we fix a logic error that was introduced recently in t.go. Coverage files should only be copied over when t.go is run in coverage mode.

t/t.go

coveralls · 2022-12-11T22:22:56Z

Coverage increased (+0.2%) to 64.089% when pulling 9c9efed on joshua/arm-fix into eae3c3a on main.

skrdgraph · 2022-12-12T07:23:32Z

This looks good 👍 - ill merge it tomorrow! I want a review by @anurags92 here as well ...

all-seeing-code

I have added my observations on the pull request. Let me know if you find them useful.

all-seeing-code · 2022-12-12T10:10:14Z

systest/backup/minio-large/docker-compose-arm64.yml

@@ -0,0 +1,97 @@
+# Auto-generated with: [./compose -a 3 -z 1 -r 1 --minio --minio_port=9001 --minio_env_file=../../backup.env -w --port_offset=0 --expose_ports=false --mem= --names=false -O ../systest/backup/minio-large/docker-compose.yml]


I don't think this is true anymore unless the compose.go produces ${COVERAGE_OUTPUT} as well in the command, which I doubt it would. Hence this should be removed to avoid misleading someone.

Agreed - it would be nice to eventually use compose.go but for now I modify this line

all-seeing-code · 2022-12-12T10:18:08Z

systest/backup/minio-large/docker-compose-arm64.yml

+      --security "whitelist=10.0.0.0/8,172.16.0.0/12,192.168.0.0/16;"
+      --tls "ca-cert=/dgraph-tls/ca.crt; server-cert=/dgraph-tls/node.crt; server-key=/dgraph-tls/node.key; internal-port=true; client-cert=/dgraph-tls/client.alpha3.crt; client-key=/dgraph-tls/client.alpha3.key;"
+  minio:
+    image: minio/minio:RELEASE.2020-11-13T20-10-18Z-arm64


How about passing in the image as an environment variable?

docker-compose.yml:

// Minio has two different images for arm and amd etc.. (descriptive comment in the docker yml) minio: image: ${MINIO_IMAGE_FOR_PLATFORM}

And in the t.go you can set the environment variable. Probably here: https://github.com/dgraph-io/dgraph/blob/035f9fc46221b3a465a42c291e3bef73d831d2cc/t/t.go#L100

This is a very nice solution... I wasn't sure if this was possible, so thank you for confirming it in your PR! I just updated this PR.

all-seeing-code · 2022-12-12T10:18:44Z

systest/backup/minio/docker-compose-arm64.yml

@@ -0,0 +1,97 @@
+# Auto-generated with: [./compose -a 3 -z 1 -r 1 --minio --minio_port=9001 --minio_env_file=../../backup.env -w --port_offset=0 --expose_ports=false --mem= --names=false -O ../systest/backup/minio/docker-compose.yml]


Same comment as above

all-seeing-code · 2022-12-12T10:18:58Z

systest/backup/minio/docker-compose-arm64.yml

+    command: /gobin/dgraph  ${COVERAGE_OUTPUT} zero --raft "idx=1;" --my=zero1:5080 --replicas=1 --logtostderr -v=2 --bindall
+      --tls "ca-cert=/dgraph-tls/ca.crt; server-cert=/dgraph-tls/node.crt; server-key=/dgraph-tls/node.key; internal-port=true; client-cert=/dgraph-tls/client.zero1.crt; client-key=/dgraph-tls/client.zero1.key;"
+  minio:
+    image: minio/minio:RELEASE.2020-11-13T20-10-18Z-arm64


Same comment as above

all-seeing-code · 2022-12-12T10:19:12Z

systest/export/docker-compose-arm64.yml

@@ -0,0 +1,94 @@
+# Auto-generated with: [./compose -l -a 3 -z 1 -r 1 --alpha_volume data:/data --minio --minio_port=9001 --minio_env_file=export.env --port_offset=0 --expose_ports=false --alpha_env_file=export.env --mem= --names=false -O ../systest/export/docker-compose.yml]


Same as above

all-seeing-code · 2022-12-12T10:19:26Z

systest/export/docker-compose-arm64.yml

+      -v=2 
+      --security "whitelist=10.0.0.0/8,172.16.0.0/12,192.168.0.0/16;"
+  minio:
+    image: minio/minio:RELEASE.2020-11-13T20-10-18Z-arm64


Same as above

t/Makefile

all-seeing-code · 2022-12-12T10:20:53Z

t/t.go

@@ -95,6 +95,7 @@ var (
 		"comma separated list of packages that needs to be skipped. "+
 			"Package Check uses string.Contains(). Please check the flag carefully")
 	runCoverage = pflag.Bool("coverage", false, "Set true to calculate test coverage")
+	arch        = pflag.String("arch", "x86", "Machine architecture (required for systest/export test)")


Add comment on which values it accepts. eg. Valid values: [arm, x86 etc.]

t/t.go

all-seeing-code · 2022-12-12T11:55:38Z

Wanted to test if the changes I proposed would work: #8508. cc: @joshua-goldstein @skrdgraph

joshua-goldstein · 2022-12-12T16:33:24Z

Wanted to test if the changes I proposed would work: #8508. cc: @joshua-goldstein @skrdgraph

I was also wondering if the environment variable would get substituted at the appropriate time... Looks like it works fine. Thanks for the idea and testing it! I've updated this PR.

all-seeing-code · 2022-12-12T16:52:56Z

@joshua-goldstein we can remove architecture flag and let the t.go figure it out itself. Much cleaner. Also can we log somewhere which architecture we are running tests on, so that we are sure that our changes flow through?

joshua-goldstein · 2022-12-12T17:01:16Z

@joshua-goldstein we can remove architecture flag and let the t.go figure it out itself. Much cleaner. Also can we log somewhere which architecture we are running tests on, so that we are sure that our changes flow through?

So sometime like a runtime.GOARCH check? We can also do that easily.

all-seeing-code · 2022-12-12T17:21:43Z

Another small comment which came up during the code review call, we can define default values for docker variables used in yml files. See: https://stackoverflow.com/questions/70772517/docker-compose-env-default-with-fallback

Should we use that and insert a different value only for ARM?

(Apologies for a separate comment, should have added this earlier but forgot).

t/Makefile

joshua-goldstein · 2022-12-12T18:48:08Z

Another small comment which came up during the code review call, we can define default values for docker variables used in yml files. See: https://stackoverflow.com/questions/70772517/docker-compose-env-default-with-fallback

Should we use that and insert a different value only for ARM?

(Apologies for a separate comment, should have added this earlier but forgot).

Also a good catch - what I did was add a default value now, so if the environment variable is not set it will use the previously used tag (see docs here).

all-seeing-code

Thanks Joshua! Looks good to me.

## Problem Systest/export backup to minio was failing on arm64. ## Solution The minio image that we require was published before multi-architecture docker images were introduced. As such, there are two tags, namely `RELEASE.2020-11-13T20-10-18Z` (which we currently use for systest/export) and `RELEASE.2020-11-13T20-10-18Z-arm64` (which is required for arm64). See the Dockerhub page [here](https://hub.docker.com/r/minio/minio/tags?page=1&name=RELEASE.2020-11-13T20-10-18Z). Unfortunately docker-compose files do not offer some sort of conditional logic. As such the easiest way to use this image is to create a new docker-compose-arm64.yml file and a flag that specifies when it should be uses at the t.go level. This is not so elegant but is the easiest solution currently since we lack a way to dynamically generate docker-compose files. This PR also improves some testing functionality. In brief, all that is required to run tests now is to run `make test` from the root dgraph directory. If one wants to run specific package tests (e.g. systest/export) one can use `make test args="--pkg=systest/export"`. What this does is pass the `--pkg=systest/export` flag to the t.go binary when it is run. This has the benefit that any number of arguments can be passed to t.go. E.g., in the process of testing this PR, I ran ``` make test args="--pkg=systest/export --arch=arm64" ``` which (after building the dgraph binary and making a dgraph docker image with local tag) builds t.go and executes `./t --pkg=systest/export --arch-arm64` Finally, we fix a logic error that was introduced recently in t.go. Coverage files should only be copied over when t.go is run in coverage mode.

github-actions bot added area/integrations Related to integrations with other projects. area/testing Testing related issues labels Dec 11, 2022

joshua-goldstein marked this pull request as ready for review December 11, 2022 20:36

joshua-goldstein requested review from akon-dey, gcxml, skrdgraph, darkn3rd, meghalims, matthewmcneely and billprovince as code owners December 11, 2022 20:36

joshua-goldstein changed the title ~~fix(ci): resolve systest failure on arm + improve testing experience~~ fix(ci): resolve systest failure on arm Dec 11, 2022

joshua-goldstein commented Dec 11, 2022

View reviewed changes

t/t.go Show resolved Hide resolved

joshua-goldstein self-assigned this Dec 11, 2022

joshua-goldstein changed the title ~~fix(ci): resolve systest failure on arm~~ fix(ci): resolve systest failures on arm Dec 12, 2022

joshua-goldstein requested a review from adityasadalage December 12, 2022 00:22

joshua-goldstein added 6 commits December 11, 2022 21:03

modify image in systest/export

adf5666

add arch flag

47301d5

make testing easier and arm work

531e3ae

fix logic error:

38a9f13

only remove coverage files in cover mode

380f3be

add custom dirs

567acb6

joshua-goldstein force-pushed the joshua/arm-fix branch from d258c24 to 567acb6 Compare December 12, 2022 03:05

skrdgraph approved these changes Dec 12, 2022

View reviewed changes

all-seeing-code reviewed Dec 12, 2022

View reviewed changes

all-seeing-code mentioned this pull request Dec 12, 2022

Use env variable instead for minio release images. #8508

Closed

joshua-goldstein added 2 commits December 12, 2022 10:18

update comment on dockerfile

b31968c

add comment in t.go

634db0c

joshua-goldstein added 3 commits December 12, 2022 10:28

add environment variable

e63d6af

remove extra docker-compose files

9b72c5a

remove compose file arm logic

3b175f6

reformat comment

9c9b5f7

joshua-goldstein added 3 commits December 12, 2022 11:06

remove arch flag, check at runtime

f494305

log architecture runtime

6488938

remove arch flag

7653085

all-seeing-code reviewed Dec 12, 2022

View reviewed changes

t/Makefile Show resolved Hide resolved

add default image tag

1a740bd

all-seeing-code self-requested a review December 12, 2022 19:26

all-seeing-code approved these changes Dec 12, 2022

View reviewed changes

remove superfluous conditional

9c9efed

skrdgraph merged commit bc71dc3 into main Dec 13, 2022

skrdgraph deleted the joshua/arm-fix branch December 13, 2022 00:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ci): resolve systest failures on arm #8503

fix(ci): resolve systest failures on arm #8503

joshua-goldstein commented Dec 11, 2022 •

edited

Loading

coveralls commented Dec 11, 2022 •

edited

Loading

skrdgraph commented Dec 12, 2022

all-seeing-code left a comment

all-seeing-code Dec 12, 2022

joshua-goldstein Dec 12, 2022

all-seeing-code Dec 12, 2022

joshua-goldstein Dec 12, 2022

all-seeing-code Dec 12, 2022

joshua-goldstein Dec 12, 2022

all-seeing-code Dec 12, 2022

joshua-goldstein Dec 12, 2022

all-seeing-code Dec 12, 2022

joshua-goldstein Dec 12, 2022

all-seeing-code Dec 12, 2022

joshua-goldstein Dec 12, 2022

all-seeing-code Dec 12, 2022

joshua-goldstein Dec 12, 2022

all-seeing-code commented Dec 12, 2022

joshua-goldstein commented Dec 12, 2022

all-seeing-code commented Dec 12, 2022

joshua-goldstein commented Dec 12, 2022

all-seeing-code commented Dec 12, 2022

joshua-goldstein commented Dec 12, 2022 •

edited

Loading

all-seeing-code left a comment

		@@ -0,0 +1,97 @@
		# Auto-generated with: [./compose -a 3 -z 1 -r 1 --minio --minio_port=9001 --minio_env_file=../../backup.env -w --port_offset=0 --expose_ports=false --mem= --names=false -O ../systest/backup/minio-large/docker-compose.yml]

		@@ -0,0 +1,94 @@
		# Auto-generated with: [./compose -l -a 3 -z 1 -r 1 --alpha_volume data:/data --minio --minio_port=9001 --minio_env_file=export.env --port_offset=0 --expose_ports=false --alpha_env_file=export.env --mem= --names=false -O ../systest/export/docker-compose.yml]

fix(ci): resolve systest failures on arm #8503

fix(ci): resolve systest failures on arm #8503

Conversation

joshua-goldstein commented Dec 11, 2022 • edited Loading

Problem

Solution

coveralls commented Dec 11, 2022 • edited Loading

skrdgraph commented Dec 12, 2022

all-seeing-code 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

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

all-seeing-code commented Dec 12, 2022

joshua-goldstein commented Dec 12, 2022

all-seeing-code commented Dec 12, 2022

joshua-goldstein commented Dec 12, 2022

all-seeing-code commented Dec 12, 2022

joshua-goldstein commented Dec 12, 2022 • edited Loading

all-seeing-code left a comment

Choose a reason for hiding this comment

joshua-goldstein commented Dec 11, 2022 •

edited

Loading

coveralls commented Dec 11, 2022 •

edited

Loading

joshua-goldstein commented Dec 12, 2022 •

edited

Loading