-
Notifications
You must be signed in to change notification settings - Fork 90
Add delete by query to index management #797
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
Conversation
/hold |
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.
Awesome job so far! It looks you got the handle from top to bottom and vice versa. 🚀
Namespace []DeleteNamespaceSpec `json:"deleteNamespace,omitempty"` //#BEE: Why is this an array? | ||
} | ||
|
||
type DeleteNamespaceSpec struct { | ||
// The unique name of the spec | ||
Name string `json:"name"` // delete-<namespace_name> | ||
// Namespaces to be deleted | ||
NamespacesToDelete []string `json:"namespace"` | ||
// Delete the records matching the namespaces which are older | ||
// than this MinAge (e.g. 1d) | ||
MinAge TimeUnit `json:"minAge"` |
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.
Besides that I suggest to improve naming a bit:
type IndeManagementDeleteNamespaceSpec struct {
// The unique name of the spec
Name string `json:"name"` // delete-<namespace_name>
// Namespace to delete documents from older than minAge.
Namespace string `json:"namespace"`
// Delete the records matching the namespaces which are older
// than this MinAge (e.g. 1d)
MinAge TimeUnit `json:"minAge"`
}
Consider the official kubernetes API conventions when adding/extending APIs:
https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md
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 can see that you changed the datatype of Namespace
from []string
to string
. This is because this one:
Namespaces []IndeManagementDeleteNamespaceSpec
json:"namespaces,omitempty"``
is already an array, right?
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.
Exactly each namespace spec is dedicated to a single namespace.
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.
@periklis but the actual thought during proposal was that a user can have multiple namespace in a single spec.
For example,
Spec-1:
Name - policy1
Namespace - openshift-api, openshift-logging
Age - 1d
Spec-2:
Name - policy2
Namespace - openshift-kube-server
Age - 5h
Even the delete_by_query api is capable enough to take comma separated index names
// | ||
MinAge TimeUnit `json:"minAge"` | ||
// +nullable | ||
Namespace []DeleteNamespaceSpec `json:"deleteNamespace,omitempty"` //#BEE: Why is this an array? |
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 believe the array
for Namespace
means that you can define a delete-by-query spec per namespace, because you can define a different MinAge
per Namespace
. @sasagarw Do you agree? If yes, we should improve the API naming because if it is confusing to us, it will certainly be confusing for API consumers.
type IndexManagementDeletePhaseSpec struct {
// The minimum age of an index before it should be deleted (e.g. 10d)
//
MinAge TimeUnit `json:"minAge"`
// The per namesapce specification to delete documents older than a given minimum age.
Namespaces []IndeManagementDeleteNamespaceSpec `json:"namespaces,omitempty"`
}
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.
Yes @periklis , you are right.
Yeah we can rename it to Namespaces
to avoid confusion.
} | ||
|
||
if policy.Phases.Delete != nil { | ||
if policy.Phases.Delete != nil { //#BEE: I think I need to read namespace value here |
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 does not require to check if policy.Phases.Delete.Namespaces
is set. If empty, the following code compiles an empty JSON list that can be checked in the python code. If empty, the python code can skip the delete by query routine.
} | ||
envvars = append(envvars, | ||
corev1.EnvVar{Name: "MIN_AGE", Value: strconv.FormatUint(minAgeMillis, 10)}, | ||
corev1.EnvVar{Name: "NAMESPACE_NAME", Value: string(namespaceNamesJson)}, //#BEE: now I can use this in the delete script |
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.
The env var name NAMESPACE_NAME
does not represent the purpose of namespaceNamesJson
well enough. Latter is a list of namespaces while former sounds like a single name. I suggest to rename to something in plural case: NAMESPACE_SPECS
, NAMEPACES
...
envvars = append(envvars, | ||
corev1.EnvVar{Name: "MIN_AGE", Value: strconv.FormatUint(minAgeMillis, 10)}, | ||
corev1.EnvVar{Name: "NAMESPACE_NAME", Value: string(namespaceNamesJson)}, //#BEE: now I can use this in the delete script | ||
corev1.EnvVar{Name: "MIN_AGE_DBQ", Value: minAgeValue}, |
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.
Assuming that we have a list of namespaces specs, a single MIN_AGE_DBQ
is not correct here. I suggest you pass create a small dict structure called namespaceSpecsJson
that gives a good handle to execute a Delete-By-Query per Namespace, e.g.:
{
"namespace_a": "1d",
"namespace_b": "2d",
},
internal/indexmanagement/scripts.go
Outdated
#here namespace is either a string or array of strings, minAge is also a string in the unit of days, e.g., '3d' | ||
def deleteByQuery(index, namespace, minAge): | ||
defaultAge='7d' |
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 should be an env var, which we pass to the k8s cronjob. Just in case a user wants to overwrite this temporarily for all delete-by-query calls.
internal/indexmanagement/scripts.go
Outdated
if namespace != None and minAge == None: | ||
s = s.query('terms', kubernetes__namespace_name=namespace) #Or just #Q('terms', tags=['kubernetes.namespace_name', namespace]) | ||
elif namespace == None and minAge != None: | ||
s = s.filter('range', **{'@timestamp': {'lt': 'now-{}'.format(minAge)}}) | ||
elif namespace == None and minAge == None: | ||
s = s.filter('range', **{'@timestamp': {'lt': 'now-{}'.format(defaultAge)}}) | ||
elif namespace != None and minAge != None: | ||
s = s.query('terms', kubernetes__namespace_name=namespace) | ||
s = s.filter('range', **{'@timestamp': {'lt': 'now-{}'.format(minAge)}}) |
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 much filtering is not required. This is complicating the code.
I would suggest you to define a query as:
body = {
"query" : {
"terms": {
"kubernetes.namespace_name": [] // Namespace
}
The above query would be used when minAge
is not defined. Which suggests to delete all data belonging to that namespace.
Similarly, if minAge
is defined then you can append the filter part to the above query, in which case, the body will look like
body = {
"query": {
"bool": {
"must": [
{
"terms": {
"kubernetes.namespace_name": [] // Namespace
}
}
],
"filter": [
{
"range": {
"@timestamp": { "lt": "now-$age" } // $age=MinAge
}
}
]
}
}
}
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.
Does this means that there should always be a namespace_name given? because otherwise it would delete the entire index if no namespace nor minAge is given. (only those entries older than defaultAge, of course)
In this case I shouldn't check for this condition?
if namespace == None and minAge == None:
s = s.filter('range', **{'@timestamp': {'lt': 'now-{}'.format(defaultAge)}})
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.
If namespace_name is not given then delete_by_query
function itself will be never called right. IMO there is no use of checking and generating dynamic body. WDYT?
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 agree.. I asked because you wrote it as a condition in your spike document. It makes more sense this way!
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.
That was mentioned in Spike document just to list all possible cases. You can implement in a way which is making more sense.
internal/indexmanagement/scripts.go
Outdated
#BEE: Call deleteByQuwey here? | ||
namespaceName=$NAMESPACE_NAME | ||
minAgeDBQ=$MIN_AGE_DBQ |
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.
You can just check if these env vars is null or not. If not null then you should be calling your defined python function here and pass these variables to that function.
Call below func inside delete():
function deleteByQuery() {
<-------- define variables and read them using $1, $2, ... ------------>
python -c 'import indexManagementClient; print(indexManagementClient.deleteByQuery(<-- pass the read variables --->))'
}
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.
Thanks for the proposed implementation. The following are my comments:
1. How are we going to control which indices the delete query is run against?
Which indices are we going to allow to execute delete_by_query operation against?
Proposed deleteByQuery()
has three arguments. One of them being the index
name. It is not clear to me where we are going to take this value from.
Proposal doc mentions:
index_pattern = mapping.Name // app* | infra* | audit*
perhaps this means we will use hardcoded index_pattern of app*,infra*,audit*
?
Related (sub-)question is which user/permission will be used when delete_by_query is run? I assume this is similar to the user that is running the job to delete indices which means pretty powerful user, right?
2. Why we do not allow single delete operation for multiple namespaces?
It seems to me that current design and implementation do not allow to run one common delete_by_query for multiple namespaces sharing common name prefix.
This is a question of both efficiency and practicality.
How about if CU will need to keep specific namespaces under control but does not have a full control over the naming of the namespace, except for common prefix?
For example an OCP user can create high number of namespaces for devel. experiments but the deal will be that all namespaces must start with John_Doe_experiment*
. Like John_Doe_experiment-000001ABX
, John_Doe_experiment-004207WBT
, ... To make sure John does no go wild with logging all his namespaces will be pruned by delete_by_query without the need to configure job for all individual namespaces.
For this purpose we can use "Prefix query" that works on non-analyzed fields (i.e. fields that are good fit for term queries as well).
3. How do we prevent running the same delete query before the previous is finished?
The code is using the Elasticsearch Python client and is calling the delete_by_query API.
BTW, some notes regarding proposed use of this API:
- it is missing the third argument – the
doc_type
which in our case is_doc
- it is setting
default_operator='AND'
which is irrelevant. It would be relevant if a query of type "Query String Query" would be used. But it is not in our case (proposed implementation is using the "terms" query – BTW, given that current implementation proposal allows to run delete operation per single namespace then use of "term" query (not "terms") would be more appropriate).
The documentation specifies, that by default the call to delete\_by\_query
API is blocking until it is complete. Internally, Elasticsearch uses Bulk API to delete matching documents. This means that the bulk queue throughput is critical. In other words if the cluster is under heavy ingestion pressure then the delete by query operation will compete with indexing requests coming from collectors. Given many CUs are running under-resourced cluster then risk that some delete queries may not be finished when a new one is started (by cron) is increasing.
It can also happen that there will be a need (a request) to run delete jobs more frequently.
I suggest to investigate possibility to run the delete operations in async manner (see wait_for_completion=false
parameter). To prevent overlapping jobs we could use the Task API to pull list of all running actions=*/delete/byquery
jobs. If there are any we can skip current cron cycle (assuming the only entity that originates these tasks is our cron job). Side effect is that this would allow us to run the delete by query operation job more frequently(?).
4. Merging segments to clear storage from deleted documents
Finally, when individual documents are deleted from index they are actually not physically removed from the disk until corresponding index segments are merged. Some segments will be merged automatically (typically smaller segments) but for some segments it can take time before they are merged.
If the ultimate goal is to reduce the size of indices then we need to control segment merging as well. But segment merging is resource intensive and time consuming operation. At this point I am not sure if we should/want to call segment force merge in the end of the cron job (which will be impossible if we decide to use delete_by_query in async manner as suggested above) or if it will be better to implement independent job that would be run more frequently and whose sole purpose would be to run some heuristics and merge segments if it is needed (and possible – for example it does not make sense to merge segments if there are no free CPUs or IO resources).
Thanks Lukas for the detailed review. Here are some comments from my side:
In the beginning I assumed that since
It seems that the delete_by_query API allows for a Prefix query:
The last 2 questions regarding async run and merging we can discuss further in a call |
// +optional | ||
Name string `json:"name"` // delete-<namespace_name> |
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 shouldn't be marked as optional
. A unique name for each spec is required to be provided.
// +optional | |
Name string `json:"name"` // delete-<namespace_name> | |
// +required | |
Name string `json:"name"` // delete-<namespace_name> |
// If MinAge for NamespaceSpec is empty, delete the records matching the namespaces which are older than this default age | ||
DefaultAge TimeUnit `json:"defaultAge"` |
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.
Why this DefaultAge
here? Already inside IndexManagementDeleteNamespaceSpec it has been defined right? You can just check that empty and use the default age.
// Namespace to delete | ||
Namespace string `json:"namespace"` |
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.
You can mark it as +required
just for validation purpose.
// The per namesapce specification to delete documents older than a given minimum age | ||
Namespaces []IndexManagementDeleteNamespaceSpec `json:"namespaceSpec"` |
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.
Since this is optional
, you can mark them +optional
for validation.
// The per namesapce specification to delete documents older than a given minimum age | ||
Namespaces []IndexManagementDeleteNamespaceSpec `json:"namespaceSpec"` |
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.
// The per namesapce specification to delete documents older than a given minimum age | |
Namespaces []IndexManagementDeleteNamespaceSpec `json:"namespaceSpec"` | |
// The per namesapce specification to delete documents older than a given minimum age | |
Namespaces []IndexManagementDeleteNamespaceSpec `json:"namespaceSpec,omitempty"` |
aa2d9ed
to
abfde6e
Compare
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.
Awesome work! Really amazing progress 🚀
I suggest to add the following changes to the PR for the sake of completeness:
- When we extend the API we should elaborate if we add the new fields by default in our samples per CRD. I believe this feature belong there too (https://github.com/openshift/elasticsearch-operator/blob/master/config/samples/logging_v1_elasticsearch.yaml)
- The new cronjobs should be tested for suspend/unspend when ES pods are not available (https://github.com/openshift/elasticsearch-operator/blob/master/internal/indexmanagement/index_management_test.go#L55)
|
||
// How often to run a new prune-namespaces cron job | ||
// +required | ||
PrunePollInterval TimeUnit `json:"prunePollInterval"` |
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.
For better visibility that this is required to be configured with Namespaces
I suggest we rename this field to something like PruneNamespacesInterval
, WDYT?
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 agree, I went with PrunePollInterval
to be consistent with PollInterval
:
PollInterval TimeUnit `json:"pollInterval"` |
But
PruneNamespacesInterval
is more descriptive
type IndexManagementDeleteNamespaceSpec struct { | ||
// The unique name of the spec | ||
// +required | ||
Name string `json:"name"` // delete-<namespace_name> |
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.
What is // delete-<namespace_name>
supposed to tell the reader here?
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 one is from the original spike document. I honestly do not use it anywhere inside the code. I'll ask Sashank why he had it there originally. Otherwise I believe it can be omitted
// Namespace to delete | ||
// +required | ||
Namespace string `json:"namespace"` |
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.
To minimize confusion of API docs readers, I suggest to adapt the field comment to something like:
// Target Namespace to delete logs older than MinAge (defaults to ...)
In addition we should mention that prefix queries like openshift*
are allowed here.
// If MinAge for NamespaceSpec is empty, delete the records matching the namespaces which are older than this default age | ||
// +required | ||
DefaultAge TimeUnit `json:"defaultAge"` |
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.
As mentioned in a previous call, we should drop this field for a constant in the index_management
package.
bundle.Dockerfile
Outdated
summary="This is the bundle for the elasticsearch-operator" \ | ||
maintainer="AOS Logging <aos-logging@redhat.com>" | ||
|
||
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.
nit. let's not add unneeded thing into a PR's diff.
func pruneNamespacesCmd(policy apis.IndexManagementPolicySpec) string { | ||
cmd := "" | ||
if policy.Phases.Delete != nil { | ||
cmd = "./prune-namespaces" | ||
} | ||
return cmd | ||
} |
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 function is actually obsolete as is. We should check for policy.Phases.Delete != nil
in the caller site and if it is nil, we should not schedule any delete-by-query cronjob at all instead of scheduling one with an empty command.
internal/indexmanagement/scripts.go
Outdated
#tasks = es_client.tasks.list(actions="delete/byquery") | ||
#print (tasks) | ||
#print (json.dumps(s.to_dict(), indent=2, sort_keys=True)) |
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 believe we can drop these commented lines as agreed upon that this call should not be executed async, right?
internal/indexmanagement/scripts.go
Outdated
print(e) | ||
sys.stdout = open('/tmp/response.txt', 'w') | ||
print(e) | ||
sys.stdout = original_stdout |
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.
What is this double print good for here? Can't we just simply print the exception into the original stdout?
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.
These were for debugging purposes, will be cleaned up soon
internal/indexmanagement/scripts.go
Outdated
for aliasBase in $writeAliases; do | ||
echo $aliasBase |
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.
Is this debugging like still required?
internal/indexmanagement/scripts.go
Outdated
|
||
const deleteThenRolloverScript = ` | ||
set -uo pipefail | ||
source /tmp/scripts/indexManagement |
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.
Is this source required? As per /tmp/scripts/delete
already sourcing the same file?
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 believe this was added by mistake, my bad!
- namespace: openshift-logging | ||
minAge: 5h |
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 a bad example actually. The Logging stack does not collect any logs from its component. This recursive collection is counter-productive. A good example would be to collect the openshift-monitoring
logs.
var ( | ||
namespaceSpecs = make(map[string]string) | ||
namespaceSpecsString = "" | ||
) | ||
namespaceSpecs = make(map[string]string) |
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.
Twice make(map[string]string)
} | ||
|
||
// prune-namespaces cron job | ||
if policy.Phases.Delete != nil && policy.Phases.Delete.Namespaces != nil { |
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.
Checking len(policy.Phases.Delete.Namespaces) != 0
is more useful here. It ensures that if your slice is nil or empty at the same time, the prune job will not be scheduled.
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 after removing all the debug print lines in the delete-by-query script.
/hold |
/unhold |
@btaani you need to make the lint happy. |
internal/indexmanagement/scripts.go
Outdated
#print (json.dumps(s.to_dict(), indent=2, sort_keys=True)) | ||
print (json.dumps(response, indent=2)) |
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.
Need to drop these debug lines
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 think I will keep this one:
print (json.dumps(response, indent=2))
as it shows the detailed response and how many documents were delete or were in conflict
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.
But except a developer no one would be interested in this information right? WDYT?
If it can be printed in higher log level then it makes sense to keep here IMO.
1033fb2
to
f01f90c
Compare
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
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: btaani, periklis The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Description
Adding delete by query to index management, along with the API changes and cron job updates
/cc @lukas-vlcek
/assign @periklis
Links