Project

General

Profile

Bug #11577

Fix htpdate pool and timeout

Added by intrigeri over 3 years ago. Updated over 3 years ago.

Status:
Resolved
Priority:
Normal
Assignee:
-
Category:
Time synchronization
Target version:
Start date:
07/18/2016
Due date:
% Done:

100%

Feature Branch:
bugfix/10494-fix-htpdate-pools
Type of work:
Code
Blueprint:
Starter:
Affected tool:

Related issues

Related to Tails - Bug #10494: Retry htpdate when it fails Rejected 07/17/2016

Associated revisions

Revision c4b04388 (diff)
Added by bertagaz over 3 years ago

Replace connect-timeout CURL option with max-time.

Refs: #10494, #11577

Revision c2a2f546
Added by intrigeri over 3 years ago

Merge remote-tracking branch 'origin/bugfix/10494-fix-htpdate-pools' into stable

Fix-committed: #11577

History

#1 Updated by intrigeri over 3 years ago

  • Related to Bug #10494: Retry htpdate when it fails added

#2 Updated by intrigeri over 3 years ago

  • Assignee changed from intrigeri to bertagaz
  • % Done changed from 50 to 60
  • QA Check changed from Ready for QA to Info Needed

It seems to me that the --max-time option might be more adequate than --connect-timeout, to reach the goal set in ce71966252f97c1c17bcef3d52d049c7ca6e122c. I mean what we want is to lower the time a call to curl can take, and we don't really care about what time the connect phase in particular takes, right?

Other than that: (static) code review passes!

#3 Updated by bertagaz over 3 years ago

  • Assignee changed from bertagaz to intrigeri

intrigeri wrote:

It seems to me that the --max-time option might be more adequate than --connect-timeout, to reach the goal set in ce71966252f97c1c17bcef3d52d049c7ca6e122c. I mean what we want is to lower the time a call to curl can take, and we don't really care about what time the connect phase in particular takes, right?

Well, I don't think it changes much, as if CURL fails in this case it's because it can't connect to the HTTP server. With max-time, I guess in theory CURL could take a bit more than 29 seconds to connect and miss the answer because it took a bit too much time to receive it, but I don't think in practice it is a real problem.

I've tested a bit this branch with --max-time and it doesn't seem to break the behavior anyway, so I've pushed c4b0438 (unmerged in other related branches though). Still this option is less tested than the connect-timeout one. It's still possible to revert the commit and reintroduce it for 2.6, if we prefer to test it a bit more. Note that this commit makes sense with #10494#note-47 as we'd have to use this option.

Now I think note #10494#note-42 is still a blocker for a merge: with 30 seconds CURL timeout, we only try for half the duration of the 'time has synced' try_for(). So either we bump this timeout to 1 minute, or we lower the try_for(), or we make htpdate retry only 1 time.

#4 Updated by intrigeri over 3 years ago

  • Assignee changed from intrigeri to bertagaz

Now I think note #10494#note-42 is still a blocker for a merge: with 30 seconds CURL timeout, we only try for half the duration of the 'time has synced' try_for(). So either we bump this timeout to 1 minute, or we lower the try_for(), or we make htpdate retry only 1 time.

I don't understand this part. As far as I understood the problem, we need to make sure that the test suite waits at least as long as htpdate can possibly run, and it seems that you're confirming this will be finally satisfied once we merge this branch, which is an improvement. What other condition do we need to satisfy, and why?

#5 Updated by bertagaz over 3 years ago

  • Assignee changed from bertagaz to intrigeri
  • QA Check changed from Info Needed to Ready for QA

intrigeri wrote:

I don't understand this part. As far as I understood the problem, we need to make sure that the test suite waits at least as long as htpdate can possibly run, and it seems that you're confirming this will be finally satisfied once we merge this branch, which is an improvement. What other condition do we need to satisfy, and why?

Well, I thought this try_for() were somehow describing the maximum amount of time an action could take. Seems I misunderstood it. It just feels a bit odd to wait for 5 minutes for something we know won't take more than a half of it. Anyway, your call.

#6 Updated by intrigeri over 3 years ago

  • Assignee changed from intrigeri to bertagaz
  • QA Check changed from Ready for QA to Dev Needed

Please reassign to me once the change is in upstream htp.git.

It just feels a bit odd to wait for 5 minutes for something we know won't take more than a half of it.

Potentially wasting a couple minutes test suite runtime here doesn't feel like a merge blocker to me. I bet there are more worthwhile places to work into optimizing test suite runtime. But whatever, if you want to work on this, feel free to, as long as this doesn't make it more fragile (in this case, the 2.5 minutes seems too simplistic, as the whole thing can take slightly more that, so it'll need a bit more guesswork and thought, which is precisely what IMO the problem at hand does not deserve).

#7 Updated by bertagaz over 3 years ago

  • Assignee changed from bertagaz to intrigeri
  • QA Check changed from Dev Needed to Ready for QA

intrigeri wrote:

Please reassign to me once the change is in upstream htp.git.

Ooops, seems I can't get to remind there's this other repo. Done.

It just feels a bit odd to wait for 5 minutes for something we know won't take more than a half of it.

Potentially wasting a couple minutes test suite runtime here doesn't feel like a merge blocker to me. I bet there are more worthwhile places to work into optimizing test suite runtime. But whatever, if you want to work on this, feel free to, as long as this doesn't make it more fragile (in this case, the 2.5 minutes seems too simplistic, as the whole thing can take slightly more that, so it'll need a bit more guesswork and thought, which is precisely what IMO the problem at hand does not deserve).

Fair enough, not a merge blocker then. Future work on #10494 can probably take care of that anyway.

#8 Updated by intrigeri over 3 years ago

  • Status changed from In Progress to 11
  • Assignee deleted (intrigeri)
  • % Done changed from 60 to 100
  • QA Check changed from Ready for QA to Pass

Merged into stable, thanks!

#9 Updated by intrigeri over 3 years ago

  • Status changed from 11 to Resolved

Also available in: Atom PDF