Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crd-handler: re-create stale CR storage on update #79114

Merged

Conversation

@roycaihw
Copy link
Member

commented Jun 17, 2019

What type of PR is this?
/kind bug

What this PR does / why we need it:
fix level-triggering storage recreation (#78913 (comment)) and a race (#79114 (comment))

Which issue(s) this PR fixes:

Fixes #78913

Does this PR introduce a user-facing change?:

The CRD handler now properly re-creates stale CR storage to reflect CRD update.

/sig api-machinery
/cc @sttts @jpbetz
cc @liggitt

@roycaihw

This comment has been minimized.

Copy link
Member Author

commented Jun 18, 2019

iiuc there is also a small chance that an update happens in between:

crdName := requestInfo.Resource + "." + requestInfo.APIGroup
crd, err := r.crdLister.Get(crdName)
if apierrors.IsNotFound(err) {
r.delegate.ServeHTTP(w, req)
return
}
if err != nil {
http.Error(w, err.Error(), http.StatusInternalServerError)
return
}
if !apiextensions.HasServedCRDVersion(crd, requestInfo.APIVersion) {
r.delegate.ServeHTTP(w, req)
return
}
// There is a small chance that a CRD is being served because NamesAccepted condition is true,
// but it becomes "unserved" because another names update leads to a conflict
// and EstablishingController wasn't fast enough to put the CRD into the Established condition.
// We accept this as the problem is small and self-healing.
if !apiextensions.IsCRDConditionTrue(crd, apiextensions.NamesAccepted) &&
!apiextensions.IsCRDConditionTrue(crd, apiextensions.Established) {
r.delegate.ServeHTTP(w, req)
return
}
terminating := apiextensions.IsCRDConditionTrue(crd, apiextensions.Terminating)
crdInfo, err := r.getOrCreateServingInfoFor(crd)

and the old crd info gets persisted in storageMap after the controller removes the cached storage

@roycaihw roycaihw force-pushed the roycaihw:crd-informer/delete-stale-strategy branch from 54bf512 to 4eda76e Jun 18, 2019
@roycaihw roycaihw force-pushed the roycaihw:crd-informer/delete-stale-strategy branch from 4eda76e to 9f30917 Jun 18, 2019
@k8s-ci-robot k8s-ci-robot added size/M and removed size/S labels Jun 18, 2019
@roycaihw roycaihw changed the title crd-handler: delete and re-create potentially stale CR storage crd-handler: re-create stale CR storage on update Jun 18, 2019
@spiffxp

This comment has been minimized.

Copy link
Member

commented Jun 19, 2019

/priority important-soon

@roycaihw

This comment has been minimized.

Copy link
Member Author

commented Jun 20, 2019

/assign @sttts

// removeStorage removes the cached storage with the given uid as key from the given storageMap. This function
// updates r.customStorage with the cleaned-up storageMap and tears down the old storage.
// NOTE: Caller MUST hold r.customStorageLock to write r.customStorage thread-safely.
func (r *crdHandler) removeStorage(storageMap crdStorageMap, uid types.UID) {

This comment has been minimized.

Copy link
@liggitt

liggitt Jun 25, 2019

Member

prefer func (r *crdHandler) removeStorage_locked(uid types.UID) { to indicate it must be locked, and look up the storage map inside the function

@roycaihw roycaihw force-pushed the roycaihw:crd-informer/delete-stale-strategy branch from 9f30917 to 48d86c6 Jun 25, 2019
@fejta-bot

This comment has been minimized.

Copy link

commented Jun 26, 2019

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

@roycaihw roycaihw force-pushed the roycaihw:crd-informer/delete-stale-strategy branch from 810833b to c452a7f Jun 26, 2019
@roycaihw

This comment has been minimized.

Copy link
Member Author

commented Jun 26, 2019

/remove-kind api-change

@liggitt updated. PTAL

@liggitt

This comment has been minimized.

Copy link
Member

commented Jun 26, 2019

/lgtm
/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, roycaihw

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 5cd9faf into kubernetes:master Jun 26, 2019
23 checks passed
23 checks passed
cla/linuxfoundation roycaihw authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
@BenTheElder

This comment has been minimized.

Copy link
Member

commented Jun 26, 2019

Thank you!

k8s-ci-robot added a commit that referenced this pull request Jul 1, 2019
…14-upstream-release-1.15

Automated cherry pick of #79114: crd-handler: level-trigger storage recreation and fix a race
@liggitt liggitt added this to the v1.16 milestone Aug 6, 2019
@roycaihw

This comment has been minimized.

Copy link
Member Author

commented Sep 17, 2019

/area custom-resources

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.