Bug 1808118: Add OperatorSpec object to image registry config #640

ricardomaraschini · 2020-05-05T06:57:59Z

Creates a property on image registry configuration to hold operator
specific customization. With these extra options we can keep track of
different log levels for the operator and operand.

openshift-ci-robot · 2020-05-05T06:58:38Z

@ricardomaraschini: This pull request references Bugzilla bug 1808118, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.5.0) matches configured target release for branch (4.5.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Details

In response to this:

Bug 1808118: Add OperatorSpec object to image registry config

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

ricardomaraschini · 2020-05-05T07:01:23Z

/assign @dmage @adambkaplan @deads2k

imageregistry/v1/types.go

dmage · 2020-05-07T09:04:58Z

/lgtm

adambkaplan · 2020-05-07T10:20:14Z

imageregistry/v1/types.go

 	// logging determines the level of logging enabled in the registry.
-	LogLevel int64 `json:"logging" protobuf:"varint,11,opt,name=logging"`
+	// +optional
+	Logging int64 `json:"logging,omitempty" protobuf:"varint,11,opt,name=logging"`


@ricardomaraschini @dmage is this now a deprecated field, or will it be used to independently set the logging for the registry?

For backwards compatibility the idea is to take this field into account if logLevel is not set. If both are set, use the "bigger" one.

@ricardomaraschini this approach IMO is confusing to admins - we need one field to be the source of truth. I would rather have the following:

If logLevel > 0, use that and ignore logging.

If logLevel == 0 and logging > 0, write the logging value to logLevel in the spec and proceed.

Because our APIs are forever, we will never remove the logging field. Therefore the operator should do the migration on the admin's behalf.

We can say that logging is deprecated and logLevel is a preferred field.

Based on the discussion on Slack:

. logLevel is not set logLevel is set

logging is not set Normal (info) log level logLevel is honored

logging is set logging is honored if logging is error or warning, logging is honored (as these values cannot be represented by logLevel), otherwise logLevel is honored

Added a note informing that logging is deprecated and asking to use logLevel instead.

I agree with @adambkaplan and we should automatically migrate values from logging into logLevel when a conversion 1:1 is possible. The problem stated by @dmage is valid, we don't know if or how many customers are using error or warning, hopefully we will in the future(openshift/insights-operator#103).

@deads2k @adambkaplan @dmage

There is not much I can do on this side as the migration path will happen on a PR against the Operator. Here we added new a field and tagged the old one as deprecated, nitty-gritty details on migrations should be discussed when a PR comes alive on controller's side.

Could we get this approved/merged?

@deads2k @adambkaplan

The logging field is not an optional today, so it's always set. It can be set to

0 (log only errors)

1 (warning)

2, 3 (info)

anything else (debug)

The normal log level is 2.

I wouldn't say that 0 is a proper value, because one may forget to set logging in their configuration object and expect the operator to use the default log level.

What are cons of the approach "when logLevel is set, use it, otherwise fallback to logging"?

ricardomaraschini · 2020-05-25T11:25:37Z

/retest

ricardomaraschini · 2020-05-25T13:43:13Z

@adambkaplan @dmage
Work to migrate Logging to LogLevel will be done through the following PR openshift/cluster-image-registry-operator#559

dmage · 2020-05-25T20:14:48Z

imageregistry/v1/types.go

-	LogLevel int64 `json:"logging" protobuf:"varint,11,opt,name=logging"`
+	// logging is deprecated, use logLevel instead.
+	// +optional
+	Logging *int64 `json:"logging,omitempty" protobuf:"varint,11,opt,name=logging"`


AFAIK we cannot change int64 -> *int64, but I'd like to get an input from @adambkaplan

This relates to #640 (comment)

This is interesting. In normal circumstances this would be a breaking change, but in our case it is not.

In the golang code we are introducing the new field Logging, so technically this is not a breaking (golang) API change.

For the JSON API, we are converting a field from required -> optional, which I believe is a non-breaking change. However, not setting anything implies a default of 0, so we need to respect that in our migration code:

logLevel = 0, logging = nil -> logs should use -v 0

logLevel = 0, logging = <non-nil> -> logs should use -v <non-nil>.

logLevel = <non-zero> -> logs should use -v <non-zero>

You can make this omitempty, but you can't change the type. @adambkaplan had similar discussion when working on #563 with @deads2k

I brought back the int64. Migration path will look like openshift/cluster-image-registry-operator@f2d81ee#diff-e3147933b179a3ade1d900049f1fa55bL26-R39

adambkaplan · 2020-05-26T18:12:23Z

/bugzilla refresh

openshift-ci-robot · 2020-05-26T18:12:25Z

@adambkaplan: This pull request references Bugzilla bug 1808118, which is invalid:

expected the bug to target the "4.5.0" release, but it targets "4.6.0" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

Details

In response to this:

/bugzilla refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

adambkaplan

/lgtm

Note that we have pushed this bug fix to 4.6. The level of API changes makes this change potentially risky to include in the 4.5 release.

openshift-bot · 2020-05-27T01:01:12Z

/bugzilla refresh

Recalculating validity in case the underlying Bugzilla bug has changed.

openshift-ci-robot · 2020-05-27T01:01:15Z

@openshift-bot: This pull request references Bugzilla bug 1808118, which is invalid:

expected the bug to target the "4.5.0" release, but it targets "4.6.0" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

Details

In response to this:

/bugzilla refresh

Recalculating validity in case the underlying Bugzilla bug has changed.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-bot · 2020-05-28T01:00:56Z

/bugzilla refresh

Recalculating validity in case the underlying Bugzilla bug has changed.

openshift-ci-robot · 2020-05-28T01:00:58Z

@openshift-bot: This pull request references Bugzilla bug 1808118, which is invalid:

expected the bug to target the "4.5.0" release, but it targets "4.6.0" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

Details

In response to this:

/bugzilla refresh

Recalculating validity in case the underlying Bugzilla bug has changed.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

ricardomaraschini · 2020-06-25T12:44:28Z

@soltysh Updated to remove the pointer and bring back the int64

ricardomaraschini · 2020-08-03T19:58:46Z

/hold

still need to finish rebasing

ricardomaraschini · 2020-08-04T07:47:50Z

@soltysh Could I use your pair of eyes on this one?

ricardomaraschini · 2020-08-04T07:48:05Z

/unhold

ricardomaraschini · 2020-08-20T09:58:55Z

@dmage @adambkaplan @soltysh Is there something holding this up? Could we get it in?

dmage · 2020-08-20T11:28:19Z

/lgtm

dmage · 2020-08-20T11:29:09Z

/assign @soltysh

soltysh

/approve

openshift-bot · 2020-08-24T14:10:59Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-08-24T15:28:26Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-08-24T15:41:27Z

/retest

Please review the full test history for this PR and help us cut down flakes.

ricardomaraschini · 2020-08-24T16:02:58Z

/retest

openshift-bot · 2020-08-24T16:07:26Z

/retest

Please review the full test history for this PR and help us cut down flakes.

Creates a property on image registry configuration to hold operator specific customization. With these extra options we can keep track of different log levels for the operator and operand.

soltysh

/lgtm

openshift-ci-robot · 2020-08-24T16:16:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adambkaplan, dmage, ricardomaraschini, soltysh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [soltysh]

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

openshift-ci-robot · 2020-08-24T16:24:12Z

@ricardomaraschini: Some pull requests linked via external trackers have merged:

The following pull requests linked via external trackers have not merged:

openshift/cluster-image-registry-operator#559 is open

These pull request must merge or be unlinked from the Bugzilla bug in order for it to move to the next state.

Bugzilla bug 1808118 has not been moved to the MODIFIED state.

Details

In response to this:

Bug 1808118: Add OperatorSpec object to image registry config

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot requested review from abhinavdahiya and adambkaplan May 5, 2020 06:58

ricardomaraschini changed the title ~~Add OperatorSpec object to image registry config~~ Bug 1808118: Add OperatorSpec object to image registry config May 5, 2020

openshift-ci-robot added bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels May 5, 2020

openshift-ci-robot assigned adambkaplan, deads2k and dmage May 5, 2020

dmage reviewed May 5, 2020

View reviewed changes

imageregistry/v1/types.go Outdated Show resolved Hide resolved

dmage reviewed May 5, 2020

View reviewed changes

imageregistry/v1/types.go Outdated Show resolved Hide resolved

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

adambkaplan reviewed May 7, 2020

View reviewed changes

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

ricardomaraschini mentioned this pull request May 25, 2020

WIP - Bug 1808118: Using OperatorSpec.LogLevel instead of Logging openshift/cluster-image-registry-operator#559

Closed

dmage reviewed May 25, 2020

View reviewed changes

openshift-ci-robot removed the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label May 26, 2020

openshift-ci-robot added the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label May 26, 2020

adambkaplan approved these changes May 26, 2020

View reviewed changes

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

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 25, 2020

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 3, 2020

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 4, 2020

ricardomaraschini requested a review from soltysh August 6, 2020 11:16

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

openshift-ci-robot assigned soltysh Aug 20, 2020

soltysh approved these changes Aug 24, 2020

View reviewed changes

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

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

Add OperatorSpec object to image registry config

c05ac28

Creates a property on image registry configuration to hold operator specific customization. With these extra options we can keep track of different log levels for the operator and operand.

soltysh approved these changes Aug 24, 2020

View reviewed changes

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

openshift-merge-robot merged commit 8625127 into openshift:master Aug 24, 2020

This was referenced Aug 25, 2020

Bug 1808118: Making logging parameter optional #725

Closed

WIP - Spec.Storage.ManagementState implementation openshift/cluster-image-registry-operator#592

Closed

openshift-ci-robot mentioned this pull request Sep 8, 2020

Bug 1808118: Implemented operator log level controller openshift/cluster-image-registry-operator#602

Merged

.	`logLevel` is not set	`logLevel` is set
`logging` is not set	`Normal` (`info`) log level	logLevel is honored
`logging` is set	logging is honored	if logging is `error` or `warning`, logging is honored (as these values cannot be represented by logLevel), otherwise logLevel is honored

Bug 1808118: Add OperatorSpec object to image registry config #640

Bug 1808118: Add OperatorSpec object to image registry config #640

Uh oh!

Conversation

ricardomaraschini commented May 5, 2020

Uh oh!

openshift-ci-robot commented May 5, 2020

Uh oh!

ricardomaraschini commented May 5, 2020

Uh oh!

Uh oh!

Uh oh!

dmage commented May 7, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ricardomaraschini commented May 25, 2020

Uh oh!

ricardomaraschini commented May 25, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ricardomaraschini May 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adambkaplan commented May 26, 2020

Uh oh!

openshift-ci-robot commented May 26, 2020

Uh oh!

adambkaplan left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-bot commented May 27, 2020

Uh oh!

openshift-ci-robot commented May 27, 2020

Uh oh!

openshift-bot commented May 28, 2020

Uh oh!

openshift-ci-robot commented May 28, 2020

Uh oh!

ricardomaraschini commented Jun 25, 2020

Uh oh!

ricardomaraschini commented Aug 3, 2020

Uh oh!

ricardomaraschini commented Aug 4, 2020

Uh oh!

ricardomaraschini commented Aug 4, 2020

Uh oh!

ricardomaraschini commented Aug 20, 2020

Uh oh!

dmage commented Aug 20, 2020

Uh oh!

dmage commented Aug 20, 2020

Uh oh!

soltysh left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-bot commented Aug 24, 2020

Uh oh!

openshift-bot commented Aug 24, 2020

Uh oh!

ricardomaraschini May 26, 2020 •

edited

Loading