Fix Unstructured field accessor #48065

ironcladlou · 2017-06-26T14:12:59Z

Fix the Unstructured GetDeletionGracePeriodSeconds accessor which was
always returning nil regardless of the underlying stored value. The
field value always appearing nil prevents Custom Resource instances
from being deleted when garbage collection is enabled for CRs and
when DeletePropagationOrphan is used. More generally, this fix means that
delete-on-update now works for CR instances.

Add some test coverage for Unstructured metadata deserialization.

The Unstructured DeletionGracePeriodSeconds field marshals as a value
type from JSON and as a pointer type via SetDeletionGracePeriodSeconds.
The GetDeletionGracePeriodSeconds method now supports handling both
int64 and *int64 values so that either underlying value can be returned.

Add a reflection-based unit test which attempts to exercise all the
Object Get/Set methods for nil handling.

Registries backed by the generic Store's `Update` implementation support delete-on-update, which allows resources to be automatically deleted during an update provided:

* Garbage collection is enabled for the Store
* The resource being updated has no finalizers
* The resource being updated has a non-nil DeletionGracePeriodSeconds equal to 0

With this fix, Custom Resource instances now also support delete-on-update behavior under the same circumstances.

deads2k · 2017-06-26T14:15:12Z

staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured.go

@@ -136,8 +136,8 @@ func getNestedInt64(obj map[string]interface{}, fields ...string) int64 {
 }

 func getNestedInt64Pointer(obj map[string]interface{}, fields ...string) *int64 {
-	if str, ok := getNestedField(obj, fields...).(*int64); ok {
-		return str
+	if str, ok := getNestedField(obj, fields...).(int64); ok {


This would correctly handle reading null?

https://play.golang.org/p/LmyITU1mA5

liggitt · 2017-06-26T14:18:30Z

staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured.go

@@ -136,8 +136,8 @@ func getNestedInt64(obj map[string]interface{}, fields ...string) int64 {
 }

 func getNestedInt64Pointer(obj map[string]interface{}, fields ...string) *int64 {
-	if str, ok := getNestedField(obj, fields...).(*int64); ok {


this works as long as our number-preserving json unmarshaler is used (stdlib turns numbers into float64)

also, getNestedSlice and getNestedMap assume values are strings and silently drop non-strings... the names should at least be updated so callers know their assumptions

deads2k · 2017-06-26T14:19:52Z

staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured_test.go

+			}`,
+			verify: func(u Unstructured) {
+				assertEqual("test", u.GetName(), "name")
+				assertNil(u.GetDeletionGracePeriodSeconds(), "deletionGracePeriodSeconds")


ah, I see. ok.

liggitt · 2017-06-26T14:22:20Z

what does SetDeletionTimestamp() do? It looks like it stores a string in the unstructured object, which would mean GetDeletionTimestamp() would not be able to read a value set with SetDeletionTimestamp().

Should make sure that the getters/setters work together with both null and present values.

ironcladlou · 2017-06-26T14:23:27Z

@liggitt

what does SetDeletionTimestamp() do? It looks like it stores a string in the unstructured object, which would mean GetDeletionTimestamp() would not be able to read a value set with SetDeletionTimestamp().

Should make sure that the getters/setters work together with both null and present values.

Oh boy, good catch. Hrm.

ironcladlou · 2017-06-26T17:55:32Z

Turns out the problem runs a little deeper: DeletionGracePeriodSeconds is stored as a value type when the Unstructured data is marshalled from JSON, while a pointer type is stored via SetDeletionGracePeriodSeconds. Working on a fix for this as well as reflective unit test coverage for all the Object accessor methods.

ash2k · 2017-06-26T19:53:16Z

This is similar to #46683.
/sig api-machinery
/kind bug

ironcladlou · 2017-06-26T20:45:09Z

@ash2k

This is similar to #46683.

Thanks, had no idea about that test file. How should I proceed? Should I add coverage to the existing tests? TestUnstructuredGetters and TestUnstructuredSetters are setting and verifying internal state; TestUnstructuredMetadataFromJSON here could be folded into TestUnstructuredGetters to cover the deletionGracePeriodSeconds and generation fields.

TestAccessorMethods here is testing Set/Get opaquely, which is different than any existing test.

cc @deads2k @liggitt

ironcladlou · 2017-06-26T21:14:30Z

I've moved my test code over to the existing suite.

ash2k · 2017-06-26T21:41:17Z

@ironcladlou I have no idea why those tests are in two separate places - I've had the same question :)

ironcladlou · 2017-06-27T13:00:30Z

Looks like this integration test was inadvertently working around the grace period accessor bug. Fixed the test in d5d9c37dea97be5ef3bc2d6f41be6e98dc45ab90.

ironcladlou · 2017-06-27T17:14:16Z

@ash2k mind reviewing this?

ash2k · 2017-06-28T00:17:18Z

@ironcladlou wow, just read the comment from your last commit - this is the behaviour I was actually expecting when I initially wrote this test. See the comment from @deads2k #46439 (comment) - it implies that the final delete is necessary to actually remove the object after all finalizers have been cleared. And I thought it is something else (other controller?) that does this normally, but not during integration tests.

ash2k · 2017-06-28T00:39:22Z

@ironcladlou maybe it is better to remove the field when the pointer is nil? This will be consistent with how unmarshaling works - it does not create a field with nil when the value is not set but just omits it. Also that is how pointers in owner references work (see PR I mentioned above).

ironcladlou · 2017-06-28T15:00:29Z

@ash2k

@ironcladlou wow, just read the comment from your last commit - this is the behaviour I was actually expecting when I initially wrote this test. See the comment from @deads2k #46439 (comment) - it implies that the final delete is necessary to actually remove the object after all finalizers have been cleared. And I thought it is something else (other controller?) that does this normally, but not during integration tests.

In this case I think delete-on-update depends primarily on whether GC is enabled for a given type of registry. In the integration tests right now, GC is definitely enabled for the CRD Store in use. I'm not entirely sure whether GC was enabled during #46439; if GC was enabled, your extra delete would have worked around the accessor bug. If GC was disabled, your extra delete would have been necessary because delete-on-update would have also been implicitly disabled.

I could also be totally wrong and missing something 😁

/cc @deads2k

ironcladlou · 2017-06-28T15:15:18Z

@ash2k

@ironcladlou maybe it is better to remove the field when the pointer is nil? This will be consistent with how unmarshaling works - it does not create a field with nil when the value is not set but just omits it. Also that is how pointers in owner references work (see PR I mentioned above).

I agree with you, but I think we're already dealing with some internal inconsistency in that regard: SetOwnerReferences adheres to your proposed omission behavior, while SetInitializers, SetLabels, SetAnnotations, and SetDeletionTimestamp will retain keys and assign nil values.

If we all agree that nil value keys should be omitted internally, I can get SetDeletionGracePeriodSeconds in line with our desired behavior, and we can address all the other fields in a followup. I'd prefer keep the scope of this PR very narrow.

/cc @deads2k

deads2k · 2017-06-28T17:11:02Z

In this case I think delete-on-update depends primarily on whether GC is enabled for a given type of registry.

Hmmm... These have gone through numerous iterations and the original namespace code issued a second delete. I actually can't be sure it wasn't a side-effect of a bug. The rest of this code looks correct, so we'll merge note the bug fix.

@ironcladlou add a release-note for it in this pull please.

deads2k · 2017-06-28T17:15:25Z

I think this moves in the right direction. I'm open to changing the nil values for consistency with normal marshalling, but I see that as a followup (open an issue to track, please).

lgtm, squash up a bit for merge.

Fix the Unstructured GetDeletionGracePeriodSeconds accessor which was always returning nil regardless of the underlying stored value. The field value always appearing nil prevents Custom Resource instances from being deleted when garbage collection is enabled for CRs and when DeletePropagationOrphan is used. More generally, this fix means that delete-on-update now works for CR instances. Add some test coverage for Unstructured metadata deserialization. The Unstructured DeletionGracePeriodSeconds field marshals as a value type from JSON and as a pointer type via SetDeletionGracePeriodSeconds. The GetDeletionGracePeriodSeconds method now supports handling both int64 and *int64 values so that either underlying value can be returned. Add a reflection-based unit test which attempts to exercise all the Object Get/Set methods for nil handling.

deads2k · 2017-06-28T17:52:03Z

/lgtm
/approve no-issue

deads2k · 2017-06-28T17:52:09Z

/retest

k8s-github-robot · 2017-06-28T17:53:28Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: deads2k, ironcladlou
We suggest the following additional approver: lavalamp

Assign the PR to them by writing /assign @lavalamp in a comment when ready.

Associated issue requirement bypassed by: deads2k

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

pkg/OWNERS
~~staging/src/k8s.io/apiextensions-apiserver/OWNERS~~ [deads2k]
~~staging/src/k8s.io/apimachinery/pkg/OWNERS~~ [deads2k]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

ironcladlou · 2017-06-28T17:53:28Z

Split out #48211 to track refactoring nil handling for Object related fields.

k8s-github-robot · 2017-06-28T17:54:07Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: deads2k, ironcladlou
We suggest the following additional approver: lavalamp

Assign the PR to them by writing /assign @lavalamp in a comment when ready.

Associated issue requirement bypassed by: deads2k

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

pkg/OWNERS
~~staging/src/k8s.io/apiextensions-apiserver/OWNERS~~ [deads2k]
~~staging/src/k8s.io/apimachinery/pkg/OWNERS~~ [deads2k]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-06-28T17:54:38Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: deads2k, ironcladlou
We suggest the following additional approver: lavalamp

Assign the PR to them by writing /assign @lavalamp in a comment when ready.

Associated issue requirement bypassed by: deads2k

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

pkg/OWNERS
~~staging/src/k8s.io/apiextensions-apiserver/OWNERS~~ [deads2k]
~~staging/src/k8s.io/apimachinery/pkg/OWNERS~~ [deads2k]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-06-28T17:55:23Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: deads2k, ironcladlou
We suggest the following additional approver: lavalamp

Assign the PR to them by writing /assign @lavalamp in a comment when ready.

Associated issue requirement bypassed by: deads2k

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

pkg/OWNERS
~~staging/src/k8s.io/apiextensions-apiserver/OWNERS~~ [deads2k]
~~staging/src/k8s.io/apimachinery/pkg/OWNERS~~ [deads2k]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-06-28T17:56:05Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: deads2k, ironcladlou
We suggest the following additional approver: lavalamp

Assign the PR to them by writing /assign @lavalamp in a comment when ready.

Associated issue requirement bypassed by: deads2k

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

pkg/OWNERS
~~staging/src/k8s.io/apiextensions-apiserver/OWNERS~~ [deads2k]
~~staging/src/k8s.io/apimachinery/pkg/OWNERS~~ [deads2k]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-06-28T17:56:47Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: deads2k, ironcladlou
We suggest the following additional approver: lavalamp

Assign the PR to them by writing /assign @lavalamp in a comment when ready.

Associated issue requirement bypassed by: deads2k

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

pkg/OWNERS
~~staging/src/k8s.io/apiextensions-apiserver/OWNERS~~ [deads2k]
~~staging/src/k8s.io/apimachinery/pkg/OWNERS~~ [deads2k]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

deads2k · 2017-06-28T17:59:56Z

@k8s-bot its the old apimachinery tests. Stop freaking out.

deads2k · 2017-06-28T18:01:39Z

@ironcladlou that package of old tests exists because we had some tests which relied on kube API types. I think that @sttts gave us an alternative test group in apimachinery since, but I we haven't revisited it.

@shiywang if you're interested in un-cycling those and moving them into apimachinery, let me know.

k8s-github-robot · 2017-06-28T19:55:23Z

Automatic merge from submit-queue (batch tested with PRs 48183, 45611, 48065)

shiywang · 2017-06-29T01:31:38Z

@deads2k I would like to work on that. checking now : )

deads2k · 2017-06-29T11:42:43Z

@deads2k I would like to work on that. checking now : )

opened #48265

Automatic merge from submit-queue (batch tested with PRs 47700, 48464, 48502) Add a refreshing discovery client Introduce a discovery client (implementing `CachedDiscoveryInterface`) which caches discovery information in memory and which can be actively refreshed by the user. This implementation fetches from discovery upon refresh and could later be improved to maintain updates from a watch. Extracted from #47665 and #46000 to help reduce the scope of #48065. ```release-note NONE ```

@lavalamp

Automatic merge from submit-queue (batch tested with PRs 49538, 49708, 47665, 49750, 49528) Enable garbage collection of custom resources Enhance the garbage collector to periodically refresh the resources it monitors (via discovery) to enable custom resource definition GC (addressing #44507 and reverting #47432). This is a replacement for #46000. /cc @lavalamp @deads2k @sttts @caesarxuchao /ref #48065 ```release-note The garbage collector now supports custom APIs added via CustomeResourceDefinition or aggregated apiservers. Note that the garbage collector controller refreshes periodically, so there is a latency between when the API is added and when the garbage collector starts to manage it. ```

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 26, 2017

k8s-github-robot assigned janetkuo and liggitt Jun 26, 2017

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. labels Jun 26, 2017

ironcladlou mentioned this pull request Jun 26, 2017

Enable garbage collection of custom resources #47665

Merged

deads2k reviewed Jun 26, 2017

View reviewed changes

liggitt reviewed Jun 26, 2017

View reviewed changes

deads2k reviewed Jun 26, 2017

View reviewed changes

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 26, 2017

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. kind/bug Categorizes issue or PR as related to a bug. labels Jun 26, 2017

ironcladlou mentioned this pull request Jun 27, 2017

Add a refreshing discovery client #48148

Closed

k8s-ci-robot requested a review from deads2k June 28, 2017 15:00

ironcladlou mentioned this pull request Jun 28, 2017

unversioned.Time#UnmarshalJSON should leave time in UTC #21402

Closed

ironcladlou force-pushed the unstructured-field-fix branch from d5d9c37 to 4f91c4b Compare June 28, 2017 17:43

ironcladlou force-pushed the unstructured-field-fix branch from 4f91c4b to 547d820 Compare June 28, 2017 17:44

deads2k added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jun 28, 2017

k8s-ci-robot assigned deads2k Jun 28, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 28, 2017

ironcladlou mentioned this pull request Jun 28, 2017

Unstructured represents nil Object fields inconsistently #48211

Closed

deads2k added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 28, 2017

k8s-github-robot merged commit d0735b9 into kubernetes:master Jun 28, 2017

ironcladlou mentioned this pull request Jul 5, 2017

Add a refreshing discovery client #48502

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Unstructured field accessor #48065

Fix Unstructured field accessor #48065

ironcladlou commented Jun 26, 2017 •

edited

Loading

deads2k Jun 26, 2017

ironcladlou Jun 26, 2017

liggitt Jun 26, 2017

deads2k Jun 26, 2017

liggitt commented Jun 26, 2017

ironcladlou commented Jun 26, 2017

ironcladlou commented Jun 26, 2017

ash2k commented Jun 26, 2017

ironcladlou commented Jun 26, 2017

ironcladlou commented Jun 26, 2017

ash2k commented Jun 26, 2017

ironcladlou commented Jun 27, 2017

ironcladlou commented Jun 27, 2017

ash2k commented Jun 28, 2017

ash2k commented Jun 28, 2017

ironcladlou commented Jun 28, 2017 •

edited

Loading

ironcladlou commented Jun 28, 2017

deads2k commented Jun 28, 2017

deads2k commented Jun 28, 2017

deads2k commented Jun 28, 2017

deads2k commented Jun 28, 2017

k8s-github-robot commented Jun 28, 2017

ironcladlou commented Jun 28, 2017

k8s-github-robot commented Jun 28, 2017

k8s-github-robot commented Jun 28, 2017

k8s-github-robot commented Jun 28, 2017

k8s-github-robot commented Jun 28, 2017

k8s-github-robot commented Jun 28, 2017

deads2k commented Jun 28, 2017

deads2k commented Jun 28, 2017

k8s-github-robot commented Jun 28, 2017

shiywang commented Jun 29, 2017 •

edited

Loading

deads2k commented Jun 29, 2017

Fix Unstructured field accessor #48065

Fix Unstructured field accessor #48065

Conversation

ironcladlou commented Jun 26, 2017 • edited Loading

deads2k Jun 26, 2017

Choose a reason for hiding this comment

ironcladlou Jun 26, 2017

Choose a reason for hiding this comment

liggitt Jun 26, 2017

Choose a reason for hiding this comment

deads2k Jun 26, 2017

Choose a reason for hiding this comment

liggitt commented Jun 26, 2017

ironcladlou commented Jun 26, 2017

ironcladlou commented Jun 26, 2017

ash2k commented Jun 26, 2017

ironcladlou commented Jun 26, 2017

ironcladlou commented Jun 26, 2017

ash2k commented Jun 26, 2017

ironcladlou commented Jun 27, 2017

ironcladlou commented Jun 27, 2017

ash2k commented Jun 28, 2017

ash2k commented Jun 28, 2017

ironcladlou commented Jun 28, 2017 • edited Loading

ironcladlou commented Jun 28, 2017

deads2k commented Jun 28, 2017

deads2k commented Jun 28, 2017

deads2k commented Jun 28, 2017

deads2k commented Jun 28, 2017

k8s-github-robot commented Jun 28, 2017

ironcladlou commented Jun 28, 2017

k8s-github-robot commented Jun 28, 2017

k8s-github-robot commented Jun 28, 2017

k8s-github-robot commented Jun 28, 2017

k8s-github-robot commented Jun 28, 2017

k8s-github-robot commented Jun 28, 2017

deads2k commented Jun 28, 2017

deads2k commented Jun 28, 2017

k8s-github-robot commented Jun 28, 2017

shiywang commented Jun 29, 2017 • edited Loading

deads2k commented Jun 29, 2017

ironcladlou commented Jun 26, 2017 •

edited

Loading

ironcladlou commented Jun 28, 2017 •

edited

Loading

shiywang commented Jun 29, 2017 •

edited

Loading