[ML] Adding reset anomaly detection jobs link to jobs list #108039
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@elasticmachine merge upstream |
jgowdyelastic
added
:ml
auto-backport
|
Pinging @elastic/ml-ui (:ml) |
jgowdyelastic
changed the title
darnautov
reviewed
Aug 11, 2021
x-pack/plugins/ml/public/application/jobs/jobs_list/components/job_actions/management.js
Outdated
Show resolved
Hide resolved
...l/public/application/jobs/jobs_list/components/reset_job_modal/open_jobs_warning_callout.tsx
Outdated
Show resolved
Hide resolved
Comment on lines
+69
to
+71
| setTimeout(() => { | ||
| refreshJobs(); | ||
| }, RESETTING_JOBS_REFRESH_INTERVAL_MS); |
There was a problem hiding this comment.
this part looks alarming, I reckon we should not rely on the hardcoded timeout. in the cloud instances, requests tend to take longer
There was a problem hiding this comment.
This is also how the delete modal works, it delays for a couple of seconds just in case the job states haven't caught up. The jobs list then starts a poll to track the real state of reverting and deleting processes.
However.... one key difference between this modal and the delete job modal is that there is an await on revert request, meaning that this 2s delay isn't needed. The revert has already been assigned a task ID by this point.
I'll remove this timeout.
There was a problem hiding this comment.
Updated in 06f04dc
There was a problem hiding this comment.
Adding this delay back in with 115ad46
We need to give elasticsearch a second for the blocked property to appear in the job.
There was a problem hiding this comment.
What happens in case of a severe network throttling? Maybe we can define and observable for jobs, and on each update look up for the blocked property?
There was a problem hiding this comment.
if the network is very slow, i can't see it making a difference. the call to reset is on an await, so this timeout won't start until the call to reset has returned.
Waiting a second to then refresh the jobs list is just there to give the task manager a bit of a breather before we ask if any tasks are running.
Maybe we can define and observable for jobs, and on each update look up for the blocked property?
This is what is coming in the next PR. It's what we already do for "deleting" jobs, but that needs updating to use the new blocked property instead.
.../plugins/ml/public/application/jobs/jobs_list/components/reset_job_modal/reset_job_modal.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/ml/public/application/jobs/jobs_list/components/utils.js
Outdated
Show resolved
Hide resolved
| @@ -76,6 +77,10 @@ export function isClosable(jobs) { | |||
| ); | |||
| } | |||
|
|
|||
| export function isResetable(jobs) { | |||
| return jobs.some((j) => j.jobState === JOB_STATE.CLOSED); | |||
There was a problem hiding this comment.
Should the reset action be shown for jobs that have never run (or ones which have already been reset)?
There was a problem hiding this comment.
I don't think there is any harm in showing the reset action for jobs which haven't been run or have already been reset.
How would we determine that? closed and no processed records?
This is a similar question to whether we should allow a job to be reset again while a ongoing reset task is running.
For deleting we allow the user to click delete again if the job is "deleting", i think in case the delete task is hanging.
@dimitris-athanasiou what do you think?
Also, should we allow a job to be deleted if an ongoing reset or revert task is running? or should we only allow the user to re-click the current task type?
There was a problem hiding this comment.
With regard to resetting a job that has never run or that has just been reset: there is no harm indeed. I would argue to keep the logic simple and let the reset action be shown in those cases. If the user hits the reset button again, it will almost be a no-op.
Also, should we allow a job to be deleted if an ongoing reset or revert task is running? or should we only allow the user to re-click the current task type?
A job can be deleted while a reset or revert task is running.
But if a reset or revert task is running, the job should not be allowed to be opened nor the other blocking action should be allowed (revert when reset is running and vice versa). If the user calls, for example, revert while reset is happening, they will get an error message letting them know the job is blocked while resetting. The UI would ideally disable the relevant action with an informative tooltip.
There was a problem hiding this comment.
But if a reset or revert task is running, the job should not be allowed to be opened nor the other blocking action should be allowed
Yes, we disable all other options, other than delete. but isn't delete a blocking action?
Is there any benefit in allowing the user to reselect reset if a reset task is already running? Like we do with delete.
So just to be clear.. when a delete, revert or reset action is running, the only action that should be available is delete?
There was a problem hiding this comment.
User should be allowed to reselect the current blocking action. The reason is for recovering from a failure. Say the user clicked reset. Then the reset task failed because of some reason. The job is still blocked on reset. The way to unblock it, is to click reset again and hopefully this time it will complete successfully and unset the job's blocked field.
There was a problem hiding this comment.
| @@ -37,7 +47,7 @@ export function actionsMenuContent( | |||
| defaultMessage: 'Start datafeed', | |||
| }), | |||
| icon: 'play', | |||
| enabled: (item) => item.deleting !== true && canStartStopDatafeed, | |||
| enabled: (item) => isJobBlocked(item) === false && canStartStopDatafeed, | |||
There was a problem hiding this comment.
I don't see these action items disabled after hitting reset. Is this to be expected, without the planned follow-up for polling?
There was a problem hiding this comment.
797c274 fixes this issue.
It's still tricky to reproduce as you need to catch the job while the reset task is running.
I think i might need to reinstate the slight delay to refresh the jobs after applying the reset, just to increase the chance that the first refresh will catch the the job why the task is running, and not before, which is happening now.
The follow up work will add a poll for the tasks so the jobs list will update once the task is complete.
Note, delete is still available. I've raised this as a question in a previous comment.
|
@elasticmachine merge upstream |
|
Not to do with the changes here, but could we hide / disable the cog icon for the multi-select actions menu if there are no actions available? For example here, where I've logged in as
@peteharverson yes i noticed that too when doing this work. i'm not sure why the button doesn't appear disabled when it is. |
peteharverson
approved these changes
Aug 13, 2021
|
@elasticmachine merge upstream |
darnautov
approved these changes
Aug 16, 2021
| @@ -41,7 +42,7 @@ import { RefreshJobsListButton } from '../refresh_jobs_list_button'; | |||
| import { DELETING_JOBS_REFRESH_INTERVAL_MS } from '../../../../../../common/constants/jobs_list'; | |||
| import { JobListMlAnomalyAlertFlyout } from '../../../../../alerting/ml_alerting_flyout'; | |||
|
|
|||
| let deletingJobsRefreshTimeout = null; | |||
| let blockingJobsRefreshTimeout = null; | |||
There was a problem hiding this comment.
should it be a part of JobsListView? not sure if it belongs to the outer scope.
spong
approved these changes
Aug 16, 2021
There was a problem hiding this comment.
Security Solution changes LGTM! Thanks! 🙂 👍
I'm just now reading up on the new reset functionality, but do you think this is something we'll want to expose via the Security UI's as well? Seems like it could be useful (especially for custom jobs) -- what do you think @randomuserid?
I've added this to the list of changes to consider as part of the Security Job Mgmt UI re-design here: #102837
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / general / Chrome UI Functional Tests.test/functional/apps/management/_runtime_fields·js.management runtime fields create runtime field should create runtime fieldStandard OutStack TraceMetrics [docs]Module Count
Async chunks
Page load bundle
Unknown metric groupsAPI count
API count missing comments
History
To update your PR or re-run it, just comment with: |
kibanamachine
added a commit
to kibanamachine/kibana
that referenced
this pull request
Aug 17, 2021
…08039) * [ML] Adding reset jobs link to jobs list * fixing types * updating types * improving react code * adding closed job warning callout * small code changes after review * updating comment for api docs * adding canResetJob to security's emptyMlCapabilities * updating apidoc * adding blocked to job summary * udating test * adding delayed refresh back in * updating tests * adding better reverting controls and labels * fixing bug in delete modal * updating job task polling for all blocking tasks * fixing types after es client update * one other type correction Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
kibanamachine
added a commit
that referenced
this pull request
Aug 17, 2021
…108971) * [ML] Adding reset jobs link to jobs list * fixing types * updating types * improving react code * adding closed job warning callout * small code changes after review * updating comment for api docs * adding canResetJob to security's emptyMlCapabilities * updating apidoc * adding blocked to job summary * udating test * adding delayed refresh back in * updating tests * adding better reverting controls and labels * fixing bug in delete modal * updating job task polling for all blocking tasks * fixing types after es client update * one other type correction Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: James Gowdy <jgowdy@elastic.co>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds reset job links to the individual action menus per jobs
and the multi-select action menu.
The reset link will no be present in the individual action menu if the selected job is not closed.
If multiple jobs are selected, and one or more are not closed, a warning is shown in the confirmation modal.
Also renames the
deleting_jobs_tasksendpoint toblocking_jobs_tasksand updates its checks so that it checks for and returns all blocking jobs tasks.Closes #102661
Closes #105789
Relates to elastic/elasticsearch#73908
Checklist
Delete any items that are not applicable to this PR.
Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
This was checked for breaking API changes and was labeled appropriately