v1.7 backports 2020-03-10 #10530
Merged
v1.7 backports 2020-03-10 #10530
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
[ upstream commit 5bf90eb ] UpdateGenerateDNS updated the cache and generate selector updates as two separate lock-holding sections. When updates happened concurrently, via DNS lookups, DNS GC, or endpoint restores, the snapshot of data that eventually updated the selectors could be out-of-order. This created an A-B-A race for selectors that needed an update and those that needed to be cleared. These races have been observed on agent starts with pending pods but may manifest between multiple pods making DNS updates or the DNS garbage collector controller and any pod. The code now holds the NameManager lock for the entirety of UpdateGenerateDNS and ForceRegenerateDNS. This forces the update to complete in its entirety before allowing another to modify the FQDN cache or registered selectors. The locking order between the NameManager lock and the SelectorCache lock has been accidental so far. When removing a selector user from the cache, the SelectorCache lock is taken, and if an FQDN selector has no more users, the NameManager is notified of this, which internally takes the NameManager lock. Reverse this by explicitly locking NamaManager for the duration of selector cache operations. From now on, if the two locks are to be held at the same time, the NameManager mutex MUST be taken first, the SelectorCache mutex second, as the name manager is a higher level construct than the selector cache. Exposing 'Lock()' and 'Unlock()' operations in an interface is not ideal, but it helps make the lockinng order explicit, so it also serves as documentation on the required locking order. NameManager now return IDs also if the selector has already been registered. This should help not get into a state where FQDNs are not passed to the selector cache in case of a race condition. Signed-off-by: Ray Bejjani <ray@isovalent.com> Signed-off-by: Jarno Rajahalme <jarno@covalent.io>
jrajahalme
added
kind/backports
houndci-bot
reviewed
Mar 10, 2020
| @@ -71,6 +71,9 @@ func allocateCIDRs(prefixes []*net.IPNet) ([]*identity.Identity, error) { | |||
| allocateCtx, cancel := context.WithTimeout(context.Background(), option.Config.IPAllocationTimeout) | |||
| defer cancel() | |||
|
|
|||
| if IdentityAllocator == nil { | |||
| return nil, fmt.Errorf("IdentityAllocator not initialized!") | |||
There was a problem hiding this comment.
error strings should not be capitalized or end with punctuation or a newline
houndci-bot
reviewed
Mar 10, 2020
| delete(d.selectors, selector) | ||
| } | ||
|
|
||
| func (d *DummyIdentityNotifier) InjectIdentitiesForSelector(fqdnSel api.FQDNSelector, ids []identity.NumericIdentity) { |
There was a problem hiding this comment.
exported method DummyIdentityNotifier.InjectIdentitiesForSelector should have comment or be unexported
| selectors map[api.FQDNSelector][]identity.NumericIdentity | ||
| } | ||
|
|
||
| func NewDummyIdentityNotifier() *DummyIdentityNotifier { |
There was a problem hiding this comment.
exported function NewDummyIdentityNotifier should have comment or be unexported
| "github.com/cilium/cilium/pkg/policy/api" | ||
| ) | ||
|
|
||
| type DummyIdentityNotifier struct { |
There was a problem hiding this comment.
exported type DummyIdentityNotifier should have comment or be unexported
|
test-me-please |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
v1.7 backports 2020-03-10
Once this PR is merged, you can update the PR labels via:
This change is