Allow dns operator to be disabled with managementState field #260

rfredette · 2021-04-08T20:01:57Z

this change allows resources normally managed by the dns operator to be left unmanaged when spec.managementState is set to Unmanaged. This will make debugging issues with dns resources easier

Also dependent on openshift/api PR #888 for API change

openshift-ci-robot · 2021-04-08T20:02:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rfredette

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

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

Miciah · 2021-04-08T20:26:50Z

This is an excellent start! Two things:

You are adding logic to report the Upgradeable status condition on the dns object (dnses.operator.openshift.io/default), and we need the clusteroperator object (clusteroperators.config.openshift.io/dns) to report the Upgradeable status condition. The clusteroperator's status is computed by the status controller under pkg/operator/controller/status/controller.go. It can use status condition that you added to the dns object to compute the status condition on the clusteroperator object (basically just copy it from the dns to the clusteroperator).
We need to make sure that we handle an empty value for managementState. Please add a unit test TestComputeDNSProgressingCondition in pkg/operator/controller/dns_status_test.go to verify that your new computeDNSProgressingCondition function properly handles this case.

Miciah · 2021-04-08T20:28:13Z

Sorry, I meant computeDNSUpgradeableCondition. Replace "Progressing" with "Upgradeable" in my previous comment.

openshift-ci-robot · 2021-04-11T09:13:57Z

@rfredette: PR needs rebase.

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 · 2021-07-10T14:14:44Z

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

rfredette · 2021-07-19T15:38:08Z

/remove-lifecycle stale

rfredette · 2021-07-19T16:35:49Z

/retest

rfredette · 2021-07-19T17:56:54Z

Updating github.com/openshift/build-machinery-go to v0.0.0-20210712174854-1bb7fd1518d3 is not a manual update; it is required in the github.com/openshift/api version update.

Miciah

Overall looks good, just a few suggestions.

Miciah · 2021-07-19T21:31:31Z

pkg/operator/controller/status/controller.go

+	var upgradeable bool
+	for _, cond := range dns.Status.Conditions {
+		if cond.Type == operatorv1.OperatorStatusTypeUpgradeable && cond.Status == operatorv1.ConditionTrue {
+			upgradeable = true


Probably better to set Upgradeable=True if the DNS doesn't have an Upgradeable status condition:

Suggested change

var upgradeable bool

for _, cond := range dns.Status.Conditions {

if cond.Type == operatorv1.OperatorStatusTypeUpgradeable && cond.Status == operatorv1.ConditionTrue {

upgradeable = true

upgradeable := true

for _, cond := range dns.Status.Conditions {

if cond.Type == operatorv1.OperatorStatusTypeUpgradeable {

upgradeable = cond.Status == operatorv1.ConditionTrue

Miciah · 2021-07-19T21:33:40Z

pkg/operator/controller/status/controller.go

+			Type:    configv1.OperatorUpgradeable,
+			Status:  configv1.ConditionFalse,
+			Reason:  "DNSNotUpgradeable",
+			Message: fmt.Sprintf("DNS %s is not upgradeable", dns.Name),


It might be useful to include cond.Message from the DNS's status condition in the clusteroperator's status condition.

Miciah · 2021-07-19T21:36:44Z

pkg/operator/controller/dns_status_test.go

@@ -19,46 +19,72 @@ func TestDNSStatusConditions(t *testing.T) {
 		availDNS, desireDNS int32
 		haveNR              bool
 		availNR, desireNR   int32
+		isManaged           bool


Besides Managed or Unmanaged, ManagementState also could be Force, Removed, or unset.

Miciah · 2021-07-19T21:37:26Z

pkg/operator/controller/dns_status_test.go

+		{testIn{true, true, 1, 3, true, 3, 3, false}, testOut{true, true, true, false}},
+		{testIn{true, true, 3, 3, true, 0, 3, false}, testOut{false, true, true, false}},
+		{testIn{true, true, 2, 3, true, 3, 3, false}, testOut{false, true, true, false}},
+		{testIn{true, true, 0, 1, true, 0, 1, false}, testOut{true, true, false, false}},


We don't necessarily need to test every combination; a few test cases where ManagementState is not Managed should be fine.

Miciah · 2021-07-19T21:39:09Z

pkg/operator/controller/controller.go

-			}
-			if err := r.ensureDNSDeleted(dns); err != nil {
-				errs = append(errs, fmt.Errorf("failed to ensure deletion for dns %s: %v", dns.Name, err))
+		if dns.Spec.ManagementState != operatorv1.Unmanaged {


What do you think about switching the if and else so the simpler case (dns.Spec.ManagementState == operatorv1.Unmanaged) comes first? It might also make sense to use a switch statement (even if we only have two cases for now, case operatorv1.Unmanaged: and default:).

Miciah · 2021-07-19T21:42:14Z

BTW, I usually squash commits and have just one commit with the vendor bump and make generate and a second commit with the implementation.

rfredette · 2021-07-20T17:38:42Z

Thanks @Miciah. I agree with all your suggestions; the latest changes should have them all implemented.

rfredette · 2021-07-20T18:59:59Z

/retest

rfredette · 2021-07-22T16:34:19Z

/retest

rfredette · 2021-07-22T19:42:42Z

/retest-required

rfredette · 2021-07-22T20:44:29Z

/retest

Miciah · 2021-07-23T00:35:12Z

pkg/operator/controller/dns_status_test.go

+		var managementState string
+		switch tc.inputs.managementState {
+		case operatorv1.Managed:
+			managementState = "Managed"
+		case operatorv1.Force:
+			managementState = "Force"
+		case operatorv1.Removed:
+			managementState = "Removed"
+		case operatorv1.Unmanaged:
+			managementState = "Unmanaged"
+		}
+		description := fmt.Sprintf("%s, %d/%d DNS pods available, %d/%d node-resolver pods available, managementState is %s", haveClusterIP, tc.inputs.availDNS, tc.inputs.desireDNS, tc.inputs.availNR, tc.inputs.desireNR, managementState)


Can you use tc.inputs.managementState directly in the print statement?

Suggested change

var managementState string

switch tc.inputs.managementState {

case operatorv1.Managed:

managementState = "Managed"

case operatorv1.Force:

managementState = "Force"

case operatorv1.Removed:

managementState = "Removed"

case operatorv1.Unmanaged:

managementState = "Unmanaged"

}

description := fmt.Sprintf("%s, %d/%d DNS pods available, %d/%d node-resolver pods available, managementState is %s", haveClusterIP, tc.inputs.availDNS, tc.inputs.desireDNS, tc.inputs.availNR, tc.inputs.desireNR, managementState)

description := fmt.Sprintf("%s, %d/%d DNS pods available, %d/%d node-resolver pods available, managementState is %s", haveClusterIP, tc.inputs.availDNS, tc.inputs.desireDNS, tc.inputs.availNR, tc.inputs.desireNR, string(tc.inputs.managementState))

We can merge as-is leave that for a follow-up though.

Miciah · 2021-07-23T00:39:56Z

Thanks!
/lgtm

openshift-ci · 2021-07-23T00:41:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Miciah, rfredette

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~~ [Miciah,rfredette]

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 requested review from candita and knobunc April 8, 2021 20:02

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 8, 2021

rfredette force-pushed the managementstate branch from 2909f8a to baf9663 Compare April 8, 2021 21:30

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

openshift-ci bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 10, 2021

rfredette force-pushed the managementstate branch from baf9663 to 5387044 Compare July 19, 2021 15:32

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

openshift-ci bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 19, 2021

rfredette force-pushed the managementstate branch from 5387044 to 01b23d7 Compare July 19, 2021 17:53

Miciah reviewed Jul 19, 2021

View reviewed changes

rfredette added 2 commits July 20, 2021 13:15

Bump openshift/api

a09f1f6

Implement managementState

7fe06da

rfredette force-pushed the managementstate branch from 01b23d7 to 7fe06da Compare July 20, 2021 17:34

Miciah reviewed Jul 23, 2021

View reviewed changes

openshift-ci bot assigned Miciah Jul 23, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 23, 2021

openshift-merge-robot merged commit a6bf664 into openshift:master Jul 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow dns operator to be disabled with managementState field #260

Allow dns operator to be disabled with managementState field #260

rfredette commented Apr 8, 2021 •

edited

openshift-ci-robot commented Apr 8, 2021

Miciah commented Apr 8, 2021

Miciah commented Apr 8, 2021

openshift-ci-robot commented Apr 11, 2021

openshift-bot commented Jul 10, 2021

rfredette commented Jul 19, 2021

rfredette commented Jul 19, 2021

rfredette commented Jul 19, 2021

Miciah left a comment

Miciah Jul 19, 2021

Miciah Jul 19, 2021

Miciah Jul 19, 2021

Miciah Jul 19, 2021

Miciah Jul 19, 2021

Miciah commented Jul 19, 2021

rfredette commented Jul 20, 2021

rfredette commented Jul 20, 2021

rfredette commented Jul 22, 2021

rfredette commented Jul 22, 2021

rfredette commented Jul 22, 2021

Miciah Jul 23, 2021

Miciah commented Jul 23, 2021

openshift-ci bot commented Jul 23, 2021

Allow dns operator to be disabled with managementState field #260

Allow dns operator to be disabled with managementState field #260

Conversation

rfredette commented Apr 8, 2021 • edited

openshift-ci-robot commented Apr 8, 2021

Miciah commented Apr 8, 2021

Miciah commented Apr 8, 2021

openshift-ci-robot commented Apr 11, 2021

openshift-bot commented Jul 10, 2021

rfredette commented Jul 19, 2021

rfredette commented Jul 19, 2021

rfredette commented Jul 19, 2021

Miciah left a comment

Choose a reason for hiding this comment

Miciah Jul 19, 2021

Choose a reason for hiding this comment

Miciah Jul 19, 2021

Choose a reason for hiding this comment

Miciah Jul 19, 2021

Choose a reason for hiding this comment

Miciah Jul 19, 2021

Choose a reason for hiding this comment

Miciah Jul 19, 2021

Choose a reason for hiding this comment

Miciah commented Jul 19, 2021

rfredette commented Jul 20, 2021

rfredette commented Jul 20, 2021

rfredette commented Jul 22, 2021

rfredette commented Jul 22, 2021

rfredette commented Jul 22, 2021

Miciah Jul 23, 2021

Choose a reason for hiding this comment

Miciah commented Jul 23, 2021

openshift-ci bot commented Jul 23, 2021

rfredette commented Apr 8, 2021 •

edited