manirajv06 commented on code in PR #1060:
URL: https://github.com/apache/yunikorn-k8shim/pull/1060#discussion_r3763973022
##########
pkg/admission/webhook_manager.go:
##########
@@ -91,6 +91,11 @@ func NewWebhookManager(conf *conf.AdmissionControllerConf)
(WebhookManager, erro
log.Log(log.AdmissionWebhook).Error("Unable to create
kubernetes config", zap.Error(err))
return nil, err
}
+ // the webhook and secret writes must be attributed to the admission
controller
+ kubeconfig.UserAgent =
client.UserAgent(client.UserAgentAdmissionController)
+ // no client side rate limiting: leaving the QPS at 0 would mean the
client-go defaults
+ // of 5 QPS / 10 burst
+ kubeconfig.QPS = -1
Review Comment:
Can we use newly introduced constructors in `kubeclient.go` to create the
`clientset` even here as well? If not available, can we create one so that all
client creation happens in single place.
##########
pkg/shim/scheduler.go:
##########
@@ -64,12 +64,12 @@ var (
)
func NewShimScheduler(scheduler api.SchedulerAPI, configs *conf.SchedulerConf,
bootstrapConfigMaps []*v1.ConfigMap) *KubernetesShim {
- kubeClient := client.NewKubeClient(configs.KubeConfig)
-
+ // all informers, cluster wide and namespaced, share one client
+ informerClientSet := client.NewInformerClientSet(configs.KubeConfig)
// we have disabled re-sync to keep ourselves up-to-date
- informerFactory :=
informers.NewSharedInformerFactory(kubeClient.GetClientSet(), 0)
+ informerFactory :=
informers.NewSharedInformerFactory(informerClientSet, 0)
Review Comment:
Can we move this into `NewAPIFactory` as well?
That way we need to pass only `configs` to `NewAPIFactory`
##########
pkg/client/apifactory.go:
##########
@@ -89,9 +90,12 @@ type APIFactory struct {
lock *locking.RWMutex
}
-func NewAPIFactory(scheduler api.SchedulerAPI, informerFactory
informers.SharedInformerFactory, configs *conf.SchedulerConf, testMode bool)
(*APIFactory, error) {
+// NewAPIFactory creates the clients shared by the shim. The clientset backing
informerFactory
+// is passed in so that the namespaced factory created here shares it: both
only run informers
+// so they need the same unlimited client and the same attribution.
+func NewAPIFactory(scheduler api.SchedulerAPI, informerClientSet
kubernetes.Interface, informerFactory informers.SharedInformerFactory, configs
*conf.SchedulerConf, testMode bool) (*APIFactory, error) {
kubeClient := NewKubeClient(configs.KubeConfig)
Review Comment:
Where is this being used? Looks like for volume binding and other operations
too. Can you take a look at this? Should we re-use the one meant for `writes`
instead of this one?
##########
pkg/admission/webhook_manager.go:
##########
@@ -91,6 +91,11 @@ func NewWebhookManager(conf *conf.AdmissionControllerConf)
(WebhookManager, erro
log.Log(log.AdmissionWebhook).Error("Unable to create
kubernetes config", zap.Error(err))
return nil, err
}
+ // the webhook and secret writes must be attributed to the admission
controller
+ kubeconfig.UserAgent =
client.UserAgent(client.UserAgentAdmissionController)
+ // no client side rate limiting: leaving the QPS at 0 would mean the
client-go defaults
+ // of 5 QPS / 10 burst
+ kubeconfig.QPS = -1
Review Comment:
I can see one available for this and used already in
https://github.com/apache/yunikorn-k8shim/pull/1060/changes#diff-d757b0e6523c6adc5df3a6423e547aca3497226a451c9fe4bd07ec7051b7b508R63
##########
pkg/conf/schedulerconf.go:
##########
@@ -91,8 +93,10 @@ const (
DefaultOperatorPlugins = "general"
DefaultDisableGangScheduling = false
DefaultEnableConfigHotRefresh = true
- DefaultKubeQPS = 1000
- DefaultKubeBurst = 1000
+ DefaultKubeQPS = 0 // client side write
limiting is opt-in: <= 0 means no limiter
Review Comment:
'0' would mean the client-go defaults of 5 QPS / 10 burst not really "no
limiter". Is my understanding correct? Should we make this as -1 if we don't
want rate limiter at all for all except for events?
##########
pkg/client/apifactory.go:
##########
@@ -89,9 +90,12 @@ type APIFactory struct {
lock *locking.RWMutex
}
-func NewAPIFactory(scheduler api.SchedulerAPI, informerFactory
informers.SharedInformerFactory, configs *conf.SchedulerConf, testMode bool)
(*APIFactory, error) {
+// NewAPIFactory creates the clients shared by the shim. The clientset backing
informerFactory
+// is passed in so that the namespaced factory created here shares it: both
only run informers
+// so they need the same unlimited client and the same attribution.
+func NewAPIFactory(scheduler api.SchedulerAPI, informerClientSet
kubernetes.Interface, informerFactory informers.SharedInformerFactory, configs
*conf.SchedulerConf, testMode bool) (*APIFactory, error) {
kubeClient := NewKubeClient(configs.KubeConfig)
Review Comment:
> Should we re-use the one meant for `writes` instead of this one?
I see the changes in `interfaces.go`. Yes, we are using the one meant for
`writes` only.
However, I can see that this has been passed to `Clients` and being used to
load config maps in `Context.go#LoadConfigMaps`. Is this correct? Are we
supposed to use the meant for `reads`? Do we need to use the one being used
here
https://github.com/apache/yunikorn-k8shim/pull/1060/changes#diff-49e2cf379c065c43983fe806d7739d9a0947f7e21f22f29f22d905cc34d1bb39R32?
--
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]
