CFE-1037: Fix issues pertaining to DNSNameResolver controller #6
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@arkadeepsen: This pull request references CFE-1037 which is a valid jira issue. In response to this:
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. |
|
/retest |
| @@ -149,6 +147,11 @@ func (resolver *Resolver) Add( | |||
| return | |||
There was a problem hiding this comment.
This return and the one on the line 138 are not protected any longer since you don't defer the unlock call.
There was a problem hiding this comment.
Added the unlock before the return.
| resolver.dnsLock.Unlock() | ||
|
|
||
| // Iterate through the dnsNameList slice. | ||
| for _, dnsName := range dnsNameList { | ||
| resolver.dnsLock.Lock() |
There was a problem hiding this comment.
There can be a context switch between the unlock and lock here. The state of the resolver can change in the mean time. Any reason you call Lock twice?
There was a problem hiding this comment.
We need to release the lock before sending the signal on the delete channel. That's why I have unlocked before the loop and then taken the lock inside it. Probably the lock can be released before sending the signal to the channel and then again take the lock on return.
There was a problem hiding this comment.
Otherwise, we can create another slice of DNS names which needs sending signal to the delete channel. Once the changes have been made to the resolver.dnsNames then we can release the lock and send the signals one by one in a loop.
| } | ||
|
|
||
| // Delete is called whenever a DNSNameResolver object is deleted. | ||
| func (resolver *Resolver) Delete(objName string) { | ||
| resolver.dnsLock.Lock() | ||
| defer resolver.dnsLock.Unlock() |
There was a problem hiding this comment.
Same comment as in Add() about the unsafe returns down the code (github doesn't allow to put a comment there).
There was a problem hiding this comment.
Added the unlock before the returns.
Add support to provide port on which the CoreDNS is listening on as an input. Add RBAC permissions for getting events related to DNSNameResolver status updates. Fix the issue of deadlock condition in the resolver.
arkadeepsen
force-pushed
the
fix-controller
branch
from
94547fd
to
2d4883d
Compare
| @@ -163,12 +161,12 @@ func (r *reconciler) Reconcile(ctx context.Context, request reconcile.Request) ( | |||
| // event then this will ensure that the resolver sends the resolution request | |||
| // for the DNS name, so that the status of the object gets updated with the | |||
| // corresponding IP addresses. | |||
| r.resolver.Add(dnsName, []ocpnetworkv1alpha1.DNSNameResolverResolvedAddress{}, matchesRegular, request.Name) | |||
| r.resolver.AddDNSNameResolverResolvedName(dnsName, []ocpnetworkv1alpha1.DNSNameResolverResolvedAddress{}, matchesRegular, request.Name) | |||
There was a problem hiding this comment.
Can the name be more concise? Something like
| r.resolver.AddDNSNameResolverResolvedName(dnsName, []ocpnetworkv1alpha1.DNSNameResolverResolvedAddress{}, matchesRegular, request.Name) | |
| r.resolver.AddResolvedName(dnsName, []ocpnetworkv1alpha1.DNSNameResolverResolvedAddress{}, matchesRegular, request.Name) |
Or even shorter:
| r.resolver.AddDNSNameResolverResolvedName(dnsName, []ocpnetworkv1alpha1.DNSNameResolverResolvedAddress{}, matchesRegular, request.Name) | |
| r.resolver.AddName(dnsName, []ocpnetworkv1alpha1.DNSNameResolverResolvedAddress{}, matchesRegular, request.Name) |
| @@ -129,7 +127,7 @@ func (r *reconciler) Reconcile(ctx context.Context, request reconcile.Request) ( | |||
| // Check if the DNSNameResolver resource is deleted. If so, delete DNS names matching the DNSNameResolver resource | |||
| // from the resolver. | |||
| if errors.IsNotFound(err) { | |||
| r.resolver.Delete(request.Name) | |||
| r.resolver.DeleteDNSNameResolver(request.Name) | |||
There was a problem hiding this comment.
Can this be consistent in naming with the Add method? Something like
| r.resolver.DeleteDNSNameResolver(request.Name) | |
| r.resolver.DeleteResolvedName(request.Name) |
| nextDNSName, nextLookupTime, numIPs, exists = resolver.add(addedResolvedAddressDetails.dnsName, | ||
| addedResolvedAddressDetails.resolvedAddresses, addedResolvedAddressDetails.matchesRegular, addedResolvedAddressDetails.objName) |
There was a problem hiding this comment.
Can we just pass the details struct down to add() instead of N args?
| regularObjInfo map[string]string | ||
| wildcardObjInfo map[string]sets.Set[string] | ||
| dnsLock sync.Mutex | ||
|
|
||
| added chan struct{} | ||
| added chan resolvedAddressDetails | ||
| deleted chan string |
There was a problem hiding this comment.
What do you think about unification of 2 channels? Something like:
added chan dnsDetails
deleted chan dnsDetailsWhere dnsDetails is renamed resolvedAddressDetails.
Then when you Add or Delete in controller you can do:
resolver.AddName(dnsDetials{dnsName, resolvedAddresses, matchesRegulr, objName})
resolved.DeleteName(dnsDetails{objName})
arkadeepsen
force-pushed
the
fix-controller
branch
from
49fe459
to
054df9c
Compare
|
@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/test-infra repository. I understand the commands that are listed here. |
|
/lgtm |
|
[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:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
@snarayan-redhat @melvinjoseph86 @CFields651 adding the labels as the changes in this PR will be imported into openshift/cluster-dns-operator#394 /label docs-approved |
openshift-ci
bot
added
docs-approved
|
@arkadeepsen: This pull request references CFE-1037 which is a valid jira issue. In response to this:
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. |
openshift-ci
bot
added
the
px-approved
This PR adds the support to provide port on which the CoreDNS is listening on as an input. RBAC permissions are added for getting events related to DNSNameResolver status updates. It also fixes the issue of deadlock condition in the resolver.