Bug 1023503 - Port RHEL 6.5 PPPoE reconnect delay to git master
Port RHEL 6.5 PPPoE reconnect delay to git master
Status: CLOSED CURRENTRELEASE
Product: Red Hat Enterprise Linux 7
Classification: Red Hat
Component: NetworkManager (Show other bugs)
7.0
Unspecified Unspecified
unspecified Severity unspecified
: rc
: ---
Assigned To: Jirka Klimes
Desktop QE
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2013-10-25 10:48 EDT by Dan Williams
Modified: 2014-06-17 22:48 EDT (History)
6 users (show)

See Also:
Fixed In Version: NetworkManager-0.9.9.0-28.git20131212.el7
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2014-06-13 06:18:18 EDT
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
rh602265-pppoe-reconnect-delay.patch (4.99 KB, patch)
2013-10-25 10:49 EDT, Dan Williams
no flags Details | Diff
[PATCH] ethernet: add reconnect delay for PPPoE connections (for master) (5.06 KB, patch)
2013-12-09 04:15 EST, Jirka Klimes
no flags Details | Diff
[PATCH v2] ethernet: add reconnect delay for PPPoE connections (rh #602265) (5.28 KB, patch)
2013-12-11 06:47 EST, Jirka Klimes
no flags Details | Diff
[PATCH] fixup! ethernet: add reconnect delay for PPPoE connections (rh #602265) (7.21 KB, patch)
2013-12-11 07:14 EST, Thomas Haller
no flags Details | Diff

  None (edit)
Description Dan Williams 2013-10-25 10:48:25 EDT
We did a patch for 6.5 to add a reconnect delay when auto-reconnecting a PPPoE connection, becuase often the PPP peer on the other end of the link needs a couple seconds to notice that the local machine has terminated the connection and to get ready to reconnect.  For some details on that, see bug #602265.

The patch introduces a short delay during stage1 of device activation if the PPPoE was run on this device within the last 7 seconds.  This allows the peer time to clean up from the connection and get ready for the reconnect.
Comment 1 Dan Williams 2013-10-25 10:49:22 EDT
Created attachment 816194 [details]
rh602265-pppoe-reconnect-delay.patch
Comment 3 Jirka Klimes 2013-12-09 04:15:47 EST
Created attachment 834237 [details]
[PATCH]  ethernet: add reconnect delay for PPPoE connections (for master)

Previous patch rebased to upstream master. (You can also see it in jk/rh1023503-pppoe).
Comment 4 Dan Williams 2013-12-09 10:03:44 EST
Looks good to me; I can test it tomorrow, but if you want to test it out yourself, you can install the rp-pppoe package on a different machine on the same LAN and then follow these directions:

http://darmawan-salihun.blogspot.com/2008/12/setting-up-basic-pppoe-server-in-linux.html

starting at the "Let's proceed to configure the machines one by one." part, and stopping at the "For the pppoe client machine, the important configuration files are" part since NetworkManager handles that.  Then you can run the pppoe server with a command like:

pppoe-server -k -I eth0 -F

and then let NetworkManager attempt to connect.  As long as the two machines are on the same LAN, the client will broadcast for a PPPoE server, the server will reply, and they will negotiate the connection.  Then you could do something like "nmcli dev eth0 disconnect; nmcli con up my-pppoe-test" and make sure you see the log messages about waiting.
Comment 5 Thomas Haller 2013-12-10 08:01:46 EST
(In reply to Jirka Klimes from comment #3)
> Created attachment 834237 [details]
> [PATCH]  ethernet: add reconnect delay for PPPoE connections (for master)
> 
> Previous patch rebased to upstream master. (You can also see it in
> jk/rh1023503-pppoe).

I think, using time() is wrong, because it's affected by daylight saving time and resetting of the clock. I'd add a function nm_utils_get_monotonic_timestamp_ns(), that should be used whenever calculating time offsets.

See th/review/jk/rh1023503-pppoe. It includes two minor fixups.
Comment 6 Thomas Haller 2013-12-10 08:04:26 EST
(In reply to Thomas Haller from comment #5)

> I think, using time() is wrong, because it's affected by daylight saving
> time and resetting of the clock.

aehh..., partly wrong. time() returns UTC, so no problem with DST.
Comment 7 Dan Williams 2013-12-10 12:00:16 EST
(In reply to Thomas Haller from comment #6)
> (In reply to Thomas Haller from comment #5)
> 
> > I think, using time() is wrong, because it's affected by daylight saving
> > time and resetting of the clock.
> 
> aehh..., partly wrong. time() returns UTC, so no problem with DST.

Yeah, it might be wrong if the system suspends; so technically it should be something like clock_gettime(CLOCK_MONOTONIC).  Though in this case it shouldn't be too large of an issue since it's a very short timeout, and the systems that are running PPPoE here won't usually be suspended.  I'm fine with either option: (a) change to use clock_gettime() or (b) leave as-is for now.
Comment 8 Jirka Klimes 2013-12-11 06:43:12 EST
(In reply to Dan Williams from comment #4)
> Looks good to me; I can test it tomorrow, but if you want to test it out
> yourself, you can install the rp-pppoe package on a different machine on the
> same LAN and then follow these directions:
> 
> http://darmawan-salihun.blogspot.com/2008/12/setting-up-basic-pppoe-server-
> in-linux.html
> 
Dan, thanks for the testing instructions. It helped to set the testing environment a lot.

I've found that there was a problem setting last_pppoe_time in device_state_changed(), because in that function activation request is already NULL and thus device_get_setting (dev, NM_TYPE_SETTING_PPPOE) returns NULL. So I moved setting last_pppoe_time to deactivate(), which works good, because it is called *before* clear_act_request() in device.c:nm_device_deactivate().

Also, I adapted the delay in g_timeout_add_seconds() to be the difference between now and the last time, because we don't probably want to wait the whole PPPOE_RECONNECT_DELAY interval, but only the remaining time.

As for time changes Thomas proposes, I would prefer not to do changes for this patch. Rather we could adapt timestamp handling in a separate branch for whole NM if we decide to go that way.
Comment 9 Jirka Klimes 2013-12-11 06:47:37 EST
Created attachment 835238 [details]
[PATCH v2] ethernet: add reconnect delay for PPPoE connections (rh #602265)

Version 2 of the previous patch. Changes:
* setting last_pppoe_time in deactivate()
* waiting only remaining time of PPPOE_RECONNECT_DELAY

The updated patch is also at upstream branch jk/rh1023503-pppoe.
Comment 10 Jirka Klimes 2013-12-11 06:55:01 EST
Updated commit message in the branch to reference bug 1023503 rather than 602265.
Comment 11 Thomas Haller 2013-12-11 07:14:20 EST
Created attachment 835245 [details]
[PATCH] fixup! ethernet: add reconnect delay for PPPoE connections (rh #602265)

(In reply to Jirka Klimes from comment #8)

> As for time changes Thomas proposes, I would prefer not to do changes for
> this patch. Rather we could adapt timestamp handling in a separate branch
> for whole NM if we decide to go that way.

well, ok about the usage of time() or not, but what about the other bits?

Notice, that the commit message does not reference this bug 1023503, but bug 602265.
Comment 12 Dan Williams 2013-12-11 11:52:47 EST
Current branch works well in my test setup.

Thomas' fixups look good and work correctly, with the exception to the changes in act_stage1_prepare().  "if (priv->last_pppoe_time)" would cause a normal Ethernet connection to delay for up to PPPOE_RECONNECT_DELAY if the last connection was PPPoE but this connection is not PPPoE.  That could be changed to:

if (device_get_setting (dev, NM_TYPE_SETTING_PPPOE) && priv->last_pppoe_time)
Comment 13 Thomas Haller 2013-12-11 12:08:26 EST
(In reply to Dan Williams from comment #12)
> Current branch works well in my test setup.
> 
> Thomas' fixups look good and work correctly, with the exception to the
> changes in act_stage1_prepare().  "if (priv->last_pppoe_time)" would cause a
> normal Ethernet connection to delay for up to PPPOE_RECONNECT_DELAY if the
> last connection was PPPoE but this connection is not PPPoE.  That could be
> changed to:
> 
> if (device_get_setting (dev, NM_TYPE_SETTING_PPPOE) && priv->last_pppoe_time)



Actually, it _would be_ correct though, because last_pppoe_time only gets set, if the device has such a setting. 
+        if (device_get_setting (device, NM_TYPE_SETTING_PPPOE))
+            NM_DEVICE_ETHERNET_GET_PRIVATE (device)->last_pppoe_time = time (NULL);

But it might be more explicit to check it again.

I would propose a different order:
 if (priv->last_pppoe_time && device_get_setting (dev, NM_TYPE_SETTING_PPPOE))
because the previous is much cheaper to evaluate.
Comment 14 Thomas Haller 2013-12-11 12:09:52 EST
(In reply to Thomas Haller from comment #13)
> (In reply to Dan Williams from comment #12)
> > Current branch works well in my test setup.
> > 
> > Thomas' fixups look good and work correctly, with the exception to the
> > changes in act_stage1_prepare().  "if (priv->last_pppoe_time)" would cause a
> > normal Ethernet connection to delay for up to PPPOE_RECONNECT_DELAY if the
> > last connection was PPPoE but this connection is not PPPoE.  That could be
> > changed to:
> > 
> > if (device_get_setting (dev, NM_TYPE_SETTING_PPPOE) && priv->last_pppoe_time)
> 
> 
> 
> Actually, it _would be_ correct though, because last_pppoe_time only gets
> set, if the device has such a setting. 
> +        if (device_get_setting (device, NM_TYPE_SETTING_PPPOE))
> +            NM_DEVICE_ETHERNET_GET_PRIVATE (device)->last_pppoe_time = time
> (NULL);
> 
> But it might be more explicit to check it again.
> 
> I would propose a different order:
>  if (priv->last_pppoe_time && device_get_setting (dev,
> NM_TYPE_SETTING_PPPOE))
> because the previous is much cheaper to evaluate.

Ok, you are right :)
It is really needed. Never mind :D
Comment 15 Dan Williams 2013-12-11 12:20:33 EST
Cheaper order is fine with me.
Comment 17 Jirka Klimes 2013-12-11 12:49:34 EST
Fix pushed to upstream master:
5fad262 ethernet: add reconnect delay for PPPoE connections (rh #1023503)
Comment 20 Ludek Smid 2014-06-13 06:18:18 EDT
This request was resolved in Red Hat Enterprise Linux 7.0.

Contact your manager or support representative in case you have further questions about the request.

Note You need to log in before you can comment on or make changes to this bug.