Use es-operator #191
Use es-operator #191
Conversation
|
This change is |
|
I will keep follow up issues in this comment
|
pavolloffay
force-pushed
the
es-operator
branch
from
1d898ea
to
a8f280d
Compare
Codecov Report
@@ Coverage Diff @@
## master #191 +/- ##
==========================================
- Coverage 96.7% 95.18% -1.53%
==========================================
Files 32 35 +3
Lines 1639 1848 +209
==========================================
+ Hits 1585 1759 +174
- Misses 41 75 +34
- Partials 13 14 +1
Continue to review full report at Codecov.
|
pavolloffay
force-pushed
the
es-operator
branch
from
4cbb79c
to
1228805
Compare
pavolloffay
force-pushed
the
es-operator
branch
from
0b0f41f
to
b45835a
Compare
|
We should follow up on:
|
There was a problem hiding this comment.
Reviewed 42 of 52 files at r1, 21 of 21 files at r2.
Reviewable status: all files reviewed, 19 unresolved discussions (waiting on @pavolloffay)
a discussion (no related file):
I'm OK with this change, except for a couple of things. Mostly, we need to either make sure that we support multiple ES instances (one per Jaeger instance, based on the Jaeger instance's name), or make it clear that a single ES will be used for all Jaeger instances in the same namespace/cluster/...
This PR is also missing documentation. And finally, please confirm you did execute the e2e tests and that they are passing.
jaeger.version, line 5 at r2 (raw file):
# stable Jaeger version. When you update this file, make sure to update the # the docs as well. latest
Please, revert this change.
Makefile, line 10 at r2 (raw file):
OPERATOR_NAME ?= jaeger-operator NAMESPACE ?= jaegertracing
Please, revert this change.
Makefile, line 51 at r2 (raw file):
.PHONY: docker docker: build
Please, revert this change.
Makefile, line 65 at r2 (raw file):
.PHONY: e2e-tests e2e-tests: cassandra es crd build docker push
Please, revert this change.
build/Dockerfile, line 7 at r2 (raw file):
" && \ yum install -y $INSTALL_PKGS && \ rpm -V $INSTALL_PKGS && \
Is this a debugging command?
deploy/role.yaml, line 73 at r2 (raw file):
- jaeger verbs: - 'get'
Should get be within single quotes here?
deploy/examples/simple-prod-deploy-es.yaml, line 14 at r2 (raw file):
resources: esIndexCleaner: eabled: true
s/eable/enable
pkg/apis/io/v1alpha1/jaeger_types.go, line 147 at r2 (raw file):
type ElasticsearchSpec struct { Resources v1.ResourceRequirements `json:"resources"` NodeCount int32 `json:"nodeCount"`
This should be a pointer, so that we can differentiate between the user saying "I want 0 nodes" from the default value for an int32 var.
pkg/storage/elasticsearch.go, line 28 at r2 (raw file):
func ShouldDeployElasticsearch(s v1alpha1.JaegerStorageSpec) bool { if strings.ToLower(s.Type) != "elasticsearch" {
We are trying to standardize on EqualFold instead of ToLower == ...: #181
pkg/storage/elasticsearch.go, line 32 at r2 (raw file):
} _, ok := s.Options.Map()["es.server-urls"] if ok {
return !ok?
pkg/storage/elasticsearch.go, line 47 at r2 (raw file):
VolumeSource: v1.VolumeSource{ Secret: &v1.SecretVolumeSource{ SecretName: "jaeger-elasticsearch",
What happens if we have two Jaeger instances? Will they use the same secret? Typically, names use the Jaeger instance name so that we are able to have N Jaeger instances in the same location (namespace/cluster/...)
pkg/storage/elasticsearch.go, line 77 at r2 (raw file):
// we assume jaeger containers are first if len(p.Containers) > 0 { // the size of arguments arr should be always 2
arr == p.Containers?
pkg/storage/elasticsearch.go, line 97 at r2 (raw file):
if err != nil { logrus.Error("Failed to create Elasticsearch certificates: ", err) return nil, errors.Wrap(err, "failed to create Elasticsearch certificates")
This will cause the error to be logged several times, won't it? Either log, or return the error, assuming the caller will check/log the error.
pkg/storage/elasticsearch.go, line 113 at r2 (raw file):
ObjectMeta: metav1.ObjectMeta{ Namespace: ed.Jaeger.Namespace, Name: "elasticsearch",
Shouldn't this be named after the Jaeger instance? Same question as before: what happens if we have two similar Jaeger CRs in the same namespace?
pkg/storage/elasticsearch_secrets.go, line 77 at r2 (raw file):
"out": string(out)}). Error("Failed to create certificates") return fmt.Errorf("error running script %s: %v", script, err)
Can you get away without logging the error in the line above? Is the content of err good enough when logged by the caller? Otherwise, we might end up with two logs for the same error.
pkg/storage/elasticsearch_secrets.go, line 85 at r2 (raw file):
return &v1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: secretName,
Same as other comments: the name should be prefixed by the instance name.
pkg/storage/elasticsearch/v1alpha1/types.go, line 11 at r2 (raw file):
// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object // TODO temporal fix https://github.com/jaegertracing/jaeger-operator/issues/206
This is just a copy from the ES operator, right?
pavolloffay
force-pushed
the
es-operator
branch
from
dc37997
to
d91be1d
Compare
There was a problem hiding this comment.
Reviewable status: 57 of 59 files reviewed, 19 unresolved discussions (waiting on @jpkrohling and @pavolloffay)
Makefile, line 10 at r2 (raw file):
Previously, jpkrohling (Juraci Paixão Kröhling) wrote…
Please, revert this change.
Done.
Makefile, line 51 at r2 (raw file):
Previously, jpkrohling (Juraci Paixão Kröhling) wrote…
Please, revert this change.
I find this useful, you need to build before packaging. I was running make docker and wondering why my change is not there..
Makefile, line 65 at r2 (raw file):
Previously, jpkrohling (Juraci Paixão Kröhling) wrote…
Please, revert this change.
Done.
build/Dockerfile, line 7 at r2 (raw file):
Previously, jpkrohling (Juraci Paixão Kröhling) wrote…
Is this a debugging command?
rpm {-V|--verify} [select-options] [verify-options]
Verifying a package compares information about the installed files in the package with information about the files taken from the package metadata stored in the rpm database. Among other things, verifying compares the size, digest, permis‐
sions, type, owner and group of each file. Any discrepancies are displayed. Files that were not installed from the package, for example, documentation files excluded on installation using the "--excludedocs" option, will be silently
ignored.
Maybe not only debugging, I have taken it from cluster logging operator. Do you want to remove it?
deploy/role.yaml, line 73 at r2 (raw file):
Previously, jpkrohling (Juraci Paixão Kröhling) wrote…
Should
getbe within single quotes here?
The '' and "" are mixed in the whole file.
deploy/examples/simple-prod-deploy-es.yaml, line 14 at r2 (raw file):
Previously, jpkrohling (Juraci Paixão Kröhling) wrote…
s/eable/enable
Done.
pkg/apis/io/v1alpha1/jaeger_types.go, line 147 at r2 (raw file):
Previously, jpkrohling (Juraci Paixão Kröhling) wrote…
This should be a pointer, so that we can differentiate between the user saying "I want 0 nodes" from the default value for an int32 var.
0 nodes is invalid. In that case operator normalizes to default - 1 node.
pkg/storage/elasticsearch.go, line 28 at r2 (raw file):
Previously, jpkrohling (Juraci Paixão Kröhling) wrote…
We are trying to standardize on
EqualFoldinstead ofToLower == ...: #181
Done.
pkg/storage/elasticsearch.go, line 32 at r2 (raw file):
Previously, jpkrohling (Juraci Paixão Kröhling) wrote…
return !ok?
No if there is URL we don't deploy ES
pkg/storage/elasticsearch.go, line 47 at r2 (raw file):
Previously, jpkrohling (Juraci Paixão Kröhling) wrote…
What happens if we have two Jaeger instances? Will they use the same secret? Typically, names use the Jaeger instance name so that we are able to have N Jaeger instances in the same location (namespace/cluster/...)
At the moment there is a limitation. ES expects secret to be elasticsearch. So now you can deploy only one ES in a namespace. I will change this secret to reflect instance name to make it better for future.
They plan to change the naming convention in future to embed instance name in objects.
pkg/storage/elasticsearch.go, line 77 at r2 (raw file):
Previously, jpkrohling (Juraci Paixão Kröhling) wrote…
arr == p.Containers?
Not sure what you mean.
pkg/storage/elasticsearch.go, line 97 at r2 (raw file):
Previously, jpkrohling (Juraci Paixão Kröhling) wrote…
This will cause the error to be logged several times, won't it? Either log, or return the error, assuming the caller will check/log the error.
Done.
pkg/storage/elasticsearch.go, line 113 at r2 (raw file):
the Jaeger instance? Same question as before: what happens if we have two similar Jaeger CRs in the same namespace?
No see previous comment
pkg/storage/elasticsearch_secrets.go, line 77 at r2 (raw file):
n you get away without logging the error in the line
I would keep the log, it contains full output of the script.
pkg/storage/elasticsearch_secrets.go, line 85 at r2 (raw file):
Previously, jpkrohling (Juraci Paixão Kröhling) wrote…
Same as other comments: the name should be prefixed by the instance name.
See previous comment, it's not possible at the moment for all secrets
pkg/storage/elasticsearch/v1alpha1/types.go, line 11 at r2 (raw file):
Previously, jpkrohling (Juraci Paixão Kröhling) wrote…
This is just a copy from the ES operator, right?
yes sir!
There was a problem hiding this comment.
Reviewed 10 of 12 files at r3.
Reviewable status: 58 of 59 files reviewed, 4 unresolved discussions (waiting on @jpkrohling and @pavolloffay)
a discussion (no related file):
build/Dockerfile, line 7 at r2 (raw file):
Previously, pavolloffay (Pavol Loffay) wrote…
rpm {-V|--verify} [select-options] [verify-options]
Verifying a package compares information about the installed files in the package with information about the files taken from the package metadata stored in the rpm database. Among other things, verifying compares the size, digest, permis‐ sions, type, owner and group of each file. Any discrepancies are displayed. Files that were not installed from the package, for example, documentation files excluded on installation using the "--excludedocs" option, will be silently ignored.Maybe not only debugging, I have taken it from cluster logging operator. Do you want to remove it?
No, it's fine.
pkg/apis/io/v1alpha1/jaeger_types.go, line 147 at r2 (raw file):
Previously, pavolloffay (Pavol Loffay) wrote…
0 nodes is invalid. In that case operator normalizes to default - 1 node.
OK, for "ReplicaSize", 0 is acceptable, even though it's currently not supported (this will change). Being consistent would be nice, but if it really can't be 0 (like, "temporarily disable ES"), then it's OK. Personally, I still prefer to have a pointer and have the normalize routine to set it to 1 if it's nil, but it's not blocking for this PR.
pkg/storage/elasticsearch.go, line 32 at r2 (raw file):
Previously, pavolloffay (Pavol Loffay) wrote…
No if there is URL we don't deploy ES
I mean:
if ok {
return false
}
return true
is the same as:
return !ok
pkg/storage/elasticsearch.go, line 47 at r2 (raw file):
Previously, pavolloffay (Pavol Loffay) wrote…
At the moment there is a limitation. ES expects secret to be
elasticsearch. So now you can deploy only one ES in a namespace. I will change this secret to reflect instance name to make it better for future.They plan to change the naming convention in future to embed instance name in objects.
Ouch. Alright, understood, thanks for the explanation.
pkg/storage/elasticsearch.go, line 77 at r2 (raw file):
Previously, pavolloffay (Pavol Loffay) wrote…
Not sure what you mean.
The code comment mentions an arr, but there's no var named arr in the code. Perhaps arr is a previous iteration of what now is p.Containers?
pkg/storage/elasticsearch_secrets.go, line 77 at r2 (raw file):
Previously, pavolloffay (Pavol Loffay) wrote…
n you get away without logging the error in the line
I would keep the log, it contains full output of the script.
Is the script the shell script with dozens of lines? If it has a bug, it will always fail, which will make the log unreadable. I'd rather version the script somehow so that people can refer to the script at a given version.
pkg/storage/elasticsearch/v1alpha1/types.go, line 11 at r2 (raw file):
Previously, pavolloffay (Pavol Loffay) wrote…
yes sir!
OK, I think you did mention this in some other file, otherwise, it would be good to have it explicit as a comment, in addition to linking to the GitHub issue. Otherwise, we (or someone in the community) might be tempted to change stuff here in the future :-)
Not blocking, though.
Makefile, line 51 at r2 (raw file):
Previously, pavolloffay (Pavol Loffay) wrote…
I find this useful, you need to build before packaging. I was running make docker and wondering why my change is not there..
It might indeed be useful, but we can talk about it on a different PR.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r4, 3 of 4 files at r5.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @pavolloffay)
pkg/storage/elasticsearch_secrets.go, line 77 at r2 (raw file):
Previously, jpkrohling (Juraci Paixão Kröhling) wrote…
Is the script the shell script with dozens of lines? If it has a bug, it will always fail, which will make the log unreadable. I'd rather version the script somehow so that people can refer to the script at a given version.
I'm marking as resolved, but would be nice to keep an eye on this.
|
It's just a log print if it makes problems we can remove. e2e tests passed |
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
pavolloffay
force-pushed
the
es-operator
branch
from
09f4b94
to
91b6545
Compare
Resolves #183
depends on: openshift/origin-aggregated-logging#1500
The elasticsearch has to be deployed manually at the moment
https://github.com/openshift/elasticsearch-operator, The ES image has to with this change openshift/origin-aggregated-logging#1500
If the es deployment has been created before jaeger deployement the ES pod has to be restarted so it can load correct certificates - actually it waits for volume with certificates, but needs to be restarted if certs are changed - jaeger redeploy. We might create a different volume with certs per jaeger deployment.
Note that ES requires limits settings on node:
minishift ssh -- 'sudo sysctl -w vm.max_map_count=262144'