tigerquoll commented on code in PR #1060:
URL: https://github.com/apache/yunikorn-k8shim/pull/1060#discussion_r3889399334
##########
pkg/client/kubeclient.go:
##########
@@ -30,44 +30,127 @@ import (
"k8s.io/client-go/rest"
"k8s.io/client-go/tools/clientcmd"
"k8s.io/client-go/util/retry"
+ "k8s.io/utils/clock"
"github.com/apache/yunikorn-k8shim/pkg/conf"
"github.com/apache/yunikorn-k8shim/pkg/log"
)
type SchedulerKubeClient struct {
clientSet *kubernetes.Clientset
- configs *rest.Config
}
-func newBootstrapSchedulerKubeClient(kc string) SchedulerKubeClient {
- config := CreateRestConfigOrDie(kc)
- configuredClient, err := kubernetes.NewForConfig(config)
- if err != nil {
- log.Log(log.ShimClient).Fatal("failed to get Clientset",
zap.Error(err))
- }
- return SchedulerKubeClient{
- clientSet: configuredClient,
- configs: config,
+// Every client identifies the concern it serves in its user agent so that
traffic can be
+// attributed on the API server side. The concern comes first to allow prefix
matching, the
+// build version is appended by UserAgent().
+const (
+ UserAgentAdmissionController = "yunikorn-admission-controller"
+
+ userAgentBootstrap = "yunikorn-bootstrap"
+ userAgentWrites = "yunikorn-scheduler/writes"
+ userAgentInformers = "yunikorn-scheduler/informers"
+ userAgentEvents = "yunikorn-scheduler/events"
+)
+
+// UserAgent appends the build version to the concern, e.g.
"yunikorn-scheduler/writes
+// (1.7.0)". The version is only set in release builds, it is left out when
empty.
+func UserAgent(concern string) string {
+ return userAgentWithVersion(concern,
conf.GetBuildInfoMap()["buildVersion"])
+}
+
+func userAgentWithVersion(concern, version string) string {
+ if version == "" {
+ return concern
}
+ return fmt.Sprintf("%s (%s)", concern, version)
}
-func newSchedulerKubeClient(kc string) SchedulerKubeClient {
- schedulerConf := conf.GetSchedulerConf()
+// rateLimitPolicy turns the configured qps and burst into the values to set
on a REST config
+// and a description of the resulting limit for logging.
+// A qps <= 0 disables client side rate limiting: this is expressed as a
negative QPS, which
+// client-go treats as "create no rate limiter", while a QPS of 0 silently
falls back to its
+// own defaults of 5 QPS / 10 burst (see RESTClientFor and
NewForConfigAndClient in
+// client-go). A QPS set without a burst is rejected by client-go, so the
burst defaults to
+// the configured qps.
Review Comment:
Done. `rateLimitPolicy` and its warnings are gone in favour of the
normalisation you suggested, with one difference: burst is the configured burst
when set, otherwise the qps - `max(burst, qps)` would silently raise an
explicit `burst < qps`. `0/0` is left to client-go's 5/10 as you asked, so
`qps: "0"` now behaves exactly as it did before this PR; the defaults stay at
`-1`. The compatibility table in the description is updated to match.
One thing I'd like to confirm with you: the scheduler client itself still
defaults to no client-side limiter (`-1`), on the measurements in the
description - the 1000/1000 bucket clipped the bind burst, and at `qps: 50` the
limiter was the throughput ceiling at 52 binds/s. If you'd rather keep a
default cap on the scheduler client, say so and I'll set one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
