-
Notifications
You must be signed in to change notification settings - Fork 40.6k
add etcd server overrides to etcd probe factory for healthz and readyz #129438
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
base: master
Are you sure you want to change the base?
Conversation
Skipping CI for Draft Pull Request. |
cc @serathius |
9ff8d88
to
6f3b982
Compare
/assign @serathius @siyuanfoundation |
6f3b982
to
bca086c
Compare
The e2e test failure seems to be unrelated. |
cc: @natherz97 |
44f7728
to
7be81fe
Compare
7be81fe
to
c2ae17b
Compare
@pacoxu Thanks for the revision! These changes lgtm. I wrote these unit tests in staging/src/k8s.io/apiserver/pkg/server/options/etcd_test.go while reviewing the PR if you would like to add them.
|
UT test caess ware added now. Thanks. |
c41b4df
to
30afd4b
Compare
sortedServers := make([]string, len(servers)) | ||
copy(sortedServers, servers) | ||
sort.Strings(sortedServers) | ||
sortedKey := strings.Join(sortedServers, ";") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's strange to use the sorted set here to determine uniqueness, but then use the unsorted set below to initialize the client ... if the order matters to the client, it should matter for uniqueness as well
I don't know enough to know if sort order matters, but we should at least be consistent ... use the same thing to determine uniqueness and client construction
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed to
- sort servers in ParseEtcdServersOverrides
- check if overrides are dup in the next loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- sort servers in ParseEtcdServersOverrides
Does changing the specified order change client behavior? If so, I don't think we should sort before passing to the client.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about this?
- keep the order in ParseEtcdServersOverrides
- make a copy for sorting and keep the order for the client
6364526
to
b9ffb7e
Compare
ping @liggitt |
/assign @liggitt |
/priority important-soon |
// multi overrides servers may in different order | ||
// example: ["apps/deployments#s2.example.com;s1.example.com","apps/replicasets#s1.example.com;s2.example.com"] | ||
// ParseEtcdServersOverrides will sort the servers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
drop this comment, ParseEtcdServersOverrides no longer sorts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dropped.
- grouping health checks for exclusion purposes & add exclude integration test Signed-off-by: Paco Xu <paco.xu@daocloud.io>
b9ffb7e
to
8cec148
Compare
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: pacoxu The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind bug feature
What this PR does / why we need it:
kubernetes/staging/src/k8s.io/apiserver/pkg/storage/storagebackend/factory/etcd3.go
Lines 277 to 278 in 6bb6944
The monitor random one of the etcd to check.
Which issue(s) this PR fixes:
Fixes #129417
Special notes for your reviewer:
Then the
healthz?verbose
is like below:cc @mengqiy
/cc @liggitt
Does this PR introduce a user-facing change?