Skip to content

integration-server-test: Add 9.0 to lazy rollover config #17257

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

Merged
merged 5 commits into from
Jun 20, 2025
Merged

integration-server-test: Add 9.0 to lazy rollover config #17257

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6d54ad5
Add 9.0 to lazy rollover due to https://github.com/elastic/elasticsea…
ericywl Jun 17, 2025
06aa23a
Remove if condition in workflow since it's skipping scheduled for som…
ericywl Jun 18, 2025
ae8c7ef
Merge branch 'main' into iserver-test-fix-818-90-lazy-rollover
ericywl Jun 18, 2025
342739b
Merge branch 'main' into iserver-test-fix-818-90-lazy-rollover
ericywl Jun 19, 2025
f19f4bb
Merge branch 'main' into iserver-test-fix-818-90-lazy-rollover
ericywl Jun 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/integration-server-test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ jobs:
SCENARIO="${{ matrix.scenario }}" make integration-server-standalone-test

notify:
if: github.ref == 'refs/heads/main'
name: Notify
runs-on: ubuntu-latest
needs:
- run-upgrade
Expand Down
1 change: 0 additions & 1 deletion .github/workflows/smoke-tests-os-sched.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@ jobs:

notify:
name: Notify
if: github.ref == 'refs/heads/main'
runs-on: ubuntu-latest
needs:
- smoke-tests-os
Expand Down
5 changes: 3 additions & 2 deletions integrationservertest/upgrade-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,9 @@ data-stream-lifecycle:
lazy-rollover-with-exceptions:
8.16:
8.17:
# 8.19 and 9.1 have the same template due to
# https://github.com/elastic/elasticsearch/pull/123166.
8.19:
9.0:
# 8.19 and 9.1 have the same template due to
# https://github.com/elastic/elasticsearch/pull/128913.
9.1:
- "8.19"
Copy link
Contributor

Choose a reason for hiding this comment

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

The linked PR gets backported to 8.19.0, 9.0.3 and 9.1.0 - IMO upgrading from 8.18 to 9.0 would now lead to a lazy rollover.
Upgrading from 8.19 to 9.1 should not lead to a lazy rollover.

So I am not sure why 9.0 is listed as a lazy rollover exception now?

Copy link
Contributor Author

@ericywl ericywl Jun 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think there may be some misunderstanding on the exceptions. The first level of the YAML are versions that will have lazy-rollover if upgraded to from another version. The second level are the exceptions, where upgrading from that version to the version listed in the first level will not yield any lazy-rollover.

So, in this case, upgrading from 8.19 to 9.1 will not have lazy-rollover. But upgrading from 8.18 to 9.0 will.

If there is a better way to represent this, please let me know. I also feel that this YAML config might be a little confusing.

Copy link
Member

Choose a reason for hiding this comment

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

It's not particularly intuitive. Do we really need the first level? All versions onwards from X (I can't remember what X is) will do lazy rollover right? Does that need to be defined in configuration, or could we put that in the code, and test every major version from there on?

Then perhaps we could define the exceptions something like:

lazy-rollover-with-exceptions:
  - from: 8.19
    to: 9.1
    reason: "8.19 and 9.1 have the same template due to https://github.com/elastic/elasticsearch/pull/128913."

Copy link
Contributor Author

@ericywl ericywl Jun 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It's not particularly intuitive. Do we really need the first level? All versions onwards from X (I can't remember what X is) will do lazy rollover right? Does that need to be defined in configuration, or could we put that in the code, and test every major version from there on?

It's not guaranteed that every new version will have lazy rollover. For example, 8.17 to 8.18 does not have lazy rollover because they share the same templates in ES apm-data plugin.

I do agree with it being unintuitive, perhaps the way you suggested (explicit from and to along with reason) would be better. Or maybe as Silvia mentioned in a previous PR, might be better to just define versions with lazy-rollover explicitly. I will look into improving it after this iteration. Thanks!

Copy link
Contributor

@endorama endorama Jun 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I've been struggling with the current setup as well while writing tests for the resource version bug. As we bumped the resource version we expect lazy rollovers in more versions and adding all exceptions was not intuitive (for example now we expect a rollover if you upgrade to 8.17.8 from any 8.17 < 8.17.8). The fact that it also supports only matching major.minor instead of a full version is limiting.

I think we should ensure that every version is expected to have a lazy rollover, exceptions are the one that don't. The rationale is that if we default to "no rollover" we may miss cases where rollover is needed but we did not upgrade the test configuration. Given how critical rollovers are this failure mode is safer.