Switch vendored etcd to v3.5.6 #114403

dims · 2022-12-11T00:33:33Z

We updated etcd server version to v3.5.6 in #114093

In this PR, we update the vendored dependency as well.

Related to #104007 as well, we got rid of one more redundant dependency form3tech-oss/jwt-go

Signed-off-by: Davanum Srinivas davanum@gmail.com

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

Signed-off-by: Davanum Srinivas <davanum@gmail.com>

k8s-ci-robot · 2022-12-11T00:33:41Z

@dims: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

dims · 2022-12-11T00:33:50Z

/sig architecture

dims · 2022-12-11T00:34:08Z

/priority important-soon

dims · 2022-12-11T00:34:19Z

/assign @liggitt

dims · 2022-12-11T00:37:14Z

ONE more down!!

@@ -1,6 +1,6 @@
 {
-	"directDependencies": 212,
-	"transitiveDependencies": 234,
-	"totalDependencies": 285,
+	"directDependencies": 211,
+	"transitiveDependencies": 233,
+	"totalDependencies": 284,
 	"maxDepthOfDependencies": 26
 }
\ No newline at end of file

dims · 2022-12-12T14:13:22Z

/assign @serathius

serathius · 2022-12-12T14:15:27Z

/lgtm
/approve

liggitt · 2022-12-12T14:37:34Z

vendor/go.etcd.io/etcd/client/v3/watch.go

+	return (strings.Compare(cancelReasonError.Error(), v3rpc.ErrGRPCInvalidAuthToken.Error()) == 0) ||
+		(strings.Compare(cancelReasonError.Error(), v3rpc.ErrGRPCAuthOldRevision.Error()) == 0)


this is stringifying the parameter error twice when it doesn't need to, and is stringifying constant API errors twice when they will never change

it's also unclear that exact string comparisons are correct for these... is there zero chance they will get wrapped by other errors that will add prefixes or suffixes? I'm surprised this isn't checking errors in a typed way

cc @ahrtr @serathius

this is stringifying the parameter error twice

Indeed there is no need to stringify the error twice. I just delivered a PR to improve this: etcd-io/etcd#14935

Thanks @liggitt for pointing this out.

it's also unclear that exact string comparisons are correct for these.

It's correct here. etcd defines a set of gRPC errors on server side. We need to translate the error to a string before responding to the client side.

The logic here is that we need to retry on some error types (ErrGRPCInvalidAuthToken and ErrGRPCAuthOldRevision) on client side. So we need to compare the error message. Note that we have strict error messages definition, there is no any wrap & unwrap.

etcd defines a set of gRPC errors on server side. We need to translate each error to a string before responding to the client side.

I'm really surprised the client is expected to compare error message strings to drive behavior. Isn't that fragile with skewed client and server versions?

I'm really surprised the client is expected to compare error message strings to drive behavior. Isn't that fragile with skewed client and server versions?

This is a valid concern. This should be a technical debt. We may consider to define client side codes in future.

I will kick off a discussion in the etcd community later, get you CCed.

FYI. etcd-io/etcd#14992

FYI. etcd-io/etcd#14992

@ahrtr will you be doing a v3.5.x release with that change?

@dims Yes, will do it for 3.5 today.

FYI. etcd-io/etcd#14995

liggitt · 2022-12-12T14:38:47Z

vendor/go.etcd.io/etcd/client/v3/watch.go

+				cancelReasonError := v3rpc.Error(errors.New(pbresp.CancelReason))
+				if shouldRetryWatch(cancelReasonError) {


this looks really weird... is this handling a canceled flow in the created case? is that correct?

even if that's correct, it looks like this is triggering even in non-error cases (pbresp.CancelReason == "") - it's always creating a new error, a new rpc error, and calling shouldRetryWatch (I also commented in that method that stringification is happening more than it should)

cc @ahrtr @serathius

See my comment above #114403 (comment)

if pbresp.CancelReason is empty, shouldn't we avoid all of this added code?

liggitt · 2022-12-12T14:41:23Z

/hold for a question on the etcd client change to the watch Created handling path

dims · 2022-12-12T16:50:22Z

/retest

ahrtr · 2022-12-12T21:16:07Z

/lgtm
/approve

Thanks @dims

k8s-ci-robot · 2022-12-12T21:16:12Z

@ahrtr: changing LGTM is restricted to collaborators

In response to this:

/lgtm
/approve

Thanks @dims

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2022-12-12T21:16:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahrtr, dims, serathius

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:

~~OWNERS~~ [dims]
~~LICENSES/OWNERS~~ [dims]
~~staging/src/k8s.io/apiextensions-apiserver/OWNERS~~ [dims]
~~staging/src/k8s.io/apiserver/OWNERS~~ [dims]
~~staging/src/k8s.io/cloud-provider/OWNERS~~ [dims]
~~staging/src/k8s.io/controller-manager/OWNERS~~ [dims]
~~staging/src/k8s.io/kube-aggregator/OWNERS~~ [dims]
~~staging/src/k8s.io/legacy-cloud-providers/OWNERS~~ [dims]
~~staging/src/k8s.io/pod-security-admission/OWNERS~~ [dims]
~~staging/src/k8s.io/sample-apiserver/OWNERS~~ [dims]
~~vendor/OWNERS~~ [dims]

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

alexzielenski · 2022-12-13T21:05:55Z

/remove-sig api-machinery

In order to fix etcd-io#12385, PR etcd-io#14322 introduced a change in which the client side may retry based on the error message returned from server side. This is not good, as it's too fragile and it's also changed the protocol between client and server. Please see the discussion in kubernetes/kubernetes#114403 Note: The issue etcd-io#12385 only happens when auth is enabled, and client side reuse the same client to watch. So we decided to rollback the change on 3.5, reasons: 1.K8s doesn't enable auth at all. It has no any impact on K8s. 2.It's very easy for client application to workaround the issue. The client just needs to create a new client each time before watching. Signed-off-by: Benjamin Wang <wachao@vmware.com>

liggitt · 2023-01-10T17:28:20Z

/test pull-kubernetes-dependencies
to run check added in #114952

k8s-ci-robot · 2023-01-10T17:33:27Z

@dims: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubernetes-dependencies	`472db10`	link	true	`/test pull-kubernetes-dependencies`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-ci-robot · 2023-01-12T05:51:23Z

@dims: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

In order to fix etcd-io#12385, PR etcd-io#14322 introduced a change in which the client side may retry based on the error message returned from server side. This is not good, as it's too fragile and it's also changed the protocol between client and server. Please see the discussion in kubernetes/kubernetes#114403 Note: The issue etcd-io#12385 only happens when auth is enabled, and client side reuse the same client to watch. So we decided to rollback the change on 3.5, reasons: 1.K8s doesn't enable auth at all. It has no any impact on K8s. 2.It's very easy for client application to workaround the issue. The client just needs to create a new client each time before watching. Signed-off-by: Benjamin Wang <wachao@vmware.com>

Switch vendored etcd to v3.5.6

472db10

Signed-off-by: Davanum Srinivas <davanum@gmail.com>

k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Dec 11, 2022

k8s-ci-robot added sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Dec 11, 2022

k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Dec 11, 2022

k8s-ci-robot assigned liggitt Dec 11, 2022

k8s-ci-robot requested review from andrewsykim and cheftako December 11, 2022 00:34

k8s-ci-robot assigned serathius Dec 12, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 12, 2022

liggitt reviewed Dec 12, 2022

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 12, 2022

ahrtr mentioned this pull request Dec 12, 2022

client: enhance the function shouldRetryWatch and added unit test etcd-io/etcd#14935

Merged

k8s-ci-robot removed the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Dec 13, 2022

ahrtr mentioned this pull request Dec 14, 2022

[3.5] clientv3: revert the client side change in 14547 etcd-io/etcd#14995

Merged

mitake mentioned this pull request Dec 31, 2022

Add a Code based member for cancel reason in WatchResponse etcd-io/etcd#15058

Open

liggitt mentioned this pull request Jan 9, 2023

upgrade etcd dependencies to v3.5.6 #114921

Closed

This was referenced Jan 9, 2023

add archived dependencies into unwanted-dependencies.json #114912

Merged

Umbrella issue: github archived dependencies (2023 Jan) #114942

Closed

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 12, 2023

dims closed this Jan 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch vendored etcd to v3.5.6 #114403

Switch vendored etcd to v3.5.6 #114403

dims commented Dec 11, 2022 •

edited

Loading

k8s-ci-robot commented Dec 11, 2022

dims commented Dec 11, 2022

dims commented Dec 11, 2022

dims commented Dec 11, 2022

dims commented Dec 11, 2022

dims commented Dec 12, 2022

serathius commented Dec 12, 2022

liggitt Dec 12, 2022

dims Dec 12, 2022

ahrtr Dec 12, 2022 •

edited

Loading

liggitt Dec 12, 2022

ahrtr Dec 12, 2022

ahrtr Dec 13, 2022

ahrtr Dec 14, 2022

dims Dec 14, 2022

ahrtr Dec 14, 2022

ahrtr Dec 14, 2022

liggitt Dec 12, 2022

dims Dec 12, 2022

ahrtr Dec 12, 2022

liggitt Dec 12, 2022

ahrtr Dec 12, 2022

liggitt commented Dec 12, 2022

dims commented Dec 12, 2022

ahrtr commented Dec 12, 2022

k8s-ci-robot commented Dec 12, 2022

k8s-ci-robot commented Dec 12, 2022

alexzielenski commented Dec 13, 2022

liggitt commented Jan 10, 2023

k8s-ci-robot commented Jan 10, 2023

k8s-ci-robot commented Jan 12, 2023

		return (strings.Compare(cancelReasonError.Error(), v3rpc.ErrGRPCInvalidAuthToken.Error()) == 0) \|\|
		(strings.Compare(cancelReasonError.Error(), v3rpc.ErrGRPCAuthOldRevision.Error()) == 0)

		cancelReasonError := v3rpc.Error(errors.New(pbresp.CancelReason))
		if shouldRetryWatch(cancelReasonError) {

Switch vendored etcd to v3.5.6 #114403

Switch vendored etcd to v3.5.6 #114403

Conversation

dims commented Dec 11, 2022 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Dec 11, 2022

dims commented Dec 11, 2022

dims commented Dec 11, 2022

dims commented Dec 11, 2022

dims commented Dec 11, 2022

dims commented Dec 12, 2022

serathius commented Dec 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahrtr Dec 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Dec 12, 2022

dims commented Dec 12, 2022

ahrtr commented Dec 12, 2022

k8s-ci-robot commented Dec 12, 2022

k8s-ci-robot commented Dec 12, 2022

alexzielenski commented Dec 13, 2022

liggitt commented Jan 10, 2023

k8s-ci-robot commented Jan 10, 2023

k8s-ci-robot commented Jan 12, 2023

dims commented Dec 11, 2022 •

edited

Loading

ahrtr Dec 12, 2022 •

edited

Loading