LOG-545: Cluster logging Elasticsearch rollover data design #108
Conversation
openshift-ci-robot
added
size/L
|
/uncc @abhinavdahiya |
enhancements/cluster-logging/cluster-logging-es-rollover-data-design.md
Outdated
Show resolved
Hide resolved
enhancements/cluster-logging/cluster-logging-es-rollover-data-design.md
Outdated
Show resolved
Hide resolved
enhancements/cluster-logging/cluster-logging-es-rollover-data-design.md
Outdated
Show resolved
Hide resolved
enhancements/cluster-logging/cluster-logging-es-rollover-data-design.md
Outdated
Show resolved
Hide resolved
|
|
||
| * ClusterLogging will expose the minimal needed set of ElasticSearch rollover policy management API in order to achieve the previously described goals | ||
| * ClusterLogging will manage rollover as ElasticSearch index management is either restricted by Elastic licensing or not available in the opensource version of OpenDistro | ||
| * Security will be addressed by using the OpenDistro security plugin. Details TBD |
There was a problem hiding this comment.
At least should we mention that Document Level Security will be required to do this?
enhancements/cluster-logging/cluster-logging-es-rollover-data-design.md
Outdated
Show resolved
Hide resolved
enhancements/cluster-logging/cluster-logging-es-rollover-data-design.md
Outdated
Show resolved
Hide resolved
enhancements/cluster-logging/cluster-logging-es-rollover-data-design.md
Outdated
Show resolved
Hide resolved
enhancements/cluster-logging/cluster-logging-es-rollover-data-design.md
Outdated
Show resolved
Hide resolved
|
/hold |
openshift-ci-robot
added
the
do-not-merge/hold
|
|
||
| ``` | ||
| #### Cluster Logging Operator | ||
| The `cluster-logging-operator` will utilized the ClusterLogging CR retention policy to spec |
| max_age: 3d | ||
| delete: | ||
| min_age: 7d | ||
| templates: |
There was a problem hiding this comment.
i think we should abstract this slightly... this is a lot of knobs. maybe we can tuck this under something similar to what you have for retentionPolicy in the ClusterLogging api
There was a problem hiding this comment.
i think we should abstract this slightly... this is a lot of knobs. maybe we can tuck this under something similar to what you have for
retentionPolicyin the ClusterLogging api
We should have a larger discussion here with respect to who is defining what? I understand the desire to hide this behind API but the general logic from the beginning has been: CLO provides abstract, EO allows more fine tuning. Given there is an opportunity to apply these performance improvents to any ES deployment we likely do not want to have CLO and Jaeger have to specify the same thing. We do however, at a minimum need a way for the parent operator to tell the child to which indices we apply these changes. This will allow a more flexible use of Elasticsearch by cluster logging and jaeger which is where we desire to go.
I'm open to suggestions but I don't think there is anyway for EO and CLO to share the same retention policy API
There was a problem hiding this comment.
Simplified the api with the assumption the EO will infer the templates and indices to create
| * Create and seed specified index templates | ||
| * Create the initial indices as needed | ||
| * Block data ingestion if needed until initial index is seeded | ||
| * Deploy curation CronJob for each entry in the specified seedings to rollover using the defined policy |
There was a problem hiding this comment.
EO will be responsible for this now?
There was a problem hiding this comment.
We probably can get away with a single curator cronjob, not sure we need one per entry
There was a problem hiding this comment.
We probably can get away with a single curator cronjob, not sure we need one per entry
I believe we can greatly simplify our curation needs. Do we really need the actual curator python code to perform this function? We should be able to reduce it to something like a curl statement which has a singular responsibility of rolling a specific index which means one per. Doesn't seem the "load" is significant
There was a problem hiding this comment.
Do we really need the actual curator python code to perform this function?
no
We should be able to reduce it to something like a curl statement
or the golang elasticsearch api, or even a generic REST api
| #### Upgrade | ||
| The `elasticsearch-operator` will migrate existing indices to the new data design: | ||
| * Indices beginning with `project.*` are indexed to `container.app-write` | ||
| * Indices beginning with `.operations.*` are indexed to `journal.infra-write` |
There was a problem hiding this comment.
is this because we currently co-locate our node and container infra logs? e.g. we just are trying to prevent losing logs with this (given we will be writing to a node. or container. for infra)
There was a problem hiding this comment.
Correct so we do not lose existing logs. It's possible we could actually spin through the .operations index and split out the journal and container logs but I imagine it is easier to move the index whole sale instead of trying to split it. Opinions?
There was a problem hiding this comment.
Why have the distinction of container vs journal logs to start?
There was a problem hiding this comment.
Only reason it exists because node logs come from journal. My original thought was to call it node.XXXX... would that be better?
There was a problem hiding this comment.
Perhaps so that it is not tied to a name referring to the implementation of how logs from the node are collected.
There was a problem hiding this comment.
replaced 'journal' with 'node'
| **Note:** Migrated container infrastructure logs are available via the `infra` alias only and | ||
| subsequently curated from the cluster during period rollover. | ||
|
|
||
| The `cluster-logging-operator` will remove previously deployed CronJobs |
There was a problem hiding this comment.
maybe specifically call out it'll remove ones that it previously owned only.
|
|
||
| The goals of this proposals are: | ||
| * Utilize a data design that aligns data schema with Elasticsearch's recommendations. | ||
| * Expose data management policy as API in the `cluster-logging-operator` and `elasticsearch-operator` in support of Cluster Logging's mission to gather container and infrastructure logs |
There was a problem hiding this comment.
is it a goal or non-goal to migrate existing customer log data to this new design?
There was a problem hiding this comment.
Seems like if this is implemented with the v7 rollout of Elasticsearch (okay, v6), then migrating from v5 to v6 could take care of moving old to new.
There was a problem hiding this comment.
it was a bit of a leading question but yes i expect data migration to be a goal. but it should be stated clearly (or indicated that it is not)
There was a problem hiding this comment.
If we don't migrate from 5 to 7 now, then we'll have to have another release real soon that will migrate from 6 to 7 since 6 is going to be EOL "soon".
We have to solve the data migration no matter what, one way or another. It would be great if this plan laid out how that migration is going to be handled.
There was a problem hiding this comment.
Added migration goal with additional details in the implementation section that follows
| |---------|-----------|---------------|---------------| | ||
| | Node (`logs.infra`)|infra, infra.journal|infra.journal-write|infra.journal-00001| | ||
| | Container Infra (`logs.infra`)|infra, infra.container|infra.container-write|infra.container-000001| | ||
| | Container Application (`logs.app`)|app.logs|app.container-write|app.container-000001| |
There was a problem hiding this comment.
Is it reasonable that audit logs should be separated from infra?
There was a problem hiding this comment.
i think so.....we're treating them separately for forwarding, right?
There was a problem hiding this comment.
It certainly is reasonable. They tend to have different usage and even data retention policy than infra logs.
There was a problem hiding this comment.
added audit logs to data model
enhancements/cluster-logging/cluster-logging-es-rollover-data-design.md
Outdated
Show resolved
Hide resolved
| The `elasticsearch-operator` will migrate existing indices to the new data design: | ||
| * Indices beginning with `project.*` are indexed to `container.app-write` | ||
| * Indices beginning with `.operations.*` are indexed to `journal.infra-write` | ||
| * Migrated indices are deleted after migration |
There was a problem hiding this comment.
How does the migration process handle the situation where the retention policy is changed along with the upgrade?
There was a problem hiding this comment.
How does the migration process handle the situation where the retention policy is changed along with the upgrade?
The data will be subject to the new retention policy. It's possible we could infer their policy from the previous curator deployment but I don't know how that manifests itself into the CL instance since the operator is not supposed to modify the spec. @bparees ?
There was a problem hiding this comment.
reducing their retention on upgrade(and thus immediately throwing away some set of logs) seems like a non-starter.
There was a problem hiding this comment.
So, if I have a lower retention policy in the new cluster, when the data is migrated, is it immediately deleted? If it is not, is that a problem?
There was a problem hiding this comment.
We could additionally handle old data retention and curation by simply doing nothing, including not migrating data to the new schema. We would leave the existing cronjob and ES would be optimized over time as old data is curated away to the point where data in the old schema is no longer present. This would be an easier path for us to follow and addresses @portante concerns. This will work as long as curator does not match any indices from the new schema which I believe it does not.
There was a problem hiding this comment.
There was a problem hiding this comment.
is curation on by default? or might some clusters not have any curation happening?
if we can tolerate both data schemas from a query/view perspective, indefinitely, then this seems like an ideal answer regardless...
There was a problem hiding this comment.
Landed on leaving aliases alone and aliasing them to the new format, relying on DSL for security, and curation to roll off.
There was a problem hiding this comment.
Landed on leaving aliases alone and aliasing them to the new format, relying on DSL for security, and curation to roll off.
leaving pre-existing indicies alone and adding them to the new alias format.
|
cc @alanconway |
| status: implementable | ||
| --- | ||
|
|
||
| # Cluster Logging Elasticsearch Rollover Data Design |
There was a problem hiding this comment.
It isn't really about just rollover data design - it is about using rollover + single indices in conjunction with DLS for access control/multi-tenancy
There was a problem hiding this comment.
This proposal is strictly about modifying the data design and introducing rollover. It is correct that in order for us to full-fill our multitenant requirements we will need to implement DLS but that is to be addressed in a separate proposal. I would expect we do both or neither.
There was a problem hiding this comment.
Seems like you need to explicitly link this design to be depend on the DLS design being accepted, otherwise this design is a non-starter.
There was a problem hiding this comment.
@portante that is not correct. Rollover addresses our scalability and performance concerns but not multi-tenancy requirements. It's possible that we could design something that sits in front of ES to restrict access which is orthogonal to roll over.
There was a problem hiding this comment.
I would expect we do both or neither.
Even without DLS the roll-over would be still great improvement. If DLS is not in place we should still push for use of roll-over.
There was a problem hiding this comment.
@portante that is not correct. Rollover addresses our scalability and performance concerns but not multi-tenancy requirements. It's possible that we could design something that sits in front of ES to restrict access which is orthogonal to roll over.
If you have N namespaces, you still have N indices if the size of the data in those indices does not meet the rollover limits. The scale of the number of indices is still based on namespaces, not size/time.
So this might alleviate the issue in some cases, but without DLS, or some other implementation that addresses multi-tenancy, it is not clear how this can be a "design" that would be accepted and implemented in the product.
|
I have concerns about handling or exposing rollover configuration in Elasticsearch CR. Handling rollover seems to me like application-specific (CLO) task and not elasticsearch concern. Consider making the retention part in ES CR more generic (to include alias name) or just manage the rollover only in CLO CR. Just for completeness, I will describe how we have implemented rollover in Jaeger. The retention policies (size, number of days) can be specified in Jaeger CR. Based on the provided configuration Jaeger operator creates a job to initialize storage (create aliases, install index template) and then two cron jobs. The first one calls rollover API and the second one removes an index from the read alias. |
I disagree with this statement given that rollover addresses the scalability and performance of Elasticsearch. It is a management API to allow ES indices to be curated based on policy. The policy should adhere to ES scalability guidelines. The policy should be flexible enough for an application to define and the EO to manage. There is no reason for the management to be replicated by multiple higher level components. I would point out Elastic will provide this management for you if you use the licensed version. Additionally, this proposal would take advantage of the OpenDistro index management if it existed in the ES6 version of their product.
CL is an abstraction of logging and not Elasticsearch. The policy will reflect data retention regardless of the implemented technology. CLO will not be controlling rollover in order to better isolate the separation of concerns
My understanding of Jaeger is that it is possible to use alternate storage engines which sounds like these policies don't apply to anything but ES. It's likely the proposed changes are made to EO that you can remove the same functionality in the Jaeger Operator. Implemented correctly, there is no reason you could still not use Jaeger rollover by simply not defining indexManagement on the Elasticsearch resource. |
| * The Curator Cronjob deployed by the `cluster-logging-operator` will be removed. The responsibilities for curation will be subsumed by | ||
| implementation of Elasticsearch rollover management. | ||
| * Curation configuration by namespace is no longer configurable and is restricted to cluster | ||
| wide settings associated with log type |
There was a problem hiding this comment.
I am not a fan of not having a possibility to control amount of logs per project. It is fair to say that it comes with risks.
Say some developer turns on TRACE level or makes unwise decision (or simply a mistake) and one or a few projects start generating a lot of logs. As of now it is at least possible to go and ad-hoc delete individual history indices for project. But when all logs are blend into a common index there is no such possibility and all other projects will get hit as well (even whole ES cluster can easily suffer) with no simple solution (except from curating all the common logs for projects sharing this index).
If we can not control this on a project level then we should at least make sure we ship appropriate alert rules to inform cluster admins that logs suddenly started growing much faster and something needs to be done about this.
There was a problem hiding this comment.
This is the reason why we need to node-level controls for logging rates, see containers/conmon#84
There was a problem hiding this comment.
@lukas-vlcek This change only would deprecate namespace curation for now. We have discussed modifying LogForwarding to allow include/exclude which maybe its possible to also address throttling.
| status: implementable | ||
| --- | ||
|
|
||
| # Cluster Logging Elasticsearch Rollover Data Design |
There was a problem hiding this comment.
I would expect we do both or neither.
Even without DLS the roll-over would be still great improvement. If DLS is not in place we should still push for use of roll-over.
Does the Jaeger operator work with a separate Elasticsearch operator? |
There was a problem hiding this comment.
Regarding which CRs should handle roll-over: Are we building the Elasticsearch CR to some more abstract notion of storage management that allows a consumer of that abstraction to specify constraints for how it operates? If so, then that it would make sense to hide the roll-over implementation detail behind that abstraction.
But if the consumer still has to "know" it is Elasticsearch under the covers, then it seems like retention should be specified once in the CL CR since the abstraction still leaks details about its implementation.
It is the same operator which watches globally. There is no way to deploy multiple versions of the CRD on the cluster, or at least there was not when we originally released it. |
The general guiding principle is CL is abstract and ES allows us to tune more knobs
There is no reason a consumer of cluster logging needs to know we are using Elasticsearch and thus the separation. |
I meant by "consumer" not the SRE team deploying cluster logging, but the difference between the CL CR and ES CR. The CL CR in a sense "depends" on the ES CR. So if the ES CR is not abstract, then the notions it offers must leak into how the CL CR is constructed. Which one could argue means it is not worth having the retention related configuration in both places. |
In that case the index management API in the elastic CR should allow configuring the alias name which will be used for rollover. My impression was that it will allow to rollover only aliases used by cluster logging.
It's tightly integrated with ECL ES operator, if one wants to use elatic.co operator all the configuration have to be provided explicitly https://www.jaegertracing.io/docs/1.15/operator/#external-elasticsearch |
| the desired aforementioned Elasticsearch CR indexManagement. | ||
| #### Elasticsearch API | ||
| ``` | ||
| apiVersion: "elasticsearch.openshift.io/v1alpha1" |
There was a problem hiding this comment.
@ewolinetz moving our discussion here:
@jcantril i'm still not sold on it being a separate CR.. is that based on https://github.com/openshift/enhancements/pull/108/files#r346559159 ?
Eric Wolinetz 16 hours ago
we could make the elasticsearch level CR be the source of truth.. this way jaeger could specify it on their CR and then CLO could also specify it for EO to consume...
The advantages of one instance per:
- the instance explicitly defines management for one index which is specific to that index
- Index Management is not dependent on ES instance other then a ref to which specific instance (not reflected here)
- Index Management changes can be processed without needing to evaluate ES changes
Disadvantages: - Need to explicitly ref the Elasticsearch instance to gather secrets, and ES service
- Handle case where ES doesnt exist
@bparees @alanconway Do you have an opinion of handling in this way versus previous commits where it was added to the ES resource?
|
|
||
| #### Assumptions | ||
|
|
||
| * ClusterLogging will expose the minimal needed set of Elasticsearch rollover policy management API in order to achieve the previously described goals |
There was a problem hiding this comment.
can we clarify which "Elasticsearch" we're referring to here? I'm assuming its the elasticsearch CRD and not the component Elasticsearch...
There was a problem hiding this comment.
This line "Elasticsearch rollover policy management API" I am referring to the actual Elasticsearch rollover api.
|
@jcantrill this lgtm. |
jcantrill
force-pushed
the
log545_rollover_data
branch
from
3618dc6
to
3117b47
Compare
|
/hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ewolinetz, jcantrill 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 |
Ref: https://jira.coreos.com/browse/LOG-545
cc @bparees @ewolinetz @richm @lukas-vlcek @igor-karpukhin @portante @pavolloffay @billmvt