Skip to content
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

Jump to bottom

controller: Rework boolean logic for consistency #176

Merged
merged 1 commit into from
Jun 9, 2020
Merged

controller: Rework boolean logic for consistency #176

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 5 additions & 5 deletions pkg/operator/controller/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,7 @@ func (r *reconciler) ensureDNSNamespace() error {
logrus.Infof("created dns namespace: %s", ns.Name)
}

if _, err := r.ensureDNSClusterRole(); err != nil {
if _, _, err := r.ensureDNSClusterRole(); err != nil {
return fmt.Errorf("failed to ensure dns cluster role for %s: %v", manifests.DNSClusterRole().Name, err)
}

Expand Down Expand Up @@ -329,7 +329,7 @@ func (r *reconciler) ensureMetricsIntegration(dns *operatorv1.DNS, svc *corev1.S
logrus.Infof("created dns metrics role binding %s/%s", mrb.Namespace, mrb.Name)
}

if _, err := r.ensureServiceMonitor(dns, svc, daemonsetRef); err != nil {
if _, _, err := r.ensureServiceMonitor(dns, svc, daemonsetRef); err != nil {
return fmt.Errorf("failed to ensure servicemonitor for %s: %v", dns.Name, err)
}

Expand All @@ -346,7 +346,7 @@ func (r *reconciler) ensureDNS(dns *operatorv1.DNS) error {
}

errs := []error{}
if daemonset, err := r.ensureDNSDaemonSet(dns, clusterIP, clusterDomain); err != nil {
if _, 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 {
trueVar := true
Expand All @@ -358,10 +358,10 @@ func (r *reconciler) ensureDNS(dns *operatorv1.DNS) error {
Controller: &trueVar,
}

if _, err := r.ensureDNSConfigMap(dns, clusterDomain); err != nil {
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 _, 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 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
22 changes: 11 additions & 11 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 @@ -15,37 +15,37 @@ import (
"k8s.io/apimachinery/pkg/types"
)

func (r *reconciler) ensureDNSClusterRole() (*rbacv1.ClusterRole, error) {
current, err := r.currentDNSClusterRole()
func (r *reconciler) ensureDNSClusterRole() (bool, *rbacv1.ClusterRole, error) {
haveCR, current, err := r.currentDNSClusterRole()
if err != nil {
return nil, err
return false, nil, err
}
desired := desiredDNSClusterRole()

switch {
case desired != nil && current == nil:
case !haveCR:
if err := r.client.Create(context.TODO(), desired); err != nil {
return nil, fmt.Errorf("failed to create dns cluster role: %v", err)
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)
case desired != nil && current != nil:
case haveCR:
if err := r.updateDNSClusterRole(current, desired); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Sidenote: A future cleanup could add a Boolean return value to the update methods that don't have one to indicate whether an update was performed. If no update was performed, the caller can return true, current, nil instead of falling through to return r.currentFoo (which incurs an unnecessary API call).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! I suspect there are quite a few places that have these extra API calls, not just in the cluster-dns-operator but also in the cluster-ingress-operator, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably, yeah. This is an opportunity to make all of this logic more consistent, correct, and efficient across both operators.

return nil, err
return true, current, err
}
}
return r.currentDNSClusterRole()
}

func (r *reconciler) currentDNSClusterRole() (*rbacv1.ClusterRole, error) {
func (r *reconciler) currentDNSClusterRole() (bool, *rbacv1.ClusterRole, error) {
current := &rbacv1.ClusterRole{}
err := r.client.Get(context.TODO(), types.NamespacedName{Name: manifests.DNSClusterRole().Name}, current)
if err != nil {
if errors.IsNotFound(err) {
return nil, nil
return false, nil, nil
}
return nil, err
return false, nil, err
}
return current, nil
return true, current, nil
}

func desiredDNSClusterRole() *rbacv1.ClusterRole {
Expand Down
24 changes: 12 additions & 12 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 @@ -47,43 +47,43 @@ var corefileTemplate = template.Must(template.New("Corefile").Parse(`{{range .Se
`))

// ensureDNSConfigMap ensures that a configmap exists for a given DNS.
func (r *reconciler) ensureDNSConfigMap(dns *operatorv1.DNS, clusterDomain string) (*corev1.ConfigMap, error) {
current, err := r.currentDNSConfigMap(dns)
func (r *reconciler) ensureDNSConfigMap(dns *operatorv1.DNS, clusterDomain string) (bool, *corev1.ConfigMap, error) {
haveCM, current, err := r.currentDNSConfigMap(dns)
if err != nil {
return nil, fmt.Errorf("failed to get configmap: %v", err)
return false, nil, fmt.Errorf("failed to get configmap: %v", err)
}
desired, err := desiredDNSConfigMap(dns, clusterDomain)
if err != nil {
return nil, fmt.Errorf("failed to build configmap: %v", err)
return haveCM, current, fmt.Errorf("failed to build configmap: %v", err)
}

switch {
case desired != nil && current == nil:
case !haveCM:
if err := r.client.Create(context.TODO(), desired); err != nil {
return nil, fmt.Errorf("failed to create configmap: %v", err)
return false, nil, fmt.Errorf("failed to create configmap: %v", err)
}
logrus.Infof("created configmap: %s", desired.Name)
case desired != nil && current != nil:
case haveCM:
if needsUpdate, updated := corefileChanged(current, desired); needsUpdate {
if err := r.client.Update(context.TODO(), updated); err != nil {
return nil, fmt.Errorf("failed to update configmap: %v", err)
return true, current, fmt.Errorf("failed to update configmap: %v", err)
}
logrus.Infof("updated configmap; old: %#v, new: %#v", current, updated)
}
}
return r.currentDNSConfigMap(dns)
}

func (r *reconciler) currentDNSConfigMap(dns *operatorv1.DNS) (*corev1.ConfigMap, error) {
func (r *reconciler) currentDNSConfigMap(dns *operatorv1.DNS) (bool, *corev1.ConfigMap, error) {
current := &corev1.ConfigMap{}
err := r.client.Get(context.TODO(), DNSConfigMapName(dns), current)
if err != nil {
if errors.IsNotFound(err) {
return nil, nil
return false, nil, nil
}
return nil, err
return false, nil, err
}
return current, nil
return true, current, nil
}

func desiredDNSConfigMap(dns *operatorv1.DNS, clusterDomain string) (*corev1.ConfigMap, error) {
Expand Down
26 changes: 13 additions & 13 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 @@ -20,23 +20,23 @@ import (
)

// ensureDNSDaemonSet ensures the dns daemonset exists for a given dns.
func (r *reconciler) ensureDNSDaemonSet(dns *operatorv1.DNS, clusterIP, clusterDomain string) (*appsv1.DaemonSet, error) {
desired, err := desiredDNSDaemonSet(dns, clusterIP, clusterDomain, r.CoreDNSImage, r.OpenshiftCLIImage, r.KubeRBACProxyImage)
func (r *reconciler) ensureDNSDaemonSet(dns *operatorv1.DNS, clusterIP, clusterDomain string) (bool, *appsv1.DaemonSet, error) {
haveDS, current, err := r.currentDNSDaemonSet(dns)
if err != nil {
return nil, fmt.Errorf("failed to build dns daemonset: %v", err)
return false, nil, err
}
current, err := r.currentDNSDaemonSet(dns)
desired, err := desiredDNSDaemonSet(dns, clusterIP, clusterDomain, r.CoreDNSImage, r.OpenshiftCLIImage, r.KubeRBACProxyImage)
if err != nil {
return nil, err
return haveDS, current, fmt.Errorf("failed to build dns daemonset: %v", err)
}
switch {
case desired != nil && current == nil:
case !haveDS:
if err := r.createDNSDaemonSet(desired); err != nil {
return nil, err
return false, nil, err
}
case desired != nil && current != nil:
case haveDS:
if err := r.updateDNSDaemonSet(current, desired); err != nil {
return nil, err
return true, current, err
}
}
return r.currentDNSDaemonSet(dns)
Expand Down Expand Up @@ -126,15 +126,15 @@ func desiredDNSDaemonSet(dns *operatorv1.DNS, clusterIP, clusterDomain, coreDNSI
}

// currentDNSDaemonSet returns the current dns daemonset.
func (r *reconciler) currentDNSDaemonSet(dns *operatorv1.DNS) (*appsv1.DaemonSet, error) {
func (r *reconciler) currentDNSDaemonSet(dns *operatorv1.DNS) (bool, *appsv1.DaemonSet, error) {
daemonset := &appsv1.DaemonSet{}
if err := r.client.Get(context.TODO(), DNSDaemonSetName(dns), daemonset); err != nil {
if errors.IsNotFound(err) {
return nil, nil
return false, nil, nil
}
return nil, err
return false, nil, err
}
return daemonset, nil
return true, daemonset, nil
}

// createDNSDaemonSet creates a dns daemonset.
Expand Down
22 changes: 11 additions & 11 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 @@ -18,37 +18,37 @@ import (
)

// ensureDNSService ensures that a service exists for a given DNS.
func (r *reconciler) ensureDNSService(dns *operatorv1.DNS, clusterIP string, daemonsetRef metav1.OwnerReference) (*corev1.Service, error) {
current, err := r.currentDNSService(dns)
func (r *reconciler) ensureDNSService(dns *operatorv1.DNS, clusterIP string, daemonsetRef metav1.OwnerReference) (bool, *corev1.Service, error) {
haveService, current, err := r.currentDNSService(dns)
if err != nil {
return nil, err
return false, nil, err
}
desired := desiredDNSService(dns, clusterIP, daemonsetRef)

switch {
case desired != nil && current == nil:
case !haveService:
if err := r.client.Create(context.TODO(), desired); err != nil {
return nil, fmt.Errorf("failed to create dns service: %v", err)
return false, nil, fmt.Errorf("failed to create dns service: %v", err)
}
logrus.Infof("created dns service: %s/%s", desired.Namespace, desired.Name)
case desired != nil && current != nil:
case haveService:
if err := r.updateDNSService(current, desired); err != nil {
return nil, err
return true, current, err
}
}
return r.currentDNSService(dns)
}

func (r *reconciler) currentDNSService(dns *operatorv1.DNS) (*corev1.Service, error) {
func (r *reconciler) currentDNSService(dns *operatorv1.DNS) (bool, *corev1.Service, error) {
current := &corev1.Service{}
err := r.client.Get(context.TODO(), DNSServiceName(dns), current)
if err != nil {
if errors.IsNotFound(err) {
return nil, nil
return false, nil, nil
}
return nil, err
return false, nil, err
}
return current, nil
return true, current, nil
}

func desiredDNSService(dns *operatorv1.DNS, clusterIP string, daemonsetRef metav1.OwnerReference) *corev1.Service {
Expand Down
22 changes: 11 additions & 11 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 @@ -18,23 +18,23 @@ import (
"k8s.io/apimachinery/pkg/runtime/schema"
)

func (r *reconciler) ensureServiceMonitor(dns *operatorv1.DNS, svc *corev1.Service, daemonsetRef metav1.OwnerReference) (*unstructured.Unstructured, error) {
func (r *reconciler) ensureServiceMonitor(dns *operatorv1.DNS, svc *corev1.Service, daemonsetRef metav1.OwnerReference) (bool, *unstructured.Unstructured, error) {
desired := desiredServiceMonitor(dns, svc, daemonsetRef)

current, err := r.currentServiceMonitor(dns)
haveSM, current, err := r.currentServiceMonitor(dns)
if err != nil {
return nil, err
return false, nil, err
}

switch {
case desired != nil && current == nil:
case !haveSM:
if err := r.client.Create(context.TODO(), desired); err != nil {
return nil, fmt.Errorf("failed to create servicemonitor %s/%s: %v", desired.GetNamespace(), desired.GetName(), err)
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())
case desired != nil && current != nil:
case haveSM:
if err := r.updateDNSServiceMonitor(current, desired); err != nil {
return nil, err
return true, current, err
}
}
return r.currentServiceMonitor(dns)
Expand Down Expand Up @@ -80,7 +80,7 @@ func desiredServiceMonitor(dns *operatorv1.DNS, svc *corev1.Service, daemonsetRe
return sm
}

func (r *reconciler) currentServiceMonitor(dns *operatorv1.DNS) (*unstructured.Unstructured, error) {
func (r *reconciler) currentServiceMonitor(dns *operatorv1.DNS) (bool, *unstructured.Unstructured, error) {
sm := &unstructured.Unstructured{}
sm.SetGroupVersionKind(schema.GroupVersionKind{
Group: "monitoring.coreos.com",
Expand All @@ -89,11 +89,11 @@ func (r *reconciler) currentServiceMonitor(dns *operatorv1.DNS) (*unstructured.U
})
if err := r.client.Get(context.TODO(), DNSServiceMonitorName(dns), sm); err != nil {
if errors.IsNotFound(err) {
return nil, nil
return false, nil, nil
}
return nil, err
return false, nil, err
}
return sm, nil
return true, sm, nil
}

func (r *reconciler) updateDNSServiceMonitor(current, desired *unstructured.Unstructured) error {
Expand Down