OCPBUGS-33750: Fix DNSNameResolver object status update issue

[arkadeepsen]

This PR fixes the issue with DNSNameResolver object status update.

The DNSNameResolver controller was sending DNS requests for a DNS name whose IP has expired at an interval 1ms. As the update event of the DNSNameResolver object is not received by the DNSNameResolver controller within the next 1ms, the controller again sent the DNS request. This resulted in the creation of numerous DNS requests within a very short period of time. The interval is changed to 2 times of the default minimum TTL (5 seconds). This interval will account for the time required for getting the update event by the controller as well as the grace period (5 seconds) to remove any IP address whose TTL has expired. This will avoid sending any extra DNS requests after the first DNS request post the TTL expiration. Additionally, if for any DNS name the latest queries did not return any new address which results in removal of the associated IP addresses after TTL expiration, then the next lookup time for such DNS names is set to default maximum TTL (30 minutes). This will avoid creation of DNS requests for the DNS name at an interval of 2 times of the default minimum TTL.

On the CoreDNS plugin side, the RetryOnConflict block was using the lister to get the DNSNameResolver object and then the client to update the status. However, if a conflict occurs the lister does not get the updated object immediately and the update again fails due to conflict. To minimize the conflict error on update, the client will be used to get the latest object instead of the lister if the resourceVersion of the DNSNameResolver object is same as the previous GET call (inspired by openshift/library-go#1668).

[openshift-ci-robot]

@arkadeepsen: This pull request references Jira Issue OCPBUGS-33750, which is invalid:

• expected the bug to target the "4.17.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

This PR fixes the issue with DNSNameResolver object status update.

The DNSNameResolver controller was sending DNS requests for a DNS name whose IP has expired at an interval 1ms. As the update event of the DNSNameResolver object is not received by the DNSNameResolver controller within the next 1ms, the controller again sent the DNS request. This resulted in the creation of numerous DNS requests within a very short period of time. The interval is changed to 2 times of the default minimum TTL (5 seconds). This interval will account for the time required for getting the update event by the controller as well as the grace period (5 seconds) to remove any IP address whose TTL has expired. This will avoid sending any extra DNS requests after the first DNS request post the TTL expiration. Additionally, if for any DNS name the latest queries did not return any new address which results in removal of the associated IP addresses after TTL expiration, then the next lookup time for such DNS names is set to default maximum TTL (30 minutes). This will avoid creation of DNS requests for the DNS name at an interval of 2 times of the default minimum TTL.

On the CoreDNS plugin side, the RetryOnConflict block was using the cache to get the DNSNameResolver object and then the client to update the status. However, if a conflict occurs the cache does not get the updated object immediately and the update again fails due to conflict. To minimize the conflict error on update, the client will be used to get the latest object instead of the cache.

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 openshift-eng/jira-lifecycle-plugin repository.

[arkadeepsen]

/jira refresh

[openshift-ci-robot]

@arkadeepsen: This pull request references Jira Issue OCPBUGS-33750, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

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

Requesting review from QA contact:
/cc @huiran0826

In response to this:

/jira refresh

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 openshift-eng/jira-lifecycle-plugin repository.

[arkadeepsen]

openshift/release#52809 updates the golang version to 1.22 for the build_root in the CI config. Adding a hold until it merges.
/hold

[candita]

/assign @alebedev87
/assign

[openshift-ci-robot]

@arkadeepsen: This pull request references Jira Issue OCPBUGS-33750, which is valid.

3 validation(s) were run on this bug

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

Requesting review from QA contact:
/cc @huiran0826

In response to this:

This PR fixes the issue with DNSNameResolver object status update.

The DNSNameResolver controller was sending DNS requests for a DNS name whose IP has expired at an interval 1ms. As the update event of the DNSNameResolver object is not received by the DNSNameResolver controller within the next 1ms, the controller again sent the DNS request. This resulted in the creation of numerous DNS requests within a very short period of time. The interval is changed to 2 times of the default minimum TTL (5 seconds). This interval will account for the time required for getting the update event by the controller as well as the grace period (5 seconds) to remove any IP address whose TTL has expired. This will avoid sending any extra DNS requests after the first DNS request post the TTL expiration. Additionally, if for any DNS name the latest queries did not return any new address which results in removal of the associated IP addresses after TTL expiration, then the next lookup time for such DNS names is set to default maximum TTL (30 minutes). This will avoid creation of DNS requests for the DNS name at an interval of 2 times of the default minimum TTL.

On the CoreDNS plugin side, the RetryOnConflict block was using the lister to get the DNSNameResolver object and then the client to update the status. However, if a conflict occurs the lister does not get the updated object immediately and the update again fails due to conflict. To minimize the conflict error on update, the client will be used to get the latest object instead of the lister if the resourceVersion of the DNSNameResolver object is same as the previous GET call.

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 openshift-eng/jira-lifecycle-plugin repository.

[arkadeepsen]

/hold cancel
openshift/release#52809 is merged

[alebedev87]

Do we want the user to do something about it? And can we do something about it? If no, it should be just an Infof.

[arkadeepsen]

I'll change it to Infof.

[arkadeepsen]

Changed to Infof.

[alebedev87]

I'm not sure I'm getting this one. Do we kinda let the expired records to be removed instead of trying to save them resending the dns request?

[alebedev87]

Also, it would be nice to group the logic of the next lookup computation into a dedicated function which would select on the channels and would give the next lookup time. This would allow us to unit test it and clearly see the use cases.

[arkadeepsen]

I'm not sure I'm getting this one. Do we kinda let the expired records to be removed instead of trying to save them resending the dns request?

When the next lookup time is reached, we get a signal on the timer.C channel in line no. 88 . The lookup for the DNS name happens in line no. 92. So when we reach here, the lookup request is already sent and we should wait for the update event for the corresponding DNSNameResolver object.

When we receive the update event, we also remove the expired IP addresses after a grace period of defaultMinTTL during reconciliation. The following code does this work:

coredns-ocp-dnsnameresolver/operator/controller/dnsnameresolver/controller.go

Lines 139 to 153 in a3f67ee

	// Check if the grace period is over for some of the IP addresses after the expiration of their respective TTLs. If so,
	// remove those IP addresses and update the status of the resource.
	if removalOfIPsRequired(&dnsNameResolver.Status) {
	if err := r.client.Status().Update(ctx, dnsNameResolver); err != nil {
	return reconcile.Result{}, err
	}
	return reconcile.Result{}, nil
	}
	// Check if the TTLs of some of the IP addresses have expired. If so, requeue
	// the reconcile request after the minimum remaining time until the grace
	// period gets over among that of the IP addresses with expired TTLs.
	if ttlExpired, remainingTime := reconcileRequired(&dnsNameResolver.Status); ttlExpired {
	return reconcile.Result{Requeue: true, RequeueAfter: remainingTime}, nil
	}

So, this block of code will not receive the new IPs/TTLs until the expired IPs are removed. If it doesn't wait until 2 * defaultMinTTL, unnecessary lookup requests will again be sent to the CoreDNS pods.

[arkadeepsen]

Separated the code for computing time till next lookup into a function and added unit test for the same.

[alebedev87]

When the next lookup time is reached, we get a signal on the timer.C channel in line no. 88 . The lookup for the DNS name happens in line no. 92. So when we reach here, the lookup request is already sent and we should wait for the update event for the corresponding DNSNameResolver object.

But we get into this condition only when the remaining duration is 0. The case you added confirms this:

		{
			name:                       "DNS exists and remaining duration is not greater than 0",
			dnsExists:                  true,
			remainingDuration:          0,
			expectedTimeTillNextLookup: 2 * defaultMinTTL,
		},

So something doesn't stick here. The remaining duration says that we need to send a new lookup asap (like it was before this PR) but we delay it to catchup with the updates.

[alebedev87]

Suggested change

	var resolverObj *ocpnetworkapiv1alpha1.DNSNameResolver
	var resourceVersion string
	var err error
	var (
	resolverObj *ocpnetworkapiv1alpha1.DNSNameResolver
	resourceVersion string
	err error
	)

[alebedev87]

I suppose that this can be covered in TestResolver. Can we do a test case for this one?

[arkadeepsen]

I think so. Let me try to add that test case.

[arkadeepsen]

Added test case for the same in TestResolver.

[openshift-ci]

@arkadeepsen: all tests passed!

Full PR test history. Your PR dashboard.

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-sigs/prow repository. I understand the commands that are listed here.

[alebedev87]

We will need to address the increasing complexity of the resolver code as the reviews become challenging. Letting the bug resolution move on.

/lgtm
/approve

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alebedev87

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 [alebedev87]

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

[openshift-ci-robot]

@arkadeepsen: Jira Issue OCPBUGS-33750: Some pull requests linked via external trackers have merged:

The following pull requests linked via external trackers have not merged:

• openshift/cluster-dns-operator#415 is open

These pull request must merge or be unlinked from the Jira bug in order for it to move to the next state. Once unlinked, request a bug refresh with /jira refresh.

Jira Issue OCPBUGS-33750 has not been moved to the MODIFIED state.

In response to this:

This PR fixes the issue with DNSNameResolver object status update.

The DNSNameResolver controller was sending DNS requests for a DNS name whose IP has expired at an interval 1ms. As the update event of the DNSNameResolver object is not received by the DNSNameResolver controller within the next 1ms, the controller again sent the DNS request. This resulted in the creation of numerous DNS requests within a very short period of time. The interval is changed to 2 times of the default minimum TTL (5 seconds). This interval will account for the time required for getting the update event by the controller as well as the grace period (5 seconds) to remove any IP address whose TTL has expired. This will avoid sending any extra DNS requests after the first DNS request post the TTL expiration. Additionally, if for any DNS name the latest queries did not return any new address which results in removal of the associated IP addresses after TTL expiration, then the next lookup time for such DNS names is set to default maximum TTL (30 minutes). This will avoid creation of DNS requests for the DNS name at an interval of 2 times of the default minimum TTL.

On the CoreDNS plugin side, the RetryOnConflict block was using the lister to get the DNSNameResolver object and then the client to update the status. However, if a conflict occurs the lister does not get the updated object immediately and the update again fails due to conflict. To minimize the conflict error on update, the client will be used to get the latest object instead of the lister if the resourceVersion of the DNSNameResolver object is same as the previous GET call (inspired by openshift/library-go#1668).

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 openshift-eng/jira-lifecycle-plugin repository.

[arkadeepsen]

/cherrypick release-4.16

[openshift-cherrypick-robot]

@arkadeepsen: new pull request created: #13

In response to this:

/cherrypick release-4.16

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-sigs/prow repository.