pkg/storage/etcd3: correctly validate resourceVersions

[stevekuznetsov]

pkg/storage/etcd3: correctly validate resourceVersions

In a number of tests, the underlying storage backend interaction will
return the revision (logical clock underpinning the MVCC implementation)
at the call-time of the RPC. Previously, the tests validated that this
returned revision was exactly equal to some previously seen revision.
This assertion is only true in systems where no other events are
advancing the logical clock. For instance, when using a single etcd
cluster as a shared fixture for these tests, the assertion is not valid
any longer. By checking that the returned revision is no older than the
previously seen revision, the validation logic is correct in all cases.

Signed-off-by: Steve Kuznetsov skuznets@redhat.com

---

Depends on #108936

/kind cleanup

NONE

/sig api-machinery
/assign @liggitt @smarterclayton @sttts @deads2k

[liggitt]

I'm unsure about this change ... it seems like something in the etcd3 store unit tests should be checking that the resourceVersion is stable in the absence of other changes. Isn't the unit test starting its own etcd instance? I'm not sure a shared etcd is a risk here

[stevekuznetsov]

@liggitt it certainly depends on what the point of the test is. If the test only passes on specific etcd fixtures, it's not really testing that the underling storage.Interface is providing the correct mechanisms as necessary for k8s to function. Given that, the test is less precise than it could be and does a bad job of documenting what, specifically, k8s expects from the underlying storage layer.

/retest

[stevekuznetsov]

it seems like something in the etcd3 store unit tests should be checking that the resourceVersion is stable in the absence of other changes

I don't think anything in k8s requires that this be the case. It's not an invariant that influences any interaction k8s has with the underlying store to my knowledge. If I'm mistaken here, I'm more than happy to write exhaustive tests to enforce this behavior from etcd.

Aside: if a client is sending linearized/quorum reads, those end up in the Raft log and bump the logical clock, so an etcd cluster which sees nothing but quorum reads will nevertheless have a higher revision even if "nothing" happened to mutate the kv store. A k8s API server will expose this higher revision in e.g. the resourceVersion on List calls.

[stevekuznetsov]

/retest

[fedebongio]

/triage accepted

[cici37]

/triage accepted

[cici37]

/triage accepted

[stevekuznetsov]

@liggitt @wojtek-t @smarterclayton, I would love to circle back to this PR. The long-and-short of it is that etcd and etcd itself decides when the internal revision has changed, and why. Among the actions that get committed to the Raft log and bump the revision are quorum reads, so even in cases where no state has been mutated, the revision may and will increase. Kubernetes exposes the raw current revision as the resourceVersion for List() calls. The tests today expect the resourceVersion on List() calls to exactly match a previous version. This is true if and only if the etcd cluster in question has no activities going on (or only non-quorum reads). This is, in my opinion:

• hyper-specific to the point of being misleading about what, specifically, Kubernetes expects from a conformant etcd cluster
• fragile in that it fails on just about every etcd fixture except for the particular ones used in these tests

By changing the tests to make the check that I've proposed, we gain readability, document the requirements on etcd better and make the tests less fragile.

[smarterclayton]

Would kine be better testable if this was fixed? Is there any real disadvantage to weakening this assumption (such as compaction / future etcd changes that lead to potentially background changes)?

[liggitt]

this is in my backlog but I won't be able to dig into it until next week at the earliest; consumed by 1.24 stuff atm

[stevekuznetsov]

@smarterclayton good questions!

Would kine be better testable if this was fixed?

Yes, but nothing here is specific to kine. This change makes it easier to test with a shared fixture in general, with no specific benefits to any individual RPC implementation.

Is there any real disadvantage to weakening this assumption ...

I would not necessarily frame this as a weakening of any assumption. This test fails today with any etcd fixture other than the one that is used in the unit tests. This test assumption does not apply to any non-toy etcd. This is why I labelled it as a misleading thing.

[stevekuznetsov]

@liggitt thanks for the confirmation :)

[stevekuznetsov]

@liggitt added a focused test for the linearized read invariant we spoke about.

[liggitt]

/lgtm
/approve
/milestone v1.25

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, stevekuznetsov

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:

• staging/src/k8s.io/apiserver/pkg/storage/OWNERS [liggitt]

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