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

e2e tests shared asset_directory can cause test failure. #3567

Closed
busma13 opened this issue Mar 13, 2024 · 2 comments
Closed

e2e tests shared asset_directory can cause test failure. #3567

busma13 opened this issue Mar 13, 2024 · 2 comments
Assignees
Labels

Comments

@busma13
Copy link
Contributor

busma13 commented Mar 13, 2024

Adding functionality to store assets in an external storage like S3 instead of in Elasticsearch (see: #3563) has surfaced a race condition in e2e tests.

  • simple-spec saves assets to the teraslice.assets_directory cache.
  • recovery-spec gets the array of asset IDs needed to create a jobSpec from the list of IDs in the teraslice.assets_directory cache.
  • Since jest runs the two spec files in parallel, simple-spec can be adding an asset while recovery-spec is reading all the assets in the directory.
  • The additional time required to save an asset in S3 slows the post('/assets') route enough to make obvious that an asset can be added to the local cache in teraslice.assets_directory, but not yet be stored in Elasticsearch.
  • If recovery-spec gets an ID that has just been added to the cache, it will add it to jobSpec.assets and then run validation on jobSpec. If the validation check for the presence of the asset happens before it has been saved to ES by simple-spec the test will fail with an error: Failure to get assets, caused by TSError: No assets found for "2909ec5fd38466cf6276cc14ede25096f1f34ee9"
@busma13 busma13 added the tests label Mar 13, 2024
@busma13 busma13 self-assigned this Mar 13, 2024
@busma13
Copy link
Contributor Author

busma13 commented Mar 13, 2024

There are a few ways that we may be able to resolve this issue:

  • Use jests test-sequencer to ensure that recovery-spec runs first.
  • Change how the jobSpec in recovery-spec gets the list of IDs for assets it needs.
  • Add a function to the teraslice-harness that will create a jobSpec that does not need to be modified by the test?

@busma13
Copy link
Contributor Author

busma13 commented Mar 14, 2024

I decided the best approach was to add a function to the teraslice-harness that returns an array of IDs belonging to the "base assets" that the e2e tests autoload. recover-spec calls this function and assigns the return value to jobSpec.assets, ensuring that job validation only checks the assets that were autoloaded.

godber pushed a commit that referenced this issue Mar 28, 2024
…ec (#3568)

This PR makes the following changes:
- export the array of e2e autoloaded assets
- Use the array of autoloaded assets in
`TerasliceHarness.getBaseAssetIds()` to get the IDs of autoloaded assets
- Use `TerasliceHarness.getBaseAssetIds()` in `recovery-spec` to set the
`jobSpec.assets` array to only the autoloaded assets.

These changes prevent the `recovery-spec` from trying to use assets
placed in the `teraslice.assets_directory` by other tests running in
parallel, that have not finished uploading to ES.
Ref: #3567
@busma13 busma13 closed this as completed Apr 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant