fix: set readiness once only

[Kavindu-Dodan]

This PR

The current readiness state can be changed because of an underlying sync provider error. This seems to be incorrect as not ready state make flagd to not receive traffic through K8s services [1]

For example, if HTTP endpoint-backed sync provider get an error, the current logic set readiness to false. Similarly, if grpc connectivity is lost for grpc provider, readiness goes to false state till connection establishment completes.

This PR fix this behavior by setting the ready state once only per sync provider. This means if there is at least a single successful sync, sync provider sets its readiness to true.

Going further, we might need to decide if we need better readiness state handling.

[1] - https://kubernetes.io/docs/tasks/configure-pod-container/configure-liveness-readiness-startup-probes/#define-readiness-probes

[codecov]

Codecov Report

Merging #465 (e8add4d) into main (f5ca5f9) will decrease coverage by 0.13%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #465      +/-   ##
==========================================
- Coverage   62.24%   62.12%   -0.13%     
==========================================
  Files          13       13              
  Lines        1658     1650       -8     
==========================================
- Hits         1032     1025       -7     
+ Misses        563      562       -1     
  Partials       63       63              

Impacted Files	Coverage Δ
pkg/sync/http/http_sync.go	43.29% <ø> (-2.71%)	⬇️
pkg/sync/grpc/grpc_sync.go	62.04% <100.00%> (+0.07%)	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[thisthat]

The readiness state should change since this tells K8s when traffic can be scheduled to that Pod, but it depends on our semantics of ready state. With this change, we lost consistency in favor of availability and partition tolerance. Given that flagd is injected as a sidecar by OFO, I suggest going ahead with this change since it makes it more resilient and doesn't block traffic to the original pod.

[toddbaert]

I may have misunderstood the first PR introducing health checks or misread the code, because I thought this was already how it was implemented:

This PR fix this behavior by setting the ready state once only per sync provider. This means if there is at least a single successful sync, sync provider sets its readiness to true.

😅

[toddbaert]

Can we add some tests to establish expectations around this behavior? Perhaps it will be easier once this is merged?

[beeme1mr]

Can you please include a section in the docs that describes the behavior in a follow up PR? An example of what I'm looking for can be found here. Users should be able to understand what endpoints are available and the behavior to expect when using them.

[skyerus]

Nice find

[Kavindu-Dodan]

@toddbaert I added readiness state validation except for K8s