[YUNIKORN-2630] Release context lock early for config changes (apache…

…#842)

Release the lock of the context in the shim when processing is done.
When the config changes are sent to the core the k8shim should not be
locked. The context changes have been finalised at that point.

The core handles its own locking and serialises config changes that come
in from the k8shim.
review: remove call through api to get config.

Closes: apache#842

Signed-off-by: Chia-Ping Tsai <[email protected]>

pkg/cache/context.go

@@ -601,40 +601,47 @@ func (ctx *Context) deletePriorityClass(obj interface{}) {
 }
 
 func (ctx *Context) triggerReloadConfig(index int, configMap *v1.ConfigMap) {
-	ctx.lock.Lock()
-	defer ctx.lock.Unlock()
-
-	conf := ctx.apiProvider.GetAPIs().GetConf()
-	if !conf.EnableConfigHotRefresh {
+	// hot reload is turned off do nothing
+	// hot reload can be turned off by an update: safety first access under lock to prevent data race
+	if !schedulerconf.GetSchedulerConf().IsConfigReloadable() {
 		log.Log(log.ShimContext).Info("hot-refresh disabled, skipping scheduler configuration update")
 		return
 	}
-
-	ctx.configMaps[index] = configMap
-	err := schedulerconf.UpdateConfigMaps(ctx.configMaps, false)
-	if err != nil {
-		log.Log(log.ShimContext).Error("Unable to update configmap, ignoring changes", zap.Error(err))
+	// update the maps in the context: return on failure, logged in the called method
+	confMap := ctx.setConfigMap(index, configMap)
+	if confMap == nil {
 		return
 	}
-
-	confMap := schedulerconf.FlattenConfigMaps(ctx.configMaps)
-
-	conf = ctx.apiProvider.GetAPIs().GetConf()
 	log.Log(log.ShimContext).Info("reloading scheduler configuration")
 	config := utils.GetCoreSchedulerConfigFromConfigMap(confMap)
 	extraConfig := utils.GetExtraConfigFromConfigMap(confMap)
 
 	request := &si.UpdateConfigurationRequest{
-		RmID:        conf.ClusterID,
-		PolicyGroup: conf.PolicyGroup,
+		RmID:        schedulerconf.GetSchedulerConf().ClusterID,
+		PolicyGroup: schedulerconf.GetSchedulerConf().PolicyGroup,
 		Config:      config,
 		ExtraConfig: extraConfig,
 	}
+	// tell the core to update: sync call that is serialised on the core side
 	if err := ctx.apiProvider.GetAPIs().SchedulerAPI.UpdateConfiguration(request); err != nil {
 		log.Log(log.ShimContext).Error("reload configuration failed", zap.Error(err))
 	}
 }
 
+// setConfigMap sets the new config map object in the list of maps maintained in the context and returns a flat map
+// of the settings from both maps
+func (ctx *Context) setConfigMap(index int, configMap *v1.ConfigMap) map[string]string {
+	ctx.lock.Lock()
+	defer ctx.lock.Unlock()
+	ctx.configMaps[index] = configMap
+	err := schedulerconf.UpdateConfigMaps(ctx.configMaps, false)
+	if err != nil {
+		log.Log(log.ShimContext).Error("Unable to update configmap, ignoring changes", zap.Error(err))
+		return nil
+	}
+	return schedulerconf.FlattenConfigMaps(ctx.configMaps)
+}
+
 // EventsToRegister returns the Kubernetes events that should be watched for updates which may effect predicate processing
 func (ctx *Context) EventsToRegister(queueingHintFn framework.QueueingHintFn) []framework.ClusterEventWithHint {
 	return ctx.predManager.EventsToRegister(queueingHintFn)

pkg/conf/schedulerconf.go

@@ -269,6 +269,12 @@ func (conf *SchedulerConf) IsTestMode() bool {
 	return conf.TestMode
 }
 
+func (conf *SchedulerConf) IsConfigReloadable() bool {
+	conf.RLock()
+	defer conf.RUnlock()
+	return conf.EnableConfigHotRefresh
+}
+
 func (conf *SchedulerConf) GetSchedulingInterval() time.Duration {
 	conf.RLock()
 	defer conf.RUnlock()