Tests: Add integration tests for the "SNAPSHOT" strategy #33
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
amotl
force-pushed
the
amo/snapshot-testing
branch
4 times, most recently
from
145666c
to
21b0990
Compare
amotl
force-pushed
the
amo/snapshot-testing
branch
from
21b0990
to
ccfde54
Compare
amotl
force-pushed
the
amo/snapshot-testing
branch
3 times, most recently
from
25d2dcf
to
114f0e2
Compare
amotl
changed the title
amotl
commented
Jul 9, 2023
tests/testcontainers/minio.py
Outdated
Comment on lines
17
to
44
| class ExtendedMinioContainer(MinioContainer): | ||
| """ | ||
| A Testcontainer for MinIO with two properties. | ||
|
|
||
| - Use the `latest` OCI image. | ||
| - Provide convenience methods for getting the Docker-internal endpoint address. | ||
| """ | ||
|
|
||
| def __init__(self, *args, **kwargs): | ||
| # Use most recent stable release of MinIO. | ||
| image = "quay.io/minio/minio:latest" | ||
| kwargs.setdefault("image", image) | ||
|
|
||
| super().__init__(*args, **kwargs) | ||
|
|
||
| def get_real_host_ip(self): | ||
| """ | ||
| To let containers talk to each other, explicitly provide the real IP address | ||
| of the container. In corresponding jargon, it appears to be the "bridge IP". | ||
| """ | ||
| return self.get_docker_client().bridge_ip(self._container.id) | ||
|
|
||
| def get_real_host_address(self): | ||
| """ | ||
| Provide Docker-internal full endpoint address `<host>:<port>` of the service. | ||
| For example, `172.17.0.4:9000`. | ||
| """ | ||
| return f"{self.get_real_host_ip()}:{self.port_to_expose}" |
There was a problem hiding this comment.
TODO: Hand in get_real_host_ip() and get_real_host_address() as a patch to upstream testcontainers-python.
There was a problem hiding this comment.
port_to_expose is not standardized across container wrapper implementations. So, get_real_host_address() will not be able to go into a generic API module.
cratedb_retention/util/database.py
Outdated
Comment on lines
71
to
132
| # TODO: DROP REPOSITORY IF EXISTS | ||
| try: | ||
| sql = f"DROP REPOSITORY {name};" | ||
| self.run_sql(sql) | ||
| except ProgrammingError as ex: | ||
| if "RepositoryUnknownException" not in str(ex): | ||
| raise | ||
|
|
||
| # TODO: CREATE REPOSITORY IF NOT EXISTS | ||
| sql = f""" | ||
| CREATE REPOSITORY | ||
| {name} | ||
| TYPE | ||
| {typename} | ||
| WITH ( | ||
| protocol = 'http', | ||
| endpoint = '{endpoint}', | ||
| access_key = '{access_key}', | ||
| secret_key = '{secret_key}', | ||
| bucket = '{bucket}' | ||
| ); | ||
| """ | ||
| self.run_sql(sql) |
There was a problem hiding this comment.
I missed those IF [NOT] EXISTS modifiers on the REPOSITORY statements. Do you think it makes sense to hand in a request to crate/crate about adding them?
CREATE|DROP REPOSITORY IF [NOT] EXISTS/cc @hlcianfagna, @proddata, @seut
amotl
force-pushed
the
amo/snapshot-testing
branch
2 times, most recently
from
3658fc0
to
a750aae
Compare
amotl
force-pushed
the
amo/snapshot-testing
branch
from
a750aae
to
0ce851e
Compare
amotl
commented
Jul 9, 2023
Comment on lines
+314
to
+315
| @pytest.mark.skip(reason="CrateDB does not support custom endpoints for Azure Blob Storage") | ||
| def test_run_snapshot_azure_blob(caplog, store, database, sensor_readings, sensor_readings_snapshot_policy, azurite): |
There was a problem hiding this comment.
TODO: Verifying snapshotting here does not work, because CrateDB does not support custom endpoints for Azure Blob Storage yet. The "skip" will be removed as soon as support has landed.
amotl
force-pushed
the
amo/snapshot-testing
branch
from
0ce851e
to
0638e3f
Compare
Provide a MinIO service to the test suite, in order to verify snapshotting to a CrateDB repository on AWS S3.
Provide an Azurite service to the test suite, in order to verify snapshotting to a CrateDB repository on Microsoft Azure Blob Storage. Does not work yet, because CrateDB does not support custom endpoints for Azure Blob Storage.
Introspect the OCI container where CrateDB is running, in order to verify snapshotting to a CrateDB repository on the filesystem.
amotl
force-pushed
the
amo/snapshot-testing
branch
from
0638e3f
to
4f47196
Compare
hammerhead
approved these changes
Jul 10, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Introduction
The "SNAPSHOT" strategy has not been tested thoroughly yet.
Improvement
This patch provides additional infrastructure to the test suite, and validates the full roundtrip by creating a snapshot repository, running the snapshot procedure, and verifying that data has moved correctly.
Details
For verifying different repository types, different helper services and APIs are used.
Does not work yet, because CrateDB does not support custom endpoints for Azure Blob Storage.