Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor test framework to support docker tests #750

Merged
merged 31 commits into from
Dec 3, 2021
Merged

refactor test framework to support docker tests #750

merged 31 commits into from
Dec 3, 2021

Conversation

housengw
Copy link
Contributor

@housengw housengw commented Nov 14, 2021

major changes:

  • getExecCommand() returns a list of ProcessBuilders instead of one ProcessBuilder.
  • execute() loops through the list of ProcessBuilders and marks the test as failed if any of the ProcessBuilders returns a non-zero exit code or times out.
  • add a special case in getExecCommand() before the target switch statement to make a list of docker related test commands

Making execute() loop through a list of ProcessBuilders allows more flexible testing.
Currently, in order to run a test that spans multiple commands (preprocessing, processing, cleanup), a script needs to be created and ran.

@housengw
Copy link
Contributor Author

Seems like docker is not natively supported on MacOS for Github Action.
https://github.521000.bestmunity/t/why-is-docker-not-installed-on-macos/17017

Copy link
Contributor

@Soroosh129 Soroosh129 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! I left some suggestions. I wonder if we could just go for testing the federated docker support using Docker Compose. If that is complicated, I would advocate for merging this in and revisiting this issue later.

It would be great if @lhstrh could also comment on the changes made to the test framework.

@Soroosh129
Copy link
Contributor

Soroosh129 commented Nov 22, 2021

I have not tested this myself, but apparently, this workaround exists to get MacOS docker support working on GitHub Actions. What do you think?

Also, have you by any chance tried enabling Windows for non-federated tests? Apparently, Docker is fully supported on Windows on GitHub Actions. Maybe it's worth giving it a try. It could help us be more confident to deploy/publish a non-federated C/Python LF application as a Docker image publicly.

@housengw
Copy link
Contributor Author

Ok. Running Linux containers on Windows seems to require running them inside a Linux VM, as per: https://docs.microsoft.com/en-us/virtualization/windowscontainers/deploy-containers/linux-containers

So I assume it suffices that we test the docker option in the Linux VM? What do you think @Soroosh129 ?

@lhstrh
Copy link
Member

lhstrh commented Nov 23, 2021

Ok. Running Linux containers on Windows seems to require running them inside a Linux VM, as per: https://docs.microsoft.com/en-us/virtualization/windowscontainers/deploy-containers/linux-containers

So I assume it suffices that we test the docker option in the Linux VM? What do you think @Soroosh129 ?

I would say that suffices. If it runs in a VM anyway, we'd essentially just be testing the VM stack.

@lhstrh
Copy link
Member

lhstrh commented Nov 23, 2021

I'll have a closer look at this PR tomorrow.

@Soroosh129
Copy link
Contributor

Ok. Running Linux containers on Windows seems to require running them inside a Linux VM, as per: https://docs.microsoft.com/en-us/virtualization/windowscontainers/deploy-containers/linux-containers

So I assume it suffices that we test the docker option in the Linux VM? What do you think @Soroosh129 ?

Interesting find. Thanks for digging into this. Based on this article and reading about how Linux-based Docker images work on MacOS (i.e., using linuxkit as a Linux VM), I agree with @lhstrh that testing on Linux seems to suffice. I think it was essential to find the limitations of what Docker can and cannot do for us. Thanks anyway :)

@housengw
Copy link
Contributor Author

housengw commented Nov 23, 2021

according to here and here, seems like python wheels do not work well with musl, which is the libc version alpine linux uses. Wheels of common packages (ex. numpy) are usually compiled for glibc. I suspect this is the reason the CI was failing for the python docker tests, although I was able to build and run the python images locally.

As a temporary fix, I have changed the python docker base image from python:alpine to python:latest, which is based on Debian. This drastically increase the size of the built image from 177MB to around 900MB.

At first glance, this image size looks pretty bad. However, docker containers that use the same base image do not make a copy of that image, so building multiple containerized python LF programs only results in a one-time overhead of 900MB. I was able to verify the disk usage using docker system df according to here

@Soroosh129
Copy link
Contributor

As a temporary fix, I have changed the python docker base image from python:alpine to python:latest, which is based on Debian. This drastically increase the size of the built image from 177MB to around 900MB.

To me, using python:latest is a perfectly reasonable choice since space-savings is not as a big deal for the Python target as it is for the C target.

With that said, I was able to produce an image of size 446 MB using python:slim and adding RUN apt-get update && apt-get install -y python3-pip. In theory, pip is all you need here since setup tools is able to automatically install any required packages using pip as long as they are listed in the install_requires field in the setup.py. We don't currently have a way for the user to add to this list, but that can be straightforward to add as a target property. Do you foresee issues if we use python:slim?

@housengw
Copy link
Contributor Author

sounds good. I think using python:slim is fine.

@housengw housengw requested a review from lhstrh November 30, 2021 06:56
Copy link
Contributor

@Soroosh129 Soroosh129 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

I still think that @lhstrh should perhaps have a look at the changes before merging in, since this PR makes modifications to the test framework.

Copy link
Member

@lhstrh lhstrh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I've made a couple suggestions here and there. It would be great if you could address them before merging this in.

Copy link
Member

@lhstrh lhstrh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀🚀🚀

@lhstrh lhstrh merged commit 850f048 into master Dec 3, 2021
@lhstrh lhstrh deleted the docker-test branch December 3, 2021 06:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants