dns: Don't hotloop on updates to DNSRecord status #484

Miciah · 2020-11-02T21:52:33Z

Add a predicate to the DNS controller's watch on DNSRecords so that only updates a DNSRecord's spec triggers reconciliation (and updates to its status do not).

Before this change, one reconciliation could update the DNSRecord's status and trigger a second reconciliation. In the case of a successful update, the second reconciliation would have no work to do, so it would complete without triggering a third reconciliation. In the case of a failed update, the second reconciliation would retry the update. If retrying the update then failed, the second reconciliation could update the DNSRecord's status to record the new failure, triggering a third reconciliation, which could similarly trigger a fourth reconciliation, and so on ad infinitum.

pkg/operator/controller/dns/controller.go (New): Add a predicate to the watch on DNSRecords to ignore updates to status.

@ironcladlou

Miciah · 2020-11-03T01:59:59Z

Seems like the predicate filtered out delete events?! At least I'm not seeing any "deleted dnsrecord" log messages in https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/openshift_cluster-ingress-operator/484/pull-ci-openshift-cluster-ingress-operator-master-e2e-aws-operator/1323382581768491008/artifacts/e2e-aws-operator/pods/openshift-ingress-operator_ingress-operator-687f7b4fbb-q5d94_ingress-operator.log.

Miciah · 2020-11-03T02:02:52Z

Latest push uses GenerationChangedPredicate, which returns true for create, delete, and generic events, and returns false for updates if the generation has not changed. Maybe that will fix handling of delete events.

Add a predicate to the DNS controller's watch on DNSRecords so that only updates a DNSRecord's spec triggers reconciliation (and updates to its status do not). Before this commit, one reconciliation could update the DNSRecord's status and trigger a second reconciliation. In the case of a successful update, the second reconciliation would have no work to do, so it would complete without triggering a third reconciliation. In the case of a failed update, the second reconciliation would retry the update. If retrying the update then failed, the second reconciliation could update the DNSRecord's status to record the new failure, triggering a third reconciliation, which could similarly trigger a fourth reconciliation, and so on ad infinitum. * pkg/operator/controller/dns/controller.go (New): Add a predicate to the watch on DNSRecords to ignore updates to status.

ironcladlou · 2020-11-03T18:54:38Z

Thanks, lgtm although there are some assumptions baked into that predicate to consider:

// * The assumption that the Generation is incremented only on writing to the spec does not hold for all APIs.
// E.g For Deployment objects the Generation is also incremented on writes to the metadata.annotations field.
// For object types other than CustomResources be sure to verify which fields will trigger a Generation increment when they are written to.
//
// * With this predicate, any update events with writes only to the status field will not be reconciled.
// So in the event that the status block is overwritten or wiped by someone else the controller will not self-correct to restore the correct status.

hopefully regression testing would reveal any possible sync issue here.

Miciah · 2020-11-03T23:53:51Z

/test e2e-aws-operator

sgreene570 · 2020-11-24T15:06:54Z

/lgtm

openshift-ci-robot · 2020-11-24T15:07:12Z

[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

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 · 2020-11-24T15:44:40Z

/retest