Add Elasticsearch index cleaner as cron job #155
Conversation
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
|
This change is |
| @@ -16,27 +16,21 @@ spec: | |||
| spec: | |||
| containers: | |||
| - name: elasticsearch | |||
| image: docker.elastic.co/elasticsearch/elasticsearch:5.6.0 | |||
| image: docker.elastic.co/elasticsearch/elasticsearch:5.6.12 | |||
There was a problem hiding this comment.
I have bumped patch version. And disabled xpack, we should not use it anyways. There an official ES6x without licensed xpack, but I would rather keep using the lowest supported version.
| // JaegerEsIndexCleanerSpec holds the options related to es-index-cleaner | ||
| type JaegerEsIndexCleanerSpec struct { | ||
| Enabled bool `json:"enabled"` | ||
| NumberOfDays int `json:"numberOfDays"` |
There was a problem hiding this comment.
Used 7 days as default. TTL in c* is 2 day though...
There was a problem hiding this comment.
The script supports other configuration properties, see https://github.com/jaegertracing/jaeger/blob/master/plugin/storage/es/esCleaner.py#L11
index prefix, es hostname, I have not exposed them directly but they are derived from storage properties map.
Codecov Report
@@ Coverage Diff @@
## master #155 +/- ##
==========================================
+ Coverage 96.32% 96.36% +0.03%
==========================================
Files 29 30 +1
Lines 1362 1430 +68
==========================================
+ Hits 1312 1378 +66
- Misses 39 40 +1
- Partials 11 12 +1
Continue to review full report at Codecov.
|
| @@ -150,6 +151,14 @@ type JaegerDependenciesSpec struct { | |||
| ElasticsearchNodesWanOnly bool `json:"elasticsearchNodesWanOnly"` | |||
| } | |||
|
|
|||
| // JaegerEsIndexCleanerSpec holds the options related to es-index-cleaner | |||
| type JaegerEsIndexCleanerSpec struct { | |||
| Enabled bool `json:"enabled"` | |||
There was a problem hiding this comment.
By default disabled.
I think we should enable it by default along with dependencies job. I would rather do it in a separate PR to keep this minimal.
There was a problem hiding this comment.
Reviewed 20 of 20 files at r1.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @pavolloffay)
deploy/examples/simple-prod.yaml, line 13 at r1 (raw file):
es: server-urls: http://elasticsearch:9200 username: elastic
Isn't this the default one?
pkg/apis/io/v1alpha1/jaeger_types.go, line 156 at r1 (raw file):
Previously, pavolloffay (Pavol Loffay) wrote…
By default disabled.
I think we should enable it by default along with dependencies job. I would rather do it in a separate PR to keep this minimal.
+1
Could you also check the other Enabled flags? I think they are *bool, mostly because they default to true. While it would be OK to have bool instead of *bool for this one, I think we should consider being consistent.
In time: I know there are other Enabled bool there, but those are recent changes as well and should be changed to *bool if we decide to be consistent among all Enabled flags.
pkg/cronjob/es_index_cleaner.go, line 18 at r1 (raw file):
func CreateEsIndexCleaner(jaeger *v1alpha1.Jaeger) *batchv1beta1.CronJob { applyIndexCleanerDefaults(&jaeger.Spec.Storage.EsIndexCleaner)
I think this might belong to the normalize() function in the strategy package.
pkg/cronjob/es_index_cleaner_test.go, line 28 at r1 (raw file):
func TestApplyEsCleanerDefaults(t *testing.T) { viper.Set("jaeger-es-index-cleaner-image", "foo")
You might need a defer viper.Reset(), otherwise other tests might be affected if they get this value from viper later on.
pkg/cronjob/spark_dependencies.go, line 24 at r1 (raw file):
func CreateSparkDependencies(jaeger *v1alpha1.Jaeger) *batchv1beta1.CronJob { applySparkDependenciesDefaults(jaeger)
I think I missed this before, but the same comment I left before applies here: this might belong to the normalize function. The idea is to centralize all this kind of logic there, which might be composed of different values from different objects (like, production with in-memory storage).
pkg/strategy/all-in-one.go, line 90 at r1 (raw file):
} if c.jaeger.Spec.Storage.Type == "elasticsearch" && c.jaeger.Spec.Storage.EsIndexCleaner.Enabled {
The same comment as I left for a previous PR: if the value is true here, it means the user has explicitly set it to true, so, there's an expectation that the cleaner will be installed. So, log.Info if Enabled == true and Storage != es.
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
There was a problem hiding this comment.
Reviewable status: 14 of 22 files reviewed, 8 unresolved discussions (waiting on @jpkrohling)
deploy/examples/simple-prod.yaml, line 13 at r1 (raw file):
Previously, jpkrohling (Juraci Paixão Kröhling) wrote…
Isn't this the default one?
it is, but I have disabled xpack. ES cleaner does not support auth
pkg/apis/io/v1alpha1/jaeger_types.go, line 156 at r1 (raw file):
Previously, jpkrohling (Juraci Paixão Kröhling) wrote…
+1
Could you also check the other
Enabledflags? I think they are*bool, mostly because they default totrue. While it would be OK to haveboolinstead of*boolfor this one, I think we should consider being consistent.In time: I know there are other
Enabled boolthere, but those are recent changes as well and should be changed to*boolif we decide to be consistent among allEnabledflags.
As I mention in the previous comment i would rather do it in a separate PR
pkg/cronjob/es_index_cleaner.go, line 18 at r1 (raw file):
Previously, jpkrohling (Juraci Paixão Kröhling) wrote…
I think this might belong to the
normalize()function in the strategy package.
+1
pkg/cronjob/es_index_cleaner_test.go, line 28 at r1 (raw file):
Previously, jpkrohling (Juraci Paixão Kröhling) wrote…
You might need a
defer viper.Reset(), otherwise other tests might be affected if they get this value from viper later on.
+1
pkg/cronjob/spark_dependencies.go, line 24 at r1 (raw file):
Previously, jpkrohling (Juraci Paixão Kröhling) wrote…
I think I missed this before, but the same comment I left before applies here: this might belong to the
normalizefunction. The idea is to centralize all this kind of logic there, which might be composed of different values from different objects (like,productionwithin-memorystorage).
Done.
pkg/strategy/all-in-one.go, line 90 at r1 (raw file):
Previously, jpkrohling (Juraci Paixão Kröhling) wrote…
The same comment as I left for a previous PR: if the value is
truehere, it means the user has explicitly set it totrue, so, there's an expectation that the cleaner will be installed. So,log.InfoifEnabled== true and Storage !=es.
Done.
There was a problem hiding this comment.
Reviewed 8 of 8 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @pavolloffay)
pkg/apis/io/v1alpha1/jaeger_types.go, line 156 at r1 (raw file):
Previously, pavolloffay (Pavol Loffay) wrote…
As I mention in the previous comment i would rather do it in a separate PR
Agreed.
CronJob for https://github.com/jaegertracing/jaeger/blob/master/plugin/storage/es/esCleaner.py#L11