dandi / backups2datalad

Mirror Dandisets as git-annex repositories
MIT License
1 stars 0 forks source link

Do not use staging for tests #53

Closed jjnesbitt closed 1 month ago

jjnesbitt commented 1 month ago

Tests in test_commands.py are using the staging deployment as a way to create and unembargo dandisets. This is not supported and should not continue. A locally spun up dandi-archive instance with minio should be used instead.

cc @yarikoptic @jwodder

jwodder commented 1 month ago

@jjnesbitt May I ask why exactly this is a problem? We've been testing this program this way for several years now.

jjnesbitt commented 1 month ago

The staging instance is meant as a "sandbox" environment for real users, not for automated tests. As it is a "production" instance (although we don't use it as our production instance), using it in this way should be avoided as it wastes resources, costs money, pollutes the staging instance with test data, etc.

What's the reason for using staging as opposed to a local environment? Wouldn't using a locally spun up docker environment be better all around for matters of reproducibility, test speed, etc.?

jwodder commented 1 month ago

@jjnesbitt

What's the reason for using staging as opposed to a local environment?

The biggest reason is that the script directly queries the S3 bucket used by the DANDI Archive instance it's operating on, and so a queryable S3 instance is needed during testing, and at the moment I'm not sure how to point aiobotocore at a minio instance instead.

jjnesbitt commented 1 month ago

The biggest reason is that the script directly queries the S3 bucket used by the DANDI Archive instance it's operating on, and so a queryable S3 instance is needed during testing, and at the moment I'm not sure how to point aiobotocore at a minio instance instead.

We point boto3 to minio for dev and testing in dandi-archive: https://github.com/dandi/dandi-archive/blob/38f2d7522ae17e820ae8e2da61fd0fa03bf59ac6/dandiapi/api/storage.py#L346-L353

It seems that aiobotocore has similar options available to create_client for example: https://github.com/aio-libs/aiobotocore/blob/7154abc9ff395af13b8cfa7b64a07a6bf7d1c07e/aiobotocore/session.py#L121-L133

So you may just be able to set endpoint_url=localhost:9000 (or whatever you choose for minio, that's the default).

jwodder commented 1 month ago

I've switched the tests to use a Dockerized Archive instance, but the tests are unable to download files from minio because Configuration of annex.security.allowed-ip-addresses does not allow accessing address ::1.

@yarikoptic What's the best way/place to set this option? Should the tests just set this via the GIT_CONFIG_PARAMETERS envvar?

yarikoptic commented 1 month ago

let's create/use temporary HOME during tests as e.g. was done in datalad: https://github.com/datalad/datalad/blob/maint/datalad/conftest.py#L86

jwodder commented 1 month ago

New problem: The code tries to get blobs' version IDs by making a HEAD request to the S3 instance and extracting the x-amz-version-id header from the response, but minio does not return such a header. (It does return an x-amz-request-id header, though, so it's at least emulating some of the S3 headers.)

@jjnesbitt Is versioning enabled in the minio container? If not, how can I enable it? If it is enabled, why isn't this header being returned?

jjnesbitt commented 1 month ago

@jjnesbitt Is versioning enabled in the minio container? If not, how can I enable it? If it is enabled, why isn't this header being returned?

It is not, although it seems there is a way to enable it. I'm tinkering with it right now. Do you just pull the docker compose config from dandi-archive, or set up your own config?

jjnesbitt commented 1 month ago

It seems it's possible to do with docker compose the following way:

  minio:
    image: minio/minio:latest
    # When run with a TTY, minio prints credentials on startup
    tty: true
    command: ["server", "/data", "--console-address", ":${DOCKER_MINIO_CONSOLE_PORT-9001}"]
    environment:
      MINIO_ROOT_USER: minioAccessKey
      MINIO_ROOT_PASSWORD: minioSecretKey
    ports:
      - ${DOCKER_MINIO_PORT-9000}:9000
      - ${DOCKER_MINIO_CONSOLE_PORT-9001}:9001
    healthcheck:
      test: ["CMD", "mc", "ready", "local"]
      interval: 1s
      timeout: 10s
      retries: 10

  # Create dev and test buckets
  createbuckets:
    image: minio/mc:latest
    environment:
      MINIO_ACCESS_KEY: minioAccessKey
      MINIO_SECRET_KEY: minioSecretKey
    depends_on:
      minio:
        condition: service_healthy
    entrypoint: >
      /bin/sh -c "
      /usr/bin/mc alias set myminio http://minio:9000 minioAccessKey minioSecretKey;
      /usr/bin/mc mb myminio/test-dandi-dandisets --with-versioning;
      /usr/bin/mc version enable myminio/test-dandi-dandisets;
      exit 0;
      "

This createbuckets service creates the test bucket and ensures the versioning on it is enabled. Would this solve your current problem?

jwodder commented 1 month ago

@jjnesbitt

jwodder commented 1 month ago

@jjnesbitt I tried running createbuckets both before and after the ./migrate commands, and the x-amz-version-id header is still absent (though there is now an x-amz-id-2 header). See commit 08c342f for exactly what I did.

jjnesbitt commented 1 month ago

Hmm, that's odd. What script/test are you running that fails to see that header? Running minio locally, I can upload an object and see an x-amz-version-id header returned when I call head_object on it.

jwodder commented 1 month ago

@jjnesbitt The failure happens during this test while running this code. The lead-up to the code is very in-depth and I don't know how much it matters.

jwodder commented 1 month ago

Current status: All tests pass, except for test_backup_zarr_entry_conflicts, which is stuck in a "retry upload" loop due to a Zarr checksum mismatch seemingly caused by versioned minio buckets not having decent handling of a key that is deleted and then used as "directory" for a later key. I have created an MVCE at https://github.com/jwodder/archive-minio-bug-20240724, and @jjnesbitt is looking into the problem.

jjnesbitt commented 1 month ago

~I've filed a bug report for Minio upstream: https://github.com/minio/minio/issues/20167~ Apparently this is expected behavior from minio https://min.io/docs/minio/kubernetes/upstream/operations/concepts/thresholds.html#id6.