/status subresource and observed generation support #331
Conversation
ash2k
changed the title
| return func(event watch.Event) (bool, error) { | ||
| metaObj := event.Object.(meta_v1.Object) | ||
| if metaObj.GetNamespace() != namespace || metaObj.GetName() != name { | ||
| return false, nil | ||
| } | ||
| b := event.Object.(*smith_v1.Bundle) | ||
| for _, rv := range resourceVersions { |
| if err == nil { | ||
| if len(b.Finalizers) > 0 { | ||
| t.Logf("Removing finalizers from Bundle %q", bundle.Name) | ||
| b.Finalizers = nil |
There was a problem hiding this comment.
It's annoying to remove finalizers manually to make the namespace go away if the test exits abruptly.
| // (if the Bundle spec wasn't updated during the last update, the status should be kept with no changes) | ||
| // The better solution could be storing "ObservedGeneration" in the status to reflect | ||
| // whether the status is up-to-date with the spec or stale | ||
| st.bundle.Status.Conditions = []cond_v1.Condition{ |
There was a problem hiding this comment.
We are sending an update here to the resource's body (not /status) which, when subresource /status is enabled, ignores .status changes. So there is no point making any changes to .status - they are ignored. See https://kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definitions/#status-subresource
| {Type: smith_v1.BundleReady, Status: cond_v1.ConditionFalse}, | ||
| {Type: smith_v1.BundleError, Status: cond_v1.ConditionFalse}, | ||
| } | ||
| _, err := st.updateObjectsToDeleteStatus() |
There was a problem hiding this comment.
This is also pointless - .status changes are ignored now in this codepath.
| @@ -87,8 +87,6 @@ func TestFinalizerAdded(t *testing.T) { | |||
| updateBundle := bundleUpdate.GetObject().(*smith_v1.Bundle) | |||
| // Make sure that the "deleteResources" finalizer was added | |||
| assert.True(t, resources.HasFinalizer(updateBundle, bundlec.FinalizerDeleteResources)) | |||
|
|
|||
| tc.assertObjectsToBeDeleted(t, m1) | |||
There was a problem hiding this comment.
When finalizer is added, we don't touch .status anymore (see above). So no need to test it.
| } | ||
| } | ||
| return true, nil | ||
| return b.Status.ObservedGeneration >= b.Generation, nil |
There was a problem hiding this comment.
Just FYI: CRDs don't get Generation incremented when annotations/labels updated, see kubernetes/kubernetes#67428
I haven't checked if ResourceVersion get incremented in that case, but just something to keep in mind.
There was a problem hiding this comment.
Just FYI: CRDs don't get Generation incremented when annotations/labels updated, see kubernetes/kubernetes#67428
Yes, and that is what I would expect. See #325 and the corresponding PR. Just found these docs https://github.com/kubernetes/community/blob/master/contributors/design-proposals/api-machinery/customresources-subresources.md#status-behavior and kubernetes/kubernetes#45539
I haven't checked if ResourceVersion get incremented in that case, but just something to keep in mind.
Yes, it does. It changes when a mutating request touches /status or / (normal resource path). I tested =)
| {Type: smith_v1.BundleError, Status: cond_v1.ConditionFalse}, | ||
| } | ||
| _, err := st.updateObjectsToDeleteStatus() | ||
| err := st.updateBundle() |
There was a problem hiding this comment.
I'm pretty sure that status subresource allows removing finalizers.
I have recently found this code in Service Catalog https://github.com/kubernetes-incubator/service-catalog/blob/master/pkg/controller/controller_instance.go#L2420 and double checked that it actually works.
So unless CRD implementation is different, you shouldn't need to invoke this update separately from status update.
Also, there should only be one update request per processing, as we all learned the hard way.
There was a problem hiding this comment.
I'm pretty sure that status subresource allows removing finalizers.
But it should not. See the design doc and the issues I linked above.
So unless CRD implementation is different, you shouldn't need to invoke this update separately from status update.
When adding finalizers we don't perform bundle processing and hence do not update its status.
Also, there should only be one update request per processing, as we all learned the hard way.
Yes. That is how it was and this PR does not change this behaviour.
|
The second commit just splits a method into two to simplify it and make the linter happy, no functional changes. |
Fixes #323.
Fixes #102.
Need to merge #330 first.