object doesn't get updated after fakeClient.Delete(object) #3059
Comments
Nihileon
changed the title
|
I assume the regular client updates it? (I think I remember that the regular client also doesn't update it, but I'm not sure) |
|
Actually it looks like it doesn't: So after someone deletes an object, their in-mem representation doesn't reflect that? That is different from all other methods and very confusing |
|
Yeah. I think I noticed this 1-2 months ago in a unit test. First I thought the fake client is the problem but then I saw that the real client has the same behavior :) |
|
I can take this one if you all don't mind. /assign @troy0820 |
|
I'm wondering if there are comparable cases in client-go and how they are handled there |
|
/unassign @troy0820 I unfortunately do not have the time to commit to work through the PR for this. I am going to unassign myself for anyone who wants to take this. |
Do we want to change the real client behavior and then map that to the fakeClient? |
I think knowing ^^ would be good to inform our decision. Do we get the object with deletionTimestamp back when we do the actual delete call? |
I don't think we do unless I am reading this wrong on the dynamic client which uses the rest.Interface. func (c *dynamicResourceClient) Delete(ctx context.Context, name string, opts metav1.DeleteOptions, subresources ...string) error {
if len(name) == 0 {
return fmt.Errorf("name is required")
}
if err := validateNamespaceWithOptionalName(c.namespace, name); err != nil {
return err
}
result := c.client.client.
Delete().
AbsPath(append(c.makeURLSegments(name), subresources...)...).
Body(&opts).
Do(ctx)
return result.Error()
} |
|
Looks like it. Not sure if there either simply is nothing in the response body or if we just don't read it |
|
I think simply modifying - Error()
+ Into(obj)From my testing, If there's a finalizer on the resource then you will get a response including the timestamp, otherwise it gets deleted, and there's no deletion timestamp. |
I had this in a branch after I closed the first one but the behavior doesn't seem to mimic what the restClient would do at least to be used outside of fakeClient. If we start assuming the response body would be there instead of a 204 "No content" I think that would be dangerous and not compatible with upstream? If we need to simulate what the fake client would provide back as a timestamp to the object that gets persisted, that's a different story on how we can create that mechanism with what you describe. |
|
I had assumed that a But try... $ kubectl create configmap test-configmap
$ kubectl delete -v9 configmap/my-configYou'll see that we get a response body Recreating the configmap and editing it to have a finalizer causes the These indicate that we get the deletion timestamp back. Running this code https://gist.github.com/bigkevmcd/2be4e0835f6a865cf7a01c1af28a3680 also shows this. configMap := &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Namespace: "default",
Name: "my-config",
},
}
if err := k8sClient.Create(context.Background(), configMap); err != nil {
log.Fatal(err)
}
log.Printf("Deletion timestamp = %v\n", configMap.ObjectMeta.DeletionTimestamp)
controllerutil.AddFinalizer(configMap, "example.com/test")
if err := k8sClient.Update(context.Background(), configMap); err != nil {
log.Fatal(err)
}
log.Printf("Deletion timestamp = %v\n", configMap.ObjectMeta.DeletionTimestamp)
if err := k8sClient.Delete(context.Background(), configMap); err != nil {
log.Fatal(err)
}
log.Printf("Deletion timestamp = %v\n", configMap.ObjectMeta.DeletionTimestamp)
if err := k8sClient.Get(context.Background(), types.NamespacedName{Namespace: "default", Name: "my-config"}, configMap); err != nil {
log.Fatal(err)
}
log.Printf("Deletion timestamp = %v\n", configMap.ObjectMeta.DeletionTimestamp)
controllerutil.RemoveFinalizer(configMap, "example.com/test")
if err := k8sClient.Update(context.Background(), configMap); err != nil {
log.Fatal(err)
}
log.Printf("Deletion timestamp = %v\n", configMap.ObjectMeta.DeletionTimestamp)
if err := k8sClient.Get(context.Background(), types.NamespacedName{Namespace: "default", Name: "my-config"}, configMap); err != nil {
log.Fatal(err)
}
log.Printf("Deletion timestamp = %v\n", configMap.ObjectMeta.DeletionTimestamp)Outputs... With a modified version of the code with the change above proposed. I think this points to a bug in the real-client in that it doesn't update the state on |
|
I've fixed the first part of this (the real client behaviour, not the fake). I'll see what can be done for the fake client for the same behaviour. |
|
I had a look at the fake Client, and both now work in the same way. With the real client, the DeletionTimestamp is only populated when the server responds with a resource, which seems to only be when the resource exists after deletion i.e. a finalizer prevents removal. Switching out the real client for the fake Client in the code above yields the same output. |
fakeClient.Delete(obj) doesn't update deletionTimestamp of the obj passed to the function. The bug is in fakeClient.deleteObject, since it gets a new object and updates the new object without rewriting the original object.
Reference: https://github.com/kubernetes-sigs/controller-runtime/blob/v0.19.0/pkg/client/fake/client.go#L1085-L1095
The text was updated successfully, but these errors were encountered: