Bug 1936022: Fix spurious reconciliation of DNS daemonset and service by Miciah · Pull Request #243 · openshift/cluster-dns-operator

Miciah · 2021-03-04T16:23:53Z

Log diffs when reconciling resources

This PR improves log output when reconciling managed resources to log the diff between the current and updated resources when performing an update.

pkg/operator/controller/controller_cluster_role.go (updateDNSClusterRole):
pkg/operator/controller/controller_dns_daemonset.go (updateDNSDaemonSet):
pkg/operator/controller/controller_dns_service.go (updateDNSService):
pkg/operator/controller/controller_service_monitor.go (updateDNSServiceMonitor): Log the diff between the current and updated resources when performing an update.

`daemonsetConfigChanged`: Fix for `terminationGracePeriodSeconds` defaulting

When comparing daemonsets to determine whether an update is required, treat implicit and explicit default values for terminationGracePeriodSeconds as equal.

Before this PR, the update logic would keep trying to revert the default value that the API set for the daemonset's terminationGracePeriodSeconds field.

Follow-up to #205, which added the logic which reconciles terminationGracePeriodSeconds.

pkg/operator/controller/controller_dns_daemonset.go (daemonsetConfigChanged): Use the new cmpTerminationGracePeriodSeconds function to compare terminationGracePeriodSeconds values so that two daemonsets are considered equal if the only difference between them is that one does not specify terminationGracePeriodSeconds and the other daemonset has the default value.
(cmpTerminationGracePeriodSeconds): New function.
pkg/operator/controller/controller_dns_daemonset_test.go (TestDaemonsetConfigChanged): Verify that daemonsetConfigChanged returns false if the only mutation is that terminationGracePeriodSeconds has been updated from empty to the default value.

`serviceChanged`: Fix for `clusterIPs` defaulting

Ignore the clusterIPs field when comparing services to determine whether an update is required.

Before this PR, the update logic would keep trying to revert the value that the API set for the service's clusterIPs field.

The clusterIPs field is new in Kubernetes 1.20 (OpenShift 4.7).

pkg/operator/controller/controller_dns_service.go (serviceChanged): Ignore the services' clusterIPs field.
pkg/operator/controller/controller_dns_service_test.go (TestDNSServiceChanged): Verify that serviceChanged returns false if the only mutation is that the clusterIPs field's value has changed.

Miciah · 2021-03-05T21:34:27Z

/test e2e-aws-operator

sgreene570 · 2021-03-05T21:37:57Z

This is fantastic! Should we look into doing something similar for the ingress operator?

sgreene570 · 2021-03-05T21:49:35Z

Looking at DNS operator logs with these changes
https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/openshift_cluster-dns-operator/243/pull-ci-openshift-cluster-dns-operator-master-e2e-aws/1367511193526210560/artifacts/e2e-aws/gather-extra/artifacts/pods/openshift-dns-operator_dns-operator-5cb7fcdbbc-rz42g_dns-operator.log

There's an abundance of white-space in the diff outputs... anything we can do to help with that? (Or do we not really care?)

openshift-ci-robot · 2021-03-06T00:26:05Z

@Miciah: An error was encountered updating to the POST state for bug 1936022 on the Bugzilla server at https://bugzilla.redhat.com. No known errors were detected, please see the full error message for details.

Full error message.


code 32000: Red Hat Bugzilla's database reported a query serialization error. Most likely this occurred because another user or process attempted to change the same data that you were attempting to change. Please press Back and retry the transaction.  UPDATE bugs SET bug_status = ? WHERE bug_id = ? at /var/www/html/bugzilla/Bugzilla/Object.pm line 544.

Please contact an administrator to resolve this issue, then request a bug refresh with /bugzilla refresh.

Details

In response to this:

Bug 1936022: Fix spurious reconciliation of DNS daemonset and service

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-ci-robot · 2021-03-06T00:26:08Z

@Miciah: This pull request references Bugzilla bug 1936022, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.8.0) matches configured target release for branch (4.8.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @lihongan

Details

In response to this:

Bug 1936022: Fix spurious reconciliation of DNS daemonset and service

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.

Miciah · 2021-03-06T00:29:31Z

This is fantastic! Should we look into doing something similar for the ingress operator?

Thanks! Yeah, the ingress operator has similar diff output for deployments but not for other resources, and it would be useful to have it for all resources. I will probably make a similar change in the ingress operator for the resources other than deployments.

Looking at DNS operator logs with these changes
https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/openshift_cluster-dns-operator/243/pull-ci-openshift-cluster-dns-operator-master-e2e-aws/1367511193526210560/artifacts/e2e-aws/gather-extra/artifacts/pods/openshift-dns-operator_dns-operator-5cb7fcdbbc-rz42g_dns-operator.log

There's an abundance of white-space in the diff outputs... anything we can do to help with that? (Or do we not really care?)

I don't know a good way to improve the whitespace with our logging solution. I used sed -ne 's/.*msg=$".*"$$/\1/gp' | jq -r . to extract and format the relevant information.

Miciah · 2021-03-06T01:18:43Z

We should backport this to OpenShift 4.7, which added the clusterIPs field in the API and added the logic to reconcile terminationGracePeriodSeconds in the ingress operator.
/cherry-pick release-4.7

openshift-cherrypick-robot · 2021-03-06T01:18:44Z

@Miciah: once the present PR merges, I will cherry-pick it on top of release-4.7 in a new PR and assign it to you.

Details

In response to this:

We should backport this to OpenShift 4.7, which added the clusterIPs field in the API and added the logic to reconcile terminationGracePeriodSeconds in the ingress operator.
/cherry-pick release-4.7

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.

Miciah · 2021-03-08T03:23:29Z

/retest

Miciah · 2021-03-08T08:30:11Z

/test e2e-upgrade

sgreene570 · 2021-03-08T15:30:00Z

I don't know a good way to improve the whitespace with our logging solution. I used sed -ne 's/.*msg=$".*"$$/\1/gp' | jq -r . to extract and format the relevant information.

Good enough for me 😁

sgreene570 · 2021-03-08T16:12:22Z

+		{
+			description: "if the termination grace period is defaulted",
+			mutate: func(daemonset *appsv1.DaemonSet) {
+				thirty := int64(30)


Can we get the API defaulted value right from the corev1 API?
(Not that I anticipate the value changing anytime soon, but IMO using the API constant would make the test cleaner)

https://github.com/kubernetes/api/blob/master/core/v1/types.go#L2601

Nifty! Thanks for the pointer! Done.

sgreene570 · 2021-03-08T16:38:27Z

This PR improves log output when reconciling managed resources to log the diff between the current and updated resources when performing an update.

pkg/operator/controller/controller_cluster_role.go (updateDNSClusterRole):

pkg/operator/controller/controller_dns_daemonset.go (updateDNSDaemonSet):

pkg/operator/controller/controller_dns_service.go (updateDNSService):

pkg/operator/controller/controller_service_monitor.go (updateDNSServiceMonitor): Log the diff between the current and updated resources when performing an update.

@Miciah any reason to not also print diffs for corefile config map updates?

This commit improves log output when reconciling managed resources to log the diff between the current and updated resources when performing an update. * pkg/operator/controller/controller_cluster_role.go (updateDNSClusterRole): * pkg/operator/controller/controller_dns_configmap.go (updateDNSConfigMap): * pkg/operator/controller/controller_dns_daemonset.go (updateDNSDaemonSet): * pkg/operator/controller/controller_dns_service.go (updateDNSService): * pkg/operator/controller/controller_service_monitor.go (updateDNSServiceMonitor): Log the diff between the current and updated resources when performing an update.

When comparing daemonsets to determine whether an update is required, treat implicit and explicit default values for terminationGracePeriodSeconds as equal. Before this commit, the update logic would keep trying to revert the default value that the API set for the daemonset's terminationGracePeriodSeconds field. Follow-up to commit f094ddf, which added the logic which reconciles terminationGracePeriodSeconds. This commit is related to bug 1936022. https://bugzilla.redhat.com/show_bug.cgi?id=1936022 * pkg/operator/controller/controller_dns_daemonset.go (daemonsetConfigChanged): Use the new cmpTerminationGracePeriodSeconds function to compare terminationGracePeriodSeconds values so that two daemonsets are considered equal if the only difference between them is that one does not specify terminationGracePeriodSeconds and the other daemonset has the default value. (cmpTerminationGracePeriodSeconds): New function. * pkg/operator/controller/controller_dns_daemonset_test.go (TestDaemonsetConfigChanged): Verify that daemonsetConfigChanged returns false if the only mutation is that terminationGracePeriodSeconds has been updated from empty to the default value.

* pkg/operator/controller/controller_dns_daemonset.go (volumeDefaultMode): Delete const. (cmpConfigMapVolumeSource): Use corev1.ConfigMapVolumeSourceDefaultMode. (cmpSecretVolumeSource): Use corev1.SecretVolumeSourceDefaultMode. * pkg/operator/controller/controller_dns_daemonset_test.go (TestDaemonsetConfigChanged): Use corev1.ConfigMapVolumeSourceDefaultMode and corev1.SecretVolumeSourceDefaultMode.

Ignore the clusterIPs field when comparing services to determine whether an update is required. Before this commit, the update logic would keep trying to revert the value that the API set for the service's clusterIPs field. The clusterIPs field is new in Kubernetes 1.20 (OpenShift 4.7). This commit is related to bug 1936022. https://bugzilla.redhat.com/show_bug.cgi?id=1936022 * pkg/operator/controller/controller_dns_service.go (serviceChanged): Ignore the services' clusterIPs field. * pkg/operator/controller/controller_dns_service_test.go (TestDNSServiceChanged): Verify that serviceChanged returns false if the only mutation is that the clusterIPs field's value has changed.

Miciah · 2021-03-22T03:30:14Z

@Miciah any reason to not also print diffs for corefile config map updates?

Nope! Done. Thanks!

sgreene570 · 2021-03-22T14:45:50Z

/lgtm

openshift-ci-robot · 2021-03-22T14:46:04Z

[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,sgreene570]

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

openshift-bot · 2021-03-22T18:46:49Z

/retest