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.

Sign up for GitHub

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

Jump to bottom

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 •
edited
Loading

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

gwbrown added 3 commits March 20, 2020 17:25
@gwbrown
Disallow negative TimeValues
8c2a742 
This commit causes negative TimeValues, other than -1 which is sometimes used as
a sentinel value, to be rejected during parsing.
@gwbrown
Adjust error message for negative TimeValues
cc2a587
@gwbrown
Check for new message in tests for rejection of negative TimeValues
8b45357
@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)

jasontedor
jasontedor reviewed Mar 24, 2020
View reviewed changes
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.

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/Phase.java Show resolved Hide resolved
server/src/main/java/org/elasticsearch/monitor/jvm/JvmGcMonitorService.java Outdated Show resolved Hide resolved
server/src/main/java/org/elasticsearch/index/shard/IndexingStats.java Outdated Show resolved Hide resolved
server/src/main/java/org/elasticsearch/index/search/stats/SearchStats.java Outdated Show resolved Hide resolved
server/src/main/java/org/elasticsearch/index/search/stats/SearchStats.java Outdated Show resolved Hide resolved
server/src/main/java/org/elasticsearch/index/search/stats/SearchStats.java Outdated Show resolved Hide resolved
henningandersen added a commit to henningandersen/elasticsearch that referenced this pull request Mar 24, 2020
@henningandersen
Reindex negative TimeValue fix
d3ad55c 
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 henningandersen mentioned this pull request Mar 24, 2020
Merged
@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
@henningandersen
Reindex negative TimeValue fix (#54057)
2a67bee 
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
@henningandersen
Reindex negative TimeValue fix (#54057)
7ce7aff 
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
@henningandersen
Reindex negative TimeValue fix (elastic#54057)
5a9d6e4 
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.
@henningandersen henningandersen mentioned this pull request Mar 24, 2020
Merged
@gwbrown gwbrown added the v7.8.0 label Mar 25, 2020
dakrone
dakrone approved these changes Mar 25, 2020
View reviewed changes
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

libs/core/src/main/java/org/elasticsearch/common/unit/TimeValue.java Outdated Show resolved Hide resolved
@gwbrown
Copy link
Contributor Author

gwbrown commented Mar 25, 2020 •
edited
Loading

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 gwbrown merged commit e9bc3e8 into elastic:master Mar 26, 2020
@gwbrown gwbrown mentioned this pull request Mar 26, 2020
Merged
gwbrown added a commit to gwbrown/elasticsearch that referenced this pull request Mar 26, 2020
@gwbrown
Disallow negative TimeValues (elastic#53913)
4182287 
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
@gwbrown
Disallow negative TimeValues (elastic#53913)
85f6960 
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 gwbrown mentioned this pull request Mar 26, 2020
Merged
gwbrown added a commit that referenced this pull request Mar 26, 2020
@gwbrown
Disallow negative TimeValues (#53913)
0d30b48 
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
@gwbrown
Disallow negative TimeValues (#53913)
8fab26a 
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 gwbrown mentioned this pull request Mar 26, 2020
Merged
gwbrown added a commit to gwbrown/elasticsearch that referenced this pull request Mar 26, 2020
@gwbrown
Disallow negative TimeValues (elastic#53913)
3543e96 
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
@gwbrown
[6.8] Disallow negative TimeValues (#53913) (#54304)
d5293ea 
* 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
@gwbrown gwbrown mentioned this pull request Mar 31, 2020
Merged
@codebrain codebrain mentioned this pull request Apr 1, 2020
Closed
This was referenced Apr 17, 2020
Closed
Merged
tlrx added a commit that referenced this pull request Apr 17, 2020
@tlrx
Mute GraphTests.testTimedoutQueryCrawl (#55397)
73f5cb6 
Relates #55396
Relates #53913
tlrx added a commit that referenced this pull request Apr 17, 2020
@tlrx
Mute GraphTests.testTimedoutQueryCrawl (#55397)
eb52df6 
Relates #55396
Relates #53913
tlrx added a commit that referenced this pull request Apr 17, 2020
@tlrx
Mute GraphTests.testTimedoutQueryCrawl (#55397)
1acf956 
Relates #55396
Relates #53913
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasontedor jasontedor jasontedor left review comments

@dakrone dakrone dakrone approved these changes

@rjernst rjernst Awaiting requested review from rjernst

Assignees
No one assigned
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
Milestone
No milestone
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
@gwbrown @elasticmachine @henningandersen @dakrone @jasontedor @jakelandis @bpintea