add fix for metricsProperties when HasPrometheusConfigFile is true. #847

jinxingwang · 2020-03-25T22:00:10Z

Fix the bug where Monitor.Prometheus.ConfigFile is set. the configMap which contains spark metrics.properties & prometheus config file will not be created & mounted, so the metricsProperties will be useless.
Add a new property metricsPropertiesFile, this will overwrite the value spark.metrics.conf in spark.properties. give a better way to specify spark metrics configuration when it is large.

jinxingwang · 2020-03-27T17:54:12Z

@liyinan926 Please review.
When I am running the build commands such as:
go generate
controller-gen ...
make build-api-docs

There are auto-generate code for previous PRs such as lifeCycle, TerminationGracePeriod
Are those not included intentionally?

liyinan926 · 2020-03-27T17:57:45Z

The authors of the previous PRs must have forgotten to run these. Please include them in this PR.

liyinan926 · 2020-03-29T20:10:07Z

pkg/apis/sparkoperator.k8s.io/v1beta2/types.go

@@ -557,6 +557,10 @@ type MonitoringSpec struct {
 	// +optional
 	// If not specified, the content in spark-docker/conf/metrics.properties will be used.
 	MetricsProperties *string `json:"metricsProperties,omitempty"`
+	// MetricsPropertiesFile is the address of file metrics.properties for configuring the Spark metric system.


Suggest change address to the container local path.

liyinan926 · 2020-03-29T20:38:54Z

pkg/controller/sparkapplication/monitoring_config.go

-			port, configFile)
-	} else {
-		glog.V(2).Infof("Using the default Prometheus configuration.")
+	if !app.HasMetricsPropertiesFile() || !app.HasPrometheusConfigFile() {


Can we refactor the if-else block like the following to make it easier to read and understand?

var javaOption string if app.HasMetricsPropertiesFile() && app.HasPrometheusConfigFile() { glog.V(2).Infof("Loading local metrics and Prometheus configuration files") ... javaOptions = ... } else { glog.V(2).Infof("Creating a ConfigMap for metrics and/or Prometheus configurations") ... javaOptions = ... }

liyinan926 · 2020-03-29T20:40:35Z

pkg/controller/sparkapplication/monitoring_config_test.go

-		if configMap.Data[metricsPropertiesKey] != test.metricsProperties {
+		if test.app.Spec.Monitoring.Prometheus.ConfigFile != nil &&
+			test.app.Spec.Monitoring.MetricsPropertiesFile == nil &&
+			len(configMap.Data) > 1 {


len(configMap.Data) != 1

liyinan926 · 2020-03-29T20:40:40Z

pkg/controller/sparkapplication/monitoring_config_test.go

+
+		if test.app.Spec.Monitoring.Prometheus.ConfigFile == nil &&
+			test.app.Spec.Monitoring.MetricsPropertiesFile != nil &&
+			len(configMap.Data) > 1 {


len(configMap.Data) != 1

…c in monitoring_config.go.

jinxingwang · 2020-03-30T16:26:52Z

@liyinan926 Please review again, thanks

liyinan926 · 2020-03-30T17:58:37Z

pkg/controller/sparkapplication/monitoring_config.go

-	if !app.HasMetricsPropertiesFile() || !app.HasPrometheusConfigFile() {
-		glog.V(2).Infof("Loading Prometheus configuration.")
+	if app.HasMetricsPropertiesFile() && app.HasPrometheusConfigFile() {
+		glog.V(2).Infof("Loading local Spark metrics and Prometheus configuration files, not creating configMap with default properties.")


The code blocks below should be moved into this if block:

if app.HasPrometheusConfigFile() { configFile := *app.Spec.Monitoring.Prometheus.ConfigFile glog.V(2).Infof("Overriding the default Prometheus configuration with config file %s in the Spark image.", configFile) javaOption = fmt.Sprintf("-javaagent:%s=%d:%s", app.Spec.Monitoring.Prometheus.JmxExporterJar, port, configFile) } if app.HasMetricsPropertiesFile() { app.Spec.SparkConf["spark.metrics.conf"] = *app.Spec.Monitoring.MetricsPropertiesFile }

And in the else block, generate javaOptions the other way.

I think I will set the logic back to the original. with clarify comments.
if app.HasMetricsPropertiesFile() && app.HasPrometheusConfigFile() {
} else {
}
not really cover all the cases. to cover all the case:
if app.HasMetricsPropertiesFile() && app.HasPrometheusConfigFile() {
Overwtie defaul spark metrics file & defaul prometheus config file
} else if app.HasMetricsPropertiesFile() {
Overwtie defaul spark metrics file & create CM with default prometheus config file
} else if app.HasPrometheusConfigFile(){
Overwtie defaul prometheus config file & create CM with default spark metrics file
} else {
create CM with default spark metrics file & defaul prometheus config file
}

I think you can combine the three else cases into one, as you already handle the metrics config file and Prometheus config file individually in buildPrometheusConfigMap. So basically there are cases: 1) no metrics config file nor Prometheus config file are specified, and 2) at least one of the two config files is not specified, and the ConfigMap needs to be created.

If I do that. there will be places for duplicate code blocks. As a example here:

if app.HasMetricsPropertiesFile() && app.HasPrometheusConfigFile() { // using PrometheusConfigFile javaOption = ... // using MetricsPropertiesFile app.Spec.SparkConf["spark.metrics.conf"] = ... } else { // Create configMap configMap := buildPrometheusConfigMap(app, configMapName) if app.HasPrometheusConfigFile() { // using PrometheusConfigFile javaOption = ... } else { // using default PrometheusConfigFile javaOption = ... } // using MetricsPropertiesFile if app.HasMetricsPropertiesFile() { app.Spec.SparkConf["spark.metrics.conf"] = ... } }

OK, let's just keep the current structure in the PR.

liyinan926 · 2020-03-31T15:44:14Z

pkg/controller/sparkapplication/monitoring_config.go

+	// If one of the metricsPropertiesFile & prometheusConfigFile is not set
+	// mounting configMap to using default configuration
+	if !app.HasMetricsPropertiesFile() || !app.HasPrometheusConfigFile() {
+		glog.V(2).Infof("Loading Prometheus configuration.")


Can we change this to Creating a ConfigMap for metrics and Prometheus configurations?

I meant the log message, not the comment :).

liyinan926 · 2020-03-31T15:59:59Z

pkg/controller/sparkapplication/monitoring_config.go

+	// If one of the metricsPropertiesFile & prometheusConfigFile is not set
+	// mounting configMap to using default configuration
+	if !app.HasMetricsPropertiesFile() || !app.HasPrometheusConfigFile() {
+		glog.V(2).Infof("Loading Prometheus configuration.")


I meant the log message, not the comment :).

liyinan926 · 2020-03-31T16:00:56Z

pkg/controller/sparkapplication/monitoring_config.go

-			port,
-			config.PrometheusConfigMapMountPath,
-			prometheusConfigKey)
+	glog.V(2).Infof("Setting the default Prometheus configuration.")


Please also remove this log line that is pretty useless.

liyinan926

LGTM. Thanks for contributing!

jinxingwang force-pushed the metricsProperties-fix branch 3 times, most recently from d143557 to d1ddea8 Compare March 26, 2020 16:00

add fix for metricsProperties when HasPrometheusConfigFile is true.

181cb74

jinxingwang force-pushed the metricsProperties-fix branch 5 times, most recently from 54a4d33 to e6345c5 Compare March 27, 2020 16:50

add new config MetricsPropertiesFile.

2461216

jinxingwang force-pushed the metricsProperties-fix branch from e6345c5 to 2461216 Compare March 27, 2020 17:02

add missing auto-generated code from privous PRs.

4112c80

jinxingwang force-pushed the metricsProperties-fix branch from 823fe63 to 4112c80 Compare March 28, 2020 00:26

liyinan926 reviewed Mar 29, 2020

View reviewed changes

fix monitoring_config_test.go test condition, redo the configmap logi…

833fd50

…c in monitoring_config.go.

liyinan926 reviewed Mar 30, 2020

View reviewed changes

redo the configmap & javaOption logic in monitoring_config.go.

715e138

jinxingwang force-pushed the metricsProperties-fix branch 2 times, most recently from 4c2c23f to 8fdcbf2 Compare March 30, 2020 19:56

set back the configmap & javaOption logic in monitoring_config.go

3ca28f2

jinxingwang force-pushed the metricsProperties-fix branch from 8fdcbf2 to 3ca28f2 Compare March 30, 2020 19:58

liyinan926 reviewed Mar 31, 2020

View reviewed changes

update log.

993d011

jinxingwang force-pushed the metricsProperties-fix branch from 94439fb to 993d011 Compare March 31, 2020 16:04

liyinan926 approved these changes Mar 31, 2020

View reviewed changes

liyinan926 merged commit 5afcce2 into kubeflow:master Mar 31, 2020

jinxingwang deleted the metricsProperties-fix branch March 31, 2020 16:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add fix for metricsProperties when HasPrometheusConfigFile is true. #847

add fix for metricsProperties when HasPrometheusConfigFile is true. #847

jinxingwang commented Mar 25, 2020 •

edited

jinxingwang commented Mar 27, 2020

liyinan926 commented Mar 27, 2020

liyinan926 Mar 29, 2020

liyinan926 Mar 29, 2020

liyinan926 Mar 29, 2020

liyinan926 Mar 29, 2020

jinxingwang commented Mar 30, 2020

liyinan926 Mar 30, 2020

jinxingwang Mar 30, 2020 •

edited

jinxingwang Mar 30, 2020

liyinan926 Mar 30, 2020

jinxingwang Mar 30, 2020 •

edited

liyinan926 Mar 31, 2020

liyinan926 Mar 31, 2020

liyinan926 Mar 31, 2020

liyinan926 Mar 31, 2020

liyinan926 Mar 31, 2020

liyinan926 left a comment

add fix for metricsProperties when HasPrometheusConfigFile is true. #847

add fix for metricsProperties when HasPrometheusConfigFile is true. #847

Conversation

jinxingwang commented Mar 25, 2020 • edited

jinxingwang commented Mar 27, 2020

liyinan926 commented Mar 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinxingwang commented Mar 30, 2020

Choose a reason for hiding this comment

jinxingwang Mar 30, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinxingwang Mar 30, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liyinan926 left a comment

Choose a reason for hiding this comment

jinxingwang commented Mar 25, 2020 •

edited

jinxingwang Mar 30, 2020 •

edited

jinxingwang Mar 30, 2020 •

edited