Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove sentinel cluster size validation no effect #536

Merged
merged 1 commit into from
Jul 1, 2023

Conversation

drivebyer
Copy link
Collaborator

Description

There is not Not validation in kubebuilder. We can use Enum to limit sentinel size to some odd number. https://book.kubebuilder.io/reference/markers/crd-validation.html

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • Testing has been performed
  • No functionality is broken
  • Documentation updated

@drivebyer
Copy link
Collaborator Author

PTAL @shubham-cmyk

@shubham-cmyk
Copy link
Member

shubham-cmyk commented Jul 1, 2023

I won't like to enforce an odd number of sentinels on users however having exactly 2 sentinels ( which is already taken care of ) could have conflict while making decisions but having a higher number of sentinels like 4,6,8 would be fine.

@drivebyer

What do you think about that

@shubham-cmyk
Copy link
Member

shubham-cmyk commented Jul 1, 2023

(For a cluster of n nodes, the quorum is n/2 + 1.)

Reference : https://martinfowler.com/articles/quorum.html

@drivebyer

@drivebyer
Copy link
Collaborator Author

hi @shubham-cmyk, according to https://redis.io/docs/management/sentinel/, the docs says:

The quorum is the number of Sentinels that need to agree about the fact the master is not reachable, in order to really mark the master as failing, and eventually start a failover procedure if possible.
However the quorum is only used to detect the failure. In order to actually perform a failover, one of the Sentinels need to be elected leader for the failover and be authorized to proceed. This only happens with the vote of the majority of the Sentinel processes.

So I think, the quorum and majority of the Sentinel processes is two things for sentinel cluster.

If clustersize is 5, the majority is 3, we can tolerate 2 sentinel failed. If If clustersize is 6, the majority is 2, we can tolerate 2 sentinel failed too. That's less efficiency.

@shubham-cmyk
Copy link
Member

I think we could remove // +kubebuilder:validation:Not=2 from the code and let the user decide the field clusterSize and quorum on it's own while providing the default value of the clusterSize=3 and quorum=2.

@drivebyer

@shubham-cmyk
Copy link
Member

shubham-cmyk commented Jul 1, 2023

Yes! We could use odd number but after 5 sentinel nodes there are very less chance of concurrent failure. so I think there would be hardly any improvement the main issue is with exactly 2 which posses most risk of failure.

@shubham-cmyk
Copy link
Member

shubham-cmyk commented Jul 1, 2023

But I am writing a validation webhook for the same where we could take care of this so right now.
You can just remove // +kubebuilder:validation:Not=2

@drivebyer

@drivebyer
Copy link
Collaborator Author

But I am writing a validation webhook for the same where we could take care of this so right now.
You can just remove // +kubebuilder:validation:Not=2

Yes of course

Signed-off-by: drivebyer <yang.wu@daocloud.io>
@shubham-cmyk shubham-cmyk merged commit ade31c3 into OT-CONTAINER-KIT:master Jul 1, 2023
16 checks passed
@drivebyer drivebyer deleted the fix_validation branch July 1, 2023 12:59
@shubham-cmyk shubham-cmyk changed the title Feat: sentinel cluster size must be odd number Remove sentinel cluster size validation no effect Jul 3, 2023
jiuker pushed a commit to jiuker/redis-operator-1 that referenced this pull request Jul 20, 2023
)

Signed-off-by: drivebyer <yang.wu@daocloud.io>
Signed-off-by: guozhi.li <guozhi.li@daocloud.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants