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

v1.6 backports 2020-10-07 #13440

Merged
merged 2 commits into from
Oct 7, 2020
Merged

v1.6 backports 2020-10-07 #13440

merged 2 commits into from
Oct 7, 2020

Conversation

qmonnet
Copy link
Member

@qmonnet qmonnet commented Oct 7, 2020

Once this PR is merged, you can update the PR labels via:

$ for pr in 13357 13408; do contrib/backporting/set-labels.py $pr done 1.6; done

joestringer and others added 2 commits October 7, 2020 11:43
[ upstream commit b71cf0d ]

Due to an extra `v` in the branch name, this script would fail with:

  $ ~/git/cilium/contrib/release/start-release.sh v1.6.12 128
  fatal: 'origin/vv1.6' is not a commit and a branch 'pr/prepare-v1.6.12' cannot be created from it

  Signal ERR caught!

  Traceback (line function script):
  62 main /home/joe/git/cilium/contrib/release/start-release.sh

Fix it.

While we're at it, update the instructions at the end for next
steps, since there's also now a `submit-backport.sh` script to send the
PR from the CLI.

Signed-off-by: Joe Stringer <joe@cilium.io>
Signed-off-by: Quentin Monnet <quentin@isovalent.com>
[ upstream commit 4a66961 ]

It has been observed that the "sessionSuccess <- true" statement can
block forever. E.g. Cilium v1.7.9:

    goroutine 742 [chan send, 547 minutes]:
    github.com/cilium/cilium/pkg/kvstore.(*etcdClient).renewLockSession(0xc000f66000, 0x2790b40, 0xc000e247c0, 0x0, 0x0)
	    /go/src/github.com/cilium/cilium/pkg/kvstore/etcd.go:657 +0x3e2
    github.com/cilium/cilium/pkg/kvstore.connectEtcdClient.func6(0x2790b40, 0xc000e247c0, 0x3aae820, 0x2)
	    /go/src/github.com/cilium/cilium/pkg/kvstore/etcd.go:819 +0x3e
    github.com/cilium/cilium/pkg/controller.(*Controller).runController(0xc000f42500)
	    /go/src/github.com/cilium/cilium/pkg/controller/controller.go:205 +0xa2a
    created by github.com/cilium/cilium/pkg/controller.(*Manager).updateController
	    /go/src/github.com/cilium/cilium/pkg/controller/manager.go:120 +0xb09

This can happen when the context cancellation timer fires after a
function which consumes the context has returned, but before
"sessionSuccess <- true" is executed. After the timeout, the receiving
goroutine is closed, making the sending block forever.

Fix this by making the sessionSuccess channel buffered. Note that after
sending we don't check whether the context has been cancelled, as we
expect that any subsequent functions which consume the context will
check for the cancellation.

Fixes: 0262854 ("etcd: Fix incorrect context usage in session renewal")
Signed-off-by: Martynas Pumputis <m@lambda.lt>
Signed-off-by: Quentin Monnet <quentin@isovalent.com>
@qmonnet qmonnet requested a review from a team as a code owner October 7, 2020 10:45
@qmonnet qmonnet added backport/1.6 kind/backports This PR provides functionality previously merged into master. labels Oct 7, 2020
@qmonnet
Copy link
Member Author

qmonnet commented Oct 7, 2020

test-backport-1.6

Copy link
Member

@brb brb left a comment

Choose a reason for hiding this comment

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

LGTM for my changes, thanks.

@gandro gandro merged commit 415a7f0 into v1.6 Oct 7, 2020
@gandro gandro deleted the pr/v1.6-backport-2020-10-07 branch October 7, 2020 18:12
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

LGTM btw.

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Oct 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/backports This PR provides functionality previously merged into master. ready-to-merge This PR has passed all tests and received consensus from code owners to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants