Homogenize logs #1168

anyasabo · 2019-06-28T17:53:27Z

Part of resolving #305

Moves all logs to either Info or Debug (V(1)).
Standardizes on using namespace and name fields where appropriate for easy searching of logs
Removes any use of Info(fmt.Sprintf())
Small refactor: removes unnecessary resource parameter in reconcileFinalizers()

operators/pkg/controller/elasticsearch/remotecluster/elasticsearch.go

operators/pkg/controller/elasticsearch/restart/annotations.go

thbkrkr · 2019-07-01T12:24:56Z

Nice work! I like the standardization on using namespace and name. I have a small fear that this leads to confusion in some cases. For example it can be the name of an Elasticsearch cluster, a pod or a secret and sometimes for me it will be not clear. I'm wondering if we should add a kind field or be explicit by adding a prefix to the name field (pod_name, secret_name, es_name)?

anyasabo · 2019-07-01T13:50:07Z

I can see the prefix making sense since some things might have more than one relevant item (e.g. you might want to log the ES cluster name and the pod name in the same message), so a separate kind item might not be clear

sebgl · 2019-07-02T08:52:38Z

operators/cmd/manager/main.go

-		opts.MetricsBindAddress = fmt.Sprintf(":%d", metricsPort)
-		log.Info(fmt.Sprintf("Exposing Prometheus metrics on /metrics%s", opts.MetricsBindAddress))
-	}
+	log.Info("Exposing Prometheus metrics on /metrics", "port", metricsPort)


Looks like this is changing the behaviour?

Ah you're right, this was sloppy. That said I wonder if we do just want to set a default port (probably 80?) and always expose prometheus metrics

sebgl · 2019-07-02T08:54:07Z

operators/pkg/controller/elasticsearch/processmanager/state.go

@@ -101,6 +101,8 @@ func (p *Process) updateState(action string, signal syscall.Signal, lastErr erro

 	err := p.state.Write()
 	if err != nil {
+		// TODO (sabo): is this necessary? seems helpful since it will be structured and easier to parse than an exit message


+1, I think we can keep it

sebgl · 2019-07-03T06:49:49Z

operators/pkg/controller/common/certificates/ca_reconcile.go

@@ -201,7 +201,7 @@ func buildCAFromSecret(caInternalSecret corev1.Secret) *CA {
 	}
 	privateKey, err := ParsePEMPrivateKey(privateKeyBytes)
 	if err != nil {
-		log.Info("Cannot parse PEM private key from CA secret, will create a new one", "err", err)
+		log.Error(err, "Cannot parse PEM private key from CA secret]")


Is the ] a typo here?
Also, I liked the "will create a new one" part.

I had removed the "will create a new one" because this function isn't doing that. This is just returning a CA if it can, or returning nil if it can't.

anyasabo · 2019-07-04T17:54:17Z

This is ready for re-review, I think I addressed your all's comments and added prefixes where necessary.

operators/pkg/controller/common/certificates/http/reconcile.go

operators/pkg/controller/kibana/kibana_controller.go

thbkrkr · 2019-07-04T19:23:39Z

More than 3 and it's perfect. Thank you for doing this job a bit ungrateful.

thbkrkr · 2019-07-08T09:12:08Z

operators/test/e2e/test/elasticsearch/steps_init.go

@@ -67,7 +67,7 @@ func (b Builder) InitTestSteps(k *test.K8sClient) test.StepList {
 						return err
 					}
 					if err == nil {
-						return fmt.Errorf("elasticsearch %s is still there", b.Elasticsearch.Name)
+						return fmt.Errorf("elasticsearch %s is still there", k8s.ExtractNamespacedName(&stack.Elasticsearch))


CI reported:

test/e2e/test/elasticsearch/steps_init.go:70:87: undefined: stack

PR #1170 did a massive refactoring of E2E tests, in particular to remove the stack concept. So the stack Stack became a b Builder.

thbkrkr

LGTM!
It remains one small bit to change related to the #1170 refactoring.

* Support for APM server configuration (#1181) * Add a config section to the APM server configuration * APM: Add support for keystore * Factorize ElasticsearchAuthSettings * Update dev setup doc + fix GKE bootstrap script (#1203) * Update dev setup doc + fix GKE bootstrap script * Update wording of container registry authentication * Ensure disks removal after removing cluster in GKE (#1163) * Update gke-cluster.sh * Implement cleanup for unused disks in GCP * Update Makefile * Update CI jobs to do proper cleanup * Normalize the raw config when creating canonical configs (#1208) This aims at counteracting the difference between JSON centric serialization and the use of YAML as the serialization format in canonical config. If not normalizing numeric values like 1 will differ when comparing configs as JSON deserializes integer numbers to float64 and YAML to uint64. * Homogenize logs (#1168) * Don't run tests if only docs are changed (#1216) * Update Jenkinsfile * Simplify notOnlyDocs() * Update Jenkinsfile * Push snapshot ECK release on successful PR build (#1184) * Update makefile's to support snapshots * Add snapshot releases to Jenkins pipelines * Cleanup * Rename RELEASE to USE_ELASTIC_DOCKER_REGISTRY * Update Jenkinsfile * Add a note on EKS inbound traffic & validating webhook (#1211) EKS users must explicitly enable communication from the k8s control plane and nodes port 443 in order for the control plane to reach the validating webhook. Should help with #896. * Update PodSpec with Hostname from PVC when re-using (#1204) * Bind the Debug HTTP server to localhost by default (#1220) * Run e2e tests against custom Docker image (#1135) * Add implementation * Update makefile's * Update Makefile * Rename Jenkisnfile * Fix review comments * Update e2e-custom.yml * Update e2e-custom.yml * Return deploy-all-in-one to normal * Delete GKE cluster only if changes not in docs (#1223) * Add operator version to resources (#1224) * Warn if unsupported distribution (#1228) The operator only works with the official ES distributions to enable the security available with the basic (free), gold and platinum licenses in order to ensure that all clusters launched are secured by default. A check is done in the prepare-fs script by looking at the existence of the Elastic License. If not present, the script exit with a custom exit code. Then the ES reconcilation loop sends an event of type warning if it detects that a prepare-fs init container terminated with this exit code. * Document Elasticsearch update strategy change budget & groups (#1210) Add documentation for the `updateStrategy` section of the Elasticsearch spec. It documents how (and why) `changeBudget` and `groups` are used by ECK, and how both settings can be specified by the user.

Jon Sabo added 7 commits June 26, 2019 14:57

WIP

a64d634

WIP

d71d844

WIP

c3a880d

Merge master

345c66d

WIP

add406a

WIP

9796501

Cleanup todos

4af1bc9

thbkrkr reviewed Jul 1, 2019

View reviewed changes

operators/pkg/controller/elasticsearch/remotecluster/elasticsearch.go Outdated Show resolved Hide resolved

thbkrkr reviewed Jul 1, 2019

View reviewed changes

operators/pkg/controller/elasticsearch/restart/annotations.go Outdated Show resolved Hide resolved

thbkrkr mentioned this pull request Jul 1, 2019

Kibana HTTP certificates #1148

Merged

5 tasks

sebgl reviewed Jul 2, 2019

View reviewed changes

Revert prom metrics port change

ff76120

sebgl reviewed Jul 3, 2019

View reviewed changes

Jon Sabo added 2 commits July 4, 2019 12:17

Add prefixes

e7d8c2b

Merge master

8618964

anyasabo changed the title ~~Homogenize logs~~ [WIP] Homogenize logs Jul 4, 2019

Jon Sabo added 2 commits July 4, 2019 12:39

Rm

723ded4

Update as

f4b1831

anyasabo changed the title ~~[WIP] Homogenize logs~~ Homogenize logs Jul 4, 2019

thbkrkr reviewed Jul 4, 2019

View reviewed changes

operators/pkg/controller/common/certificates/http/reconcile.go Outdated Show resolved Hide resolved

thbkrkr reviewed Jul 4, 2019

View reviewed changes

operators/pkg/controller/common/certificates/http/reconcile.go Outdated Show resolved Hide resolved

thbkrkr reviewed Jul 4, 2019

View reviewed changes

operators/pkg/controller/kibana/kibana_controller.go Outdated Show resolved Hide resolved

Jon Sabo added 3 commits July 5, 2019 16:11

Merge master

6a396c9

Update owner labels

6413622

Fix merge conflict

86f58c9

thbkrkr reviewed Jul 8, 2019

View reviewed changes

thbkrkr approved these changes Jul 8, 2019

View reviewed changes

Update e2e

086390b

anyasabo merged commit 11b8614 into elastic:master Jul 8, 2019

anyasabo deleted the debuglogs branch July 8, 2019 18:57

thbkrkr mentioned this pull request Jul 16, 2019

Implement rolling upgrades with StatefulSets #1219

Merged

pebrc added >refactoring v0.9.0 labels Jul 19, 2019

thbkrkr mentioned this pull request Aug 2, 2019

Refactor nodes orchestration to rely on StatefulSets #1463

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Homogenize logs #1168

Homogenize logs #1168

anyasabo commented Jun 28, 2019

thbkrkr commented Jul 1, 2019

anyasabo commented Jul 1, 2019

sebgl Jul 2, 2019

anyasabo Jul 3, 2019

sebgl Jul 2, 2019

sebgl Jul 3, 2019

anyasabo Jul 4, 2019

anyasabo commented Jul 4, 2019

thbkrkr commented Jul 4, 2019

thbkrkr Jul 8, 2019 •

edited

Loading

thbkrkr left a comment

Homogenize logs #1168

Homogenize logs #1168

Conversation

anyasabo commented Jun 28, 2019

thbkrkr commented Jul 1, 2019

anyasabo commented Jul 1, 2019

sebgl Jul 2, 2019

Choose a reason for hiding this comment

anyasabo Jul 3, 2019

Choose a reason for hiding this comment

sebgl Jul 2, 2019

Choose a reason for hiding this comment

sebgl Jul 3, 2019

Choose a reason for hiding this comment

anyasabo Jul 4, 2019

Choose a reason for hiding this comment

anyasabo commented Jul 4, 2019

thbkrkr commented Jul 4, 2019

thbkrkr Jul 8, 2019 • edited Loading

Choose a reason for hiding this comment

thbkrkr left a comment

Choose a reason for hiding this comment

thbkrkr Jul 8, 2019 •

edited

Loading