Bug #10185

Fix tails-i2p's "i2p_has_bootstrapped" function in newer java/newer I2P versions

Added by kytv 2015-09-12 06:57:33 . Updated 2015-11-03 11:30:23 .

Status:
Resolved
Priority:
High
Assignee:
Category:
Target version:
Tails_1.7
Start date:
2015-09-12
Due date:
% Done:

100%

Feature Branch:
kytv/test/6306-tests-for-I2P
Type of work:
Code
Blueprint:

Starter:
Affected tool:
I2P
Deliverable for:
266

Description

Something has changed in either recent versions of Java or I2P. I suspect the change may have happened around the same time that netstat -4nlp stopped working and the command needed to be changed to netstat -nlp.

In short, the current script will often report prematurely that I2P is ready when it really is not.

I have fixed the problem in a local branch but am still testing it.

Subtasks

Related issues

Related to Tails - Bug #10461: Determine when I2P is ready for use Confirmed 2015-10-31
Blocks Tails - Feature #6306: Write tests for I2P Resolved 2013-09-26 2015-07-15

History

#1 Updated by kytv 2015-09-12 06:57:53

#2 Updated by intrigeri 2015-09-15 12:03:12

  • Subject changed from Fix @tails-i2p@'s @i2p_has_bootstrapped@ function in newer java/newer I2P versions to Fix tails-i2p's "i2p_has_bootstrapped" function in newer java/newer I2P versions

#3 Updated by intrigeri 2015-09-15 12:04:11

  • Priority changed from Normal to High

(Blocks a high-prio ticket.)

#4 Updated by intrigeri 2015-09-17 13:27:20

  • blocks #8668 added

#5 Updated by intrigeri 2015-09-17 13:27:45

  • Deliverable for set to 266

(Since it blocks Feature #6306.)

#6 Updated by kytv 2015-09-20 15:34:54

  • Target version changed from Tails_1.6 to Tails_1.7

#7 Updated by sajolida 2015-10-01 07:22:39

This was due on July 15, please make sure we can at least deliver in time for the next milestone on October 15.

#8 Updated by kytv 2015-10-05 14:15:43

  • Assignee changed from kytv to anonym
  • % Done changed from 0 to 30
  • QA Check set to Ready for QA
  • Feature Branch set to kytbugfix/10185-fix_i2p_start_script_and_bootstrap_checking_functionv:

This is mostly a workaround for an upstream bug.

The tests for Feature #6306 cannot be run without an iso built from this branch.

#9 Updated by anonym 2015-10-07 07:57:30

  • Feature Branch changed from kytbugfix/10185-fix_i2p_start_script_and_bootstrap_checking_functionv: to kytv/bugfix/10185-fix_i2p_start_script_and_bootstrap_checking_function

#10 Updated by anonym 2015-10-07 08:09:20

  • Assignee changed from anonym to kytv
  • QA Check changed from Ready for QA to Dev Needed

Review:

+    sleep ${I2P_TUNNEL_BUILD_TIMEOUT} # initial 3 minute wait
+    wait_until ${I2P_TUNNEL_BUILD_TIMEOUT} i2p_has_bootstrapped # potentially 3 additional minutes

… but is there a reason not to spam i2p_has_bootstrapped via wait_until instead? It’s not exactly expensive.

+    touch /run/i2p/i2p_has_bootstrapped
[...]
+        rm -f /run/i2p/i2p_has_bootstrapped

What’s the purpose of this file? It’s not used anywhere from what I can tell. Also it’s confusing to have it named exactly the same as a function imported to that script.

+        wait_and_see_if_i2p_fails_to_bootstrap && bootstrap_failure
         wait_until_i2p_has_bootstrapped || bootstrap_failure

This looks confusing, and I sort of don’t know what to expect. Could you perhaps change the names of the function you added? Or both of them? Perhaps unite them? Introduce if:s instead of &&/||? Something, please! :)

I’ve not tested this yet, but I know you have (extensively :)).

#11 Updated by anonym 2015-10-12 07:07:16

Also, have you tested how your changes behave when restarting i2p after a successful bootstrap? In particular, the static sleep of three minutes (already commented about above) seem like a bad idea, since re-bootstrapping ought to be quicker.

#12 Updated by kytv 2015-10-25 08:19:39

anonym wrote:
> Review:
>
> […]
>
> … but is there a reason not to spam i2p_has_bootstrapped via wait_until instead? It’s not exactly expensive.

I opted not to at the time because i2p_has_bootstrapped was reporting that I2P was ready because it was prematurely reporting success and I was attempting to go for robustness. I’ll try going back to using wait_until.

>
> […]
>
> What’s the purpose of this file? It’s not used anywhere from what I can tell. Also it’s confusing to have it named exactly the same as a function imported to that script.

It was used by Feature #6306 (unless using wait_until will work. TBD).

>
> […]
>
> This looks confusing, and I sort of don’t know what to expect. Could you perhaps change the names of the function you added? Or both of them? Perhaps unite them? Introduce if:s instead of &&/||? Something, please! :)

Sure.

>
> I’ve not tested this yet, but I know you have (extensively :)).

(Oh, indeed I have…)

#13 Updated by kytv 2015-10-26 03:04:50

  • Assignee changed from kytv to anonym
  • QA Check changed from Dev Needed to Ready for QA

#14 Updated by anonym 2015-10-26 07:01:32

  • Assignee changed from anonym to kytv
  • QA Check changed from Ready for QA to Dev Needed

Commits 5f82330 and dc8ab8d doesn’t really fix the problem I have with your approach (also, the latter commit does a lot more than just “Improve function name”). We’re still guaranteed, via i2p_fails_to_bootstrap(), to not even start checking if bootstrapping has succeeded until I2P_BOOTSTRAP_TIMEOUT seconds (currently 210) has passed.

To me it feels like we should do something smarter than i2p_fails_to_bootstrap(), which currently only waits for the failure mode. It seems we should wait for failure or success, and then act according to that outcome. Possibly we want a timeout that implies failure, but if we know the approximate amount of time i2p spends on trying to reseed, it should be a bit higher than that.

Any way, I guess the original problem still won’t be solved. It could be that the current longer-than-necessary wait we do in i2p_fails_to_bootstrap() is what makes i2p_has_bootstrapped() work “reliably” in your tests. Is there nothing else we can look for? Is nothing logged by I2P when it has built a suitable tunnel for the web proxy?

#15 Updated by kytv 2015-10-26 12:17:04

anonym wrote:

> To me it feels like we should do something smarter than i2p_fails_to_bootstrap(), which currently only waits for the failure mode. It seems we should wait for failure or success, and then act according to that outcome. Possibly we want a timeout that implies failure, but if we know the approximate amount of time i2p spends on trying to reseed, it should be a bit higher than that.

That’s basically what the 210 seconds is for. When I’ve sabotaged the network so I2P couldn’t possibly reseed/bootstrap (IIRC) it was no more than 180 seconds to try all of the hosts. I used the additional time as a buffer.

> Any way, I guess the original problem still won’t be solved. It could be that the current longer-than-necessary wait we do in i2p_fails_to_bootstrap() is what makes i2p_has_bootstrapped() work “reliably” in your tests. Is there nothing else we can look for? Is nothing logged by I2P when it has built a suitable tunnel for the web proxy?

The primary reason for these changes was to attempt to get some sort of “failsafe” within the test suite, to try to find a point at which testing can commence.

Unfortunately there’s nothing logged when the proxy’s ready. I’ll make a wishlist bug for that as that would make things a bit easier.

In the meantime I’ll try to come up with something better but what’s here should work—I’ve run the new i2p.feature a lot with success.

#16 Updated by mercedes508 2015-10-27 05:35:02

When testing Tails 1.7~rc1 I always had the “i2p is ready” even though I did disconnect the laptop just after getting the “i2p router console is ready” notification.

Is this related to that ticket? If not another ticket should be created.

#17 Updated by anonym 2015-10-27 05:38:08

kytv wrote:
> anonym wrote:
>
> > To me it feels like we should do something smarter than i2p_fails_to_bootstrap(), which currently only waits for the failure mode. It seems we should wait for failure or success, and then act according to that outcome. Possibly we want a timeout that implies failure, but if we know the approximate amount of time i2p spends on trying to reseed, it should be a bit higher than that.
>
> That’s basically what the 210 seconds is for. When I’ve sabotaged the network so I2P couldn’t possibly reseed/bootstrap (IIRC) it was no more than 180 seconds to try all of the hosts. I used the additional time as a buffer.

Yes, but since we run if i2p_fails_to_bootstrap; then ... we’ll always wait the full 210 seconds, even if I2P would magically bootstrap in much less than that, correct? Actually, perhaps it’s not so magical if I2P is restarted after a successful bootstrap, which I guess takes “much less” time.

Couldn’t we do something like this (untested):

--- a/config/chroot_local-includes/usr/local/lib/tails-shell-library/i2p.sh
+++ b/config/chroot_local-includes/usr/local/lib/tails-shell-library/i2p.sh
@@ -40,6 +40,16 @@ i2p_reseed_completed() {
     grep -q "Reseed complete" "${I2P_WRAPPER_LOG}"
 }

+i2p_reseed_status() {
+    if i2p_reseed_completed; then
+        echo success
+    elif i2p_reseed_failed; then
+        echo failure
+    elif i2p_reseed_started; then
+        echo running
+    fi
+}
+
 i2p_built_a_tunnel() {
     netstat -nlp | grep -qwF "$(i2p_eep_proxy_address)"
 }
diff --git a/config/chroot_local-includes/usr/local/sbin/tails-i2p b/config/chroot_local-includes/usr/local/sbin/tails-i2p
index 7c61664..f1316dd 100644
--- a/config/chroot_local-includes/usr/local/sbin/tails-i2p
+++ b/config/chroot_local-includes/usr/local/sbin/tails-i2p
@@ -44,8 +44,8 @@ wait_until_i2p_router_console_is_ready() {
     wait_until ${I2P_STARTUP_TIMEOUT} i2p_router_console_is_ready
 }

-i2p_fails_to_bootstrap() {
-    wait_until ${I2P_BOOTSTRAP_TIMEOUT} i2p_reseed_failed
+wait_until_i2p_has_bootstrapped() {
+    wait_until ${I2P_BOOTSTRAP_TIMEOUT} '[ "$(i2p_reseed_status)" = success ]'
 }

 notify_router_console_success() {
@@ -85,9 +85,7 @@ case "${1}" in
         service i2p start
         wait_until_i2p_router_console_is_ready || startup_failure
         notify_router_console_success
-        if i2p_fails_to_bootstrap; then
-            bootstrap_failure
-        fi
+        wait_until_i2p_has_bootstrapped || bootstrap_failure
         wait_until_i2p_builds_a_tunnel || bootstrap_failure
         notify_bootstrap_success
         ;;

If this doesn’t work, it seems that your “unnecessary” amount of waiting in i2p_fails_to_bootstrap() is the only thing that makes wait_until_i2p_builds_a_tunnel() “work” at this point, and then I don’t think your branch has improved the situation, really. It has just hidden the problem, and if it works it’s just coincidental. :/

> > Any way, I guess the original problem still won’t be solved. It could be that the current longer-than-necessary wait we do in i2p_fails_to_bootstrap() is what makes i2p_has_bootstrapped() work “reliably” in your tests. Is there nothing else we can look for? Is nothing logged by I2P when it has built a suitable tunnel for the web proxy?
>
> The primary reason for these changes was to attempt to get some sort of “failsafe” within the test suite, to try to find a point at which testing can commence.

Sure, I would just like to avoid the issue above at the same time.

> Unfortunately there’s nothing logged when the proxy’s ready. I’ll make a wishlist bug for that as that would make things a bit easier.

Excellent! Please also file a Wait ticket for us, referring to that wishlist bug.

> In the meantime I’ll try to come up with something better but what’s here should work—I’ve run the new i2p.feature a lot with success.

Ok. Please try, possibly by investigating my diff above. If no solution is found, I’m not sure what to do.

#18 Updated by kytv 2015-10-28 11:54:37

  • Assignee changed from kytv to anonym
  • QA Check changed from Dev Needed to Info Needed

anonym wrote:
> kytv wrote:

> > That’s basically what the 210 seconds is for. When I’ve sabotaged the network so I2P couldn’t possibly reseed/bootstrap (IIRC) it was no more than 180 seconds to try all of the hosts. I used the additional time as a buffer.
>
> Yes, but since we run if i2p_fails_to_bootstrap; then ... we’ll always wait the full 210 seconds, even if I2P would magically bootstrap in much less than that, correct? Actually, perhaps it’s not so magical if I2P is restarted after a successful bootstrap, which I guess takes “much less” time.

I think the real fix will really have to come from upstream.
>
> Couldn’t we do something like this (untested):

Looks nice, will test.

> […]
>
> If this doesn’t work, it seems that your “unnecessary” amount of waiting in i2p_fails_to_bootstrap() is the only thing that makes wait_until_i2p_builds_a_tunnel() “work” at this point, and then I don’t think your branch has improved the situation, really. It has just hidden the problem, and if it works it’s just coincidental. :/

I personally think it was the “unnecessary” waiting which was the “magic”.

> > Unfortunately there’s nothing logged when the proxy’s ready. I’ll make a wishlist bug for that as that would make things a bit easier.
>
> Excellent! Please also file a Wait ticket for us, referring to that wishlist bug.

I filed tickets upstream. I still need to create the ticket for us.

> > In the meantime I’ll try to come up with something better but what’s here should work—I’ve run the new i2p.feature a lot with success.
>
> Ok. Please try, possibly by investigating my diff above. If no solution is found, I’m not sure what to do.

The retries in the tests in Feature #6306 “workaround” this a bit so that even if the testing starts before I2P is ready the step (like loading the homepage or reconnecting to IRC) is retried.

Although this ticket (AFAICT) cannot be resolved for 1.7 because of the freeze, shall I re-work Feature #6306 to work with the rc1 ISO with changes that can be reverted once this is resolved/merged?

#19 Updated by anonym 2015-10-29 10:01:44

  • Assignee changed from anonym to kytv
  • QA Check changed from Info Needed to Dev Needed

kytv wrote:
> anonym wrote:
> > If this doesn’t work, it seems that your “unnecessary” amount of waiting in i2p_fails_to_bootstrap() is the only thing that makes wait_until_i2p_builds_a_tunnel() “work” at this point, and then I don’t think your branch has improved the situation, really. It has just hidden the problem, and if it works it’s just coincidental. :/
>
> I personally think it was the “unnecessary” waiting which was the “magic”.

Me too. Ok, so let’s add a static sleep() at the appropriate place for the appropriate amount of time, and file a ticket to remove it and use the proper fix, that is blocked by the “wait” ticket you will file for the upstream ticket you just filed. Ok?

> The retries in the tests in Feature #6306 “workaround” this a bit so that even if the testing starts before I2P is ready the step (like loading the homepage or reconnecting to IRC) is retried.
>
> Although this ticket (AFAICT) cannot be resolved for 1.7 because of the freeze, shall I re-work Feature #6306 to work with the rc1 ISO with changes that can be reverted once this is resolved/merged?

This is a bugfix, so it’s not affected by the feature freeze. Let’s do what I said above, ok? I haven’t looked at Feature #6303 yet, but “retrying” to get around this problem seems wrong. Users would hit that issue, and think stuff is broken.

#20 Updated by kytv 2015-10-29 10:14:56

anonym wrote:

> This is a bugfix, so it’s not affected by the feature freeze.

♥

> Let’s do what I said above, ok? I haven’t looked at Feature #6303 yet, but “retrying” to get around this problem seems wrong. Users would hit that issue, and think stuff is broken.

+1

#21 Updated by kytv 2015-10-31 11:41:03

  • related to Bug #10461: Determine when I2P is ready for use added

#22 Updated by kytv 2015-11-01 07:31:17

  • Assignee changed from kytv to anonym
  • QA Check changed from Dev Needed to Ready for QA
  • Feature Branch changed from kytv/bugfix/10185-fix_i2p_start_script_and_bootstrap_checking_function to kytv/test/6306-tests-for-I2P

#23 Updated by anonym 2015-11-02 14:35:08

  • Status changed from In Progress to Fix committed
  • % Done changed from 30 to 100

Applied in changeset commit:bdc1cc730715eca28928646025b0021fe191f5d5.

#24 Updated by anonym 2015-11-02 16:01:06

  • Assignee deleted (anonym)
  • QA Check changed from Ready for QA to Pass
  • Feature Branch changed from kytv/test/6306-tests-for-I2P to kytv/test/6306-tests-for-I2P

#25 Updated by anonym 2015-11-03 11:30:23

  • Status changed from Fix committed to Resolved