refactoring

Signed-off-by: Mikhail Scherba <mikhail.scherba@flant.com>

pkg/addon-operator/operator.go

@@ -680,8 +680,9 @@ func (op *AddonOperator) HandleConvergeModules(t sh_task.Task, logLabels map[str
 
 	if op.ConvergeState.Phase == converge.WaitBeforeAll {
 		logEntry.Infof("ConvergeModules: beforeAll hooks done, run modules")
+		var state *module_manager.ModulesState
 
-		state, handleErr := op.ModuleManager.RefreshEnabledState(t.GetLogLabels())
+		state, handleErr = op.ModuleManager.RefreshEnabledState(t.GetLogLabels())
 		if handleErr == nil {
 			// TODO disable hooks before was done in DiscoverModulesStateRefresh. Should we stick to this solution or disable events later during the handling each ModuleDelete task?
 			// Disable events for disabled modules.
@@ -950,28 +951,26 @@ func (op *AddonOperator) StartModuleManagerEventHandler() {
 				// dynamically_enabled_extender
 				case dynamic_extender.DynamicExtenderEvent:
 					// we don't need to schedule any tasks as DynamicExtender is inherintelly :
-					graphStateChanged, err := op.ModuleManager.UpdateGraphState()
-					if err != nil {
-						eventLogEntry.Errorf("Couldn't update the graph's state: %v", err)
-					}
+					graphStateChanged := op.ModuleManager.RecalculateGraph()
 
 					if graphStateChanged {
 						// ConvergeModules may be in progress, Reset converge state.
-						op.ConvergeState.Phase = converge.StandBy
 						convergeTask := converge.NewConvergeModulesTask(
 							"ReloadAll-After-GlobalHookDynamicUpdate",
 							converge.ReloadAllModules,
 							logLabels,
 						)
 						firstTask := op.engine.TaskQueues.GetMain().GetFirst()
 						if firstTask != nil && RemoveCurrentConvergeTasks(op.engine.TaskQueues.GetMain(), firstTask.GetId()) {
-							logEntry.Infof("ConvergeModules: global hook dynamic modification detected,  restart current converge process (%s)", op.ConvergeState.Phase)
+							logEntry.Infof("ConvergeModules: global hook dynamic modification detected, restart current converge process (%s)", op.ConvergeState.Phase)
 							op.engine.TaskQueues.GetMain().AddFirst(convergeTask)
 							op.logTaskAdd(eventLogEntry, "DynamicExtender is updated, put first", convergeTask)
 						} else {
 							logEntry.Infof("ConvergeModules:  global hook dynamic modification detected, rerun all modules required")
 							op.engine.TaskQueues.GetMain().AddLast(convergeTask)
 						}
+						// ConvergeModules may be in progress Reset converge state.
+						op.ConvergeState.Phase = converge.StandBy
 					}
 
 				// kube_config_extender
@@ -988,12 +987,7 @@ func (op *AddonOperator) StartModuleManagerEventHandler() {
 						}
 						// Config is valid now, add task to update ModuleManager state.
 						op.ModuleManager.SetKubeConfigValid(true)
-
-						graphStateChanged, err := op.ModuleManager.UpdateGraphState()
-						if err != nil {
-							eventLogEntry.Errorf("Couldn't update the graph's state: %v", err)
-							continue
-						}
+						graphStateChanged := op.ModuleManager.RecalculateGraph()
 
 						var (
 							kubeConfigTask sh_task.Task
@@ -1018,8 +1012,6 @@ func (op *AddonOperator) StartModuleManagerEventHandler() {
 						}
 
 						if event.GlobalSectionChanged || graphStateChanged {
-							// ConvergeModules may be in progress Reset converge state.
-							op.ConvergeState.Phase = converge.StandBy
 							convergeTask = converge.NewConvergeModulesTask(
 								"ReloadAll-After-KubeConfigChange",
 								converge.ReloadAllModules,
@@ -1039,6 +1031,8 @@ func (op *AddonOperator) StartModuleManagerEventHandler() {
 								logEntry.Infof("ConvergeModules: kube config modification detected, rerun all modules required")
 								op.engine.TaskQueues.GetMain().AddLast(convergeTask)
 							}
+							// ConvergeModules may be in progress Reset converge state.
+							op.ConvergeState.Phase = converge.StandBy
 						} else {
 							modulesToRerun := []string{}
 							for _, moduleName := range event.ModuleValuesChanged {

pkg/addon-operator/operator_test.go

@@ -142,6 +142,7 @@ func assembleTestAddonOperator(t *testing.T, configPath string) (*AddonOperator,
 
 	err = op.InitModuleManager()
 	g.Expect(err).ShouldNot(HaveOccurred(), "Should init ModuleManager")
+	_ = op.ModuleManager.RecalculateGraph()
 
 	return op, result
 }

pkg/module_manager/models/modules/basic.go

@@ -379,21 +379,6 @@ func (bm *BasicModule) RunEnabledScript(tmpDir string, precedingEnabledModules [
 
 	logEntry := log.WithFields(utils.LabelsToLogFields(logLabels))
 	enabledScriptPath := filepath.Join(bm.Path, "enabled")
-
-	f, err := os.Stat(enabledScriptPath)
-	if os.IsNotExist(err) {
-		logEntry.Debugf("MODULE '%s' is ENABLED. Enabled script doesn't exist!", bm.Name)
-		return true, nil
-	} else if err != nil {
-		logEntry.Errorf("Cannot stat enabled script '%s': %s", enabledScriptPath, err)
-		return false, err
-	}
-
-	if !utils_file.IsFileExecutable(f) {
-		logEntry.Errorf("Found non-executable enabled script '%s'", enabledScriptPath)
-		return false, fmt.Errorf("non-executable enable script")
-	}
-
 	configValuesPath, err := bm.prepareConfigValuesJsonFile(tmpDir)
 	if err != nil {
 		logEntry.Errorf("Prepare CONFIG_VALUES_PATH file for '%s': %s", enabledScriptPath, err)

pkg/module_manager/module_manager.go

@@ -399,15 +399,13 @@ func (mm *ModuleManager) RefreshEnabledState(logLabels map[string]string) (*Modu
 	})
 	logEntry := log.WithFields(utils.LabelsToLogFields(refreshLogLabels))
 
-	enabledModules, err := mm.moduleScheduler.GetEnabledModuleNames()
+	enabledModules, modulesDiff, err := mm.moduleScheduler.GetGraphState()
 	if err != nil {
 		return nil, err
 	}
 
 	logEntry.Infof("Enabled modules: %+v", enabledModules)
 
-	modulesDiff := mm.moduleScheduler.GleanGraphDiff()
-
 	var (
 		modulesToEnable  []string
 		modulesToDisable []string
@@ -421,6 +419,7 @@ func (mm *ModuleManager) RefreshEnabledState(logLabels map[string]string) (*Modu
 		}
 	}
 	modulesToDisable = utils.SortReverseByReference(modulesToDisable, mm.modules.NamesInOrder())
+	modulesToEnable = utils.SortByReference(modulesToEnable, mm.modules.NamesInOrder())
 
 	logEntry.Debugf("Refresh state results:\n"+
 		"    enabledModules: %v\n"+
@@ -808,9 +807,9 @@ func (mm *ModuleManager) applyEnabledPatch(enabledPatch utils.ValuesPatch, exten
 	return nil
 }
 
-// UpdateGraphState runs corresponding scheduler method that returns true if the graph's state has changed
-func (mm *ModuleManager) UpdateGraphState() (bool, error) {
-	return mm.moduleScheduler.UpdateGraphState()
+// RecalculateGraph runs corresponding scheduler method that returns true if the graph's state has changed
+func (mm *ModuleManager) RecalculateGraph() bool {
+	return mm.moduleScheduler.RecalculateGraph()
 }
 
 // GlobalSynchronizationNeeded is true if there is at least one global

pkg/module_manager/scheduler/extenders/script_enabled/script.go

@@ -1,26 +1,43 @@
 package script_enabled
 
 import (
+	"errors"
 	"fmt"
 	"os"
+	"path/filepath"
 	"sync"
 
+	log "github.com/sirupsen/logrus"
+
 	"github.com/flant/addon-operator/pkg/module_manager/scheduler/extenders"
 	"github.com/flant/addon-operator/pkg/module_manager/scheduler/node"
+	utils_file "github.com/flant/shell-operator/pkg/utils/file"
 )
 
 const (
 	Name extenders.ExtenderName = "ScriptEnabled"
+
+	noEnabledScript     scriptState = "NoEnabledScript"
+	nonExecutableScript scriptState = "NonExecutableScript"
+	statError           scriptState = "StatError"
 )
 
+type scriptState string
+
 type Extender struct {
-	tmpDir       string
-	basicModules map[string]node.ModuleInterface
+	tmpDir                 string
+	basicModuleDescriptors map[string]moduleDescriptor
 
 	l              sync.RWMutex
 	enabledModules []string
 }
 
+type moduleDescriptor struct {
+	module           node.ModuleInterface
+	scriptState      scriptState
+	stateDescription string
+}
+
 func NewExtender(tmpDir string) (*Extender, error) {
 	info, err := os.Stat(tmpDir)
 	if err != nil {
@@ -32,16 +49,37 @@ func NewExtender(tmpDir string) (*Extender, error) {
 	}
 
 	e := &Extender{
-		basicModules:   make(map[string]node.ModuleInterface),
-		enabledModules: make([]string, 0),
-		tmpDir:         tmpDir,
+		basicModuleDescriptors: make(map[string]moduleDescriptor),
+		enabledModules:         make([]string, 0),
+		tmpDir:                 tmpDir,
 	}
 
 	return e, nil
 }
 
 func (e *Extender) AddBasicModule(module node.ModuleInterface) {
-	e.basicModules[module.GetName()] = module
+	moduleD := moduleDescriptor{
+		module: module,
+	}
+
+	enabledScriptPath := filepath.Join(module.GetPath(), "enabled")
+	f, err := os.Stat(enabledScriptPath)
+	if err != nil {
+		if os.IsNotExist(err) {
+			moduleD.scriptState = noEnabledScript
+			log.Debugf("MODULE '%s' is ENABLED. Enabled script doesn't exist!", module.GetName())
+		} else {
+			moduleD.scriptState = statError
+			moduleD.stateDescription = fmt.Sprintf("Cannot stat enabled script for '%s' module: %v", module.GetName(), err)
+			log.Errorf(moduleD.stateDescription)
+		}
+	} else {
+		if !utils_file.IsFileExecutable(f) {
+			moduleD.scriptState = nonExecutableScript
+			log.Warnf("Found non-executable enabled script for '%s' module - assuming enabled state", module.GetName())
+		}
+	}
+	e.basicModuleDescriptors[module.GetName()] = moduleD
 }
 
 func (e *Extender) Name() extenders.ExtenderName {
@@ -59,18 +97,37 @@ func (e *Extender) Reset() {
 }
 
 func (e *Extender) Filter(moduleName string) (*bool, error) {
-	if module, found := e.basicModules[moduleName]; found {
-		enabled, err := module.RunEnabledScript(e.tmpDir, e.enabledModules, map[string]string{"operator.component": "ModuleManager.Scheduler", "extender": "script_enabled"})
-		if err != nil {
-			return nil, err
+	if moduleDescriptor, found := e.basicModuleDescriptors[moduleName]; found {
+		var err error
+		var enabled bool
+
+		switch moduleDescriptor.scriptState {
+		case "":
+			enabled, err = moduleDescriptor.module.RunEnabledScript(e.tmpDir, e.enabledModules, map[string]string{"operator.component": "ModuleManager.Scheduler", "extender": "script_enabled"})
+			if err != nil {
+				err = fmt.Errorf("Failed to execute '%s' module's enabled script: %v", moduleDescriptor.module.GetName(), err)
+			}
+
+		case statError:
+			log.Errorf(moduleDescriptor.stateDescription)
+			enabled = false
+			err = errors.New(moduleDescriptor.stateDescription)
+
+		case nonExecutableScript:
+			enabled = true
+			log.Warnf("Found non-executable enabled script for '%s' module - assuming enabled state", moduleDescriptor.module.GetName())
+
+		case noEnabledScript:
+			enabled = true
+			log.Debugf("MODULE '%s' is ENABLED. Enabled script doesn't exist!", moduleDescriptor.module.GetName())
 		}
 
 		if enabled {
 			e.l.Lock()
-			e.enabledModules = append(e.enabledModules, module.GetName())
+			e.enabledModules = append(e.enabledModules, moduleDescriptor.module.GetName())
 			e.l.Unlock()
 		}
-		return &enabled, nil
+		return &enabled, err
 	}
 	return nil, nil
 }

pkg/module_manager/scheduler/extenders/script_enabled/script_test.go

@@ -1,6 +1,8 @@
 package script_enabled
 
 import (
+	"errors"
+	"fmt"
 	"os"
 	"testing"
 
@@ -22,40 +24,57 @@ func TestExtender(t *testing.T) {
 			Name:                "ingress-nginx",
 			Order:               402,
 			EnabledScriptResult: true,
+			Path:                "./testdata/402-ingress-nginx/",
 		},
 		{
 			Name:                "cert-manager",
 			Order:               30,
 			EnabledScriptResult: true,
+			Path:                "./testdata/030-cert-manager/",
 		},
 		{
 			Name:                "node-local-dns",
 			Order:               20,
 			EnabledScriptResult: true,
+			EnabledScriptErr:    fmt.Errorf("Exit code 1"),
+			Path:                "./testdata/020-node-local-dns/",
 		},
 		{
 			Name:                "admission-policy-engine",
 			Order:               15,
 			EnabledScriptResult: false,
+			Path:                "./testdata/015-admission-policy-engine/",
 		},
 		{
 			Name:                "chrony",
 			Order:               45,
 			EnabledScriptResult: false,
+			Path:                "./testdata/045-chrony/",
 		},
 	}
 
 	for _, m := range basicModules {
 		e.AddBasicModule(m)
 		enabled, err := e.Filter(m.Name)
-		assert.NoError(t, err)
 		switch m.GetName() {
-		case "ingress-nginx", "cert-manager", "node-local-dns":
-			assert.Equal(t, *enabled, true)
+		case "ingress-nginx":
+			assert.Equal(t, true, *enabled)
+			assert.Equal(t, nil, err)
+		case "cert-manager":
+			assert.Equal(t, true, *enabled)
+			assert.Equal(t, nil, err)
+		case "node-local-dns":
+			assert.Equal(t, false, *enabled)
+			assert.Equal(t, errors.New("Failed to execute 'node-local-dns' module's enabled script: Exit code 1"), err)
 		case "admission-policy-engine", "chrony":
-			assert.Equal(t, *enabled, false)
+			assert.Equal(t, false, *enabled)
+			assert.Equal(t, nil, err)
 		}
 	}
-	expected := []string{"ingress-nginx", "cert-manager", "node-local-dns"}
-	assert.Equal(t, e.enabledModules, expected)
+
+	expected := []string{"ingress-nginx", "cert-manager"}
+	assert.Equal(t, expected, e.enabledModules)
+
+	err = os.RemoveAll(tmp)
+	assert.NoError(t, err)
 }

pkg/module_manager/scheduler/extenders/script_enabled/testdata/402-ingress-nginx/enabled

@@ -0,0 +1,3 @@
+#!/bin/bash
+
+exit 0

pkg/module_manager/scheduler/node/node.go

@@ -8,23 +8,33 @@ type ModuleInterface interface {
 	RunEnabledScript(string, []string, map[string]string) (bool, error)
 	GetName() string
 	GetOrder() uint32
+	GetPath() string
 }
 
 type MockModule struct {
 	EnabledScriptResult bool
+	EnabledScriptErr    error
 	Name                string
+	Path                string
 	Order               uint32
 }
 
 func (m MockModule) GetName() string {
 	return m.Name
 }
 
+func (m MockModule) GetPath() string {
+	return m.Path
+}
+
 func (m MockModule) GetOrder() uint32 {
 	return m.Order
 }
 
 func (m MockModule) RunEnabledScript(_ string, _ []string, _ map[string]string) (bool, error) {
+	if m.EnabledScriptErr != nil {
+		return false, m.EnabledScriptErr
+	}
 	return m.EnabledScriptResult, nil
 }