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

cache etcd client on readyz #797

Closed
wants to merge 1 commit into from
Closed

cache etcd client on readyz #797

wants to merge 1 commit into from

Conversation

tjungblu
Copy link
Contributor

Ran this with 5000 QPS in parallel with 12 threads using vegeta without issues. Also reconnects automatically when etcd goes down, which is great.

pprof also shows no TLS handshake hotpath anymore.

@openshift-ci openshift-ci bot requested review from dusk125 and marun April 21, 2022 15:39
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 21, 2022

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: tjungblu
To complete the pull request process, please assign sttts after the PR has been reviewed.
You can assign the PR to them by writing /assign @sttts in a comment when ready.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tjungblu
Copy link
Contributor Author

TODO figure out the reconnect behaviour of GRPC: https://stackoverflow.com/questions/56067076/grpc-connection-management-in-golang

@tjungblu
Copy link
Contributor Author

the health check also seems to reuse the client already:

// Create single target checks one check per target
for _, target := range targets {
// one client per target to eliminate lock racing
client, err := newETCD3Client(ctx, tlsInfo, targets)
if err != nil {
return nil, err
}
// pin endpoint for check
client.SetEndpoints(target)
for _, healthCheckFunc := range singleTargetChecks {
healthCheck := health.NewCheck(lg, client, []string{target})
healthCheck.Probe = healthCheckFunc
healthChecks = append(healthChecks, healthCheck)
}
}

@tjungblu
Copy link
Contributor Author

/retest-required

1 similar comment
@tjungblu
Copy link
Contributor Author

/retest-required

@tjungblu
Copy link
Contributor Author

increase in CPU usage from https://bugzilla.redhat.com/show_bug.cgi?id=2076831

image

@tjungblu
Copy link
Contributor Author

/retest-required

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 25, 2022

@tjungblu: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-gcp-five-control-plane-replicas 9154786 link false /test e2e-gcp-five-control-plane-replicas
ci/prow/e2e-aws-single-node 9154786 link true /test e2e-aws-single-node

Full PR test history. Your PR dashboard.

Instructions 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/test-infra repository. I understand the commands that are listed here.

@tjungblu
Copy link
Contributor Author

/hold

probably worth refactoring with the changes from #801

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 26, 2022
@tjungblu
Copy link
Contributor Author

closing this in favour of #801 where the caching is properly implemented

@tjungblu tjungblu closed this Apr 28, 2022
@tjungblu tjungblu deleted the readyzimp branch November 28, 2022 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant