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

vendor: bump etcd v3.3.27 #41791

Merged
merged 1 commit into from
Jan 5, 2022
Merged

vendor: bump etcd v3.3.27 #41791

merged 1 commit into from
Jan 5, 2022

Conversation

kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented Dec 14, 2020

fixes #31182

Bump etcd to v3.3.27, which includes etcd-io/etcd#12552,
to fix #31182

Full diff: etcd-io/etcd@v3.3.25...v3.3.27

This is a draft, pending a 3.3.26 release (etcd-io/etcd#12698)

@kolyshkin kolyshkin marked this pull request as draft December 14, 2020 20:04
@joakim-tjernlund
Copy link

Ping?

@thaJeztah
Copy link
Member

I think we're waiting for the etcd maintainers to tag a new release; etcd-io/etcd#12552 (comment)

@kolyshkin
Copy link
Contributor Author

kolyshkin commented Feb 4, 2021

Updated to use the upstream repo as the PR is merged. No changes other than the line in vendor.conf (and a rebase)

@joakim-tjernlund
Copy link

CI still broken?

Comment on lines 18 to 19
"github.com/json-iterator/go"
"github.com/modern-go/reflect2"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it adds two new dependencies (that are currently not vendored), which looks to cause the CI failure

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@kolyshkin
Copy link
Contributor Author

Depending on how long it will take etcd-io/etcd#12698 to be fixed, maybe we should merge this as is.

@thaJeztah
Copy link
Member

Depending on how long it will take etcd-io/etcd#12698 to be fixed, maybe we should merge this as is

It was previously a CoreOS project; do you know if the maintainers are now at Red Hat? (if so, do you have some internal cages to rattle? 😇 😅).

@joakim-tjernlund
Copy link

This is not moving forward ...

@joakim-tjernlund
Copy link

Guys, this is a really old bug, was fixed many months ago. It also reflects badly on docker/moby as this bug makes life miserably for OTHER apps but docker and you don't care.
Please make sure to merge it into maintenance branches as well.

@thaJeztah
Copy link
Member

@joakim-tjernlund not helpful; might want to complain in the upstream etcd project instead

@kolyshkin kolyshkin marked this pull request as ready for review March 12, 2021 00:35
@kolyshkin
Copy link
Contributor Author

kolyshkin commented Mar 12, 2021

I think this can be merged as is now since

and the only downside is it's using non-tagged version of etcd

@kolyshkin
Copy link
Contributor Author

Backport to 20.10 should also be trivial (once this is merged).

@joakim-tjernlund
Copy link

@joakim-tjernlund not helpful; might want to complain in the upstream etcd project instead

I have complained to etcd, nothing happens.
You can only wait so long for upstream. You are bundling this pkg so it becomes your problem in the end.

@joakim-tjernlund
Copy link

Backport to 20.10 should also be trivial (once this is merged).

What etcd does 20.10.x use? 19.x is getting old and soon obsolete so 20.10 is the way forward.

@joakim-tjernlund
Copy link

Yet a release but without this fix ...

@joakim-tjernlund
Copy link

If you move to the etcd 3.4 branch you will get the fix ...
3.3 is apparently dead since long .

@joakim-tjernlund
Copy link

3.3.27 has been out for quite a while now ...

Bump etcd to v3.3.27, which includes etcd-io/etcd#12552,
to fix moby#31182

Full diff: etcd-io/etcd@v3.3.25...v3.3.27

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah changed the title vendor: bump etcd vendor: bump etcd v3.3.27 Jan 3, 2022
@thaJeztah thaJeztah added this to the 21.xx milestone Jan 3, 2022
@thaJeztah
Copy link
Member

Rebased and moved to v3.3.27

Copy link
Contributor Author

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (I'm the author so I can't technically LGTM it)

@kolyshkin
Copy link
Contributor Author

I guess this needs to be backported as well.

Copy link
Member

@tianon tianon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM; the test failure seems like a teardown race, not necessarily related?

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (although the last push was from me 😂)

@thaJeztah
Copy link
Member

I guess this needs to be backported as well.

I added the cherry-pick label, but it may need 2bef937 to be cherry-picked as well, which brings updated dependencies, so a rather large diff (perhaps we're able to reduce the diff)

@thaJeztah
Copy link
Member

I re-ran CI a couple of times to make sure the vendor check also passed, but it's a bit of a whack-a-mole with flaky tests in different Jenkins stages.

The remaining failure in the last run is TestFollowLogsHandleDecodeErr, which is known flaky, which will be addressed by #43105 once merged;

 === Failed
 === FAIL: daemon/logger/loggerutils TestFollowLogsHandleDecodeErr (0.01s)
 time="2022-01-04T23:38:45Z" level=error msg="Log reader notified that it must re-open log file, some log data may not be streamed to the client." file=/tmp/TestFollowLogsHandleDecodeErr497049835
     logfile_test.go:317: assertion failed: 5 (int) != 6 (dec.resetCount int)

I'll go ahead and bring this one in

@joakim-tjernlund
Copy link

I guess this needs to be backported as well.

Did this get backported ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

docker daemon locks files in /dev
4 participants