storage: ensure non-expired context before each liveness update attempt

[nvanbenschoten]

Fixes #25430.

Before this change, a liveness update could get stuck in an infinite
loop if its context expired. This is because it would continue to retry
and continue to get errRetryLiveness errors due to
AmbiguousResultErrors created by DistSender.

Release note: None

[cockroach-teamcity]

This change is 

[windchan7]

Do you think it makes more sense to move the check down to updateLivenessAttempt or not? Right now updateLiveness is the only caller of that function so it doesn't really matter much.

---

Review status: 0 of 1 files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[nvanbenschoten]

That's where I had it before, but I moved it up because updateLiveness is the function that's "aware" of the retry loop, so I think it makes more sense there. My reasoning was that we don't add these context cancellation checks at the beginning of every function, only in cases like this that need it.

[windchan7]

Great! Thanks for the explanation. LGTM!

[windchan7]

I'm also wondering why we are not replacing this endless for loop with a retry.XXX. That will save the hassle of checking if context has been cancelled or not at every place like this.

[nvanbenschoten]

There's always a debate between whether bounded loops help avoid problems or just mask issues (like this one) and make them harder to find. I think for now we should keep this as-is. The context cancellation should at-least bound the loop to some maximum duration.

[bdarnell]

---

Review status: 0 of 1 files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[bdarnell]

Should we backport this to 2.0? Is this just a test flake or could it explain some user issues like the import failures?

[nvanbenschoten]

This could be more serious than a test flake, so I think we should backport it. If the user issues you're referring to included failed node liveness heartbeat: context deadline exceeded loops then it could explain them.

[nvanbenschoten]

bors r+

[bdarnell]

If the user issues you're referring to included failed node liveness heartbeat: context deadline exceeded loops then it could explain them.

I don't have a specific issue in mind but I know we've seen some reports of liveness problems (especially in conjunction with IMPORT).

[craig]

Build succeeded

• GitHub CI (Cockroach)

[tbg]

This seems bad enough to backport.

On Thu, May 17, 2018, 10:56 PM craig[bot] ***@***.***> wrote: Merged #25631 <#25631>. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#25631 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AE135Ce9nMN_FfFGSshzAlTJ5JSujLzyks5tzePtgaJpZM4UDsK3> .

--

…

-- Tobias