-
Notifications
You must be signed in to change notification settings - Fork 70
make the state manager safe to use in parallel #932
Conversation
add a test that makes many updates to the state in parallel add a function to update and return updated state
} | ||
|
||
// SerializeNamespace serializes to disk the namespace to use for helm template | ||
func (m *MManager) SerializeNamespace(namespace string) error { | ||
debug := level.Debug(log.With(m.Logger, "method", "serializeHelmValues")) | ||
debug := level.Debug(log.With(m.Logger, "method", "serializeNamespace")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏
return nil | ||
state.V1.Kustomize = kustomize | ||
return state, nil | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am loving all the code we get to remove as a side effect of this refactor 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is pretty close. Let me know what you think about those suggestions
@@ -497,7 +493,7 @@ func TestMManager_ResetLifecycle(t *testing.T) { | |||
t.Run(tt.name, func(t *testing.T) { | |||
req := require.New(t) | |||
m := &MManager{ | |||
Logger: log.NewNopLogger(), | |||
Logger: &logger.TestLogger{T: t}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏
there can never be too many tests
0b4b060
to
420b2bc
Compare
449faf6
to
0388f58
Compare
pkg/state/manager.go
Outdated
Name: metadata.Name, | ||
updatedState, err := updater(currentState.Versioned()) | ||
if err != nil { | ||
return VersionedState{}, errors.Wrap(err, "run state update function in safe updater") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason not to return nil
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah - it's not nil
able
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh is State
not an interface?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤦♂
I wrote that when it returned VersionedState
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This compiles for me
diff --git a/pkg/state/manager.go b/pkg/state/manager.go
index e2842e5e..adee747a 100644
--- a/pkg/state/manager.go
+++ b/pkg/state/manager.go
@@ -92,7 +92,7 @@ func (m *MManager) StateUpdate(updater Update) (State, error) {
currentState, err := m.TryLoad()
if err != nil {
- return VersionedState{}, errors.Wrap(err, "tryLoad in safe updater")
+ return nil, errors.Wrap(err, "tryLoad in safe updater")
}
updatedState, err := updater(currentState.Versioned())
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah - thus the commit (made at the same time as your comment - 10:54 😆)
What I Did
make the state manager safe to use in parallel
How I Did it
All state updates are now protected by locks.
How to verify it
Look at the unit test that attempts to create a race condition by hammering one field of the state in parallel
Description for the Changelog
Fixed a race condition in ship state updates
Picture of a Ship (not required but encouraged)