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

Stop using ec2 runners in the conda+linux workflow #1302

Merged
merged 4 commits into from
Feb 14, 2024
Merged

Stop using ec2 runners in the conda+linux workflow #1302

merged 4 commits into from
Feb 14, 2024

Conversation

poodlewars
Copy link
Collaborator

@poodlewars poodlewars commented Feb 5, 2024

Reference Issues/PRs

What does this implement or fix?

@poodlewars

Stop using the ec2 runners for the conda build and return to ubuntu-latest.

@joe-iddon

Also improve (fix) the mongo storage test fixture to be able to fall back on a locally managed server using the mongodb-org package. This only occurs after trying the CI_MONGO_HOST env var, and localhost addresses first, since we'd usually expect the service which is defined using:

    services:
      mongodb:
        image: mongo:4.4

to be running on one of these. But sometimes it is not yet running, so we must have mongodb-org installed locally to fallback on.

Any other comments?

  • On master at the moment with ec2 runners, the Build and Test workflow takes 1h 4m and the Build with conda workflow is taking taking 50m, but limited by the conda+mac workflow. This branch is currently taking 58m for the conda+linux worfklow, which is increased from the 33m with ec2, but still less than the conda+mac workflow (which is unchanged by this PR).
  • I found that the conda+mac workflow will fail (hits 6h timeout) if run without the -n parallelism option. This is because the S3 storage fixture receives too many requests. With the usual 4 workers, it works fine. I'm just noting that here; I've not made any changes to this workflow.
  • I've added the -v option to pytest, so if there's a failing test we can see quickly rather than having to re-run with this option.
  • In debugging this, I wrote a standalone program to test our mongo test server fixture. It is available on this branch if somebody needs to debug this fixture again.

Checklist

Checklist for code changes...
  • Have you updated the relevant docstrings, documentation and copyright notice?
  • Is this contribution tested against all ArcticDB's features?
  • Do all exceptions introduced raise appropriate error messages?
  • Are API changes highlighted in the PR description?
  • Is the PR labelled as enhancement or bug so it appears in autogenerated release notes?

@joe-iddon joe-iddon self-assigned this Feb 13, 2024
@joe-iddon joe-iddon changed the title Do not use ec2 Stop using ec2 runners in the conda+linux workflow Feb 13, 2024
@joe-iddon joe-iddon marked this pull request as ready for review February 13, 2024 17:55
@@ -46,6 +50,7 @@ def __init__(self, mongo_uri: str, name: Optional[str] = None, client: Optional[
self.client = client or MongoClient(mongo_uri)
if not name:
while True:
logger.log(logging.INFO, "Searching for new name")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Minor but logger.info is more idiomatic isn't it? This API with the enum is for when you calculate the log level dynamically

Copy link
Collaborator

Choose a reason for hiding this comment

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

Understood, I'll change it

@@ -21,6 +22,9 @@
if TYPE_CHECKING:
from pymongo import MongoClient

logger = logging.getLogger("Mongo Storage Fixture")
logging.basicConfig(level=logging.DEBUG)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do we want this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think having informative log messages is useful, but I agree having a default call to basicConfig to actually enable them will be frustrating for other users of this server fixture.

Therefore I'd suggest to remove the basicConfig, and then for debugging the fixture users can import the log object and call basicConfig themselves if they would like to. This is how pytest-server-fixtures does it.

It's very useful for working on the CI as only the locally managed mongo server can be recreated locally, whereas the CI_MONGO_HOST one from the GitHub service would be harder to set up.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Makes sense. How about letting the log level be set with an env var so that it's easy to turn it on for CI debugging?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That makes sense. I've used ARCTICDB_mongo_test_fixture_loglevel as the env var.

I've also made all the fixture logs to DEBUG level, and configure the root logger to be INFO since that's the default for all our other logs.

I've manually tested that this works:

  • With INFO just get a few logs coming through from the locally managed mongo instance, but not from the fixture:
$ ARCTICDB_mongo_test_fixture_loglevel=INFO python -m pytest -vs tests/integration/arcticdb/test_arctic.py | head -n 20
============================= test session starts ==============================
[cut output]
tests/integration/arcticdb/test_arctic.py::test_library_creation_deletion[mongo-EncodingVersion.V1] 2024-02-14T11:34:57.088+0000 I  CONTROL  [main] Automatically disabling TLS 1.0, to force-enable TLS 1.0 specify --sslDisabledProtocols 'none'
2024-02-14T11:34:57.090+0000 W  ASIO     [main] No TransportLayer configured during NetworkInterface startup
[cut output]
  • With DEBUG you can see the test fixture logs:
$ ARCTICDB_mongo_test_fixture_loglevel=DEBUG python -m pytest -vs tests/integration/arcticdb/test_arctic.py | head -n 20
============================= test session starts ==============================
[ cut output]
tests/integration/arcticdb/test_arctic.py::test_library_creation_deletion[mongo-EncodingVersion.V1] DEBUG:Mongo Storage Fixture:Env var CI_MONGO_HOST not set, so will try localhost.
DEBUG:Mongo Storage Fixture:Could not connect to localhost, so falling back to managed instance.
About to run: ['mongod', '--port', '15099', '--dbpath', '/tmp/tmp10vn8sdrManagedMongoDBServer']
2024-02-14T11:41:24.576+0000 I  CONTROL  [main] Automatically disabling TLS 1.0, to force-enable TLS 1.0 specify --sslDisabledProtocols 'none'
2024-02-14T11:41:24.579+0000 W  ASIO     [main] No TransportLayer configured during NetworkInterface startup
[cut output]

@joe-iddon joe-iddon merged commit e554819 into master Feb 14, 2024
110 checks passed
@joe-iddon joe-iddon deleted the no-ec2 branch February 14, 2024 14:20
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