Add kind: Elasticsearch to black_list so it is not marked for deletion - #988
Conversation
mvallati
changed the title
kind: Elasticsearch to black_list so it is not prunedkind: Elasticsearch to black_list so it is not marked for deletion
9f20a02 to
a4b4504
Compare
| result = fetch_resources(namespaced: namespaced).map do |resource| | ||
| next unless resource["verbs"].one? { |v| v == "delete" } | ||
| if resource["kind"] == "Elasticsearch" | ||
| elasticsearch_found = true |
There was a problem hiding this comment.
This could probably be simplified if we don't use the elasticsearch_found variable entirely and instead just emit the metric here like:
StatsD.client.gauge('elasticsearch_resources.count', 1, tags: @namespace_tags) if resource["kind"] == "Elasticsearch"
| next unless resource["verbs"].one? { |v| v == "delete" } | ||
| if resource["kind"] == "Elasticsearch" | ||
| elasticsearch_found = true | ||
| next |
There was a problem hiding this comment.
This next is a bit redundant since the next line:
next if black_list.include?(resource["kind"])
would cover Elasticsearch anyways.
| [resource["apigroup"], resource["version"], resource["kind"]].compact.join("/") | ||
| end.compact | ||
|
|
||
| StatsD.client.gauge('elasticsearch_resources.count', 1, tags: @namespace_tags) if elasticsearch_found |
There was a problem hiding this comment.
Both can work but I think a counter could make more sense here instead of a gauge since the number of times that a specific app can fail will keep going up.
There was a problem hiding this comment.
The naming of the metric could be something more specific as well. Essentially this metric tells us when the resource was attempted to be deleted so the name should reflect that as well.
| [resource["apigroup"], resource["version"], resource["kind"]].compact.join("/") | ||
| end.compact | ||
|
|
||
| StatsD.client.gauge('elasticsearch_resources.count', 1, tags: @namespace_tags) if elasticsearch_found |
There was a problem hiding this comment.
Can you verify what is actually stored in @namespace_tags at this point in time? Tracing back where it is used in this file in the CustomResourceDefinition model which inherits from the KubernetesResrouce model, the constructor makes it seem like its for extra optional statsd tags
a4b4504 to
17a885b
Compare
| black_list = %w(Namespace Node ControllerRevision Event) | ||
| fetch_resources(namespaced: namespaced).map do |resource| | ||
| black_list = %w(Namespace Node ControllerRevision Event Elasticsearch) | ||
| result = fetch_resources(namespaced: namespaced).map do |resource| |
There was a problem hiding this comment.
We could probably keep what was previously there and no need to assign this to result and instead just have fetch_resources() as the last function call in this function.
| [resource["apigroup"], resource["version"], resource["kind"]].compact.join("/") | ||
| end.compact | ||
|
|
||
| result |
de5b7a2 to
d2ea72a
Compare
| fetch_resources(namespaced: namespaced).map do |resource| | ||
| next unless resource["verbs"].one? { |v| v == "delete" } | ||
| if resource["kind"] == "Elasticsearch" | ||
| StatsD.client.count('elasticsearch_resource_deletion_attempt.count', 1, tags: namespace) |
There was a problem hiding this comment.
The format that tags expects is something on the lines of https://github.com/Shopify/krane/blob/main/lib/krane/kubernetes_resource.rb#L602-L614
So its <label>:<value> stored in an array
3bf1922 to
2b780c6
Compare
| fetch_resources(namespaced: namespaced).map do |resource| | ||
| next unless resource["verbs"].one? { |v| v == "delete" } | ||
| if resource["kind"] == "Elasticsearch" | ||
| StatsD.client.count('elasticsearch_resource_deletion_attempt.count', 1, %W(context:#{context} namespace:#{namespace})) |
There was a problem hiding this comment.
The tags need to be in the tags key when emitting the metric. Similar to
Line 68 in d1d42e5
Both count and increment should do very similar things but I would use whichever function is more used in this repo just to remain consistent.
There was a problem hiding this comment.
changed it to increment to match existing code
6048bd5 to
df3ac2f
Compare
df3ac2f to
1634f46
Compare
3 participants
TLDR of what we're trying to accomplish with this PR
Relevant issue: https://github.com/shop/issues-search-platform/issues/493
This PR adds the
Elasticsearchresource type to theblack_listso that it is not marked for deletion if someone removes it from their manifest files. We are also emitting a metric so that we know if an Elasticsearch resource was removed from an app's manifest files so that we can be alerted.The problem we need to solve
This is a necessary part of the search platform's team to migrate apps over to the search control plane and to the infrastructure monorepo. We have a step-wise process for apps:
kind: Elasticsearchfrom their manifest fileskind: Elasticsearchwill be removed from their appSince these 2 steps are not done in immediate succession, there's the opportunity for teams to accidentally delete their
kind: Elasticsearch. This is a plausible scenario because their elasticsearch instance is now being deployed via the search control plane, and it's easy for the communication of DO NOT DELETE. to get lost overtime.If the
Elasticsearchresource were to be accidentally deleted, it will break their elasticsearch, and it may not be recoverable. AddingElasticsearchto theblack_listwill prevent apps from entering this state, and will ensure their elasticsearch continues to run and exist while they wait to complete step 2 and migrate to the infrastructure monorepo.Why the metric?
We added the metric so that we can alert on it in the even a team does remove their
kind: Elasticsearchfrom their manifests files. If we do not alert, we will have no way of knowing this has occurred. If the app has not migrated to the infrastructure monorepo and removes theirkind: Elasticsearch, we will need to add it back to their manifest files until they have migrated to the monorepo. Otherwise, the Elasticsearch will be orphaned and no longer connected to the app. This will be problematic if the app is migrated to a new cluster (gcp version upgrades, regional evacuation), the Elasticsearch would not be tied to the app and would not migrate with it, and it would likely not be recoverable. We will determine as a team how we want to use this alert and will create a playbook.Worth mentioning
As a result of this change that no Elasticsearch resources will be deleted. We will either have to manually delete them when apps successfully migrate to the infrastructure monorepo, or they will remained orphaned and will be deleted when the kubernetes cluster is deleted. There is no "problem" with them being orphaned, other than that they are not being used and costing money to do nothing.