CRD: fix allocation logic of identities with the same set of labels #11040
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
aanm
added
kind/bug
|
test-me-please |
|
test-me-please |
tgraf
approved these changes
Apr 19, 2020
|
test-me-please |
christarazi
reviewed
Apr 20, 2020
The same logic was being performed inside the loop so we can execute it right after each loop and remove the duplicated source code that is outside the loop. Signed-off-by: André Martins <andre@cilium.io>
The logic to retrieve an identity from an ID can be useful so this commit adds a dedicated function for this functionality. Signed-off-by: André Martins <andre@cilium.io>
Signed-off-by: André Martins <andre@cilium.io>
When performing an identity release and if 2 or more identities exist for the same set of labels, after performing a lookup in the local CRD store, this lookup will return a random identity since its lookup is done by the set of labels which can be repeated in this local CRD store. As we don't verify if the ID of this identity is the one that we should remove the node reference from we can end up removing the node reference from the wrong Identity. To avoid this we should perform the local CRD store lookup by the unique ID and not rely on labels. The same issue does not happen in KVStore mode since we have a KVStore key specific for each node so we know exactly which key we can remove based on the node that is removing the reference from. Signed-off-by: André Martins <andre@cilium.io>
This will help the understanding of the Get function caveats for the CRD mode. Signed-off-by: André Martins <andre@cilium.io>
When performing an AcquireReference and if 2 or more identities exist for the same set of labels, after performing a lookup in the local CRD store, this lookup will return a random identity since its lookup is done by the set of labels which can be repeated in this local CRD store. As we don't verify if the ID of the identity gotten from the local CRD store we might endup acquiring the reference for the identity that does not have the ID that we want to acquire the reference with. To avoid this we should perform the local CRD store lookup by the unique ID and not rely on the key labels. The same issue does not happen in KVStore mode since we have a KVStore key specific for each node so we know exactly which ID belongs to which node. Signed-off-by: André Martins <andre@cilium.io>
It is useful to have this information being printed in non-debug mode to help tracking down potential bugs. Signed-off-by: André Martins <andre@cilium.io>
|
test-me-please |
christarazi
approved these changes
Apr 20, 2020
maintainer-s-little-helper
bot
moved this from Needs backport from master
to Backport pending to v1.7
in 1.7.3
maintainer-s-little-helper
bot
moved this from Needs backport from master
to Backport pending to v1.7
in 1.7.3
maintainer-s-little-helper
bot
moved this from Backport pending to v1.7
to Backport done to v1.7
in 1.7.3
maintainer-s-little-helper
bot
moved this from Backport pending to v1.7
to Backport done to v1.7
in 1.7.3
maintainer-s-little-helper
bot
moved this from Needs backport from master
to Backport pending to v1.6
in 1.6.9
maintainer-s-little-helper
bot
moved this from Needs backport from master
to Backport pending to v1.6
in 1.6.9
maintainer-s-little-helper
bot
moved this from Backport pending to v1.6
to Backport done to v1.6
in 1.6.9
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.
Please read per commit, the overall idea is that in CRD mode we don't map which node has a particular identity and without this mapping it becomes tricky knowing which identity a particular node should hold or release the reference of.