Update Allocate method to reuse lease if present #792
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment was marked as resolved.
This comment was marked as resolved.
EmilyShepherd
added a commit
to EmilyShepherd/kiOS
that referenced
this pull request
Dec 10, 2022
This leak was causing failures and crashes on the system. This has also been submitted as a [patch upstream][upstream] however as that PR is still pending, we will add it here as it is required to ensure the system remains functional. [upstream]: containernetworking/plugins#792
EmilyShepherd
force-pushed
the
check-dhcp
branch
from
535afb3 to
cdf566d
Compare
|
Bump @dcbw 🙏 |
|
Looks good. I tested the PR via cnitool using bridge plugin and isc-dhcpd-server |
mccv1r0
approved these changes
Jan 4, 2023
squeed
reviewed
Jan 9, 2023
plugins/ipam/dhcp/lease.go
Outdated
| @@ -67,6 +67,7 @@ type DHCPLease struct { | |||
| broadcast bool | |||
| stopping uint32 | |||
| stop chan struct{} | |||
| check chan bool | |||
There was a problem hiding this comment.
nit: it's convention to use the type chan struct{} to indicate that a channel carries no information.
|
Looks great! One minor nit, then we can get this merged. |
squeed
referenced
this pull request
Jan 9, 2023
Currently, hostname is set in the original DHCPREQUEST but not the renewal. With some DHCP server implementations (such as FreeBSD dhcpd), this leads to the hostname being cleared in the lease table. This behavior is inconsistent with other DHCP clients such as dhclient which set the hostname on the renewal request as well. To fix, use the same options for acquire and renew. This is compatible with RFC 2131 (see table 5). Signed-off-by: Akhil Velagapudi <[email protected]>
Previously, the Allocate method of the daemon always created a new Lease object. However, as both the CNI ADD and CHECK commands call Allocate, and CHECK can be called multiple times, this resulted in multiple Lease objects being created per pod. Each of these leases was long lived with its own maintain() loop - however the daemon only kept track of the most recent one, meaning any old lease objects remained running forever (and held open their NetNS files). After a long enough period, this resulted in the system crashing out with "too many files" or a similar error limits-related error. This commit updates the behaviour of Allocate() to first check if a Lease already exists for the given clientID. If none is found, one is created as before. If a Lease is found, a new Check() mechanism is called, which simply wakes up the maintain() loop to cause it to check the status of the lease. This may fix containernetworking#329. Signed-off-by: Emily Shepherd <[email protected]>
EmilyShepherd
force-pushed
the
check-dhcp
branch
from
cdf566d to
0fc229d
Compare
squeed
approved these changes
Jan 10, 2023
|
Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, the Allocate method of the daemon always created a new Lease object. However, as both the CNI ADD and CHECK commands call Allocate, and CHECK can be called multiple times, this resulted in multiple Lease objects being created per pod.
Each of these leases was long lived with its own maintain() loop - however the daemon only kept track of the most recent one, meaning any old lease objects remained running forever (and held open their NetNS files). After a long enough period, this resulted in the system crashing out with "too many files" or a similar error limits-related error.
This commit updates the behaviour of Allocate() to first check if a Lease already exists for the given clientID. If none is found, one is created as before. If a Lease is found, a new Check() mechanism is called, which simply wakes up the maintain() loop to cause it to check the status of the lease.
This may fix #329.
Signed-off-by: Emily Shepherd [email protected]