ResettableRESTMapper to make it possible to reset wrapped mappers #105623

ash2k · 2021-10-12T08:36:35Z

What type of PR is this?

/kind cleanup
/sig api-machinery
/sig cli

What this PR does / why we need it:

Having a proper API to reset a REST mapper is useful:

As it is seen in this PR, garbage collector uses this functionality.
cli-util uses this functionality via reflection here (see also Bug: apply sets that include both CRD and CR fail when not using a caching RestMapper kubernetes-sigs/cli-utils#421 (comment)).

Currently those implementations are fragile as if a mapper is wrapped, it becomes impossible to reset it properly (i.e. reset the delegate).

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2021-10-12T08:36:38Z

@ash2k: The label(s) sig/apimachinery cannot be applied, because the repository doesn't have them.

In response to this:

What type of PR is this?

/kind cleanup
/sig apimachinery
/sig cli

What this PR does / why we need it:

Having a proper API to reset a REST mapper is useful:

As it is seen in this PR, garbage collector uses this functionality.

cli-util uses this functionality via reflection here (see also Bug: apply sets that include both CRD and CR fail when not using a caching RestMapper kubernetes-sigs/cli-utils#421 (comment)).

Currently those implementations are fragile as if a mapper is wrapped, it becomes impossible to reset it properly (i.e. reset the delegate).

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?
NONE
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

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.

fedebongio · 2021-10-12T20:12:29Z

/assign @yliaog
/triage accepted

staging/src/k8s.io/apimachinery/pkg/api/meta/lazy.go

ash2k · 2021-10-13T06:31:19Z

/retest

yliaog · 2021-10-13T16:52:45Z

/lgtm

ash2k · 2021-10-13T21:25:19Z

/assign @deads2k

ash2k · 2021-11-05T23:46:59Z

@deads2k PTAL

ash2k · 2021-11-07T01:10:11Z

@seans3 I wonder what can be done to get this merged before 1.23 freeze (November 16th). I'd like to use it in cli-utils instead of the reflection hack.

seans3

For SIG CLI:

/lgtm

yliaog · 2021-11-15T23:30:53Z

/approve

aojea · 2021-11-16T08:17:57Z

staging/src/k8s.io/apimachinery/pkg/api/meta/restmapper.go

@@ -519,3 +519,12 @@ func (m *DefaultRESTMapper) RESTMappings(gk schema.GroupKind, versions ...string
 	}
 	return mappings, nil
 }
+
+// MaybeResetRESTMapper calls Reset() on the mapper if it is a ResettableRESTMapper.
+func MaybeResetRESTMapper(mapper RESTMapper) bool {


naive question, it seems the bool output is never used , is it worth having it?

It's not used at the moment, but may be used in the future, including by users of this library. I'm more than happy to remove it if this will increase chances of the PR getting merged.

no no, I was curious only, wait for the maintainers opinion

let me ping @liggitt

My preference is to not have it.

My preference is to not have it.

Given timing, I'm willing to take the removal of this bool as a followup.

deads2k · 2021-11-16T22:07:41Z

/approve
/milestone v1.23

k8s-ci-robot · 2021-11-16T22:08:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ash2k, deads2k, seans3, yliaog

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:

~~pkg/controller/garbagecollector/OWNERS~~ [deads2k]
~~staging/src/k8s.io/apimachinery/pkg/OWNERS~~ [deads2k]
~~staging/src/k8s.io/cli-runtime/OWNERS~~ [deads2k,seans3]
~~staging/src/k8s.io/client-go/OWNERS~~ [deads2k,yliaog]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ash2k · 2021-11-16T22:45:30Z

@deads2k Thank you very much! I'll open a followup to remove the return value today (to be merged later).

k8s-ci-robot requested review from deads2k and lavalamp October 12, 2021 08:37

ash2k mentioned this pull request Oct 12, 2021

Bug: apply sets that include both CRD and CR fail when not using a caching RestMapper kubernetes-sigs/cli-utils#421

Closed

k8s-ci-robot assigned yliaog Oct 12, 2021

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Oct 12, 2021

yliaog reviewed Oct 12, 2021

View reviewed changes

staging/src/k8s.io/apimachinery/pkg/api/meta/lazy.go Outdated Show resolved Hide resolved

ash2k force-pushed the ash2k/resettable-rest-mapper branch from 71a1ae7 to 496de42 Compare October 13, 2021 05:15

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 13, 2021

ash2k requested a review from yliaog October 13, 2021 05:18

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 13, 2021

k8s-ci-robot assigned deads2k Oct 13, 2021

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 5, 2021

ResettableRESTMapper to make it possible to reset wrapped mappers

de4598d

ash2k force-pushed the ash2k/resettable-rest-mapper branch from 496de42 to de4598d Compare November 5, 2021 23:45

k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 5, 2021

seans3 approved these changes Nov 15, 2021

View reviewed changes

k8s-ci-robot assigned seans3 Nov 15, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 15, 2021

aojea reviewed Nov 16, 2021

View reviewed changes

k8s-ci-robot added this to the v1.23 milestone Nov 16, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 16, 2021

k8s-ci-robot merged commit 165b581 into kubernetes:master Nov 17, 2021

ash2k deleted the ash2k/resettable-rest-mapper branch November 17, 2021 01:52

ash2k mentioned this pull request Nov 17, 2021

Remove return value from MaybeResetRESTMapper() #106472

Merged

This was referenced Dec 24, 2021

Update dependencies to Kubernetes v1.23.1 kubernetes-sigs/cli-utils#497

Merged

Applier constructor and run: builder kubernetes-sigs/cli-utils#485

Merged

ash2k mentioned this pull request Jan 23, 2022

fix: remove reflection hack kubernetes-sigs/cli-utils#517

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ResettableRESTMapper to make it possible to reset wrapped mappers #105623

ResettableRESTMapper to make it possible to reset wrapped mappers #105623

ash2k commented Oct 12, 2021 •

edited

k8s-ci-robot commented Oct 12, 2021

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

fedebongio commented Oct 12, 2021

ash2k commented Oct 13, 2021

yliaog commented Oct 13, 2021

ash2k commented Oct 13, 2021

ash2k commented Nov 5, 2021

ash2k commented Nov 7, 2021

seans3 left a comment

yliaog commented Nov 15, 2021

aojea Nov 16, 2021

ash2k Nov 16, 2021

aojea Nov 16, 2021

aojea Nov 16, 2021

deads2k Nov 16, 2021

deads2k Nov 16, 2021

ash2k Nov 17, 2021

deads2k commented Nov 16, 2021

k8s-ci-robot commented Nov 16, 2021

ash2k commented Nov 16, 2021

ResettableRESTMapper to make it possible to reset wrapped mappers #105623

ResettableRESTMapper to make it possible to reset wrapped mappers #105623

Conversation

ash2k commented Oct 12, 2021 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Oct 12, 2021

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

fedebongio commented Oct 12, 2021

ash2k commented Oct 13, 2021

yliaog commented Oct 13, 2021

ash2k commented Oct 13, 2021

ash2k commented Nov 5, 2021

ash2k commented Nov 7, 2021

seans3 left a comment

Choose a reason for hiding this comment

yliaog commented Nov 15, 2021

aojea Nov 16, 2021

Choose a reason for hiding this comment

ash2k Nov 16, 2021

Choose a reason for hiding this comment

aojea Nov 16, 2021

Choose a reason for hiding this comment

aojea Nov 16, 2021

Choose a reason for hiding this comment

deads2k Nov 16, 2021

Choose a reason for hiding this comment

deads2k Nov 16, 2021

Choose a reason for hiding this comment

ash2k Nov 17, 2021

Choose a reason for hiding this comment

deads2k commented Nov 16, 2021

k8s-ci-robot commented Nov 16, 2021

ash2k commented Nov 16, 2021

ash2k commented Oct 12, 2021 •

edited