controller/operators: label ConfigMaps, don't assume they are #3119
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
|
/approve |
openshift-ci
bot
added
the
approved
dinhxuanvu
reviewed
Nov 22, 2023
There was a problem hiding this comment.
First of all, the reason behind the label filter is to reduce the memory usage that can be a problem with OLM as it literally caches every configmap on the cluster which can be a lot. In practice, OLM doesn't really care much about configmaps except for the ones that it creates for bundle unpack operation.
As you pointed out, it is possible for older configmaps not to have that label. However, there are codes to handle that situation by adding the label to those configmaps in case of cache miss. I wonder if there is a bug somewhere that prevents that logic from working. Did you observe this issue recently?
Either way, I have no problems with this fix as long as the filtering will be applied later to prevent OLM from caching every configmap.
|
@dinhxuanvu yes, there must be a bug in the code to label on cache miss, as we're seeing this in the wild. |
ncdc
reviewed
Nov 27, 2023
In the past, OLM moved to using a label selector to filter the informers that track ConfigMaps in the cluster. However, when this was done, previous ConfigMaps on the cluster that already existed were not labelled. Therefore, on old clusters there is a mix of data - ConfigMaps that OLM created and managed but has now forgotten since they are missing labels, and conformant objects with the label. We use ConfigMaps to track whether or not Jobs should be labelled - if a Job has an OwnerReference to a ConfigMap and the ConfigMap has an OwnerReference to an OLM GVK, we know that the Job is created and managed by OLM. During runtime, the two-hop lookup described above is done by using a ConfigMap informer, so we're light on client calls during the labelling phase of startup. However, before the recent labelling work went in, the ConfigMap informer was *already* filtered by label, so our lookups were dead-ends for the few old ConfigMaps that had never gotten labels in the past. However, on startup we use live clients to determine if there are unlabelled objects we need to handle, so we end up in a state where the live lookup can detect the errant Jobs but the informer-based labellers can't see them as needing labels. This commit is technically a performance regression, as it reverts the unequivocal ConfigMap informer filtering - we see all ConfigMaps on the cluster during startup, but continue to filter as expected once everything has labels. Ideally, we can come up with some policies for cleanup of things like these Jobs and ConfigMaps in the future; at a minimum all of the OLM objects should be labelled and visible to the OLM operators from here on out. Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>
stevekuznetsov
force-pushed
the
skuznets/label-configmaps
branch
from
cf0c02c
to
a90ba66
Compare
ncdc
previously approved these changes
Nov 27, 2023
Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>
ncdc
previously approved these changes
Nov 29, 2023
stevekuznetsov
commented
Nov 29, 2023
stevekuznetsov
force-pushed
the
skuznets/label-configmaps
branch
from
7fbf8fd
to
db9172c
Compare
joelanford
reviewed
Nov 29, 2023
ConfigMaps provided for the internal source type are user-created and won't have our labels, so we need to use a live client to fetch them. Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>
stevekuznetsov
force-pushed
the
skuznets/label-configmaps
branch
from
db9172c
to
39202f3
Compare
Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>
ncdc
previously approved these changes
Nov 29, 2023
|
/lgtm |
Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>
ncdc
approved these changes
Nov 29, 2023
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: awgreene, ncdc, stevekuznetsov 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 |
|
/lgtm |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
the flakes are so horrendous now, holy crap. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
Our good friend |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
Wow! three! |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
Merged
via the queue into
operator-framework:master
with commit Nov 30, 2023
e2b3768
16 checks passed
|
wow! |
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.
In the past, OLM moved to using a label selector to filter the informers that track ConfigMaps in the cluster. However, when this was done, previous ConfigMaps on the cluster that already existed were not labelled. Therefore, on old clusters there is a mix of data - ConfigMaps that OLM created and managed but has now forgotten since they are missing labels, and conformant objects with the label.
We use ConfigMaps to track whether or not Jobs should be labelled - if a Job has an OwnerReference to a ConfigMap and the ConfigMap has an OwnerReference to an OLM GVK, we know that the Job is created and managed by OLM.
During runtime, the two-hop lookup described above is done by using a ConfigMap informer, so we're light on client calls during the labelling phase of startup. However, before the recent labelling work went in, the ConfigMap informer was already filtered by label, so our lookups were dead-ends for the few old ConfigMaps that had never gotten labels in the past. However, on startup we use live clients to determine if there are unlabelled objects we need to handle, so we end up in a state where the live lookup can detect the errant Jobs but the informer-based labellers can't see them as needing labels.
This commit is technically a performance regression, as it reverts the unequivocal ConfigMap informer filtering - we see all ConfigMaps on the cluster during startup, but continue to filter as expected once everything has labels.
Ideally, we can come up with some policies for cleanup of things like these Jobs and ConfigMaps in the future; at a minimum all of the OLM objects should be labelled and visible to the OLM operators from here on out.