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

Fixture for Minio testing #31688

Merged
merged 8 commits into from
Jul 4, 2018
Merged

Fixture for Minio testing #31688

merged 8 commits into from
Jul 4, 2018

Conversation

ywelsch
Copy link
Contributor

@ywelsch ywelsch commented Jun 29, 2018

Adds a Minio fixture to run the S3 repository tests against Minio. I've also collapsed the single qa subproject into the s3-repository project, which simplifies the code structure (having it all in one place) and helps to avoid having too many Gradle subprojects.

@ywelsch ywelsch added >enhancement :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v7.0.0 v6.4.0 labels Jun 29, 2018
@ywelsch ywelsch requested a review from tlrx June 29, 2018 14:47
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

This is great! LGTM, just a couple suggestions.

String s3Bucket = System.getenv("amazon_s3_bucket")
String s3BasePath = System.getenv("amazon_s3_base_path")

if (!s3AccessKey && !s3SecretKey && !s3Bucket && !s3BasePath) {
Copy link
Member

Choose a reason for hiding this comment

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

I think if only some of these are set, this should be a configuration failure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in f14a5f7

logger.info("Shutting down minio with pid ${minioPid}")
}

final Object pid = "${ -> minioPid }"
Copy link
Member

Choose a reason for hiding this comment

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

Instead of depending on a global var, can you use a property on the startMinio task?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, I've fixed this in f8522c8 and also had to make this JDK8 compatible (the pid() method only exists from JDK9 on forward, but we still run gradle with JDK8 due to thirdPartyAudit checks).

Copy link
Member

@tlrx tlrx left a comment

Choose a reason for hiding this comment

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

I left a comment about credentials. I'm not a big fan of placing everything into a single gradle build file as it is already 400 lines and if we add more tests (AWS session tokens, client side encryption) it will grow accordingly.

Also, the test fails locally with java.net.UnknownHostException: bucket-test.localhost

minioAddress,
minioDataDir)
minio.addShutdownHook { minio.destroy() }
minio.environment().put('MINIO_ACCESS_KEY', s3AccessKey)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need to pass the S3 credentials here? We could use anything, just in case the process spits out the credentials in logs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed through 9ca7b52

@ywelsch
Copy link
Contributor Author

ywelsch commented Jul 2, 2018

@tlrx as we discussed and figured out, the failure you were experiencing on your machine was because it does not resolve requests to subdomains of localhost. While this seems to work OOTB on Mac and some flavors of Linux (e.g. my Ubuntu installation), this does not look to work on Debian 9. To fix this, I've pushed 05072a5, so that we don't use path-style access.

@ywelsch
Copy link
Contributor Author

ywelsch commented Jul 4, 2018

Thanks @rjernst and @tlrx

ywelsch added a commit that referenced this pull request Jul 4, 2018
Adds a Minio fixture to run the S3 repository tests against Minio. Also collapses the single qa
subproject into the s3-repository project, which simplifies the code structure (having it all in one
place) and helps to avoid having too many Gradle subprojects.
dnhatn added a commit that referenced this pull request Jul 4, 2018
* 6.x:
  Fix not waiting for Netty ThreadDeathWatcher in IT (#31758) (#31789)
  [Docs] Correct default window_size (#31582)
  S3 fixture should report 404 on unknown bucket (#31782)
  [ML] Limit ML filter items to 10K (#31731)
  Fixture for Minio testing (#31688)
  [ML] Return statistics about forecasts as part of the jobsstats and usage API (#31647)
  [DOCS] Add missing get mappings docs to HLRC (#31765)
  [DOCS] Starting Elasticsearch (#31701)
  Fix coerce validation_method in GeoBoundingBoxQueryBuilder (#31747)
  Painless: Complete Removal of Painless Type (#31699)
  Consolidate watcher setting update registration (#31762)
  [DOCS] Adds empty 6.3.1 release notes page
  ingest: Introduction of a bytes processor (#31733)
  [test] don't run bats tests for suse boxes (#31749)
  Add analyze API to high-level rest client (#31577)
  Implemented XContent serialisation for GetIndexResponse (#31675)
  [DOCS] Typos
  DOC: Add examples to the SQL docs (#31633)
  Add support for AWS session tokens (#30414)
  Watcher: Reenable start/stop yaml tests (#31754)
  JDBC: Fix stackoverflow on getObject and timestamp conversion (#31735)
  Support multiple system store types (#31650)
  Add write*Blob option to replace existing blob (#31729)
  Split CircuitBreaker-related tests (#31659)
  Painless: Add Context Docs (#31190)
  Docs: Remove missing reference
  Migrate scripted metric aggregation scripts to ScriptContext design (#30111)
  Watcher: Fix chain input toXcontent serialization (#31721)
  Remove _all example (#31711)
  rest-high-level: added get cluster settings (#31706)
  Docs: Match the examples in the description (#31710)
  [Docs] Correct typos (#31720)
  Extend allowed characters for grok field names (#21745) (#31653) (#31722)
  [DOCS] Check for Windows and *nix file paths (#31648)
  [ML] Validate ML filter_id (#31535)
  Fix gradle4.8 deprecation warnings (#31654)
  Update numbers to reflect 4-byte UTF-8-encoded characters (#27083)
dnhatn added a commit that referenced this pull request Jul 4, 2018
* master:
  [ML] Rate limit established model memory updates (#31768)
  [Docs] Correct default window_size (#31582)
  S3 fixture should report 404 on unknown bucket (#31782)
  Detach Transport from TransportService (#31727)
  [ML] Limit ML filter items to 10K (#31731)
  [ML] Return statistics about forecasts as part of the jobsstats and usage API (#31647)
  Fixture for Minio testing (#31688)
  [DOCS] Add missing get mappings docs to HLRC (#31765)
  [DOCS] Starting Elasticsearch (#31701)
  Painless: Complete Removal of Painless Type (#31699)
  Fix not waiting for Netty ThreadDeathWatcher in IT (#31758)
  Consolidate watcher setting update registration (#31762)
  Build: re-enabled bwc (#31769)
  ingest: Introduction of a bytes processor (#31733)
  Fix coerce validation_method in GeoBoundingBoxQueryBuilder (#31747)
  Add analyze API to high-level rest client (#31577)
  [DOCS] Typos
  DOC: Add examples to the SQL docs (#31633)
  Add support for AWS session tokens (#30414)
  Watcher: Reenable start/stop yaml tests (#31754)
  Implemented XContent serialisation for GetIndexResponse (#31675)
  JDBC: Fix stackoverflow on getObject and timestamp conversion (#31735)
  resolveHasher defaults to NOOP (#31723)
  Account for XContent overhead in in-flight breaker
  Split CircuitBreaker-related tests (#31659)
  Add write*Blob option to replace existing blob (#31729)
  Painless: Add Context Docs (#31190)
  Watcher: Fix chain input toXcontent serialization (#31721)
  Docs: Match the examples in the description (#31710)
  rest-high-level: added get cluster settings (#31706)
  [Docs] Correct typos (#31720)
  Clean up double semicolon code typos (#31687)
  [DOCS] Check for Windows and *nix file paths (#31648)
  [ML] Validate ML filter_id (#31535)
  Revert long lines
  Fix TransportChangePasswordActionTests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs >enhancement v6.4.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants