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

Disallow negative TimeValues #53913

Merged
merged 21 commits into from
Mar 26, 2020
Merged

Disallow negative TimeValues #53913

merged 21 commits into from
Mar 26, 2020

Conversation

gwbrown
Copy link
Contributor

@gwbrown gwbrown commented Mar 21, 2020

This commit causes negative TimeValues, other than -1 which is sometimes used as
a sentinel value, to be rejected during parsing.

Also introduces a hack to allow ILM to load policies which were written to the
cluster state with a negative min_age, treating those values as 0, which should
match the behavior of prior versions.

Fixes #54041

This commit causes negative TimeValues, other than -1 which is sometimes used as
a sentinel value, to be rejected during parsing.
@gwbrown gwbrown added :Core/Infra/Core Core issues without another label v8.0.0 v7.7.0 v6.8.9 labels Mar 21, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (:Core/Infra/Core)

@gwbrown gwbrown added the >bug label Mar 23, 2020
@gwbrown gwbrown requested a review from dakrone March 23, 2020 23:55
@gwbrown gwbrown marked this pull request as ready for review March 23, 2020 23:56
@gwbrown gwbrown requested a review from rjernst March 23, 2020 23:57
@gwbrown gwbrown added the :Data Management/ILM+SLM Index and Snapshot lifecycle management label Mar 23, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features (:Core/Features/ILM+SLM)

Copy link
Member

@jasontedor jasontedor 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 some comments.

henningandersen added a commit to henningandersen/elasticsearch that referenced this pull request Mar 24, 2020
Reindex would use timeValueNanos(System.nanoTime()). The intended use
for TimeValue is as a duration, not as absolute time. In particular,
this could result in negative TimeValue's, being unsupported in elastic#53913.
@henningandersen
Copy link
Contributor

Nice catch, @gwbrown . Unfortunately, reindex used timeValueNanos(System.nanoTime()) in a few places and I worry that this fix could break that (nanoTime is allowed to return negative values). I opened #54057 to address that, which I think is a prerequisite for this PR to be merged.

henningandersen added a commit that referenced this pull request Mar 24, 2020
Reindex would use timeValueNanos(System.nanoTime()). The intended use
for TimeValue is as a duration, not as absolute time. In particular,
this could result in negative TimeValue's, being unsupported in #53913.
Modified to use the bare long nano-second value.
henningandersen added a commit that referenced this pull request Mar 24, 2020
Reindex would use timeValueNanos(System.nanoTime()). The intended use
for TimeValue is as a duration, not as absolute time. In particular,
this could result in negative TimeValue's, being unsupported in #53913.
Modified to use the bare long nano-second value.
henningandersen added a commit to henningandersen/elasticsearch that referenced this pull request Mar 24, 2020
Reindex would use timeValueNanos(System.nanoTime()). The intended use
for TimeValue is as a duration, not as absolute time. In particular,
this could result in negative TimeValue's, being unsupported in elastic#53913.
Modified to use the bare long nano-second value.
@gwbrown gwbrown added the v7.8.0 label Mar 25, 2020
Copy link
Member

@dakrone dakrone left a comment

Choose a reason for hiding this comment

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

LGTM assuming CI is happy, thanks for addressing this Gordon

@gwbrown
Copy link
Contributor Author

gwbrown commented Mar 25, 2020

All the tests passed but I'm going to run one more time to be sure, as many of the failures previously were due to random values, which are only sometimes negative.

@elasticmachine test this please

gwbrown and others added 2 commits March 25, 2020 17:25
Co-Authored-By: Lee Hinman <dakrone@users.noreply.github.com>
@gwbrown gwbrown merged commit e9bc3e8 into elastic:master Mar 26, 2020
gwbrown added a commit to gwbrown/elasticsearch that referenced this pull request Mar 26, 2020
This commit causes negative TimeValues, other than -1 which is sometimes used as
a sentinel value, to be rejected during parsing.

Also introduces a hack to allow ILM to load policies which were written to the
cluster state with a negative min_age, treating those values as 0, which should
match the behavior of prior versions.
gwbrown added a commit to gwbrown/elasticsearch that referenced this pull request Mar 26, 2020
This commit causes negative TimeValues, other than -1 which is sometimes used as
a sentinel value, to be rejected during parsing.

Also introduces a hack to allow ILM to load policies which were written to the
cluster state with a negative min_age, treating those values as 0, which should
match the behavior of prior versions.
gwbrown added a commit that referenced this pull request Mar 26, 2020
This commit causes negative TimeValues, other than -1 which is sometimes used as
a sentinel value, to be rejected during parsing.

Also introduces a hack to allow ILM to load policies which were written to the
cluster state with a negative min_age, treating those values as 0, which should
match the behavior of prior versions.
gwbrown added a commit that referenced this pull request Mar 26, 2020
This commit causes negative TimeValues, other than -1 which is sometimes used as
a sentinel value, to be rejected during parsing.

Also introduces a hack to allow ILM to load policies which were written to the
cluster state with a negative min_age, treating those values as 0, which should
match the behavior of prior versions.
gwbrown added a commit to gwbrown/elasticsearch that referenced this pull request Mar 26, 2020
This commit causes negative TimeValues, other than -1 which is sometimes used as
a sentinel value, to be rejected during parsing.

Also introduces a hack to allow ILM to load policies which were written to the
cluster state with a negative min_age, treating those values as 0, which should
match the behavior of prior versions.
gwbrown added a commit that referenced this pull request Mar 27, 2020
* Disallow negative TimeValues (#53913)

This commit causes negative TimeValues, other than -1 which is sometimes used as
a sentinel value, to be rejected during parsing.

Also introduces a hack to allow ILM to load policies which were written to the
cluster state with a negative min_age, treating those values as 0, which should
match the behavior of prior versions.

* Re-apply IndexLifecycleRunner changes lost in merge

* Make SnapshotStatsTests generate more realistic times

* Fix negative TimeValue in deprecation test

* Re-apply CcrStatsResponseTests fix lost in merge
tlrx added a commit that referenced this pull request Apr 17, 2020
tlrx added a commit that referenced this pull request Apr 17, 2020
tlrx added a commit that referenced this pull request Apr 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Core/Infra/Core Core issues without another label :Data Management/ILM+SLM Index and Snapshot lifecycle management v6.8.9 v7.7.0 v7.8.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Creating an ILM policy that has a negative min_age can cause problems loading cluster state
7 participants