headlamp-k8s / headlamp

A Kubernetes web UI that is fully-featured, user-friendly and extensible
https://headlamp.dev
Apache License 2.0
2.42k stars 170 forks source link

Fix health check AlertNotification #2571

Open callmevladik opened 2 weeks ago

callmevladik commented 2 weeks ago

Describe the bug

AlertNotification has "Try again" button which doesn't do anything, the potential reason could be that some of the dependencies in useEffect are not updated or badly composed registerSetInterval function with some closure complexity.

To Reproduce

Steps to reproduce the bug:

  1. Go to any page with possible AlertNotification
  2. Mock error behaviour
  3. Click try again button, so that immediately after clicking the "healthz" request must be triggered instantly and AlertNotification must disappear, but actually nothing happens.

For testing purposes

As I didn't know how to mock server healthz response, I just setup [counter, setCounter] state, added condition "if counter % 2 === 0" inside performHealthCheck with setting error if true, else "return checkerFunction()...", also I added click handler which increments counter and invokes performHealthCheck with new counter value.

So with my mocking the scenario was:

  1. I see error, because counter === 0, I don't see any "healthz" requests.
  2. I click "Try again" and check browser network tab
  3. I see new "healthz" request instantly and AlertNotification disappears
  4. I see new "healthz" requests each 5 secs after

`export function PureAlertNotification({ checkerFunction }: PureAlertNotificationProps) { const [networkStatusCheckTimeFactor, setNetworkStatusCheckTimeFactor] = React.useState(0); const [error, setError] = React.useState<null | string | boolean>(null); const { t } = useTranslation(); const { pathname } = useLocation(); const [counter, setCounter] = React.useState(0);

const performHealthCheck = React.useCallback((counter: number) => { if (!window.navigator.onLine) { setError('Offline'); return; }

// Don't check for the cluster health if we are not on a cluster route.
if (!getCluster()) {
  setError(null);
  return;
}

if (counter % 2 === 0) {
  setError('Counter error');
} else {
  return checkerFunction()
    .then(() => setError(false))
    .catch(err => {
      setError(err.message);
      setNetworkStatusCheckTimeFactor(prev => prev + 1);
    });
}

}, []);

React.useEffect(() => { const interval = setInterval( () => performHealthCheck(counter), (networkStatusCheckTimeFactor + 1) * NETWORK_STATUS_CHECK_TIME ); return () => clearInterval(interval); }, [performHealthCheck, networkStatusCheckTimeFactor, counter]);

// Make sure we do not show the alert notification if we are not on a cluster route. React.useEffect(() => { if (!getCluster()) { setError(null); } }, [pathname]);

const showOnRoute = React.useMemo(() => { for (const route of ROUTES_WITHOUT_ALERT) { const routePath = getRoutePath(getRoute(route)); if (matchPath(pathname, routePath)?.isExact) { return false; } } return true; }, [pathname]);

if (!error || !showOnRoute) { return null; }

const handleClick = () => { setCounter(prev => { const newValue = prev + 1; performHealthCheck(newValue); return newValue; }); };

return ( <Alert variant="filled" severity="error" sx={theme => ({ color: theme.palette.common.white, background: theme.palette.error.main, textAlign: 'center', display: 'flex', paddingTop: theme.spacing(0.5), paddingBottom: theme.spacing(1), paddingRight: theme.spacing(3), justifyContent: 'center', position: 'fixed', zIndex: theme.zIndex.snackbar + 1, top: '0', alignItems: 'center', left: '50%', width: 'auto', transform: 'translateX(-50%)', })} action={ <Button sx={theme => ({ color: theme.palette.error.main, borderColor: theme.palette.error.main, background: theme.palette.common.white, lineHeight: theme.typography.body2.lineHeight, '&:hover': { color: theme.palette.common.white, borderColor: theme.palette.common.white, background: theme.palette.error.dark, }, })} onClick={handleClick} size="small"

{t('translation|Try Again')} }

<Typography variant="body2" sx={theme => ({ paddingTop: theme.spacing(0.5), fontWeight: 'bold', fontSize: '16px', })}

{t('translation|Lost connection to the cluster.')} ); }`

skoeva commented 2 weeks ago

Hi, thanks for opening this PR! Regarding the issue here, it would be best to log it in the repo here

image

And then you can link the issue to this PR and avoid having a super long PR description. This should generally be reserved for a short description of the changes, along with any helpful instructions to test the changes for anyone reviewing.

Feel free to reference our contribution guidelines and reach out if you have any questions :)

illume commented 1 week ago

Hi @callmevladik

thanks for this fix! Appreciate it.

A few notes about the CI build errors: