etcd3/watcher: Event.Object should have the same rev as etcd delete #25396
Conversation
|
/cc @xiang90 @wojtek-t @timothysc It's a problem I found with test failure. After fixing it, the test failure never happen. |
| // which it gets deleted. Users use that revision to tell ordering. | ||
| rev = e.rev | ||
| } | ||
| oldObj, err = decodeObj(codec, versioner, getResp.Kvs[0].Value, rev) |
There was a problem hiding this comment.
this is strange... why shall we change the revision of the old object? for the watch issue, can you show me the code for etcd2? i would assume there are two objs in the event curObj and oldObj and e.Rev should be the rev of curObj. So the rev of the oldObj should not matter at all for ordering purpose. No?
There was a problem hiding this comment.
The etcd2 code is written in first comment too:
kubernetes/pkg/storage/etcd/etcd_watcher.go
Lines 437 to 442 in 995f022
watch.Event contains only one object.
So the rev of the oldObj should not matter at all for ordering purpose. No?
I have the same concern. The object is actually returned via Informer too.
There was a problem hiding this comment.
OK... This seems to be fragile...
There was a problem hiding this comment.
They use the event number to signal the last known resource version in the informer in case there is a watch break.
k8s-github-robot
added
size/M
|
LGTM |
xiang90
added
release-note-none
hongchaodeng
force-pushed
the
w
branch
2 times, most recently
from
5b8257e
to
f656dc6
Compare
k8s-github-robot
added
needs-ok-to-merge
and removed
lgtm
| @@ -123,7 +123,7 @@ func TestDeleteTriggerWatch(t *testing.T) { | |||
| if err := store.Delete(ctx, key, &api.Pod{}, nil); err != nil { | |||
| t.Fatalf("Delete failed: %v", err) | |||
| } | |||
| testCheckResult(t, 0, watch.Deleted, w, storedObj) | |||
| testCheckResult(t, 0, watch.Deleted, w, nil) | |||
There was a problem hiding this comment.
can we still check the expected storeObject? we probably need to change the rev of the storeObj? Or if we do not care about the triggered result, we should explicitly express that when calling the testCheckResult func.
instead of previous object's revision.
| @@ -227,14 +265,17 @@ func (c *testCodec) Decode(data []byte, defaults *unversioned.GroupVersionKind, | |||
| return nil, nil, errors.New("Expected decoding failure") | |||
| } | |||
|
|
|||
| func testCheckResult(t *testing.T, i int, expectEventType watch.EventType, w watch.Interface, expectObj *api.Pod) { | |||
| func testCheckResult(t *testing.T, i int, expectEventType watch.EventType, w watch.Interface, expectObj *api.Pod, ignoreResult bool) { | |||
There was a problem hiding this comment.
actually... how about adding another function called testExpectReceivingEvent, which only tries to receive event from the given chan? I dislike the bool arg.
hongchaodeng
force-pushed
the
w
branch
2 times, most recently
from
30f2a64
to
3592152
Compare
| @@ -227,14 +265,25 @@ func (c *testCodec) Decode(data []byte, defaults *unversioned.GroupVersionKind, | |||
| return nil, nil, errors.New("Expected decoding failure") | |||
| } | |||
|
|
|||
| func testCheckEventType(t *testing.T, expectEventType watch.EventType, w watch.Interface) { | |||
There was a problem hiding this comment.
@xiang90
Your suggestion sounds actually a good idea.
I have split another function to check event type specifically.
xiang90
added
the
lgtm
|
GCE e2e build/test passed for commit 30f2a64ca8386b5c9b1184d4b5c374fddc07cc2f. |
|
Automatic merge from submit-queue |
What's the problem?
When a delete is watched, the revision should be larger than any previous to guarantee ordering. However, currently etcd3 decodes the previous rev into returned object:
kubernetes/pkg/storage/etcd3/watcher.go
Line 322 in 995f022
This will break, for example, cacher's assumption here if it re-watch.
kubernetes/pkg/storage/cacher.go
Lines 579 to 581 in 995f022
The etcd2 impl. also takes the current ModifiedIndex to ensure it's a larger number:
kubernetes/pkg/storage/etcd/etcd_watcher.go
Lines 437 to 442 in 995f022
What's this PR?
It fixes above problem by using etcd's delete revision.