I'm a bit unsure of the attempts to retain previous behavior regarding connecting to peers eagerly, though I'm still looking at the options we have available. I'm also a bit concerned about the fact that Connect() is experimental.

I'm also not sure if this is true:

the manager would report connected right away for all peers that could create a ClientConn before we could confirm if we can actually reach them.

Because at least in some areas it's reporting the underlying connection state, so it would report idle in that case, until an RPC is made.

Can you clarify what you mean? Perhaps you could show what hubble list nodes reports with and without the extra logic? Maybe also what the hubble list nodes reports before and after an hubble observe flows?

I'm a bit unsure of the attempts to retain previous behavior regarding connecting to peers eagerly, though I'm still looking at the options we have available. I'm also a bit concerned about the fact that Connect() is experimental.

I'm also not sure if this is true:

the manager would report connected right away for all peers that could create a ClientConn before we could confirm if we can actually reach them.

Because at least in some areas it's reporting the underlying connection state, so it would report idle in that case, until an RPC is made.

Can you clarify what you mean? Perhaps you could show what hubble list nodes reports with and without the extra logic? Maybe also what the hubble list nodes reports before and after an hubble observe flows?

My bad, I actually meant connected as in the conn being set on the peer struct from manager.connect() and the log statement "connected" emitted for the peer. I felt this could be semi-lying compared to the previous impl where we would fail right away.

But I just looked at the observer server, and it does filter on the state, and I guess the peer will correctly report a failed state on first rpc and be filtered out?

I think I'll move this back to draft and play a bit with this plus update/add tests to see how it behaves.

Alright I must admit I made my life more difficult for no reason. I updated the PR and removed the WaitForReady logic which after testing does not seem necessary at all.

/test

I'm a bit unsure of the attempts to retain previous behavior regarding connecting to peers eagerly, though I'm still looking at the options we have available. I'm also a bit concerned about the fact that Connect() is experimental.
I'm also not sure if this is true:

the manager would report connected right away for all peers that could create a ClientConn before we could confirm if we can actually reach them.

Because at least in some areas it's reporting the underlying connection state, so it would report idle in that case, until an RPC is made.
Can you clarify what you mean? Perhaps you could show what hubble list nodes reports with and without the extra logic? Maybe also what the hubble list nodes reports before and after an hubble observe flows?

My bad, I actually meant connected as in the conn being set on the peer struct from manager.connect() and the log statement "connected" emitted for the peer. I felt this could be semi-lying compared to the previous impl where we would fail right away.

But I just looked at the observer server, and it does filter on the state, and I guess the peer will correctly report a failed state on first rpc and be filtered out?

I think I'll move this back to draft and play a bit with this plus update/add tests to see how it behaves.

Can you provide the output before/after first RPC?

Can you provide the output before/after first RPC?

It says connected for all peers regardless if an RPC has been made or not.

I also tested with hubble how it reacts when a hubble peer disappears:

$ ./hubble list nodes -P --tls --tls-allow-insecure
NAME                                                            STATUS      AGE     FLOWS/S   CURRENT/MAX-FLOWS
hubble-grpc-dialcontext/hubble-grpc-dialcontext-control-plane   Connected   2m54s   1.24      248/4095 (  6.06%)
hubble-grpc-dialcontext/hubble-grpc-dialcontext-worker          Connected   7m54s   17.47     4095/4095 (100.00%)

# kill cilium node

$ ./hubble list nodes -P --tls --tls-allow-insecure
NAME                                                            STATUS      AGE     FLOWS/S   CURRENT/MAX-FLOWS
hubble-grpc-dialcontext/hubble-grpc-dialcontext-control-plane   Connected   2m55s   1.29      260/4095 (  6.35%)
hubble-grpc-dialcontext/hubble-grpc-dialcontext-worker          Error       N/A     N/A       N/A

$ ./hubble list nodes -P --tls --tls-allow-insecure
NAME                                                            STATUS        AGE     FLOWS/S   CURRENT/MAX-FLOWS
hubble-grpc-dialcontext/hubble-grpc-dialcontext-control-plane   Connected     2m56s   1.31      265/4095 (  6.47%)
hubble-grpc-dialcontext/hubble-grpc-dialcontext-worker          Unavailable   N/A     N/A       N/A

$ ./hubble list nodes -P --tls --tls-allow-insecure
NAME                                                            STATUS      AGE     FLOWS/S   CURRENT/MAX-FLOWS
hubble-grpc-dialcontext/hubble-grpc-dialcontext-control-plane   Connected   3m11s   1.48      318/4095 (  7.77%)
hubble-grpc-dialcontext/hubble-grpc-dialcontext-worker          Connected   6s      25.67     205/4095 (  5.01%)

Can you provide the output before/after first RPC?

It says connected for all peers regardless if an RPC has been made or not.

I also tested with hubble how it reacts when a hubble peer disappears:

$ ./hubble list nodes -P --tls --tls-allow-insecure
NAME                                                            STATUS      AGE     FLOWS/S   CURRENT/MAX-FLOWS
hubble-grpc-dialcontext/hubble-grpc-dialcontext-control-plane   Connected   2m54s   1.24      248/4095 (  6.06%)
hubble-grpc-dialcontext/hubble-grpc-dialcontext-worker          Connected   7m54s   17.47     4095/4095 (100.00%)

# kill cilium node

$ ./hubble list nodes -P --tls --tls-allow-insecure
NAME                                                            STATUS      AGE     FLOWS/S   CURRENT/MAX-FLOWS
hubble-grpc-dialcontext/hubble-grpc-dialcontext-control-plane   Connected   2m55s   1.29      260/4095 (  6.35%)
hubble-grpc-dialcontext/hubble-grpc-dialcontext-worker          Error       N/A     N/A       N/A

$ ./hubble list nodes -P --tls --tls-allow-insecure
NAME                                                            STATUS        AGE     FLOWS/S   CURRENT/MAX-FLOWS
hubble-grpc-dialcontext/hubble-grpc-dialcontext-control-plane   Connected     2m56s   1.31      265/4095 (  6.47%)
hubble-grpc-dialcontext/hubble-grpc-dialcontext-worker          Unavailable   N/A     N/A       N/A

$ ./hubble list nodes -P --tls --tls-allow-insecure
NAME                                                            STATUS      AGE     FLOWS/S   CURRENT/MAX-FLOWS
hubble-grpc-dialcontext/hubble-grpc-dialcontext-control-plane   Connected   3m11s   1.48      318/4095 (  7.77%)
hubble-grpc-dialcontext/hubble-grpc-dialcontext-worker          Connected   6s      25.67     205/4095 (  5.01%)

Hmm, I was hoping it would report "idle" or "unknown" until we've actually established a connection, after which it would report Connected/Unavailable.

Upon investigation, it looks liket it sets the state to Connected based on the gRPC state not being Shutdown/TransientError. However, it then goes onto make a ServerStatus RPC, so I guess as long as that succeeds, it is in a connected state, so I think it's working as expected.

/test

/test

/test

Thank you @devodev, patch LGTM now. Please open an issue to address the upcoming removal of both the .Values.hubble.relay.dialTimeout Helm value along with the --dial-timeout flag of Hubble Relay so we don't forget about them.

Here we go: #36240