Skip to content

etcd: Fix session renewal controllers#12553

Merged
qmonnet merged 2 commits intomasterfrom
pr/tgraf/kvstore-session-renew
Jul 17, 2020
Merged

etcd: Fix session renewal controllers#12553
qmonnet merged 2 commits intomasterfrom
pr/tgraf/kvstore-session-renew

Conversation

@tgraf
Copy link
Copy Markdown
Contributor

@tgraf tgraf commented Jul 16, 2020

No description provided.

@tgraf tgraf added kind/bug This is a bug in the Cilium logic. needs-backport/1.7 labels Jul 16, 2020
@tgraf tgraf requested a review from a team as a code owner July 16, 2020 09:50
@maintainer-s-little-helper maintainer-s-little-helper Bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jul 16, 2020
@tgraf tgraf added the release-note/bug This PR fixes an issue in a previous release of Cilium. label Jul 16, 2020
@maintainer-s-little-helper maintainer-s-little-helper Bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jul 16, 2020
@coveralls
Copy link
Copy Markdown

coveralls commented Jul 16, 2020

Coverage Status

Coverage decreased (-0.02%) to 37.018% when pulling d1c0c40 on pr/tgraf/kvstore-session-renew into c56ee44 on master.

@tgraf
Copy link
Copy Markdown
Contributor Author

tgraf commented Jul 16, 2020

test-me-please

Comment thread pkg/kvstore/etcd.go Outdated
tgraf added 2 commits July 16, 2020 14:55
Due to creating a context with a timeout prior to blocking on the
session channel to wait for it to end, the timeout of the context will
likely already hvae expired by the time the kvstore operation is
performed. It is thus cancelled immediately, requiring a subsequent
controller call. This prolongs the time the session is renewed and
causes unnecessary controller failures which can be mistaken for etcd
issues.

Also pass the etcd client context to the controller to bind the
lifecycle of a controller run to it.

This fixes errors like these:
```
  kvstore-etcd-lock-session-renew   20s ago        10s ago      1       unable to renew etcd lock session: context deadline exceeded
```

Signed-off-by: Thomas Graf <thomas@cilium.io>
The controller functions could potentially block forever while waiting
on channels to close. Bind waiting on the channel and check of the etcd
version to the controller and etcd client context so these operations
get cancelled.

Signed-off-by: Thomas Graf <thomas@cilium.io>
@tgraf tgraf force-pushed the pr/tgraf/kvstore-session-renew branch from 35af682 to d1c0c40 Compare July 16, 2020 12:55
Comment thread pkg/kvstore/etcd.go
ec.controllers.UpdateController("kvstore-etcd-session-renew",
controller.ControllerParams{
// Stop controller function when etcd client is terminating
Context: ec.client.Ctx(),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🤯

@tgraf
Copy link
Copy Markdown
Contributor Author

tgraf commented Jul 17, 2020

test-me-please

@qmonnet
Copy link
Copy Markdown
Member

qmonnet commented Aug 3, 2020

Backport to v1.6 in #12748.

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

Labels

kind/bug This is a bug in the Cilium logic. release-note/bug This PR fixes an issue in a previous release of Cilium.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants