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

[infra] Use correct ML API to query blocking tasks #167779

Merged
merged 5 commits into from Oct 5, 2023

Conversation

miltonhultgren
Copy link
Contributor

@miltonhultgren miltonhultgren commented Oct 2, 2023

While working on #47477, I found that attempting to re-create a ML job faces a 404 because it uses an endpoint that has been removed / changed.

This PR updates to use the newer endpoint to find which tasks are blocking in the ML system (like job deletion) and changes the types to match the new API.

@miltonhultgren miltonhultgren added release_note:fix Feature:Logs UI Logs UI feature Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services backport:prev-minor Backport to the previous minor version (i.e. one version back from main) labels Oct 2, 2023
@miltonhultgren miltonhultgren requested a review from a team as a code owner October 2, 2023 13:38
@elasticmachine
Copy link
Contributor

Pinging @elastic/infra-monitoring-ui (Team:Infra Monitoring UI)

@apmmachine
Copy link
Contributor

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
  • /oblt-deploy-serverless : Deploy a serverless Kibana instance using the Observability test environments.
  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

Comment on lines +126 to +128
onReject: (e) => {
throw new Error(`Failed to clean up previous ML job: ${e}`);
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This part was weird, if I don't do this, then the Promise is not rejected even if the API call throws an error (which is why we didn't see this API move).
@weltenwort Do you know if that's expected?

Copy link
Member

Choose a reason for hiding this comment

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

My guess is that useTrackedPromise is swallowing errors and expecting callers to look for and respond to the "rejected" state instead, https://github.com/miltonhultgren/kibana/blob/d91fe9fc92133cfa52c4b9aff664018b241f2ac3/x-pack/plugins/infra/public/utils/use_tracked_promise.ts#L222

Copy link
Member

Choose a reason for hiding this comment

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

Not sure which makes more sense here exactly, but you could look at cleanUpModuleRequest.state for "rejected" and respond, rather than throwing this new error?

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'm throwing here because this function is called further down and then as a Promise with a catch to update the reducer state, if I don't catch I'm outside of that flow and the setup call is done ahead of time which causes an inconsistent state.

@miltonhultgren miltonhultgren enabled auto-merge (squash) October 5, 2023 07:54
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #53 / Actions migrations "before all" hook for "7.10.0 migrates the casesConfiguration to be the incidentConfiguration in config, then 7.11.0 removes incidentConfiguration"

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
infra 1.9MB 1.9MB +627.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@miltonhultgren miltonhultgren merged commit 48b66d7 into elastic:main Oct 5, 2023
20 checks passed
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 5, 2023
While working on elastic#47477, I found
that attempting to re-create a ML job faces a 404 because it uses an
endpoint that has been removed / changed.

This PR updates to use the newer endpoint to find which tasks are
blocking in the ML system (like job deletion) and changes the types to
match the new API.

(cherry picked from commit 48b66d7)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.11

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Oct 5, 2023
…168075)

# Backport

This will backport the following commits from `main` to `8.11`:
- [[infra] Use correct ML API to query blocking tasks
(#167779)](#167779)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Milton
Hultgren","email":"milton.hultgren@elastic.co"},"sourceCommit":{"committedDate":"2023-10-05T09:39:23Z","message":"[infra]
Use correct ML API to query blocking tasks (#167779)\n\nWhile working on
#47477, I found\r\nthat
attempting to re-create a ML job faces a 404 because it uses
an\r\nendpoint that has been removed / changed.\r\n\r\nThis PR updates
to use the newer endpoint to find which tasks are\r\nblocking in the ML
system (like job deletion) and changes the types to\r\nmatch the new
API.","sha":"48b66d72dc8fc40fdf21a8c812cfd7659686ccf2","branchLabelMapping":{"^v8.12.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix","Feature:Logs
UI","Team:Infra Monitoring
UI","backport:prev-minor","v8.12.0"],"number":167779,"url":"#167779
Use correct ML API to query blocking tasks (#167779)\n\nWhile working on
#47477, I found\r\nthat
attempting to re-create a ML job faces a 404 because it uses
an\r\nendpoint that has been removed / changed.\r\n\r\nThis PR updates
to use the newer endpoint to find which tasks are\r\nblocking in the ML
system (like job deletion) and changes the types to\r\nmatch the new
API.","sha":"48b66d72dc8fc40fdf21a8c812cfd7659686ccf2"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v8.12.0","labelRegex":"^v8.12.0$","isSourceBranch":true,"state":"MERGED","url":"#167779
Use correct ML API to query blocking tasks (#167779)\n\nWhile working on
#47477, I found\r\nthat
attempting to re-create a ML job faces a 404 because it uses
an\r\nendpoint that has been removed / changed.\r\n\r\nThis PR updates
to use the newer endpoint to find which tasks are\r\nblocking in the ML
system (like job deletion) and changes the types to\r\nmatch the new
API.","sha":"48b66d72dc8fc40fdf21a8c812cfd7659686ccf2"}}]}] BACKPORT-->

Co-authored-by: Milton Hultgren <milton.hultgren@elastic.co>
dej611 pushed a commit to dej611/kibana that referenced this pull request Oct 17, 2023
While working on elastic#47477, I found
that attempting to re-create a ML job faces a 404 because it uses an
endpoint that has been removed / changed.

This PR updates to use the newer endpoint to find which tasks are
blocking in the ML system (like job deletion) and changes the types to
match the new API.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to the previous minor version (i.e. one version back from main) Feature:Logs UI Logs UI feature release_note:fix Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v8.11.0 v8.12.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants