Bug 19501
| Summary: | [PATCH] pump only attempts renew once | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [Retired] Red Hat Linux | Reporter: | Scott Renfro <scott> | ||||
| Component: | pump | Assignee: | Elliot Lee <sopwith> | ||||
| Status: | CLOSED RAWHIDE | QA Contact: | David Lawrence <dkl> | ||||
| Severity: | medium | Docs Contact: | |||||
| Priority: | medium | ||||||
| Version: | 6.2 | ||||||
| Target Milestone: | --- | ||||||
| Target Release: | --- | ||||||
| Hardware: | i386 | ||||||
| OS: | Linux | ||||||
| Whiteboard: | |||||||
| Fixed In Version: | Doc Type: | Bug Fix | |||||
| Doc Text: | Story Points: | --- | |||||
| Clone Of: | Environment: | ||||||
| Last Closed: | 2001-07-20 05:08:22 UTC | Type: | --- | ||||
| Regression: | --- | Mount Type: | --- | ||||
| Documentation: | --- | CRM: | |||||
| Verified Versions: | Category: | --- | |||||
| oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |||||
| Cloudforms Team: | --- | Target Upstream Version: | |||||
| Embargoed: | |||||||
| Attachments: |
|
||||||
Created attachment 4490 [details]
patch to correct pump only trying renew once
This bug is fixed in 7.0 already -- thanks. The 7/8 bit is specified by the dhcp RFC. I think that's pushing it as well,but... I misinterpreted the 7/8 bit, actually. That's the time to move to the REBINDING state (which pump doesn't implement), not the time to begin RENEWAL (which should be at 1/2 the existing lease), where you put it. I'm not going to change this until I implement REBINDING properly though (which is your second patch, plus a new internal state). Are you sure this bug is corrected? I've reviewed the source code included with both pump-0.8.3-2.src.rpm and pump-0.8.4.src.rpm and in both cases, the following line is present in pump.c: 0.8.4 pump.c:377 if ((intf[closest].renewAt += pumpUptime() + 30) > 0.8.3-2 pump.c:370 if ((intf[closest].renewAt += pumpUptime() + 30) > I'm proposing that the += above should actually be simply = Is there a more up to date src rpm I should be reviewing instead? -scott I misread your patch -- your fix is correct. cool; thanks. Has this since been fixed? Apologies for unresponsiveness of previous pump maintainer... Patch wasn't applied, done. thanks for finally closing this out and improving pump's behavior on dhcp networks |
A simple typo in pump.c causes pump to only attempt a single renew. If the renew fails, we intend to try again in 30 seconds, but the use of += instead of = causes the value of renewAt to be too large and almost always exceed leaseExpiration. Found this in code review following the incident described below the patch. --- patch file --- diff -urN pump-0.8.2.orig/pump.c pump-0.8.2/pump.c --- pump-0.8.2.orig/pump.c Tue Aug 8 06:25:43 2000 +++ pump-0.8.2/pump.c Fri Oct 20 19:55:53 2000 @@ -365,7 +365,12 @@ "failed to renew lease for device %s", intf[closest].device); - if ((intf[closest].renewAt += pumpUptime() + 30) > + /* if the renewal failed, then set renewAt to + * try again in 30 seconds AND then if renewAt's + * value is after the lease expiration then + * try to get a fresh lease for the interface + */ + if ((intf[closest].renewAt = pumpUptime() + 30) > intf[closest].leaseExpiration) { o = overrides; while (*o->intf.device && --- further info --- Long story short. Had a IP address conflicts on local segment. Tracked down to rh6.2 box configured for dhcp with pump. Pump had tried to renew the lease and failed on first try. Pump then immediately exited (based on syslog). Source code analysis reveals several problems in pump.c in lines 368-390. First: this bug (in this report) causes pump to only attempt renew once (in nearly all cases). Second: that once a lease has expired, pump only tries once to get a new lease and does so immediately after a renew fails. In nearly all cases, the new lease request will fail for the same reason the renewal fails (assuming even very short network problems). Reporting this in a separate bug report. Am submitting a separate bug report with untested patch for this. Third: When a lease expires and pump gives up trying to get a new lease, it doesn't actually call pumpDisableInterface or otherwise take the IP address from the interface before calling exit(1). As a result, the interface remains up and configured for an IP address that it no longer ''owns.'' This causes IP address conflicts on DHCP networks. I don't run RH and have only looked at the code in passing so don't know whether or not you want to solve this by calling pumpDisableInterface() or do something else. Fourth: when we first calculate renewAt value at dhcp.c:1109, we set it to 7/8 the duration of the lease. For short leases, this doesn't give much time to renew if a problem occurs. isc and ms both use 1/2 for this factor. This is something of an opinion issue, so I won't submit a separate report on this issue. -scott