[Fleet] Fix bulk action dropdown by jillguyonnet · Pull Request #166475 · elastic/kibana

jillguyonnet · 2023-09-14T15:17:48Z

Summary

This PR addresses two current issues affecting agent selection in Fleet UI:

When there are inactive agents that are not listed on the current page and "Select everything on all pages" is clicked, the count of actionable agents is incorrect (cf. this comment for details). This can have two consequences:
1. Incorrect and sometimes negative agent count in the "Actions" dropdown.
2. Disabled menu items in the "Actions" dropdown.
The "Select everything on all pages button is incorrectly displayed when there are agents on managed policies on the page and there is no pagination (cf. this comment).

Testing steps

Reproduce the bug (main branch)

Have a fleet server with a managed policy. One way of doing this is to create the usual fleet server policy with fleet-server-policy id and run the following in Dev Tools:

PUT kbn:/api/fleet/agent_policies/fleet-server-policy
{
  "name": "Managed agent policy",
  "description": "",
  "namespace": "default",
  "monitoring_enabled": [
    "logs",
    "metrics"
  ],
  "is_managed": true
}

On another agent policy (Agent policy 1), enroll 30 agents, have them all inactive except for one. This can be achieved like so:
- Enroll 30 agents with horde
- Set agent policy inactivity timeout to 60 seconds
- Kill horde agents
- Enroll another agent (e.g. via a Docker container or a VM)
In Fleet's main page (agents list), show agents for all policies and hide inactive (default filters), so 2 agents are visible (fleet server + active agent).
Select all agents on first page:
- Actions menu is enabled and correctly shows agent count as 1 (since fleet server is not selectable).
- "Select everything on all pages" is displayed (incorrect 🐞).
Click "Select everything on all pages": the Actions menu is disabled and agent count is negative (-29) (incorrect 🐞).

Test the fixes (this branch)

With the above setup:

In Fleet's main page (agents list), show agents for all policies and hide inactive (default filters), so 2 agents are visible (fleet server + active agent).
Select all agents on first page. Check that:
- Actions menu is enabled and correctly shows agent count as 1 (since fleet server is not selectable).
- "Select everything on all pages" is NOT displayed (fix ✅).
Enroll more agents with horde so that you have enough active agents to trigger pagination (you should still have your 30 inactive agents as well).
Select all agents on first page. The "Select everything on all pages" button should be displayed.
Click "Select everything on all pages": check that the Actions menu is enabled and shows the correct count of agents.

Additional testing

The following scenarios should be checked as they depend on the area of the code that was amended:

Checklist

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 renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

ghost · 2023-09-14T15:18:04Z

🤖 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!)

jillguyonnet · 2023-09-19T09:10:10Z

@elasticmachine merge upstream

…-ref HEAD~1..HEAD --fix'

elasticmachine · 2023-09-19T10:57:32Z

Pinging @elastic/fleet (Team:Fleet)

joshdover

Is there a test here that reproduces the bug and would fail if the code changes weren't present? It wasn't obvious to me which test covers this scenario

joshdover · 2023-09-19T12:01:09Z

-                showInactive,
-              }),
-            ]);
+          const [


This useCallback hook is getting really big - should we extract this into a dedicated hook that can easily be tested in isolation? I worry that we don't have much coverage on this logic simply becuase we have to test the whole component.

juliaElastic · 2023-09-19T13:01:03Z

    const props: any = {
-      totalAgents: 10,
+      totalAgentsPaginated: 10,
+      totalInactiveAgentsPaginated: 0,


What do the paginated variables represent? It would be good to add some explaining comments why we need totalInactiveAgents and totalInactiveAgentsPaginated as separate vars.

I agree the naming is somewhat cumbersome, this is because there was a confusion about what "total" meant: in the current implementation, totalAgents meant all agents on the page (paginated API call) while totalInactiveAgents meant all inactive agents as it was obtained from getAgentStatus. This confusion caused the bug.

We need to know how many inactive agents there are on the page (so totalInactiveAgentsPaginated) for the Actions menu logic.

As for totalInactiveAgents, it is actually used by the AgentStatusFilter component as part of logic to determine whether there are some newly inactive agents. I had a look at it, but it looks to me like it does rely on the total number of inactive agents.

WDYT? Could there be a way to make the above simpler or clearer?

Thanks for the explanation, it helps.
I think then we could make the naming clearer, what about renaming totalAgents to totalAgentsOnCurrentPage and totalInactiveAgentsPaginated to inactiveAgentsOnCurrentPage?

jillguyonnet · 2023-09-20T15:12:26Z

Hey @juliaElastic - thanks for your question about paginated variables, as I was refactoring the variable names I found out that my understanding hadn't been completely correct. I've made new changes with hopefully clearer naming. In the agent list root page (agent_list_page/index.tsx), these are the relevant variables:

agentsOnCurrentPage: list of agents shown on the current page
shownAgents: number of agents listed in the table (not just the current page)
inactiveShownAgents: number of inactive agents listed in the table
totalInactiveAgents: number of inactive agents in total regardless of filtering (so that inactiveShownAgents is either 0 or equal to totalInactiveAgents depending on whether inactive agents are included in the filter)
totalManagedAgentIds: list of agent ids with managed policy in total
managedAgentsOnCurrentPage: number of agents with managed policy on current page

I am not completely happy with the data fetching that file in the current state. As Josh pointed out, it is getting very long, plus I am concerned that the additional fetching of managed agents data is not implemented in the most efficient way. I might require a few pointers in order to sort that out.

jillguyonnet · 2023-09-20T16:32:47Z

@elasticmachine merge upstream

jillguyonnet · 2023-09-21T09:00:18Z

@elasticmachine merge upstream

jillguyonnet · 2023-09-25T14:15:18Z

@elasticmachine merge upstream

jillguyonnet · 2023-09-26T08:45:04Z

@elastic/fleet I was checking that the tests cover what we want and I need to clarify the following question: the logic for the number of agents considered for bulk actions is different between manual and query modes. Is this expected? Note that this is existing logic.

Edit: found a mention that active is only set to false in the index if the agent is unenrolled, so I guess what I'm looking for is confirmation that this the definition of "active" that should be valid in this logic.

Details:

Assuming I have 1 fleet server, 1 active agent, 30 inactive agents (by inactive, I mean the Fleet status, not the active field in the agent, which is true).
When I select inactive agents manually, they are counted towards bulk action.
When I click "Select everything on all pages" and there are selected inactive agents, they are not counted towards bulk actions.

Manual mode:

Query mode:

Inactive Fleet status but active agent:

jillguyonnet · 2023-09-26T09:03:05Z

Per conversation with @jlind23 - we'll address the above question separately.

juliaElastic · 2023-09-26T09:03:49Z

When I click "Select everything on all pages" and there are selected inactive agents, they are not counted towards bulk actions.

Is there an extra check/filter that excludes inactive agents in this case?
I don't recall deliberately excluding inactive agents, we could check the history to see why it was implemented this way.
I think it would be good to have consistency in manual and query actions, to me it sounds simpler to include inactive in both cases.

juliaElastic · 2023-09-26T09:11:01Z

+            showInactive ? totalInactiveAgentsResponse.data.results.inactive || 0 : 0
+          );
+
+          const managedAgentPolicies = managedAgentPoliciesResponse.data?.items ?? [];


should we add more unit tests on the logic here?

juliaElastic

LGTM, thanks for the changes.

kibana-ci · 2023-09-26T09:27:23Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: d1ab6fc

Failed CI Steps

Test Failures

[job] [logs] FTR Configs #23 / serverless examples UI Field formats example "before all" hook for "renders field formats example 1"

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
`fleet`	1.2MB	1.2MB	+425.0B

History

💛 Build #161796 was flaky 80583bd
💔 Build #160664 failed 948ee22
💔 Build #160483 failed aec1828
💔 Build #160434 failed 4c8f7a9
💛 Build #159865 was flaky ec48163

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

jillguyonnet · 2023-09-26T09:27:37Z

Thanks @juliaElastic for reviewing! I'll open a separate issue for the inactive agents question, I agree it should be consistent.

Regarding unit testing on managed agents, this is tested in children components as it only affects the props the parent is passing down. There is another task on this table coming up (https://github.com/elastic/ingest-dev/issues/1937) which will likely complexify the state, which I think would be a good opportunity to refactor how it is handled.

joshdover · 2023-09-26T14:21:51Z

@jillguyonnet Can we backport this to 8.10.x as well?

jillguyonnet · 2023-09-26T15:13:46Z

@joshdover Ah sorry! Can this be done after merging?

joshdover · 2023-09-26T16:04:00Z

No worries, just wanted to make sure there weren't any technical reasons not to. Yes you can by updating the labels, I went ahead and did it. The automation should pick it up soon.

kibanamachine · 2023-09-26T16:12:35Z

💔 All backports failed

Status	Branch	Result
❌	8.10	Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 166475

Questions ?

Please refer to the Backport tool documentation

jillguyonnet · 2023-09-27T07:51:21Z

💚 All backports created successfully

Status	Branch	Result
✅	8.10

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

Questions ?

Please refer to the Backport tool documentation

Closes elastic#164083 Related to elastic/sdh-beats#3759 Related to elastic#157844 This PR addresses two current issues affecting agent selection in Fleet UI: 1. When there are inactive agents that are not listed on the current page and "Select everything on all pages" is clicked, the count of actionable agents is incorrect (cf. [this comment](elastic#164083 (comment)) for details). This can have two consequences: 1. Incorrect and sometimes negative agent count in the "Actions" dropdown. 2. Disabled menu items in the "Actions" dropdown. 2. The "Select everything on all pages button is incorrectly displayed when there are agents on managed policies on the page and there is no pagination (cf. [this comment](elastic#164083 (comment))). (cherry picked from commit 591df70) # Conflicts: # x-pack/plugins/fleet/public/applications/fleet/sections/agents/agent_list_page/components/bulk_actions.test.tsx

# Backport This will backport the following commits from `main` to `8.10`: - [[Fleet] Fix bulk action dropdown (#166475)](#166475)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)

[Fleet] Fix bulk action dropdown

d1941c7

jillguyonnet added Team:Fleet - DEPRECATED Use Team:streams-ui v8.11.0 release_note:fix labels Sep 14, 2023

jillguyonnet added 2 commits September 18, 2023 17:18

Fix existing tests

a819ee3

Add unit tests for agents selection status component

b18a22b

kibanamachine and others added 3 commits September 19, 2023 05:10

Merge branch 'main' into fleet/fix-bulk-action-dropdown

d92e5ad

Remove unused var

70ad1c6

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

ec48163

…-ref HEAD~1..HEAD --fix'

jillguyonnet marked this pull request as ready for review September 19, 2023 10:57

jillguyonnet requested a review from a team as a code owner September 19, 2023 10:57

joshdover reviewed Sep 19, 2023

View reviewed changes

juliaElastic reviewed Sep 19, 2023

View reviewed changes

Fixes

4c8f7a9

Merge branch 'main' into fleet/fix-bulk-action-dropdown

aec1828

Merge branch 'main' into fleet/fix-bulk-action-dropdown

948ee22

kpollich self-requested a review September 22, 2023 15:20

Fix data fetching

5063321

kibanamachine and others added 2 commits September 25, 2023 10:15

Merge branch 'main' into fleet/fix-bulk-action-dropdown

80583bd

Remove commented out code

d1ab6fc

juliaElastic reviewed Sep 26, 2023

View reviewed changes

juliaElastic approved these changes Sep 26, 2023

View reviewed changes

jillguyonnet mentioned this pull request Sep 26, 2023

[Fleet] Inactive status agents are only considered for bulk actions when manually selected #167269

Closed

jillguyonnet merged commit 591df70 into elastic:main Sep 26, 2023

kibanamachine added the backport:skip This PR does not require backporting label Sep 26, 2023

jillguyonnet deleted the fleet/fix-bulk-action-dropdown branch September 26, 2023 13:15

jillguyonnet mentioned this pull request Sep 26, 2023

[Fleet] Bulk action dropdown is disabled and shows negative agent counts #164083

Closed

joshdover added backport:prev-minor and removed backport:skip This PR does not require backporting labels Sep 26, 2023

jillguyonnet mentioned this pull request Sep 27, 2023

[8.10] [Fleet] Fix bulk action dropdown (#166475) #167368

Merged

kibanamachine added the v8.10.3 label Sep 27, 2023

This was referenced Oct 1, 2023

[8.10] fix(slo): Handle partial indicator url state (#167247) #167722

Merged

[8.10] backport SLO fixes #168231

Merged

criamico mentioned this pull request Jan 22, 2024

[Fleet]: Inactive agents count gets incorrect when everything on all pages is selected. #171914

Closed

Conversation

jillguyonnet commented Sep 14, 2023 • edited by lucabelluccini Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Testing steps

Reproduce the bug (main branch)

Test the fixes (this branch)

Additional testing

Checklist

Uh oh!

ghost commented Sep 14, 2023

🤖 GitHub comments

Uh oh!

jillguyonnet commented Sep 19, 2023

Uh oh!

elasticmachine commented Sep 19, 2023

Uh oh!

joshdover left a comment

Choose a reason for hiding this comment

Uh oh!

joshdover Sep 19, 2023

Choose a reason for hiding this comment

Uh oh!

juliaElastic Sep 19, 2023

Choose a reason for hiding this comment

Uh oh!

jillguyonnet Sep 19, 2023

Choose a reason for hiding this comment

Uh oh!

juliaElastic Sep 19, 2023

Choose a reason for hiding this comment

Uh oh!

jillguyonnet commented Sep 20, 2023

Uh oh!

jillguyonnet commented Sep 20, 2023

Uh oh!

jillguyonnet commented Sep 21, 2023

Uh oh!

jillguyonnet commented Sep 25, 2023

Uh oh!

jillguyonnet commented Sep 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jillguyonnet commented Sep 26, 2023

Uh oh!

juliaElastic commented Sep 26, 2023

Uh oh!

juliaElastic Sep 26, 2023

Choose a reason for hiding this comment

Uh oh!

juliaElastic left a comment

Choose a reason for hiding this comment

Uh oh!

kibana-ci commented Sep 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Async chunks

History

Uh oh!

jillguyonnet commented Sep 26, 2023

Uh oh!

joshdover commented Sep 26, 2023

Uh oh!

jillguyonnet commented Sep 26, 2023

Uh oh!

joshdover commented Sep 26, 2023

Uh oh!

kibanamachine commented Sep 26, 2023

💔 All backports failed

Manual backport

Questions ?

Uh oh!

jillguyonnet commented Sep 27, 2023

💚 All backports created successfully

Questions ?

Uh oh!

Reviewers

jillguyonnet commented Sep 14, 2023 •

edited by lucabelluccini

Loading

jillguyonnet commented Sep 26, 2023 •

edited

Loading

kibana-ci commented Sep 26, 2023 •

edited

Loading