Refactor StackApplier

[twz123]

Description

Part of #1814.

Changes to StackApplier:

• Combine Start/Stop methods into a simple Run method that exits when the given context is done. The start method blocked anyways, exiting when the Stop method was called. The context approach is somewhat more straight forward.
• Only consider fsnotify events for manifest files.
• Also log fsnotify watcher errors in the StackApplier.
• Synchronize access to the internal applier, as calls to it might happen concurrently.
• Remove the unused Healthy method.

Changes to the Manager:

• Always remove a stack from internal map, even if its deletion failed. The directory is gone anyways, and a readdition wouldn't work otherwise, because of the old, stopped stack still being in the map.
• Retry stack running on failure.

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation update

How Has This Been Tested?

• Manual test
• Auto test added

Checklist:

• My code follows the style guidelines of this project
• My commit messages are signed-off
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• Any dependent changes have been merged and published in downstream modules
• I have checked my code and corrected any misspellings

[mikhail-sakhnov]

any actual profit from making this configurable? asking because delegates imo makes code harder to follow during debug.

[twz123]

The intent wasn't to make it configurable, but to synchronize access to the Applier via a Mutex. Alternatively, we can put the Mutex and the Applier into the StackApplier struct, allowing the Applier to be called without locking the Mutex and making it less obvious which parts of the struct are actually guarded by that Mutex.

Using the function pointers was basically a shorthand for something like this:

type StackApplier struct {
	// ...
	applier applier
}

type applier interface {
	Apply(context.Context) error
	Delete(context.Context) error
}

type syncApplier struct {
	sync.Mutex
	applier applier
}

func (a *syncApplier) Apply(ctx context.Context) error {
	a.Lock()
	defer a.Unlock()
	return a.applier.Apply(ctx)
}

func (a *syncApplier) Delete(ctx context.Context) error {
	a.Lock()
	defer a.Unlock()
	return a.applier.Delete(ctx)
}

func NewStackApplier(path string, kubeClientFactory kubernetes.ClientFactoryInterface) *StackApplier {
	applier := NewApplier(path, kubeClientFactory)
	return &StackApplier{
		// ...
		applier: &syncApplier{applier: &applier},
	}
}

(In theory, a unit test could leverage the doApply, doDelete stuff as well, but that was not the goal.)

[mikhail-sakhnov]

looks much better now, few minor nitpicks are in the comments.

[mikhail-sakhnov]

LGTM