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

Skip building plugins moved to modules #1624

Merged
merged 4 commits into from
Nov 21, 2022

Conversation

b-deam
Copy link
Member

@b-deam b-deam commented Nov 17, 2022

Since 8.0, we've converted the repository-azure, repository-gcs and repository-s3 plugins into Elasticsearch modules, so that they are always included. Whilst adding or removing these plugins still succeeds, it is now a no-op.

However, if you try and build these plugins with something like:

esrally install --http-port=29200 --node=rally-node --master-nodes=rally-node --car=4gheap,trial-license --seed-hosts="127.0.0.1:29300" --revision "@2022-02-04" --elasticsearch-plugins repository-s3,repository-gcs,repository-azure --plugin-params="s3_client_name:mys3client,s3_access_key:XXXXX,s3_secret_key:YYYYY,s3_session_token:ZZZZZ" --source-build-method=docker

esrally build --revision "@2022-02-04" --elasticsearch-plugins repository-s3,repository-gcs,repository-azure --plugin-params="s3_client_name:mys3client,s3_access_key:XXXXX,s3_secret_key:YYYYY,s3_session_token:ZZZZZ"

It will fail, with this error in Rally's ~/.rally/logs/build.log:

FAILURE: Build failed with an exception.

* What went wrong:
Project 'repository-s3' not found in project ':plugins'.

* Try:
> Run with --stacktrace option to get the stack trace.
> Run with --info or --debug option to get more log output.
> Run with --scan to get full insights.

* Get more help at https://help.gradle.org

BUILD FAILED in 6s

For now we still need to retain the existing post-install hooks (see elastic/rally-teams#76), so this is a first step in gracefully skipping any build steps.

Must be merged with:

Relates:

@b-deam b-deam requested a review from dliappis November 17, 2022 01:13
@b-deam b-deam self-assigned this Nov 17, 2022
@b-deam b-deam changed the title Handle plugins moved to modules Skip building plugins moved to modules Nov 17, 2022
@b-deam

This comment was marked as resolved.

Copy link
Contributor

@dliappis dliappis left a comment

Choose a reason for hiding this comment

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

Works great. Do you think we should add a note in the migrate.rst doc documenting this (and the change in https://github.com/elastic/rally-teams/pull/76/files)?

@dliappis
Copy link
Contributor

I also hit a weird issue when specifying a build revision as a SHA:

If I use a revision with a timestamp like the reproductions you provided in the description it works fine (using either build or install) e.g.

$ esrally build --revision "@2022-11-17" --elasticsearch-plugins repository-azure --plugin-params="azure_client_name:default,azure_account:SOMEAZUREACCOUNT,azure_key:SOMEAUZREKEY" --team-path=../rally-teams

[INFO] Creating installable binary from source files
[INFO] Creating installable binary from source files
{
  "elasticsearch": "/home/dl/.rally/benchmarks/distributions/src/elasticsearch-e68e28e4cb9-linux-x86_64.tar.gz"
}

(Note that I have checked out elastic/rally-teams#76 in a sibling director and referenced with --team-path)

However, when I specify the same revision using a commit SHA (again with build or install), things start to fail:

$ esrally install --http-port=29200 --node=rally-node --master-nodes=rally-node --car=4gheap,trial-license --seed-hosts="127.0.0.1:29300" --revision "e68e28e4cb983cf06be5612b38078bc7d9e02f48" --elasticsearch-plugins repository-azure --plugin-params="azure_client_name:default,azure_account:SOMEAZUREACCOUNT,azure_key:SOMEAUZREKEY" --team-path=../rally-teams
 
[ERROR] Cannot install. No revision specified for plugin [repository-azure].

@dliappis

This comment was marked as outdated.

@b-deam b-deam added this to the 2.7.1 milestone Nov 17, 2022
@b-deam b-deam added the enhancement Improves the status quo label Nov 17, 2022
@b-deam
Copy link
Member Author

b-deam commented Nov 18, 2022

I also hit a weird issue when specifying a build revision as a SHA:

If I use a revision with a timestamp like the reproductions you provided in the description it works fine (using either build or install) e.g.

$ esrally build --revision "@2022-11-17" --elasticsearch-plugins repository-azure --plugin-params="azure_client_name:default,azure_account:SOMEAZUREACCOUNT,azure_key:SOMEAUZREKEY" --team-path=../rally-teams

[INFO] Creating installable binary from source files
[INFO] Creating installable binary from source files
{
  "elasticsearch": "/home/dl/.rally/benchmarks/distributions/src/elasticsearch-e68e28e4cb9-linux-x86_64.tar.gz"
}

(Note that I have checked out elastic/rally-teams#76 in a sibling director and referenced with --team-path)

However, when I specify the same revision using a commit SHA (again with build or install), things start to fail:

$ esrally install --http-port=29200 --node=rally-node --master-nodes=rally-node --car=4gheap,trial-license --seed-hosts="127.0.0.1:29300" --revision "e68e28e4cb983cf06be5612b38078bc7d9e02f48" --elasticsearch-plugins repository-azure --plugin-params="azure_client_name:default,azure_account:SOMEAZUREACCOUNT,azure_key:SOMEAUZREKEY" --team-path=../rally-teams
 
[ERROR] Cannot install. No revision specified for plugin [repository-azure].

Ah, I missed that spot. Fixed in 010e08b.

@b-deam
Copy link
Member Author

b-deam commented Nov 18, 2022

Works great. Do you think we should add a note in the migrate.rst doc documenting this (and the change in https://github.com/elastic/rally-teams/pull/76/files)?

Good suggestion - addressed in 10ad4fa

Copy link
Contributor

@dliappis dliappis left a comment

Choose a reason for hiding this comment

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

Works great. Left a few small comments, but no need for another review cycle.

if plugin.moved_to_module:
# TODO: https://github.com/elastic/rally/issues/1622
continue

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a pass here, rather than a continue? (I mean there is no loop)

also a nit: shouldn't the line below be converted to an elif?

Copy link
Member Author

@b-deam b-deam Nov 20, 2022

Choose a reason for hiding this comment

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

I think this is a pass here, rather than a continue? (I mean there is no loop)

I think continue is the correct choice here because it is inside a for loop.

also a nit: shouldn't the line below be converted to an elif?

Addressed in f6d36e0

Copy link
Contributor

Choose a reason for hiding this comment

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

I think continue is the correct choice here because it is inside a for loop.

Ah ofc missed that 👍

@b-deam b-deam merged commit ba2ff70 into elastic:master Nov 21, 2022
danielmitterdorfer added a commit to danielmitterdorfer/rally that referenced this pull request Dec 16, 2022
In elastic#1624 we've ensured that plugins that have turned into modules are
now skipped from the mandatory plugin check. However, we've also not
passed any plugin parameters to the provisioner and this made post
installation hooks fail, if they expected parameters to be passed on the
command line via `--plugin-params` (e.g. `repository-gcs`). With this
commit we make sure that `--plugin-params` is still honored even in
these cases.

Relates elastic#1624
danielmitterdorfer added a commit that referenced this pull request Dec 21, 2022
In #1624 we've ensured that plugins that have turned into modules are
now skipped from the mandatory plugin check. However, we've also not
passed any plugin parameters to the provisioner and this made post
installation hooks fail, if they expected parameters to be passed on the
command line via `--plugin-params` (e.g. `repository-gcs`). With this
commit we make sure that `--plugin-params` is still honored even in
these cases.

Relates #1624
@pquentin pquentin added the highlight A substantial improvement that is worth mentioning separately in release notes label Mar 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improves the status quo highlight A substantial improvement that is worth mentioning separately in release notes tech debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants