Refactor service-ca and payload config controller (part 2) #293

mfojtik · 2020-06-29T08:22:47Z

Follow-up for #292

This continue to split the main operator loop. In this iteration the "block 2" is pushed into its own controller.

mfojtik · 2020-06-29T13:00:12Z

/retest

stlaz

first, faster pass

pkg/controllers/serviceca/service_ca_controller.go

mfojtik · 2020-06-30T09:57:45Z

PullBuilderImageFailed: Failed pulling builder image.

/retest

deads2k · 2020-06-30T12:25:28Z

pkg/controllers/cliconfig/cli_config_controller.go

+// knownConditionNames lists all condition types used by this controller.
+// These conditions are operated and defaulted by this controller.
+// Any new condition used by this controller sync() loop should be listed here.
+var knownConditionNames = sets.NewString(


you're going to all the trouble of splitting bits out and still have three conditions? That seems weird.

now there is more than three :-)

pkg/controllers/cliconfig/cli_config_controller.go

deads2k · 2020-06-30T12:44:08Z

pkg/controllers/cliconfig/cli_config_controller.go

+	if err != nil {
+		return nil, []operatorv1.OperatorCondition{
+			{
+				Type:    "CLIConfigDegraded",


This isn't related to the CLI. You want this degraded message to point to where the customer should look for a fix or what the impact of the failure is. In this case, OAuthServerIngressDegraded?

@deads2k the result of this controller sync loop is "v4-0-config-system-cliconfig

i mean i'm open for suggestions of the name for this controller :-)

openshift-ci-robot · 2020-07-07T08:54:51Z

@mfojtik: mfojtik unauthorized: /override is restricted to Repo administrators, approvers in top level OWNERS file.

In response to this:

/override ci/prow/e2e-aws
/override ci/prow/e2e-aws-console-login
/override ci/prow/e2e-aws-operator
/override ci/prow/e2e-aws-upgrade

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.

mfojtik · 2020-07-07T13:31:31Z

/retest

mfojtik · 2020-07-07T14:44:43Z

/retest

mfojtik · 2020-07-07T15:50:44Z

/retest

stlaz

bunch of comments, I am not sure I like different controllers setting similarly-named conditions when they're unable to deal with the same resources

pkg/controllers/serviceca/service_ca_controller.go

stlaz · 2020-07-08T10:15:28Z

pkg/controllers/serviceca/service_ca_controller.go

+	foundConditions := []operatorv1.OperatorCondition{}
+
+	// make sure we create the service before we start asking about service certs
+	if _, err := c.serviceLister.Services("openshift-authentication").Get("oauth-openshift"); err != nil {


Did you mean to go degraded when the service gets removed?

i would go Degraded in any error case here, service removed is one of them? do you think we need to handle 404 differently?

right, I originally thought this controller attempted to sync the service itself as well, but that's done elsewhere.

pkg/operator2/oauth.go

pkg/controllers/serviceca/service_ca_controller.go

pkg/controllers/cliconfig/cli_config_controller.go

stlaz · 2020-07-08T11:26:28Z

pkg/controllers/cliconfig/cli_config_controller.go

+	cliConfigBytes, err := runtime.Encode(encoder, cliConfig)
+	if err != nil {
+		return []operatorv1.OperatorCondition{
+			{
+				Type:    "OAuthConfigDegraded",
+				Status:  operatorv1.ConditionTrue,
+				Reason:  "EncodeFailed",
+				Message: fmt.Sprintf("Failed to encode CLI config: %v", err),
+			},
+		}
+	}
+
+	observedConfig, err := grabPrefixedConfig(operatorConfig.Spec.ObservedConfig.Raw, configobservation.OAuthServerConfigPrefix)
+	if err != nil {
+		return []operatorv1.OperatorCondition{
+			{
+				Type:    "OAuthConfigDegraded",
+				Status:  operatorv1.ConditionTrue,
+				Reason:  "GetOAuthServerConfigFailed",
+				Message: fmt.Sprintf("Unable to get oauth-server configuration: %v", err),
+			},
+		}
+	}
+
+	completeConfigBytes, err := resourcemerge.MergePrunedProcessConfig(&osinv1.OsinServerConfig{}, nil, cliConfigBytes, observedConfig, operatorConfig.Spec.UnsupportedConfigOverrides.Raw)
+	if err != nil {
+		return []operatorv1.OperatorCondition{
+			{
+				Type:    "OAuthConfigDegraded",
+				Status:  operatorv1.ConditionTrue,
+				Reason:  "MergeConfigFailed",
+				Message: fmt.Sprintf("Failed to merge config with unsupportedConfigOverrides: %v", err),
+			},
+		}
+	}
+
+	expectedCLIConfig := getCliConfigMap(completeConfigBytes)
+
+	_, _, err = resourceapply.ApplyConfigMap(c.configMaps, recorder, expectedCLIConfig)
+	if err != nil {
+		return []operatorv1.OperatorCondition{
+			{
+				Type:    "OAuthConfigDegraded",
+				Status:  operatorv1.ConditionTrue,
+				Reason:  "ApplyFailed",
+				Message: fmt.Sprintf("Failed to apply CLI configuration %q: %v", expectedCLIConfig.Name, err),
+			},
+		}
+	}
+
+	return nil


the actual logic gets covered up by construction of the conditions :/ I can try to deal with this in a follow-up

mfojtik · 2020-07-08T12:07:38Z

/retest

stlaz · 2020-07-08T12:15:28Z

/lgtm

openshift-ci-robot · 2020-07-08T12:15:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mfojtik, stlaz

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~~ [stlaz]

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

mfojtik · 2020-07-08T12:59:40Z

/retest

openshift-bot · 2020-07-08T13:54:37Z

/retest

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

mfojtik · 2020-07-08T14:02:34Z

/retest

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 Jun 29, 2020

openshift-ci-robot requested review from stlaz and sttts June 29, 2020 08:23

mfojtik changed the title ~~WIP: Refactor service-ca controller~~ WIP: Refactor service-ca and cli config controller Jun 29, 2020

stlaz reviewed Jun 29, 2020

View reviewed changes

pkg/controllers/serviceca/service_ca_controller.go Outdated Show resolved Hide resolved

pkg/controllers/serviceca/service_ca_controller.go Outdated Show resolved Hide resolved

pkg/controllers/serviceca/service_ca_controller.go Show resolved Hide resolved

mfojtik force-pushed the refactor-serviceca branch 2 times, most recently from 5fa4ef7 to 1d61807 Compare June 30, 2020 07:12

mfojtik changed the title ~~WIP: Refactor service-ca and cli config controller~~ WIP: Refactor service-ca and cli config controller (part 2) Jun 30, 2020

mfojtik force-pushed the refactor-serviceca branch from 1d61807 to 8d481c8 Compare June 30, 2020 09:04

mfojtik mentioned this pull request Jun 30, 2020

Refactor deployment controller (part 3) #295

Merged

deads2k reviewed Jun 30, 2020

View reviewed changes

pkg/controllers/cliconfig/cli_config_controller.go Outdated Show resolved Hide resolved

deads2k reviewed Jun 30, 2020

View reviewed changes

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 4, 2020

mfojtik force-pushed the refactor-serviceca branch from 8d481c8 to ee313b8 Compare July 7, 2020 07:44

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 7, 2020

mfojtik changed the title ~~WIP: Refactor service-ca and cli config controller (part 2)~~ Refactor service-ca and cli config controller (part 2) Jul 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 Jul 7, 2020

mfojtik force-pushed the refactor-serviceca branch from ee313b8 to 9c3838a Compare July 7, 2020 08:03

go.mod: add spew

bbe539b

mfojtik force-pushed the refactor-serviceca branch from 9c3838a to f58379d Compare July 7, 2020 08:20

mfojtik force-pushed the refactor-serviceca branch 2 times, most recently from 886464f to 783e137 Compare July 7, 2020 12:58

stlaz reviewed Jul 8, 2020

View reviewed changes

mfojtik added 2 commits July 8, 2020 14:04

service-ca: refactor service-ca block to separate controller

9f3bee4

payload: refactor cli config block to payload config controller

992c2cd

mfojtik force-pushed the refactor-serviceca branch from 783e137 to 992c2cd Compare July 8, 2020 12:05

mfojtik changed the title ~~Refactor service-ca and cli config controller (part 2)~~ Refactor service-ca and payload config controller (part 2) Jul 8, 2020

openshift-ci-robot assigned stlaz Jul 8, 2020

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 8, 2020

openshift-merge-robot merged commit 721d9f5 into openshift:master Jul 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor service-ca and payload config controller (part 2) #293

Refactor service-ca and payload config controller (part 2) #293

mfojtik commented Jun 29, 2020

mfojtik commented Jun 29, 2020

stlaz left a comment

mfojtik commented Jun 30, 2020

deads2k Jun 30, 2020

mfojtik Jul 7, 2020

deads2k Jun 30, 2020 •

edited

mfojtik Jul 7, 2020

mfojtik Jul 7, 2020

openshift-ci-robot commented Jul 7, 2020

mfojtik commented Jul 7, 2020

mfojtik commented Jul 7, 2020

mfojtik commented Jul 7, 2020

stlaz left a comment

stlaz Jul 8, 2020

mfojtik Jul 8, 2020

stlaz Jul 8, 2020

stlaz Jul 8, 2020

mfojtik commented Jul 8, 2020

stlaz commented Jul 8, 2020

openshift-ci-robot commented Jul 8, 2020

mfojtik commented Jul 8, 2020

openshift-bot commented Jul 8, 2020

mfojtik commented Jul 8, 2020

Refactor service-ca and payload config controller (part 2) #293

Refactor service-ca and payload config controller (part 2) #293

Conversation

mfojtik commented Jun 29, 2020

mfojtik commented Jun 29, 2020

stlaz left a comment

Choose a reason for hiding this comment

mfojtik commented Jun 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k Jun 30, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci-robot commented Jul 7, 2020

mfojtik commented Jul 7, 2020

mfojtik commented Jul 7, 2020

mfojtik commented Jul 7, 2020

stlaz left a 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

mfojtik commented Jul 8, 2020

stlaz commented Jul 8, 2020

openshift-ci-robot commented Jul 8, 2020

mfojtik commented Jul 8, 2020

openshift-bot commented Jul 8, 2020

mfojtik commented Jul 8, 2020

deads2k Jun 30, 2020 •

edited