The fake client should be able to cl.Update(...) an object after calling cl.Status().Update(...)

[berlin-ab]

Here's a Draft PR that tests the behavior:

#2486

This is using the main branch.

[troy0820]

/kind support

Wouldn't what you propose, make the object re-queue after the first update? Meaning, it wouldn't even get to the status update.

If you want to update the status, then you could update that first and not requeue by writing a predicate for that so it won't requeue on status updates. I'm trying to understand this case where it wouldn't requeue as an event for update to allow you to get to the status update.

[berlin-ab]

If you want to update the status, then you could update that first and not requeue by writing a predicate for that so it won't requeue on status updates. I'm trying to understand this case where it wouldn't requeue as an event for update to allow you to get to the status update.

While this is a good suggestion, I think we should still have the fake client match the behavior of the real client for ResourceVersion during cl.Status().Update().

[troy0820]

The behavior from my understanding doesn't allow this. If you update an object that changes the resource version of that object and trying to update it again (any client, status included) without getting the latest resource would result in a conflict. Patching doesn't do this but with update it respects the resource version as part of its check to allow it to go through. We have a check that errors if the resource version isn't the same to mimic this behavior of what the apiserver does.

@alvaroaleman thoughts?

[berlin-ab]

Using my example CR api.MyDatabase against Minikube and a real c-r client:

	t.Run("test multiple updates succeed", func(t *testing.T) {
		scheme := runtime.NewScheme()
		_ = client.AddMyCRDTo(scheme)
		
		ctx := context.Background()
		testClient, err := k8sclient.New(config.GetConfigOrDie(), k8sclient.Options{
			Scheme: scheme,
		})
		require.NoError(t, err)

		db := &api.MyDatabase{}
		db.SetName("my-name")
		db.SetNamespace("default")
		_ = testClient.Delete(ctx, db)

		db.Spec.Instances = ptr.To(1)
		err = testClient.Create(ctx, db)
		require.NoError(t, err)
		t.Logf("resource version: %v", db.ResourceVersion)

		db.Status.IsUp = ptr.To(true)
		err = testClient.Status().Update(ctx, db)
		require.NoError(t, err)
		t.Logf("resource version: %v", db.ResourceVersion)

		db.Spec.Instances = ptr.To(2)
		err = testClient.Update(ctx, db)
		require.NoError(t, err)
		t.Logf("resource version: %v", cluster.ResourceVersion)

		db.Status.IsUp = ptr.To(false)
		err = testClient.Status().Update(ctx, db)
		require.NoError(t, err)
		t.Logf("resource version: %v", db.ResourceVersion)
	})

These tests pass with this output:

=== RUN test_multiple_updates_succeed
    db_controller_test.go:47: resource version: 124223 // testClient.Create()
    db_controller_test.go:53: resource version: 124224 // testClient.Status().Update()
    db_controller_test.go:58: resource version: 124225 // testClient.Update()
    db_controller_test.go:64: resource version: 124226 // testClient.Status().Update()
PASS

[berlin-ab]

This behavior of the real client suggests that when Update() or Status().Update() are called, the server sends back the new ResourceVersion, and therefore the controller can call Update() or Status().Update() again without needing to re-Get the object.

[alvaroaleman]

Yeah, Berlin Ab is right, I introduced this bug yesterday. My bad and good catch. Will file a fix

[troy0820]

/remove-kind support
/kind bug
Very good catch, and thank you all for that explanation.