-
Notifications
You must be signed in to change notification settings - Fork 719
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use annotation to track created remote clusters #2891
Use annotation to track created remote clusters #2891
Conversation
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.
LGTM
if err != nil { | ||
return remoteClustersInEs, err | ||
} | ||
for remoteClusterName := range remoteClusterSettings.PersistentSettings.Cluster.RemoteClusters { |
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 guess we can ignore transient settings for the purposes of this function?
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 is the assumption I made since we are only interested in clusters created by ECK.
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.
LGTM
// 3.1 Schedule its deletion from the Elasticsearch settings | ||
// 3.2 Otherwise remove it from the annotation | ||
// 4. Update the annotation on the Elasticsearch object | ||
// 5. Apply the settings through the Elasticsearch API |
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 love that sort of comment 👍
Jenkins test this please |
* Use annotation to track created remote clusters
The PR fixes an issue where the annotation used to cache the Elasticsearch state wrt remote clusters may become inconsistent and could prevent some updates to be applied.
The annotation is kept but only to track the remote clusters that have been previously managed by ECK. In order to be consistent with the other API calls in ECK the algorithm never tries to optimize the number of calls performed on the ES cluster.
It can be noted that it still exists an edge case where ECK may delete a cluster managed by the user:
remote-foo
is declared as remote in the Specremote-foo
is removed from the specremote-foo
manuallyremote-foo
💥At each iteration a log is printed with the upserted and deleted remote clusters.
Last PR to completely fix #2864 , related to #2880
@anyasabo I think it should be included in
1.1.0
to avoid a mix of "remote clusters related" labels on Elasticsearch resources in the future.