Do not request ES to clear routing allocation exclude at every reconciliation #2610
Conversation
This commits stores the last updated value of cluster routing allocation exclude in an annotation of the Elasticsearch resource. Before doing any call to the ES API for that setting, we check the existing annotation. If its value is the same as the one we're about to set, skip the ES API call. If not, do the call then store the setting value in the annotation. The annotation can be removed at any time: it will just trigger a call to the Elasticsearch API with the right expected value, and the annotation will be set again. Users can manipulate this setting behind our back, in which case the operator will only react based on the annotation value. We consider it's the user responsibility to not mess cluster settings up at this point.
sebgl
changed the title
sebgl
changed the title
| @@ -76,12 +88,45 @@ func IsMigratingData(ctx context.Context, shardLister esclient.ShardLister, podN | |||
| return nodeIsMigratingData(podName, shards, excludedNodes), nil | |||
| } | |||
|
|
|||
| // AllocationExcludeAnnotationName returns the allocation exclude value stored in an annotation. | |||
| // May be empty if not set. | |||
| func allocationExcludeFromAnnotation(es esv1.Elasticsearch) string { | |||
There was a problem hiding this comment.
comment/func mismatch private/public
also not sure if I would have put this map lookup in a function in the first place (that needs a comment) given that it is used exactly once.
There was a problem hiding this comment.
I thought this kind of problem was detected by the linter ?
There was a problem hiding this comment.
It looks like it is disabled by default: golangci/golangci-lint#843
There was a problem hiding this comment.
also not sure if I would have put this map lookup in a function in the first place (that needs a comment) given that it is used exactly once
That's true. I wanted to separate the high-level logic (MigrateData) from the storage mechanism (annotations get/set), but indeed it feels a bit artificial here since getting from a map is pretty simple. No strong opinion, if I have to choose I would keep the current code though.
| @@ -8,8 +8,20 @@ import ( | |||
| "context" | |||
| "strings" | |||
|
|
|||
| logf "sigs.k8s.io/controller-runtime/pkg/log" | |||
|
|
|||
There was a problem hiding this comment.
this is IntelliJ separating the various import repos automatically
| @@ -76,12 +88,45 @@ func IsMigratingData(ctx context.Context, shardLister esclient.ShardLister, podN | |||
| return nodeIsMigratingData(podName, shards, excludedNodes), nil | |||
| } | |||
|
|
|||
| // AllocationExcludeAnnotationName returns the allocation exclude value stored in an annotation. | |||
| // May be empty if not set. | |||
| func allocationExcludeFromAnnotation(es esv1.Elasticsearch) string { | |||
There was a problem hiding this comment.
I thought this kind of problem was detected by the linter ?
anyasabo
changed the title
pebrc
added
>non-issue
and removed
>enhancement
|
Reverted in #2880 (thus |
This commits stores the last updated value of cluster routing allocation
exclude in an annotation of the Elasticsearch resource.
Before doing any call to the ES API for that setting, we check the
existing annotation. If its value is the same as the one we're about to
set, skip the ES API call.
If not, do the call then store the setting value in the annotation.
The annotation can be removed at any time: it will just trigger a call
to the Elasticsearch API with the right expected value, and the
annotation will be set again.
Users can manipulate this setting behind our back, in which case the
operator will only react based on the annotation value. We consider it's
the user responsibility to not mess cluster settings up at this point.
Fixes #1522.