Make sure to read association configuration again from annotations if it was cleared #5489
Conversation
| } | ||
| assert.Equal(t, 2, len(esMon.AssocConfs)) | ||
|
|
||
| // simulate the case where the assocConfs map is reset, which can happen if the resource is updated |
There was a problem hiding this comment.
simulate the case where the assocConfs map is reset, which can happen if the resource is updated
Sorry, I'm not sure to understand. assocConfs is not persisted, I think it is expected for it to be empty before FetchWithAssociations kicks in ?
There was a problem hiding this comment.
The issue happens after FetchWithAssociations, when the resource is updated, it resets the map. This test shows that if it is reset after SetAssociationConf(), the map is repopulated on the fly by AssociationConf().
Maybe I should go further and delete SetAssociationConf() to avoid confusion. We could even delete the FetchWithAssociations() and just rely on AssociationConf() called in different places.
There was a problem hiding this comment.
We could even delete the FetchWithAssociations() and just rely on AssociationConf() called in different places.
Thanks for the clarification ! My understanding is that it means that FetchWithAssociations is unreliable 😕 I wonder if it's really worth keeping it along with the unserialized annotations ? IIUC they are only here to spare a few CPU cycles by not using json.Unmarshal when the association config. is needed ?
|
As I already mentioned I still have the feeling that these "non-persisted" annotations bring more complexity than value, but overall I think I'm fine keeping them. |
Co-authored-by: Michael Morello <[email protected]>
Co-authored-by: Michael Morello <[email protected]>
Co-authored-by: Michael Morello <[email protected]>
|
As this is already in place and is now well isolated and reliable, I am in favor of keeping it. |
thbkrkr
changed the title
… it was reset (elastic#5489) This commit introduces two new functions `GetAndSetAssociationConf()` and `GetAndSetAssociationConfByRef()` to be called in the implementation of `AssociationConf()` for each Association to be sure that if there is an update of the associated resource that carries the association that clears the `assocConf` map, then we make sure to read it back from the annotation. This fix makes the `assocConf` map reliable and has also the nice side-effect to avoid an unnecessary initial restart when deploying a self-monitoring Elasticsearch cluster.
This commit introduces two new functions
GetAndSetAssociationConf()andGetAndSetAssociationConfByRef()to be called in the implementation ofAssociationConf()for eachAssociationto be sure that if there is an update of the associated resource that carries the association that resets the assocConf, then we make sure to read it back from the annotation.This should be called very rarely because we re-read the annotation only if the current assoc conf is not set.
Due to a circular dependency, I move the functions in the
pkg/apis/common/v1package.Note: I couldn't reproduce the issue that resets non persisted attributes during an update with the fakeClient.
Resolves #4709.