Fix hashring for all possible failure scenarios

[spaparaju]

This PR adds a new flag 'allow-only-ready-replicas' for hashring to contain Thanos Receive replicas which are in the 'Ready' status.

Under this flag, this PR includes fixes for :

1. hashring containing non-Ready replicas if a replica turns from 'Ready' status to a 'non-ready' status.
2. hashring containing non-Ready replicas if a scale up does not succeed to the completion (spec.Replicas != status.ReadyReplicas)

Signed-off-by: SriKrishna Paparaju paparaju@gmail.com

[bill3tt]

Reading the code it appears that we have always mixed up a pod in Running phase with a container Ready to serve requests - it would be good to clean that up in this PR.

Also - I'm wondering if there is a smarter way we can be informed of changes to the hashrings we are watching?

The newFilteredStatefulSetInformer function accepts a filtering function - perhaps we could parse out the pods that are suitable there?

edit: I think we could use the ListOptions.FieldSelectors to do this.

[spaparaju]

Reading the code it appears that we have always mixed up a pod in Running phase with a container Ready to serve requests - it would be good to clean that up in this PR.

Also - I'm wondering if there is a smarter way we can be informed of changes to the hashrings we are watching?

The newFilteredStatefulSetInformer function accepts a filtering function - perhaps we could parse out the pods that are suitable there?

edit: I think we could use the ListOptions.FieldSelectors to do this.

pods get to Running status once the readiness probe is successful.
The current sync loop leverages status changes notified through Kube. informers.

[bill3tt]

@spaparaju and I chatted about this PR last Thursday, there are a couple of things to do before merging:

• Test out this change in a local cluster and verify that we see the behaviour we desire & report back results in this PR.
• Eiher update the naming of the flag to be running rather than ready to better represent the work currently being done or update the mechanism for waiting to poll for readiness in the pods.

[spaparaju]

@spaparaju and I chatted about this PR last Thursday, there are a couple of things to do before merging:

• Test out this change in a local cluster and verify that we see the behaviour we desire & report back results in this PR.
  Tested on Minikube. Here are the recordings of the testing: reproduce the hash-ring update problem and how this PR solves the update problem.
• Eiher update the naming of the flag to be running rather than ready to better represent the work currently being done or update the mechanism for waiting to poll for readiness in the pods.
  renamed the flag to include 'running' instead of the word 'ready'

[bill3tt]

Really nice demos there @spaparaju what an effective way to do code review :) LGTM

[matej-g]

Looking good!

[matej-g]

The CI seem to be stuck, probably for some time already 😞, we should take a look.

[bill3tt]

@bwplotka do you have perms to nuke this and start it again?

[matej-g]

Hm so I also don't seem to have permission to change settings, since I don't see the Settings tab for this repo, we might need to escalate to someone who is able to check if there is any related CI setting.

[bwplotka]

Not sure about change, but does not look significant, lgtm

[bwplotka]

Why this was moved?

[bwplotka]

Unfortunately I don't have perms to this - also it looks like there is no link to drone CI so connection did not even start. Maybe @squat has some ideas?

If not we should spend 30m to move to githubActions quickly. Otherwsie we would merge this PR without any CI.

[squat]

I think that the hosted version of Drone CI hasn't been working so well ever since it was acquired by Harness or it has been entirely decommissioned :/ you can't even find https://cloud.drone.io on google anymore. I just disabled and re-enabled the project in Drone and now webhooks are working again, but CI still doesn't run. I think it's time to finally move this project onto GitHub Actions. And also rename master -> main. This repo needs a bit of TLC

[michael-burt]

I opened #89 to address the issue where non-ready replicas were being populated in the hashring configuration. My approach is similar to the one taken in this PR, however I filter only for Ready replicas since Running is not sufficient to ensure that the replica is able to accept write remote write requests.

[matej-g]

Unless there are objections, I think we should close this in favor of #89, I believe the approach to change hashring only when scaling the stateful set to be preferable here.