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

System tests on k8s #793

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

System tests on k8s #793

wants to merge 20 commits into from

Conversation

rtuck99
Copy link
Contributor

@rtuck99 rtuck99 commented Feb 3, 2025

This addresses

The changes here mainly consist of changes to the unit tests to allow the ISPyB endpoint to be configured via the ISPYB_CONFIG_PATH during tests, which was previously overridden.

Also there are changes to allow sample ID, visit and suchlike to be configured as an interim so that it is still possible to run the system tests individually locally against the ispyb_hyperion_dev.cfg database.

The actual system test infrastructure can be found in the associated GitLab merge request

Copy link

codecov bot commented Feb 3, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.94%. Comparing base (cc5bae2) to head (40cc1c3).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #793   +/-   ##
=======================================
  Coverage   86.93%   86.94%           
=======================================
  Files         102      102           
  Lines        6967     6971    +4     
=======================================
+ Hits         6057     6061    +4     
  Misses        910      910           
Components Coverage Δ
i24 SSX 72.84% <ø> (ø)
hyperion 96.33% <ø> (ø)
other 96.62% <100.00%> (+<0.01%) ⬆️

@rtuck99 rtuck99 marked this pull request as ready for review February 4, 2025 11:32
@rtuck99 rtuck99 force-pushed the 633_system_tests_on_k8s branch 2 times, most recently from 6ed7f61 to b02c969 Compare February 6, 2025 11:30
@@ -165,7 +166,7 @@ legacy_tox_ini = """
[tox]
skipsdist=True

[testenv:{pre-commit,type-checking,tests,docs}]
[testenv:{pre-commit,type-checking,tests,docs,systemtests}]

Choose a reason for hiding this comment

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

Nit: We've been calling it system-test in other places, not really bothered but would be nice to be consistent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, dashes have a special meaning within tox:

https://tox.wiki/en/latest/user_guide.html

Test environment names can consist of alphanumeric characters and dashes; for example: py311-django42. The name will be split on dashes into multiple factors, meaning py311-django42 will be split into two factors: py311 and django42.

Copy link

@callumforrester callumforrester Feb 11, 2025

Choose a reason for hiding this comment

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

Hmm, should we also change type-checking and pre-commit then?

@olliesilvester olliesilvester changed the title 633 system tests on k8s System tests on k8s Feb 10, 2025
@olliesilvester olliesilvester self-assigned this Feb 10, 2025
Copy link
Contributor

@olliesilvester olliesilvester left a comment

Choose a reason for hiding this comment

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

A few comments, mostly questions, but looks good. I'd like to test them locally but I'm waiting on a SC ticket to get the gcr account

@@ -0,0 +1,20 @@
System Tests
Copy link
Contributor

Choose a reason for hiding this comment

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

If there are any system tests which we permanently want to leave in the option to locally run them, we should explain how to run them here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think long term the plan would be that rather than run locally directly against the ispyb dev database + expeye staging, we would run them locally against the test instances running in podman. Then we could get rid of the various ST_xxx config options.

This is what I'm currently doing, however there are a few minor wrinkles with podman that I have run into.
It's somewhat complicated:

  • There are 3 or 4 different ways of running compose files:
    • podman with podman-compose (what I'm currently using)
    • podman with docker-compose plugin (I think technically this is now the more actively developed solution)
    • docker with docker-compose standalone (Now deprecated, I didn't try this)
    • docker with docker-compose plugin (current docker solution)
    • I don't know if podman with docker-compose standalone works, I didn't try

However I seem to run into slightly different problems depending which of the above I'm using. Note that a lot of these issues are also dependent on how you are running podman/docker and whether they are running with root privileges. So it's doubly complicated as if I ran them on my laptop or at home instead of my workstation I'd probably get different results.

  • podman with podman-compose - I get caching issues with images not getting always getting pulled from gcr.io, you can work around by deleting them but this is annoying. However with the compose file I have (using the default bridging network), it doesn't expose the services to the host so you can't run the tests locally in the debugger. Unfortunately if you try host networking, then you run into other problems in that the containers can't talk to each other bc no hostnames.
  • podman with docker-cli - I ran into docker strangeness as it seems to use the docker local repository and also authentication. But also ran into similar but slightly different problems with networking. I think a lot of this is possibly due to not having root therefore it doesn't set up routing properly.
  • docker I didn't try but assume similar problems w/o root.

Bottom line is, for ideal local dev scenario, the test containers need to be routable from the outside so that you can run individual python test modules locally against the test service instances. The services also need to be routable to each other.
If you want to run the test container locally as well it needs to be able to reach the outside in order to pull from GH/pypi

The solution I have at the moment is to run in podman with the test instance via the bridging network but able to mount a local RO git repo, this is reasonable but to use it you have to shell in to the container.

It is definitely possible to get what I want with docker as I have pretty much exactly this setup on my home server, but there I have root and also the server is configured as a router. </end of rant>

@pytest.fixture(autouse=True)
def use_dev_ispyb_unless_overridden_by_environment():
ispyb_config_path = os.environ.get(
"ISPYB_CONFIG_PATH", CONST.SIM.DEV_ISPYB_DATABASE_CFG
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is ISPYB_CONFIG_PATH set? I can't find a corresponding name in the Dockerfiles

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't override it for the containerised system tests - the test credentials are mounted to the default place.

tests/unit_tests/conftest.py Show resolved Hide resolved
@rtuck99 rtuck99 force-pushed the 633_system_tests_on_k8s branch from b02c969 to 40cc1c3 Compare February 13, 2025 12: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