bug: values/secrets generation not running concurrently during 'diff' #810
Comments
|
@travisgroth Ah, good catch! I remember that #737 was due to that concurrent go routines managed by My initial attempt was to have a granular locking inside the Perhaps we need to quit lazy builds of |
|
OK...I'm going down the same path. It looked like HelmState.Env, but I'm actually still not clear how we even wind up in this scenario. Mergo shouldn't be updating it. Multiple readers should be safe at that stage since we already have env constructed, I think? Here's a race detector run with the mutex in question removed: Changing the call to mergo.Merge() to use a deep copy of the HelmState.Env maps, removes the race condition: if err := mergo.Merge(&vals, st.Env.DeepCopy().Defaults, mergo.WithOverride); err != nil {
return nil, err
}
if err := mergo.Merge(&vals, st.Env.DeepCopy().Values, mergo.WithOverride); err != nil {
return nil, err
}To me, this looks as though merges of map sources are not concurrently safe in mergo. However, I can't reproduce with a contrived test of just HelmState.Values(). Any thoughts on what else might be at play here? |
|
Additional commentary - I think the Mutex inside Env didn't work because of some copying. Moving the mutex up to HelmState also eliminates the race: func (st *HelmState) Values() (map[string]interface{}, error) {
st.envMutex.Lock()
defer st.envMutex.Unlock()
vals := map[string]interface{}{}
if err := mergo.Merge(&vals, st.Env.Defaults, mergo.WithOverride); err != nil {
return nil, err
}
if err := mergo.Merge(&vals, st.Env.Values, mergo.WithOverride); err != nil {
return nil, err
}
vals, err := maputil.CastKeysToStrings(vals)
if err != nil {
return nil, err
}
return vals, nil
} |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
I noticed that
helmfile diffdidn't appear to be decrypting secrets concurrently. Seems the mutex from #737 is effectively single-threading the call from higher in the stack:helmfile/pkg/state/state.go
Lines 729 to 733 in a584aea
I'm not totally clear on which
mapneeds protecting, but after looking through the stack trace and code, I get the feeling it could be more granular. Happy to work through a PR to address the issue more directly, but I'm missing some context on where in the code the issue lies.The text was updated successfully, but these errors were encountered: