Skip to content

pkg/operator/controller cleanup #178

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-robot merged 2 commits into from
Jun 13, 2020
Merged

pkg/operator/controller cleanup #178

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d43f43f
controller: Remove extra API get calls for DNS
sgreene570 Jun 10, 2020
8859d7f
controller: Prevent nil dereferences in controller.go
sgreene570 Jun 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions pkg/operator/controller/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,8 +346,10 @@ func (r *reconciler) ensureDNS(dns *operatorv1.DNS) error {
}

errs := []error{}
if _, daemonset, err := r.ensureDNSDaemonSet(dns, clusterIP, clusterDomain); err != nil {
if haveDS, daemonset, err := r.ensureDNSDaemonSet(dns, clusterIP, clusterDomain); err != nil {
errs = append(errs, fmt.Errorf("failed to ensure daemonset for dns %s: %v", dns.Name, err))
} else if !haveDS {
errs = append(errs, fmt.Errorf("failed to get daemonset for dns %s", dns.Name))
} else {
trueVar := true
daemonsetRef := metav1.OwnerReference{
Expand All @@ -361,8 +363,10 @@ func (r *reconciler) ensureDNS(dns *operatorv1.DNS) error {
if _, _, err := r.ensureDNSConfigMap(dns, clusterDomain); err != nil {
errs = append(errs, fmt.Errorf("failed to create configmap for dns %s: %v", dns.Name, err))
}
if _, svc, err := r.ensureDNSService(dns, clusterIP, daemonsetRef); err != nil {
if haveSvc, svc, err := r.ensureDNSService(dns, clusterIP, daemonsetRef); err != nil {
errs = append(errs, fmt.Errorf("failed to create service for dns %s: %v", dns.Name, err))
} else if !haveSvc {
errs = append(errs, fmt.Errorf("failed to get service for dns %s", dns.Name))
} else if err := r.ensureMetricsIntegration(dns, svc, daemonsetRef); err != nil {
errs = append(errs, fmt.Errorf("failed to integrate metrics with openshift-monitoring for dns %s: %v", dns.Name, err))
}
Expand Down
15 changes: 9 additions & 6 deletions pkg/operator/controller/controller_cluster_role.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,15 @@ func (r *reconciler) ensureDNSClusterRole() (bool, *rbacv1.ClusterRole, error) {
return false, nil, fmt.Errorf("failed to create dns cluster role: %v", err)
}
logrus.Infof("created dns cluster role: %s/%s", desired.Namespace, desired.Name)
return r.currentDNSClusterRole()
case haveCR:
if err := r.updateDNSClusterRole(current, desired); err != nil {
if updated, err := r.updateDNSClusterRole(current, desired); err != nil {
return true, current, err
} else if updated {
return r.currentDNSClusterRole()
}
}
return r.currentDNSClusterRole()
return true, current, nil
Copy link
Contributor

@Miciah Miciah Jun 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@sgreene570 sgreene570 Jun 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@Miciah Miciah Jun 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@sgreene570 sgreene570 Jun 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@Miciah Miciah Jun 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. Thanks!

Copy link
Contributor

@Miciah Miciah Jun 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@sgreene570 sgreene570 Jun 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@Miciah Miciah Jun 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

@sgreene570 sgreene570 Jun 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

@sgreene570 sgreene570 Jun 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

func (r *reconciler) currentDNSClusterRole() (bool, *rbacv1.ClusterRole, error) {
Expand All @@ -53,17 +56,17 @@ func desiredDNSClusterRole() *rbacv1.ClusterRole {
return cr
}

func (r *reconciler) updateDNSClusterRole(current, desired *rbacv1.ClusterRole) error {
func (r *reconciler) updateDNSClusterRole(current, desired *rbacv1.ClusterRole) (bool, error) {
changed, updated := clusterRoleChanged(current, desired)
if !changed {
return nil
return false, nil
}

if err := r.client.Update(context.TODO(), updated); err != nil {
return fmt.Errorf("failed to update dns cluster role %s/%s: %v", updated.Namespace, updated.Name, err)
return false, fmt.Errorf("failed to update dns cluster role %s/%s: %v", updated.Namespace, updated.Name, err)
}
logrus.Infof("updated dns cluster role: %s/%s", updated.Namespace, updated.Name)
return nil
return true, nil
}

func clusterRoleChanged(current, expected *rbacv1.ClusterRole) (bool, *rbacv1.ClusterRole) {
Expand Down
25 changes: 19 additions & 6 deletions pkg/operator/controller/controller_dns_configmap.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,15 +63,15 @@ func (r *reconciler) ensureDNSConfigMap(dns *operatorv1.DNS, clusterDomain strin
return false, nil, fmt.Errorf("failed to create configmap: %v", err)
}
logrus.Infof("created configmap: %s", desired.Name)
return r.currentDNSConfigMap(dns)
case haveCM:
if needsUpdate, updated := corefileChanged(current, desired); needsUpdate {
if err := r.client.Update(context.TODO(), updated); err != nil {
return true, current, fmt.Errorf("failed to update configmap: %v", err)
}
logrus.Infof("updated configmap; old: %#v, new: %#v", current, updated)
if updated, err := r.updateDNSConfigMap(current, desired); err != nil {
return true, current, err
} else if updated {
return r.currentDNSConfigMap(dns)
}
}
return r.currentDNSConfigMap(dns)
return true, current, nil
}

func (r *reconciler) currentDNSConfigMap(dns *operatorv1.DNS) (bool, *corev1.ConfigMap, error) {
Expand Down Expand Up @@ -121,6 +121,19 @@ func desiredDNSConfigMap(dns *operatorv1.DNS, clusterDomain string) (*corev1.Con
return cm, nil
}

func (r *reconciler) updateDNSConfigMap(current, desired *corev1.ConfigMap) (bool, error) {
changed, updated := corefileChanged(current, desired)
if !changed {
return false, nil
}

if err := r.client.Update(context.TODO(), updated); err != nil {
return false, fmt.Errorf("failed to update configmap: %v", err)
}
logrus.Infof("updated configmap; old: %#v, new: %#v", current, updated)
return true, nil
}

func corefileChanged(current, expected *corev1.ConfigMap) (bool, *corev1.ConfigMap) {
if cmp.Equal(current.Data, expected.Data, cmpopts.EquateEmpty()) {
return false, current
Expand Down
15 changes: 9 additions & 6 deletions pkg/operator/controller/controller_dns_daemonset.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,15 @@ func (r *reconciler) ensureDNSDaemonSet(dns *operatorv1.DNS, clusterIP, clusterD
if err := r.createDNSDaemonSet(desired); err != nil {
return false, nil, err
}
return r.currentDNSDaemonSet(dns)
case haveDS:
if err := r.updateDNSDaemonSet(current, desired); err != nil {
if updated, err := r.updateDNSDaemonSet(current, desired); err != nil {
return true, current, err
} else if updated {
return r.currentDNSDaemonSet(dns)
}
}
return r.currentDNSDaemonSet(dns)
return true, current, nil
}

// ensureDNSDaemonSetDeleted ensures deletion of daemonset and related resources
Expand Down Expand Up @@ -147,17 +150,17 @@ func (r *reconciler) createDNSDaemonSet(daemonset *appsv1.DaemonSet) error {
}

// updateDNSDaemonSet updates a dns daemonset.
func (r *reconciler) updateDNSDaemonSet(current, desired *appsv1.DaemonSet) error {
func (r *reconciler) updateDNSDaemonSet(current, desired *appsv1.DaemonSet) (bool, error) {
changed, updated := daemonsetConfigChanged(current, desired)
if !changed {
return nil
return false, nil
}

if err := r.client.Update(context.TODO(), updated); err != nil {
return fmt.Errorf("failed to update dns daemonset %s/%s: %v", updated.Namespace, updated.Name, err)
return false, fmt.Errorf("failed to update dns daemonset %s/%s: %v", updated.Namespace, updated.Name, err)
}
logrus.Infof("updated dns daemonset: %s/%s", updated.Namespace, updated.Name)
return nil
return true, nil
}

// daemonsetConfigChanged checks if current config matches the expected config
Expand Down
15 changes: 9 additions & 6 deletions pkg/operator/controller/controller_dns_service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,15 @@ func (r *reconciler) ensureDNSService(dns *operatorv1.DNS, clusterIP string, dae
return false, nil, fmt.Errorf("failed to create dns service: %v", err)
}
logrus.Infof("created dns service: %s/%s", desired.Namespace, desired.Name)
return r.currentDNSService(dns)
case haveService:
if err := r.updateDNSService(current, desired); err != nil {
if updated, err := r.updateDNSService(current, desired); err != nil {
return true, current, err
} else if updated {
return r.currentDNSService(dns)
}
}
return r.currentDNSService(dns)
return true, current, nil
}

func (r *reconciler) currentDNSService(dns *operatorv1.DNS) (bool, *corev1.Service, error) {
Expand Down Expand Up @@ -75,17 +78,17 @@ func desiredDNSService(dns *operatorv1.DNS, clusterIP string, daemonsetRef metav
return s
}

func (r *reconciler) updateDNSService(current, desired *corev1.Service) error {
func (r *reconciler) updateDNSService(current, desired *corev1.Service) (bool, error) {
changed, updated := serviceChanged(current, desired)
if !changed {
return nil
return false, nil
}

if err := r.client.Update(context.TODO(), updated); err != nil {
return fmt.Errorf("failed to update dns service %s/%s: %v", updated.Namespace, updated.Name, err)
return false, fmt.Errorf("failed to update dns service %s/%s: %v", updated.Namespace, updated.Name, err)
}
logrus.Infof("updated dns service: %s/%s", updated.Namespace, updated.Name)
return nil
return true, nil
}

func serviceChanged(current, expected *corev1.Service) (bool, *corev1.Service) {
Expand Down
15 changes: 9 additions & 6 deletions pkg/operator/controller/controller_service_monitor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,15 @@ func (r *reconciler) ensureServiceMonitor(dns *operatorv1.DNS, svc *corev1.Servi
return false, nil, fmt.Errorf("failed to create servicemonitor %s/%s: %v", desired.GetNamespace(), desired.GetName(), err)
}
logrus.Infof("created servicemonitor %s/%s", desired.GetNamespace(), desired.GetName())
return r.currentServiceMonitor(dns)
case haveSM:
if err := r.updateDNSServiceMonitor(current, desired); err != nil {
if updated, err := r.updateDNSServiceMonitor(current, desired); err != nil {
return true, current, err
} else if updated {
return r.currentServiceMonitor(dns)
}
}
return r.currentServiceMonitor(dns)
return true, current, nil
}

func desiredServiceMonitor(dns *operatorv1.DNS, svc *corev1.Service, daemonsetRef metav1.OwnerReference) *unstructured.Unstructured {
Expand Down Expand Up @@ -96,17 +99,17 @@ func (r *reconciler) currentServiceMonitor(dns *operatorv1.DNS) (bool, *unstruct
return true, sm, nil
}

func (r *reconciler) updateDNSServiceMonitor(current, desired *unstructured.Unstructured) error {
func (r *reconciler) updateDNSServiceMonitor(current, desired *unstructured.Unstructured) (bool, error) {
changed, updated := serviceMonitorChanged(current, desired)
if !changed {
return nil
return false, nil
}

if err := r.client.Update(context.TODO(), updated); err != nil {
return fmt.Errorf("failed to update dns servicemonitor %s/%s: %v", updated.GetNamespace(), updated.GetName(), err)
return false, fmt.Errorf("failed to update dns servicemonitor %s/%s: %v", updated.GetNamespace(), updated.GetName(), err)
}
logrus.Infof("updated dns servicemonitor: %s/%s", updated.GetNamespace(), updated.GetName())
return nil
return true, nil
}

func serviceMonitorChanged(current, expected *unstructured.Unstructured) (bool, *unstructured.Unstructured) {
Expand Down