From 8c21eb017cecf6ddd0536d1124231c2cfb663552 Mon Sep 17 00:00:00 2001 From: Timur Tuktamyshev Date: Wed, 26 Nov 2025 18:04:14 +0300 Subject: [PATCH] fix readiness bug Signed-off-by: Timur Tuktamyshev --- pkg/module_manager/models/modules/basic.go | 37 +++++++++++-------- .../models/modules/basic_test.go | 24 ++++++++++++ 2 files changed, 45 insertions(+), 16 deletions(-) diff --git a/pkg/module_manager/models/modules/basic.go b/pkg/module_manager/models/modules/basic.go index 66660fa6..d8cc9351 100644 --- a/pkg/module_manager/models/modules/basic.go +++ b/pkg/module_manager/models/modules/basic.go @@ -246,7 +246,7 @@ func (bm *BasicModule) RegisterHooks(logger *log.Logger) ([]*hooks.ModuleHook, e return nil, nil } - hks, err := bm.searchModuleHooks() + hks, hasReadiness, err := bm.searchModuleHooks() if err != nil { return nil, fmt.Errorf("search module hooks failed: %w", err) } @@ -267,26 +267,27 @@ func (bm *BasicModule) RegisterHooks(logger *log.Logger) ([]*hooks.ModuleHook, e } bm.hooks.registered = true + bm.hasReadiness = hasReadiness return hks, nil } -func (bm *BasicModule) searchModuleHooks() ([]*hooks.ModuleHook, error) { +func (bm *BasicModule) searchModuleHooks() ([]*hooks.ModuleHook, bool, error) { shellHooks, err := bm.searchModuleShellHooks() if err != nil { - return nil, fmt.Errorf("search module shell hooks: %w", err) + return nil, false, fmt.Errorf("search module shell hooks: %w", err) } goHooks := bm.searchModuleGoHooks() - batchHooks, err := bm.searchModuleBatchHooks() + batchHooks, hasReadiness, err := bm.searchModuleBatchHooks() if err != nil { - return nil, fmt.Errorf("search module batch hooks: %w", err) + return nil, false, fmt.Errorf("search module batch hooks: %w", err) } if len(shellHooks)+len(batchHooks) > 0 { if err := bm.AssembleEnvironmentForModule(environmentmanager.ShellHookEnvironment); err != nil { - return nil, fmt.Errorf("Assemble %q module's environment: %w", bm.GetName(), err) + return nil, false, fmt.Errorf("Assemble %q module's environment: %w", bm.GetName(), err) } } @@ -311,7 +312,7 @@ func (bm *BasicModule) searchModuleHooks() ([]*hooks.ModuleHook, error) { return mHooks[i].GetPath() < mHooks[j].GetPath() }) - return mHooks, nil + return mHooks, hasReadiness, nil } func (bm *BasicModule) searchModuleShellHooks() ([]*kind.ShellHook, error) { @@ -373,18 +374,22 @@ func (bm *BasicModule) searchModuleShellHooks() ([]*kind.ShellHook, error) { return hks, nil } -func (bm *BasicModule) searchModuleBatchHooks() ([]*kind.BatchHook, error) { +// searchModuleBatchHooks searches for batch hooks and returns them along with hasReadiness flag. +// This function has no side effects - it doesn't modify bm.hasReadiness directly. +// The caller (RegisterHooks) is responsible for setting bm.hasReadiness after successful registration. +func (bm *BasicModule) searchModuleBatchHooks() ([]*kind.BatchHook, bool, error) { hooksDir := filepath.Join(bm.Path, "hooks") if _, err := os.Stat(hooksDir); os.IsNotExist(err) { - return nil, nil + return nil, false, nil } hooksRelativePaths, err := RecursiveGetBatchHookExecutablePaths(bm.safeName(), hooksDir, bm.logger, hooksExcludedDir...) if err != nil { - return nil, err + return nil, false, err } hks := make([]*kind.BatchHook, 0) + hasReadiness := false // sort hooks by path sort.Strings(hooksRelativePaths) @@ -393,20 +398,20 @@ func (bm *BasicModule) searchModuleBatchHooks() ([]*kind.BatchHook, error) { for _, hookPath := range hooksRelativePaths { hookName, err := normalizeHookPath(filepath.Dir(bm.Path), hookPath) if err != nil { - return nil, fmt.Errorf("could not get hook name: %w", err) + return nil, false, fmt.Errorf("could not get hook name: %w", err) } sdkcfgs, err := kind.GetBatchHookConfig(bm.safeName(), hookPath) if err != nil { - return nil, fmt.Errorf("getting sdk config for '%s': %w", hookName, err) + return nil, false, fmt.Errorf("getting sdk config for '%s': %w", hookName, err) } if sdkcfgs.Readiness != nil { - if bm.hasReadiness { - return nil, fmt.Errorf("multiple readiness hooks found in %s", hookPath) + if hasReadiness { + return nil, false, fmt.Errorf("multiple readiness hooks found in %s", hookPath) } - bm.hasReadiness = true + hasReadiness = true // add readiness hook nestedHookName := fmt.Sprintf("%s-readiness", hookName) @@ -422,7 +427,7 @@ func (bm *BasicModule) searchModuleBatchHooks() ([]*kind.BatchHook, error) { } } - return hks, nil + return hks, hasReadiness, nil } func RecursiveGetBatchHookExecutablePaths(moduleName, dir string, logger *log.Logger, excludedDirs ...string) ([]string, error) { diff --git a/pkg/module_manager/models/modules/basic_test.go b/pkg/module_manager/models/modules/basic_test.go index fdb56ebd..74589a4f 100644 --- a/pkg/module_manager/models/modules/basic_test.go +++ b/pkg/module_manager/models/modules/basic_test.go @@ -306,3 +306,27 @@ exit 0 require.NotNil(t, erGetter) require.Equal(t, "Kubernetes version is too low", *erGetter) } + +// TestHasReadinessNotSetOnFailedRegistration tests that hasReadiness is only set +// after successful hook registration. This prevents stale hasReadiness state when +// registration is retried after an error (e.g., in AssembleEnvironmentForModule). +func TestHasReadinessNotSetOnFailedRegistration(t *testing.T) { + tmpModuleDir := t.TempDir() + + bm, err := NewBasicModule("test-readiness-registration", tmpModuleDir, 1, utils.Values{}, nil, nil) + require.NoError(t, err) + + logger := log.NewLogger() + bm.WithLogger(logger) + bm.WithDependencies(stubDeps(logger)) + + // Simulate stale state from a previous failed registration attempt + bm.hasReadiness = true + + // RegisterHooks should overwrite hasReadiness based on actual search results + _, err = bm.RegisterHooks(logger) + require.NoError(t, err) + + // hasReadiness should be false (no hooks in empty module) + require.False(t, bm.hasReadiness, "hasReadiness should reflect actual hooks found") +}