Modify updateConditions to use linearizable Get#405
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: cmallikarjunah The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @cmallikarjunah. Thanks for your PR. I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Signed-off-by: Chandana Mallikarjuna <Chandana.Mallikarjuna@ibm.com>
4ab9619 to
4232002
Compare
| cancel() | ||
| _ = cli.Close() | ||
|
|
||
| if err == nil || errors.Is(err, rpctypes.ErrPermissionDenied) { |
There was a problem hiding this comment.
It's better if this part of the code is reviewed by @ahrtr . Could you please check if this is the right way to check the cluster health?
| availableCondition.Message = fmt.Sprintf("Cluster is available: linearizable read succeeded (%d members)", len(s.memberListResp.Members)) | ||
| } else { | ||
| availableCondition.Message = fmt.Sprintf("Etcd cluster has %d/%d healthy members, quorum requires %d", healthyCount, len(s.memberListResp.Members), quorum) | ||
| availableCondition.Message = fmt.Sprintf("Cluster is unavailable: no member responded to a linearizable read (%d members)", len(s.memberListResp.Members)) |
There was a problem hiding this comment.
I think it would be good to keep how many PRs out of all are available. With this change only total members is being published as a part of the statues.
Probably, if the IsClusterAvailable function can return that healthycount, that could be better.
| } | ||
|
|
||
| // No leader elected or leader not yet established, so fail fast. | ||
| if errors.Is(err, rpctypes.ErrNoLeader) || errors.Is(err, rpctypes.ErrNotLeader) { |
There was a problem hiding this comment.
If the cluster logic is right, it is necessary to return the error or log it here. Please take care of the same.
2 participants
Ref Issue: #357
NOTE:
Added a separate function IsClusterAvailable() instead of modifying ClusterHealth(). ClusterHealth uses Serializable Get and is needed to check health against each member by other methods.
IsClusterAvailable issues a linearizable Get against each provided endpoint in turn and returns true as soon as any one responds successfully.