replace gets conflict error from watch cache

[deads2k]

I0222 06:34:25.223] node "node-v1-test" created
W0222 06:34:25.446] Error from server (Conflict): error when replacing "STDIN": Operation cannot be fulfilled on nodes "node-v1-test": the object has been modified; please apply your changes to the latest version and try again
W0222 06:34:25.453] !!! [0222 06:34:25] Call tree:
W0222 06:34:25.455] !!! [0222 06:34:25] 1: /go/src/k8s.io/kubernetes/hack/make-rules/test-cmd-util.sh:2853 run_pod_tests(...)
W0222 06:34:25.458] !!! [0222 06:34:25] 2: hack/make-rules/test-cmd.sh:142 runTests(...)
I0222 06:34:25.759] +++ [0222 06:34:25] Clean up complete

@kubernetes/sig-cli-api-reviews

[deads2k]

Found in https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/batch/pull-kubernetes-unit/19373/ on submit queue.

[ncdc]

I've seen this multiple times as well. It is coming from here:

kubernetes/hack/make-rules/test-cmd-util.sh

Lines 669 to 678 in 7a8c467

	kubectl replace -f - "${kube_flags[@]}" << __EOF__
	{
	"kind": "Node",
	"apiVersion": "v1",
	"metadata": {
	"name": "node-v1-test",
	"annotations": {"a":"b"}
	}
	}
	__EOF__

[ncdc]

I'm not sure what the expectation is with this particular part of the test suite. It creates a node, then immediately tries to run kubectl replace on it. Should it always succeed, regardless of conflict?

[smarterclayton]

kubectl replace is effectively a "stomp" (all PUTs are stomps from the perspective of the user). I don't think as a user I expect kubectl replace to fail on conflict up to some reasonable retry limit.

[ncdc]

I think this might be happening because the kubectl replace call is fighting with the attach/detach reconciler, which is doing a two way merge patch to update the node's status. I'm continuing to investigate.

[pwittrock]

@ncdc Assigning to you since you are investigating.

[pwittrock]

cc @ymqytw

[ncdc]

I've yet to reproduce this locally but I think my analysis is probably correct. kubectl replace does not currently retry on conflict. Do we think it should? cc @kubernetes/sig-cli-bugs @fabianofranz @soltysh @deads2k @liggitt

[deads2k]

kubectl replace does not currently retry on conflict. Do we think it should?

If you want an unconditional update (retry on conflict), then I think you want something different. Why don't we just create something different to replace on?

[liggitt]

from a client's perspective,

kubernetes/hack/make-rules/test-cmd-util.sh

Lines 669 to 678 in 7a8c467

	kubectl replace -f - "${kube_flags[@]}" << __EOF__
	{
	"kind": "Node",
	"apiVersion": "v1",
	"metadata": {
	"name": "node-v1-test",
	"annotations": {"a":"b"}
	}
	}
	__EOF__

is an unconditional replace (no resource version provided in the input), which is what makes surfacing that error really odd...

[ncdc]

There is --force but that does a delete followed by a create

[ncdc]

@liggitt I'm not clear on the code path that results in the conflict error surfacing. I tried looking at the rest code that handles updates but it wasn't the easiest to follow.

[liggitt]

relevant code is at https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go#L394-L440

[ncdc]

Yeah, that's where I was looking. I am confused about what happens w.r.t. resource version for the updated object (the one supplied to kubectl replace) if the input doesn't contain a resource version.

[liggitt]

@wojtek-t I think this is the issue:

https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/storage/cacher.go#L518-L535

The caching storage implements guaranteedupdate and uses the object in the watch cache as the current object

[deads2k]

The caching storage implements guaranteedupdate and uses the object in the watch cache as the current object

I think I'd expect it to do that, but on a conflict failure, I'd expect it to retry live. Seems like most of the time, the cache would save us a get.

[pwittrock]

@deads2k Can I mark this as non-release-blocker to reduce to noise for the release managers?

[ncdc]

@pwittrock I'd say yes

[marun]

This issue is likely related to a recent failure: https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/logs/ci-kubernetes-test-go/3796/

[ncdc]

Potential fix #43152

[ncdc]

At least for the replace issue

[liggitt]

after further investigation, this doesn't affect unconditional replace

kubectl turns unconditional replaces into conditional ones by pre-fetching the existing object and filling in a resource version. given that behavior of kubectl, the test script is vulnerable to legitimate version conflicts.

however, the watch cache issue still exposes clients to illegitimate version conflicts:

• a client that updated, then updated again with the RV returned from the first update should not see a conflict, but can

the referenced test-cmd test can flake for the following reasons:

• the resource changed between when kubectl fetched it and when it submitted the update (fixed in Retry kubectl test replace on conflict #43192 by retrying client-side)
• the version held by the watch cache happened to be stale when the update was done (fixed in etcd3 store: retry with live object on conflict if there was a suggestion #43152 by retrying server-side)

[liggitt]

reopening to track server-side issue, moving to 1.6.1

[liggitt]

still seen in kubectl test in https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/43489/pull-kubernetes-unit/23566/

[k8s-github-robot]

This Issue hasn't been active in 75 days. It will be closed in 14 days (Jun 20, 2017).

cc @deads2k @ncdc

You can add 'keep-open' label to prevent this from happening, or add a comment to keep it open another 90 days