OCPBUGS-17157: pkg/controller: use a metadata watch for CRDs #3014
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@stevekuznetsov: This pull request references Jira Issue OCPBUGS-17157, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. 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 kubernetes/test-infra repository. |
| @@ -64,7 +64,7 @@ ifeq (, $(wildcard $(KUBEBUILDER_ASSETS)/kube-apiserver)) | |||
| endif | |||
|
|
|||
| cover.out: | |||
| go test $(MOD_FLAGS) -tags "json1" -v -race -coverprofile=cover.out -covermode=atomic \ | |||
There was a problem hiding this comment.
Not necessary but the DX of watching 10k lines of output scroll by on success makes it impossible to find test failures and should be an anti-pattern.
| @@ -1183,7 +1193,7 @@ func (a *Operator) handleClusterServiceVersionDeletion(obj interface{}) { | |||
| } | |||
|
|
|||
| for i, crdName := range desc.ConversionCRDs { | |||
| crd, err := a.lister.APIExtensionsV1().CustomResourceDefinitionLister().Get(crdName) | |||
| crd, err := a.opClient.ApiextensionsInterface().ApiextensionsV1().CustomResourceDefinitions().Get(context.TODO(), crdName, metav1.GetOptions{}) | |||
There was a problem hiding this comment.
This is the crux of the trade-off. In the rare case that a CSV is deleted, we incur server cost and latency to do a live GET for the data we need. Better than holding onto 20MiB of every CRD ever when we would only ever be concerned with a tiny subset, and with the spec of that tiny subset in an even smaller set of cases.
There was a problem hiding this comment.
But it's an improvement...
stevekuznetsov
force-pushed
the
skuznets/crd-metadata-watch
branch
from
dee5598 to
51f7ffd
Compare
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: stevekuznetsov, tmshort 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
|
/retest |
|
Let me dig into e2e, this passes locally for me ... :| |
stevekuznetsov
force-pushed
the
skuznets/crd-metadata-watch
branch
from
51f7ffd to
cc19c1b
Compare
stevekuznetsov
force-pushed
the
skuznets/crd-metadata-watch
branch
2 times, most recently
from
a5b9f39 to
3f8b4f1
Compare
|
/retest |
|
Reproducing the failure locally, looking into it. |
stevekuznetsov
force-pushed
the
skuznets/crd-metadata-watch
branch
from
3f8b4f1 to
0886a7a
Compare
Using a full LIST+WATCH is an optimization, with trade-offs. Holding the state of the world for CRDs in memory when we rarely, if ever, actually need to access them is a bad use of that trade-off, especially when the sum total size of CRDs on even the most basic cluster is O(20MiB). Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>
stevekuznetsov
force-pushed
the
skuznets/crd-metadata-watch
branch
from
0886a7a to
b8f147f
Compare
|
@tmshort should be good to go now |
|
/lgtm |
|
@stevekuznetsov: Jira Issue OCPBUGS-17157: Some pull requests linked via external trackers have merged:
The following pull requests linked via external trackers have not merged: 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 Issue OCPBUGS-17157 has not been moved to the MODIFIED state. 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 kubernetes/test-infra repository. |
Using a full LIST+WATCH is an optimization, with trade-offs. Holding the state of the world for CRDs in memory when we rarely, if ever, actually need to access them is a bad use of that trade-off, especially when the sum total size of CRDs on even the most basic cluster is O(20MiB).