Skip to content

[upgrade assistant] Stop rollup jobs before reindexing#212815

Merged
mattkime merged 23 commits intoelastic:8.xfrom
mattkime:upgrade_assistant_rollup_support
Apr 14, 2025
Merged

[upgrade assistant] Stop rollup jobs before reindexing#212815
mattkime merged 23 commits intoelastic:8.xfrom
mattkime:upgrade_assistant_rollup_support

Conversation

@mattkime
Copy link
Contributor

@mattkime mattkime commented Feb 28, 2025

This PR improve support for rollup indices. Rollup indices can be handled like normal indices but jobs should be stopped before reindexing begins or index is marked read only. Also handles case where the rollup job is already stopped.

To review: Mark the following read only and make sure rollup jobs are handled as appropriate: Rollup index with and without job running, normal index.

Follow up to #212592 and #214656

Closes: #211850

@mattkime mattkime changed the title support rollup indices as regular indices. Disable rollup job before … [upgrade assistant] Stop rollup jobs before reindexing Mar 18, 2025
@mattkime mattkime added release_note:skip Skip the PR/issue when compiling release notes backport:skip This PR does not require backporting labels Mar 18, 2025
@mattkime mattkime marked this pull request as ready for review March 19, 2025 06:04
@mattkime mattkime requested a review from kibanamachine as a code owner March 19, 2025 06:04
@sabarasaba sabarasaba self-requested a review March 19, 2025 09:07
Copy link
Member

@sabarasaba sabarasaba left a comment

Choose a reason for hiding this comment

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

tested with the shared data folder and it seems to be working correctly

left few nits, let me know what you think

let rollupJob;
if (rollupIndices.length > 0) {
// there should only be one job
rollupJob = rollupCaps[rollupIndices[0]].rollup_jobs[0].job_id;
Copy link
Member

Choose a reason for hiding this comment

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

nit: this seems a bit brittle, any chance we can make it more resilient?

@mattkime
Copy link
Contributor Author

/ci

@mattkime
Copy link
Contributor Author

/ci

@mattkime
Copy link
Contributor Author

/ci

@mattkime
Copy link
Contributor Author

/ci

@mattkime mattkime self-assigned this Mar 21, 2025
@mattkime mattkime added the Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more t// label Mar 21, 2025
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-management (Team:Kibana Management)

Copy link
Member

@sabarasaba sabarasaba left a comment

Choose a reason for hiding this comment

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

left few comments, let me know what you think

Copy link
Contributor

@jloleysens jloleysens left a comment

Choose a reason for hiding this comment

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

Left a few qs, not blockers but I think it would be nice to add an E2E test. Maybe in x-pack/test/upgrade_assistant_integration/upgrade_assistant/reindexing.ts.

// stop related rollup job if it exists
const rollupJob = await getRollupJobByIndexName(esClient, index);
if (rollupJob) {
await esClient.rollup.stopJob({ id: rollupJob });
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also wait_for_completion: true here? Or might it take too long?

const { indexName, rollupJob } = reindexOp.attributes;

if (rollupJob) {
await esClient.rollup.stopJob({ id: rollupJob, wait_for_completion: true });
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens when it fails to stop the job? Do we provide users with actionable information about the issue like a manual workaround?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this gets into the expectations surrounding this code. If any other aspect of reindexing fails, do we tell the users about a manual work around?

There's not much reason to expect this to fail. Actually, I'm not sure I'd have much advice for the user if this failed since I'm unfamiliar with what could cause it to fail.

Copy link
Contributor

@jloleysens jloleysens Apr 7, 2025

Choose a reason for hiding this comment

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

Yeah generally we return the error message. My question was more about what the content of a failure message is and is it something users can self-service? Generally UA reindexing error messages are self-servicable but I'm not familiar with common failure conditions here so I thought I'd ask.

It might be the case that these are, for example, retryable or something we can handle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked with @martijnvg and he stated that there's no reason to anticipate any error states when stopping rollups so I think I'll leave the code as is.

@mattkime mattkime added backport:skip This PR does not require backporting and removed backport:skip This PR does not require backporting labels Apr 9, 2025
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

cc @mattkime

@mattkime mattkime merged commit c776bc4 into elastic:8.x Apr 14, 2025
8 checks passed
mattkime added a commit that referenced this pull request Apr 21, 2025
…to 9.1 (#218049)

## Summary

forward port of #212815

---

This PR improve support for rollup indices. Rollup indices can be
handled like normal indices but jobs should be stopped before reindexing
begins or index is marked read only. Also handles case where the rollup
job is already stopped.

To review: Mark the following read only and make sure rollup jobs are
handled as appropriate: Rollup index with and without job running,
normal index.

Follow up to #212592 and
#214656

Closes: #211850
pgayvallet pushed a commit to pgayvallet/kibana that referenced this pull request Apr 22, 2025
…to 9.1 (elastic#218049)

## Summary

forward port of elastic#212815

---

This PR improve support for rollup indices. Rollup indices can be
handled like normal indices but jobs should be stopped before reindexing
begins or index is marked read only. Also handles case where the rollup
job is already stopped.

To review: Mark the following read only and make sure rollup jobs are
handled as appropriate: Rollup index with and without job running,
normal index.

Follow up to elastic#212592 and
elastic#214656

Closes: elastic#211850
akowalska622 pushed a commit to akowalska622/kibana that referenced this pull request May 29, 2025
…to 9.1 (elastic#218049)

## Summary

forward port of elastic#212815

---

This PR improve support for rollup indices. Rollup indices can be
handled like normal indices but jobs should be stopped before reindexing
begins or index is marked read only. Also handles case where the rollup
job is already stopped.

To review: Mark the following read only and make sure rollup jobs are
handled as appropriate: Rollup index with and without job running,
normal index.

Follow up to elastic#212592 and
elastic#214656

Closes: elastic#211850
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:skip This PR does not require backporting Feature:Upgrade Assistant release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more t//

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants