Operator refactor - validation and import cleanup #1356

pweil- · 2020-08-03T21:12:33Z

Round 1 - go!

Let's see what clean up we can do here @timflannagan1.

This has the following commits that can be reviewed independently

move the type declarations to their own file and try to comment them up for future readers
since the reporting operator was not exporting anything, introduce an interface and unexport the type. Consumers deal with the interface now
move all the smaller validation blurbs from the various sections that are called during New, newOperator, and Run methods and consolidate on a single validation call to ensure the config is good during New. No good config == no operator to run. Bonus: Add tests for all validations
rename cb* imports to remove references to chargeback

There are a couple questions sprinkled in where things weren't obvious.

…xported

…ion, remove validation sections from operator runtime methods where possible.

pweil- · 2020-08-03T21:14:11Z

/assign @timflannagan1

timflannagan · 2020-08-03T21:22:18Z

pkg/operator/operator.go

-	log "github.com/sirupsen/logrus"
-	"github.com/taozle/go-hive-driver"
+	"github.com/kube-reporting/metering-operator/pkg/db"
+	cbClientset "github.com/kube-reporting/metering-operator/pkg/generated/clientset/versioned"


If we're tackling this kind of work now, I would expect to also rename this import and all of its occurrences too, to something like meteringClientset or anything that doesn't involve the old "chargeback" name.

If we're tackling this kind of work now

We don't have to merge this now, I think it should be no-op so it shouldn't be too risky but bugs first and we can merge these when master opens if that't the right thing to do.

I would expect to also rename this import and all of its occurrences too, to something like meteringClientset or anything that doesn't involve the old "chargeback" name.

Easy enough, I'll add a new commit for this refactor

pkg/operator/operator.go

timflannagan

I can take a better look tomorrow when my brain is probably fresher, but I think this is awesome so far. A lot of this part of the codebase has been somewhat neglected for a while now and could use some general maintenance work.

timflannagan · 2020-08-03T21:28:02Z

Something else that I would like to tackle, but is probably out of the scope of this kind of work, is improving (or adding) comments to types/methods/functions to the various reporting-operator components, like what does New() ... or op.Run achieve such that a person not familiar with controllers would be able to see what we're trying to achieve with those functions.

timflannagan · 2020-08-03T21:30:30Z

pkg/operator/operator.go

+	errs := IsValidConfig(&cfg)
+	if errs != nil {
+		return nil, errs
 	}


Building off my comment from above, I wonder if we should be doing all of the reporting configuration validation in the cmd/reporting-operator driver function that provides the entry point for this operator, rather than in the constructor.

it can be called from the cmd package but it should not be done there. Commands in Kube/OpenShift follow a pattern of Complete, Validate, Run. We should refactor ours to match and call the IsValidConfig method in the command's Validate method. However we should keep it in new as well to protect against programmatic access to the operator no matter how it's started in the future.

Example of the recommended pattern: https://github.com/openshift/oc/blob/a695d74ef1aee9a3f605d38dd8b6fae2062b63fc/pkg/cli/tag/tag.go#L93-L116

pweil- · 2020-08-04T11:44:06Z

improving (or adding) comments to types/methods/functions to the various reporting-operator components,

this is 100% a goal here. Anything we touch in a refactor should get comments IMO, hence the API comments.

pweil- · 2020-08-04T12:15:01Z

Some other observations for the discussion (perhaps this PR, perhaps future)

figure out why we have so many threads in a single operator pod and if we can reduce and maintain performance (possibly removing the need for any mutex guarded code depending on the answer)
reduce the configuration complexity where possible. Example is the namespace configuration where we require AllNamespaces to be set with TargetNamespaces when we could just key on TargetNamespaces and do the right thing. Same for the TLS configs with a boolean that turns it off or on when we could just have the existence of the certs enable it.
move items that are ancillary to the operator into their own packages

pkg/operator/operator.go

pweil- · 2020-08-04T14:25:27Z

time="08-04-2020 13:35:39" level=debug msg="Created the mysql namespace" context=metering-validhdfs-mysqldatabase
error: multiple images or templates matched "mysql:5.7"

The argument "mysql:5.7" could apply to the following Docker images, OpenShift image streams, or templates:

* Image stream "mysql" (tag "8.0") in project "openshift"
  Use --image-stream="openshift/mysql:8.0" to specify this image or template

* Image stream "mysql" (tag "latest") in project "openshift"
  Use --image-stream="openshift/mysql:latest" to specify this image or template

timflannagan · 2020-08-04T14:27:02Z

time="08-04-2020 13:35:39" level=debug msg="Created the mysql namespace" context=metering-validhdfs-mysqldatabase
error: multiple images or templates matched "mysql:5.7"

The argument "mysql:5.7" could apply to the following Docker images, OpenShift image streams, or templates:

* Image stream "mysql" (tag "8.0") in project "openshift"
  Use --image-stream="openshift/mysql:8.0" to specify this image or template

* Image stream "mysql" (tag "latest") in project "openshift"
  Use --image-stream="openshift/mysql:latest" to specify this image or template

#1353

pweil- · 2020-08-04T16:40:48Z

/retest

pweil- · 2020-08-05T13:59:16Z

/retest

pweil- · 2020-08-06T13:23:36Z

/retest

pweil- · 2020-08-07T11:29:46Z

Removing WIP from this - it's probably big enough that we shouldn't add anything else too it yet if we want to merge. I am going to call out a few pieces (in review comments) that I think we need to be aware of though before we merge.

pweil- · 2020-08-07T11:33:44Z

/retest

pweil- · 2020-08-07T11:30:49Z

pkg/operator/validation.go

+// TODO this requires a port to be specified, is that one of our requirements?
+func isValidHostPort(hp string, name string) error {
+	if len(hp) > 0 {
+		if _, _, err := net.SplitHostPort(hp); err != nil {


this will fail if a port is not given - can someone confirm that that is a correct behavior?

This gets called on api, metrics, pprof, presto, hive

I think it's valid to make the admin config the port for our API.
Metrics = Prom, has a default, might be ok to not spec.
pprof seems like it has a default port as well, so maybe okay to not spec.
Presto has a default port, might be ok to not spec.
Hive has a default thrift service port as well, so maybe that might not need being requeired to specify either.

Ok, I will remove the calls/tests for apis that have default ports.

pweil- · 2020-08-07T11:31:55Z

pkg/operator/validation.go

+	// PrometheusDataSourceMaxBackfillImportDuration overrides PrometheusDataSourceGlobalImportFromTime
+	// don't set both.
+	if cfg.PrometheusDataSourceGlobalImportFromTime != nil && cfg.PrometheusDataSourceMaxBackfillImportDuration > 0 {
+		errs = append(errs, fmt.Errorf("prometheusDataSourceGlobalImportFromTime and prometheusDataSourceMaxBackfillImportDuration cannot both be set"))


previously we stated that one overrides the other. Introducing this will cause explicit failure. Acceptable?

I'm not sure where I land on this validation check. I think it sounds reasonable looking at the codebase and making sure we reject configurations that won't be respected. @bentito any opinion on this validation check.

@bentito - thoughts on this one?

Reading the usage in start.go, it seems like this validates the stated usage. If it's Prom import question, they're both just ways of getting at how far back to try to grab Prom data when metering somehow gets out of sync. I think PrometheusDataSourceGlobalImportFromTime has more potential to be pretty awful for cluster workload, but I don't think that's being asked here.

just validation of input. Sounds like I can leave it. Thanks

pweil- · 2020-08-07T11:33:17Z

pkg/operator/validation.go

+
+	errs = append(errs, isValidHostPort(cfg.HiveHost, "hiveHost"))
+
+	if !cfg.HiveUseTLS {


previously we allowed config to be set even if this was set to false. Should we continue?

Same as my comment below. I think this is fine UX, reject any configurations like this if you're disabling TLS on the reporting-operator side for that component.

pweil- · 2020-08-07T11:33:23Z

pkg/operator/validation.go

+
+	errs = append(errs, isValidHostPort(cfg.PrestoHost, "prestoHost"))
+
+	if !cfg.PrestoUseTLS {


previously we allowed config to be set even if this was set to false. Should we continue?

I would imagine that makes sense, i.e. rejecting a configuration where you explicitly set TLS-related flags yet disable TLS entirely for the reporting-operator -> presto communications.

pweil- · 2020-08-10T11:44:49Z

/retest

pweil- · 2020-08-11T12:33:45Z

@bentito @timflannagan1 updates from the review in the last commit. Thanks.

pweil- · 2020-08-11T13:47:21Z

/retest

pweil- · 2020-08-11T16:38:45Z

/retest

pweil- · 2020-08-11T17:52:31Z

/retest

pweil- · 2020-08-11T18:55:17Z

/retest

pweil- · 2020-08-11T19:58:18Z

/retest

timflannagan · 2020-08-11T20:17:36Z

/retest

timflannagan · 2020-08-12T13:50:13Z

@pweil- @bentito where are we at with this? There's nothing in here that I think we should hold off until 4.7 master opens up so merging this sooner than later is probably best.

pweil- · 2020-08-12T14:03:34Z

@timflannagan1 all review comments from @bentito have been addressed (left the prometheusDataSourceGlobalImportFromTime/prometheusDataSourceMaxBackfillImportDuration and removed the non-api host/port validations and tests).

Unless there is anything else I think this can merge.

timflannagan · 2020-08-12T14:06:08Z

/lgtm
/approve

openshift-ci-robot · 2020-08-12T14:06:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pweil-, timflannagan1

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:

~~OWNERS~~ [timflannagan1]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

pweil- · 2020-08-13T11:54:05Z

/retest

timflannagan · 2020-08-13T15:09:13Z

🎉

Paul Weil added 3 commits August 3, 2020 14:13

move types to types.go and add initial comments for exported fields

15c6fce

create reporting operator interface and move the internal type to une…

51d5032

…xported

move validation to it's own file, add validation tests for configurat…

3bfd918

…ion, remove validation sections from operator runtime methods where possible.

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 3, 2020

openshift-ci-robot requested review from bentito and timflannagan August 3, 2020 21:12

openshift-ci-robot assigned timflannagan Aug 3, 2020

timflannagan reviewed Aug 3, 2020

View reviewed changes

pkg/operator/operator.go Show resolved Hide resolved

timflannagan reviewed Aug 3, 2020

View reviewed changes

rename cb* imports to remove chargeback references

7e4015b

timflannagan reviewed Aug 4, 2020

View reviewed changes

pkg/operator/operator.go Show resolved Hide resolved

bentito reviewed Aug 4, 2020

View reviewed changes

pkg/operator/operator.go Show resolved Hide resolved

bentito reviewed Aug 4, 2020

View reviewed changes

pkg/operator/operator.go Outdated Show resolved Hide resolved

pweil- mentioned this pull request Aug 4, 2020

reporting operator refactoring #1357

Open

16 tasks

pweil- changed the title ~~WIP: Operator refactor~~ Operator refactor - validation and import cleanup Aug 7, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 7, 2020

pweil- commented Aug 7, 2020

View reviewed changes

updates from validation review

74d6ce9

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 12, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 12, 2020

openshift-merge-robot merged commit 9b659da into kube-reporting:master Aug 13, 2020

pweil- deleted the operator-refactor branch August 13, 2020 15:11


		errs = append(errs, isValidHostPort(cfg.HiveHost, "hiveHost"))

		if !cfg.HiveUseTLS {


		errs = append(errs, isValidHostPort(cfg.PrestoHost, "prestoHost"))

		if !cfg.PrestoUseTLS {

Operator refactor - validation and import cleanup #1356

Operator refactor - validation and import cleanup #1356

Conversation

pweil- commented Aug 3, 2020 • edited Loading

pweil- commented Aug 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timflannagan left a comment

Choose a reason for hiding this comment

timflannagan commented Aug 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pweil- commented Aug 4, 2020

pweil- commented Aug 4, 2020

pweil- commented Aug 4, 2020

timflannagan commented Aug 4, 2020

pweil- commented Aug 4, 2020

pweil- commented Aug 5, 2020

pweil- commented Aug 6, 2020

pweil- commented Aug 7, 2020

pweil- commented Aug 7, 2020

Choose a reason for hiding this comment

bentito Aug 10, 2020 • edited Loading

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

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

pweil- commented Aug 10, 2020

pweil- commented Aug 11, 2020

pweil- commented Aug 11, 2020

pweil- commented Aug 11, 2020

pweil- commented Aug 11, 2020

pweil- commented Aug 11, 2020

pweil- commented Aug 11, 2020

timflannagan commented Aug 11, 2020

timflannagan commented Aug 12, 2020

pweil- commented Aug 12, 2020

timflannagan commented Aug 12, 2020

openshift-ci-robot commented Aug 12, 2020

pweil- commented Aug 13, 2020

timflannagan commented Aug 13, 2020

pweil- commented Aug 3, 2020 •

edited

Loading

bentito Aug 10, 2020 •

edited

Loading