feat: check connectivity with health check

[feloy]

What does this PR do?

check connectivity with health check, in the context of experimental kubernetes states,
and saves the state in a ContextsConnectivityRegistry.

The next step is to dispatch the state to the frontend.

(The first commit of the PR is a copy of the one in #10074)

Screenshot / video of UI

backend only

What issues does this PR fix or reference?

Part of #9938

How to test this PR?

To test the new version, set this preference in ~/.local/share/containers/podman-desktop/configuration/settings.json:

  "kubernetes.statesExperimental": true

You can have some cluster behind a VPN to provoke some timeout and check by editing the kubeconfig file several times quickly (by changing the current context for example), that previous checks are aborted and only a response from the last (un-aborted) check is received.

Some logs can be added to the ContextsConnectivityRegistry to display the state each time it is changed

• Tests are covering the bug fix or the new feature

[benoitf]

IMHO I don't see the need of the registry, and the event in the health checker as it can just keep the value

ExperimentalChecker:

• map of healthzChecker
  for each checker there is inside the value if it's reachable or not

many disposables can be removed, and half of the code

[feloy]

IMHO I don't see the need of the registry, and the event in the health checker as it can just keep the value

ExperimentalChecker:

• map of healthzChecker
  for each checker there is inside the value if it's reachable or not

many disposables can be removed, and half of the code

I would like to validate that this pattern is valid with the idea that we want to send to the frontend all these states Map<contextName: string, state: { checking, reachable }> every time one of the state changes. In the current pattern, the registry is used to save this data ready to be sent, and as the object sending the data every time a set is called.

With this new pattern, we would also need some signal from the checkers to know when the state of a checker changes and to collect all the data from the checkers to send it.

[feloy]

I agree that the onReadiness(); checkReadiness(); is confusing, and that setting checking and reachable not atomically is confusing.

IMHO, as the checkers have a short life (they are killed as soon as the kubeconfig is modified again), it seems not reasonable to keep the state of the check inside them, as they will be deleted / recreated every time the update is called.

Here is a new proposal by simplfying the onReadiness / checkReadiness part and having an atomic setState (prototype, tests need to be finalized)

The registry is very simple, but I expect to add code to send the data to the frontend in this registry.

@benoitf WDYT?

[benoitf]

I think my issue is why having a separate registry. State should be closer to the object being checked.

IMHO, as the checkers have a short life (they are killed as soon as the kubeconfig is modified again), it seems not reasonable to keep the state of the check inside them, as they will be deleted / recreated every time the update is called.

On my side I see no issue having the state part of the object responsible to check the state of a context.
Also why they would be recreated/deleted if their data is the same as before ?
I can update the configuration of another context
They're the object closest to the state of the cluster. (the 'isolated kubeconfig part') If this part is not changing no need to delete/recreate them.

If you don't want to keep it as part of the HealthCheck object, make another layer between the two, but not a side instance
ContextChecker instance would use a HealthCheck instance.

Also the HealthCheck class is dedicated to a specific context as it's using a kubeconfig object which is a context. So I think it's missing in the name that it's for a specific Context. HealthCheckContext.

The registry is very simple, but I expect to add code to send the data to the frontend in this registry.

Yes it was my worries when I read the code. To me we should separate the inner state of the data vs what we want to expose publicly having the state the closest possible of the object maintaining it.

[benoitf]

wondering if options are not part of the constructor

[feloy]

I think my issue is why having a separate registry. State should be closer to the object being checked.

IMHO, as the checkers have a short life (they are killed as soon as the kubeconfig is modified again), it seems not reasonable to keep the state of the check inside them, as they will be deleted / recreated every time the update is called.

On my side I see no issue having the state part of the object responsible to check the state of a context. Also why they would be recreated/deleted if their data is the same as before ? I can update the configuration of another context They're the object closest to the state of the cluster. (the 'isolated kubeconfig part') If this part is not changing no need to delete/recreate them.

OK, I was checking if the whole kubeconfig is changed or not, but we can do the check at the isolated kube config level, and creating a health checker only if its specific context changes. This will make the checkers long-running, and make your pattern possible

I'll make the changes.

Thanks for your feedback

[feloy]

Not sure if we want to merge with this, but it can be useful for testing/reviewing, and I can remove it when LGTM'd

[benoitf]

ok for me to keep this trace

[benoitf]

thanks for all the improvements

it looks like easier to understand, and each part is doing and managing its own data, so in case of failure, easy to know which part is failing and it we want to enhance parts, contract being clear, we can easily swap one component by another

[axel7083]

LGTM