Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix incorrectly set resource version in List #64150

Merged
merged 1 commit into from May 23, 2018

Conversation

@wojtek-t
Copy link
Member

wojtek-t commented May 22, 2018

Fix : #64147

Fix incorrectly propagated ResourceVersion in ListRequests returning 0 items.
@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented May 22, 2018

[MILESTONENOTIFIER] Milestone Pull Request Labels Incomplete

@deads2k @liggitt @wojtek-t

Action required: This pull request requires label changes. If the required changes are not made within 3 days, the pull request will be moved out of the v1.11 milestone.

kind: Must specify exactly one of kind/bug, kind/cleanup or kind/feature.
priority: Must specify exactly one of priority/critical-urgent, priority/important-longterm or priority/important-soon.
sig owner: Must specify at least one label prefixed with sig/.

Help
if err != nil {
return err
}
if len(getResp.Kvs) == 0 {

This comment has been minimized.

@liggitt

liggitt May 22, 2018

Member

returning early with return s.versioner.UpdateList(listObj, uint64(getResp.Header.Revision), "") means a smaller diff and makes it clearer the exceptional case is handled

This comment has been minimized.

@wojtek-t

wojtek-t May 22, 2018

Author Member

I don't agree with this comment - our goal shouldn't be having as small diff as possible.
I really think that with this change the code is much more understandable.

This comment has been minimized.

@liggitt

liggitt May 22, 2018

Member

our goal shouldn't be having as small diff as possible

I assume we want to backport this... that was my thought about minimizing diff

This comment has been minimized.

@wojtek-t

wojtek-t May 22, 2018

Author Member

Yes - I think we want to backport it.
but, it won't generate conflict on 1.10 and 1.9 and I think it's more important to have this code cleaner than to avoid conflict in 1.8. WDYT?

This comment has been minimized.

@liggitt

liggitt May 22, 2018

Member

cleaner is in the eye of the beholder, but I don't feel that strongly

This comment has been minimized.

@wojtek-t

wojtek-t May 23, 2018

Author Member

Yes - that's true.
But if you don't feel very strongly about this, I would prefer leaving as is.

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented May 22, 2018

what did the v2 store do? would like to see a test for this against the etcd3, etcd2, and cacher store impls

@timothysc timothysc removed their request for review May 22, 2018

@wojtek-t wojtek-t force-pushed the wojtek-t:fix_list_resource_version branch from 872296b to 6b9383d May 22, 2018

@k8s-ci-robot k8s-ci-robot added size/L and removed size/M labels May 22, 2018

@wojtek-t

This comment has been minimized.

Copy link
Member Author

wojtek-t commented May 22, 2018

what did the v2 store do? would like to see a test for this against the etcd3, etcd2, and cacher store impls

I added tests.
In fact v2 store is also broken, but I don't really know how to fix it - for now added a TODO.

@liggitt - PTAL

@@ -347,6 +347,7 @@ func (h *etcdHelper) GetToList(ctx context.Context, key string, resourceVersion
metrics.RecordEtcdRequestLatency("get", getTypeName(listPtr), startTime)
if err != nil {
if etcdutil.IsEtcdNotFound(err) {
// TODO: ResourceVersion should be set here.

This comment has been minimized.

@liggitt

liggitt May 22, 2018

Member

in a not found case, the current index comes back with the error:

type Error struct {
Code int `json:"errorCode"`
Message string `json:"message"`
Cause string `json:"cause"`
Index uint64 `json:"index"`
}

$ curl http://localhost:2379/v2/keys/foo -v
*   Trying ::1...
* TCP_NODELAY set
* Connection failed
* connect to ::1 port 2379 failed: Connection refused
*   Trying 127.0.0.1...
* TCP_NODELAY set
* Connected to localhost (127.0.0.1) port 2379 (#0)
> GET /v2/keys/foo HTTP/1.1
> Host: localhost:2379
> User-Agent: curl/7.54.0
> Accept: */*
> 
< HTTP/1.1 404 Not Found
< Content-Type: application/json
< X-Etcd-Cluster-Id: cdf818194e3a8c32
< X-Etcd-Index: 11
< Date: Tue, 22 May 2018 15:08:02 GMT
< Content-Length: 70
< 
{"errorCode":100,"message":"Key not found","cause":"/foo","index":11}

note the index matches the X-Etcd-Index header, which is what is hoisted into response.Index in success cases:

func unmarshalSuccessfulKeysResponse(header http.Header, body []byte) (*Response, error) {
var res Response
err := codec.NewDecoderBytes(body, new(codec.JsonHandle)).Decode(&res)
if err != nil {
return nil, ErrInvalidJSON
}
if header.Get("X-Etcd-Index") != "" {
res.Index, err = strconv.ParseUint(header.Get("X-Etcd-Index"), 10, 64)
if err != nil {
return nil, err
}
}
res.ClusterID = header.Get("X-Etcd-Cluster-ID")
return &res, nil
}

This comment has been minimized.

@wojtek-t

wojtek-t May 22, 2018

Author Member

Maybe it's a test issue, but in the test that I added, I'm getting "response=nil" in this case...

This comment has been minimized.

@wojtek-t

wojtek-t May 22, 2018

Author Member

@liggitt - what etcd version you used in your experiment? Maybe that matters?

This comment has been minimized.

@wojtek-t

wojtek-t May 22, 2018

Author Member

Actually - this seem to be an issue of etcd client library:
https://github.com/kubernetes/kubernetes/blob/master/vendor/github.com/coreos/etcd/client/keys.go#L654

In case of status different than OK and Created (so 200 and 201), the returned response is nil.

This comment has been minimized.

@liggitt

liggitt May 22, 2018

Member

I'm getting "response=nil" in this case...

yes. the index is in the error object:

type Error struct {
Code int `json:"errorCode"`
Message string `json:"message"`
Cause string `json:"cause"`
Index uint64 `json:"index"`
}

This comment has been minimized.

@wojtek-t

wojtek-t May 22, 2018

Author Member

Aaah ok - thanks. It works now. PTAL

@wojtek-t wojtek-t force-pushed the wojtek-t:fix_list_resource_version branch from 6b9383d to 6a470d1 May 22, 2018

@wojtek-t

This comment has been minimized.

Copy link
Member Author

wojtek-t commented May 22, 2018

etcd v2 should also be fixed now. PTAL

@@ -347,7 +347,8 @@ func (h *etcdHelper) GetToList(ctx context.Context, key string, resourceVersion
metrics.RecordEtcdRequestLatency("get", getTypeName(listPtr), startTime)
if err != nil {
if etcdutil.IsEtcdNotFound(err) {
return nil
etcdErr := err.(etcd.Error)

This comment has been minimized.

@liggitt

liggitt May 22, 2018

Member

maybe be a little more fault tolerant... do a conditional cast, and only call UpdateList if the cast is successful and etcdErr.Index != 0?

This comment has been minimized.

@wojtek-t

wojtek-t May 23, 2018

Author Member

Fair enough - fixed.

@wojtek-t wojtek-t force-pushed the wojtek-t:fix_list_resource_version branch from 6a470d1 to a3578c8 May 23, 2018

@wojtek-t
Copy link
Member Author

wojtek-t left a comment

@liggitt - PTAL

@@ -347,7 +347,8 @@ func (h *etcdHelper) GetToList(ctx context.Context, key string, resourceVersion
metrics.RecordEtcdRequestLatency("get", getTypeName(listPtr), startTime)
if err != nil {
if etcdutil.IsEtcdNotFound(err) {
return nil
etcdErr := err.(etcd.Error)

This comment has been minimized.

@wojtek-t

wojtek-t May 23, 2018

Author Member

Fair enough - fixed.

if err != nil {
return err
}
if len(getResp.Kvs) == 0 {

This comment has been minimized.

@wojtek-t

wojtek-t May 23, 2018

Author Member

Yes - that's true.
But if you don't feel very strongly about this, I would prefer leaving as is.

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented May 23, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label May 23, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented May 23, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, wojtek-t

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:

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

@wojtek-t

This comment has been minimized.

Copy link
Member Author

wojtek-t commented May 23, 2018

/test pull-kubernetes-e2e-kops-aws

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented May 23, 2018

Automatic merge from submit-queue (batch tested with PRs 64102, 63303, 64150, 63841). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 9716f58 into kubernetes:master May 23, 2018

18 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation wojtek-t authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

k8s-github-robot pushed a commit that referenced this pull request May 29, 2018

Kubernetes Submit Queue
Merge pull request #64395 from wojtek-t/automated-cherry-pick-of-#641…
…50-upstream-release-1.10

Automatic merge from submit-queue.

Automated cherry pick of #64150 upstream release 1.10

Cherry pick of #64150 on release-1.10.
#64150 : Fix incorrectly set resource version in List
@jpbetz

This comment has been minimized.

Copy link
Contributor

jpbetz commented May 30, 2018

/lgtm

k8s-github-robot pushed a commit that referenced this pull request May 31, 2018

Kubernetes Submit Queue
Merge pull request #64397 from wojtek-t/automated-cherry-pick-of-#641…
…50-upstream-release-1.9

Automatic merge from submit-queue.

Automated cherry pick of #64150 upstream release 1.9

Cherry pick of #64150 on release-1.9.
#64150 : Fix incorrectly set resource version in List

@wojtek-t wojtek-t deleted the wojtek-t:fix_list_resource_version branch Jul 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.