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.

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

[Fleet] Fix syncing issue in Agent flyout #135734

Merged
merged 4 commits into from
Jul 5, 2022
Merged

[Fleet] Fix syncing issue in Agent flyout #135734

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dbe18e6
[Fleet] Fix syncing issue in Agent flyout
criamico Jul 5, 2022
6bd8326
Use context based solution instead of polling
kpollich Jul 5, 2022
78f7eec
Add clarifying comment
kpollich Jul 5, 2022
5f8d45a
Fix types
kpollich Jul 5, 2022
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
46 changes: 26 additions & 20 deletions x-pack/plugins/fleet/public/components/agent_enrollment_flyout/instructions.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,6 @@ import { FleetServerRequirementPage } from '../../applications/fleet/sections/ag

import { AGENTS_PREFIX, FLEET_SERVER_PACKAGE, SO_SEARCH_LIMIT } from '../../constants';

import { useFleetServerUnhealthy } from '../../applications/fleet/sections/agents/hooks/use_fleet_server_unhealthy';

import { Loading } from '..';

import { policyHasFleetServer } from '../../services';
Expand All @@ -42,11 +40,16 @@ export const Instructions = (props: InstructionProps) => {
refreshAgentPolicies,
} = props;
const fleetStatus = useFleetStatus();
const { isUnhealthy: isFleetServerUnhealthy } = useFleetServerUnhealthy();
const REFRESH_INTERVAL = 10 * 1000;
Copy link
Member

Choose a reason for hiding this comment

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

there is no way to fix the root problem and refresh the status when fleet server is added? or at least only once when the flyout is open?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reason when the flyout is first open the fleetStatus is not ready, so you have to reload the page to get it. I couldn't find a solution that refreshes the state, I tried in several ways using useEffect but they mostly ended up int too many rerenders errors.

Copy link
Member

Choose a reason for hiding this comment

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

I don't know if polling is the right solution here. Is there a way we can add fleetStatus.refresh to wherever we currently wait on the the existence of a Fleet Server? Because fleetStatus is a context object, we should be able to call refresh and update any other consumers of that context object from anywhere in the component tree.

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 tried using fleetStatus.refresh without the setInterval as a first try and didn't work as expected.
An issue that I had a lot with this component is that react signals too many rerenders, which is probably a sign that we are doing something weird.

The solutions I could think of are basically 1- the polling that I implemented here 2- doing as many checks as possible in the parent components of the flyout, but since is called several times in the code it would be a lot of refactoring for a small bug fix.

Copy link
Member

@nchaulet nchaulet Jul 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@criamico if you put the fleetStatus.refresh in a useEffect with an empty array it should only refresh once no?
something like this
useEffect(() => fleetStatus.refresh(), []);


useEffect(() => {
refreshAgentPolicies();
}, [refreshAgentPolicies]);
const interval = setInterval(() => {
fleetStatus.refresh();
refreshAgentPolicies();
}, REFRESH_INTERVAL);

return () => clearInterval(interval);
}, [fleetStatus, REFRESH_INTERVAL, refreshAgentPolicies]);

const fleetServerAgentPolicies: string[] = useMemo(
() => agentPolicies.filter((pol) => policyHasFleetServer(pol)).map((pol) => pol.id),
Expand All @@ -64,27 +67,30 @@ export const Instructions = (props: InstructionProps) => {
.join(' or ')}`,
});

const fleetServers = agents?.items || [];
const agentsWithFleetServers = agents?.items || [];

const fleetServerHosts = useMemo(() => {
return settings?.fleet_server_hosts || [];
const hasFleetServerHosts = useMemo(() => {
return (settings?.fleet_server_hosts || []).length > 0;
}, [settings]);

if (isLoadingAgents || isLoadingAgentPolicies) return <Loading size="l" />;

const hasNoFleetServerHost = fleetStatus.isReady && fleetServerHosts.length === 0;
const showAgentEnrollment = useMemo(
() => hasFleetServerHosts && fleetStatus.isReady && agentsWithFleetServers.length > 0,
[hasFleetServerHosts, fleetStatus.isReady, agentsWithFleetServers.length]
);

const showAgentEnrollment =
fleetStatus.isReady &&
!isFleetServerUnhealthy &&
fleetServers.length > 0 &&
fleetServerHosts.length > 0;
const showFleetServerEnrollment = useMemo(
() =>
!showAgentEnrollment ||
(fleetStatus.missingRequirements ?? []).some((r) => r === FLEET_SERVER_PACKAGE),
[fleetStatus.missingRequirements, showAgentEnrollment]
);

const showFleetServerEnrollment =
fleetServers.length === 0 ||
isFleetServerUnhealthy ||
(fleetStatus.missingRequirements ?? []).some((r) => r === FLEET_SERVER_PACKAGE);
const hasNoFleetServerHost = useMemo(
() => fleetStatus.isReady && (settings?.fleet_server_hosts || []).length === 0,
[fleetStatus.isReady, settings?.fleet_server_hosts]
);

if (isLoadingAgents || isLoadingAgentPolicies) return <Loading size="l" />;
if (!isIntegrationFlow && showAgentEnrollment) {
setSelectionType('radio');
} else {
Expand Down