pkg/operator/controller cleanup #178

sgreene570 · 2020-06-11T16:12:33Z

This PR adds checks to ensure that the DNS service and daemonset are not nil in pkg/operator/controller/controller.go before being dereferenced (this is unlikely to happen but still technically possible if a successive get call to the API were to fail after a successful create/update call on a resource).

This PR also improves the updateDNS<thing> flow for the applicable DNS resources by removing an unnecessary API get call in the event that the resource already exists and no updates were performed. In doing this, I broke out the update logic in pkg/operator/controller/controller_dns_configmap.go to it's own function for consistency.

This is essentially a continuation of #176.

Removes extra API get calls in pkg/operator/controller when a DNS resource already exists and no updates are peformed.

Check that the DNS service and daemonset are actually available and throw an error if they are not, to prevent the controller from dereferencing (unlikely) nil values.

Miciah · 2020-06-11T16:47:10Z

pkg/operator/controller/controller_cluster_role.go

 		}
 	}
-	return r.currentDNSClusterRole()
+	return true, current, nil


What do you think about changing this to return haveCR, current, nil, and similar for the other ensureFoo methods? I think it would be semantically more correct (intension instead of extension) and resilient to refactoring. The following example from a recent change you made comes to mind: https://github.com/openshift/cluster-ingress-operator/blob/9edc269df6f9c346509e0e8dc6b647262fdf5a4f/pkg/operator/controller/ingress/load_balancer_service.go#L85-L87
Using the variable (the intension) instead of a literal value (the extension of the variable) should work in all cases, and then ensureLoadBalancerService doesn't need to be an exceptional case (or to put it another way, sticking to the pattern wouldn't introduce a defect in cases like ensureLoadBalancerService if the pattern were return haveFoo, current, nil). Disclaimer: I may be overthinking this.

While your proposed change is definitely more semantically correct, I think the current approach is fine. ensureLoadBalancerService will always have some minor defect from the ensureFoo pattern because it is fundamentally different than the other ensureFoo functions (ensureLoadBalancerService is the only ensureFoo function whose Foo may not be needed at all).

Also, whenever ensureFoo returns false for it's first return value, the second return value is always nil, not current, even though current will evaluate to nil.

To me, returning true, current, nil is slightly easier to read and understand. true implies that current is not nil. haveFoo on the other hand does not. Do you think it would be reasonable to continue using the extension case here, or do you feel very strongly about switching to the intension case?

ensureLoadBalancerService is the only ensureFoo function whose Foo may not be needed at all

The following ensureFoo methods, all in cluster-ingress-operator, may determine that no Foo is needed:

ensureLoadBalancerService

ensureWildcardDNSRecord

ensureNodePortService

ensureRsyslogConfigMap

ensureRouterPodDisruptionBudget

So this is a frequent case.

Also, whenever ensureFoo returns false for it's first return value, the second return value is always nil, not current, even though current will evaluate to nil.

True, if the Boolean return value is false, then the pointer return value must be nil (so if you return false, that implies that returning an explicit nil value is semantically correct, and perhaps it is more readable to do so), and if the Boolean is true, then the pointer must be non-nil (so if you return true, that implies that you must return current). However, that does not imply that returning current implies that it is non-nil (that is, returning current does not imply that you must return true); if the Boolean value is unknown, then the pointer value is unknown, but they are still entangled (they're either true and non-nil or false and nil, respectively). I would object to logic that assigned inconsistent values to haveFoo and current, even within the internal logic of the method.

To me, returning true, current, nil is slightly easier to read and understand. true implies that current is not nil. haveFoo on the other hand does not. Do you think it would be reasonable to continue using the extension case here, or do you feel very strongly about switching to the intension case?

I disagree that true implies non-nil, but readability is subjective, and if my argument has not persuaded you, then I'll defer to your judgment.

I definitely see where you are coming from, and I think there are valid points on both sides of the coin. I am going to suggest we continue following the current pattern of returning true, current nil for the sake readability.

Fair enough. Thanks!

A minor correction to my earlier comment:

I disagree that true implies non-nil

I wrote that in error. I definitely agree that true implies non-nil.

The following ensureFoo methods, all in cluster-ingress-operator, may determine that no Foo is needed:
ensureLoadBalancerService
ensureWildcardDNSRecord
ensureNodePortService
ensureRsyslogConfigMap
ensureRouterPodDisruptionBudget

Looks like ensureLoadBalancerService and ensureWildcardDNSRecord do not make use of the wantFoo variable used in the other ensureFoo methods in that list. At first glance, adding a wantFoo variable to ensureLoadBalancerService would remove the need to return an unknown haveLB. If we draft another PR to add the wantFoo variable to the methods that are missing it, we could unify on returning true, current, nil across both operators. Thoughts?

Looks like ensureLoadBalancerService and ensureWildcardDNSRecord do not make use of the wantFoo variable used in the other ensureFoo methods in that list.

Oh, snap! This change was wrong—sorry I missed the problem during review: openshift/cluster-ingress-operator@408e341#diff-812ac76222452adcda3e374d185812bbL44-R50

ensureWildcardDNSRecord needs to check whether desiredWildcardRecord actually returns a record. Ideally, desiredWildcardRecord should have a Boolean return value added, and we should use that where we previously had desired != nil. (Sidenote: desiredWildcardRecord's godoc needs to be corrected; it has ensureWildcardDNSRecord where it should have desiredWildcardRecord.)

We should also add a Boolean return value to desiredLoadBalancerService and use it in ensureLoadBalancerService, yeah.

In general, I favor having every desiredFoo return a Boolean "wants" value, analogously to how ensureFoo returns a Boolean "has" value, but if you want to add it only to desiredLoadBalancerService and desiredWildcardRecord, which definitely should have it, that would be fine.

Thanks for catching that!

My mistake for accidentally removing those important conditions! I can correct cluster-ingress-operator#408 by adding the mentioned Boolean return values next week. 👍 Thanks!

Refactoring some of these things has been more cumbersome than I anticipated. Thanks for the assistance @Miciah ! 😁

Miciah · 2020-06-11T18:28:10Z

/lgtm

openshift-ci-robot · 2020-06-11T18:28:26Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

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

openshift-bot · 2020-06-13T03:55:40Z

/retest