-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Enhance DHCP functionality by adding lease timeout support #12410
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…s components. Updated DhcpEntryCommand to include lease time, modified VmDhcpConfig to handle lease time, and adjusted related scripts and configurations to accommodate this new parameter. This allows for configurable DHCP lease durations, improving flexibility in network management.
|
Congratulations on your first Pull Request and welcome to the Apache CloudStack community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md)
|
DaanHoogland
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks generally good, one remark and also needs testing of course
| DhcpLeaseTimeout( | ||
| "Network", | ||
| ManagementServer.class, | ||
| Integer.class, | ||
| "dhcp.lease.timeout", | ||
| "0", | ||
| "DHCP lease time in seconds for VMs. Use 0 for infinite lease time (default). A non-zero value sets the lease duration in seconds.", | ||
| null), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this config mechanism is outdated. I think it is most suitable as ConfigKey in AgentManager/AgentManagerImpl. There are some examples there to look at.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 with ConfigKey
which scope is best ? Global, Zone or so? @DaanHoogland
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or in NetworkOrchestrationService, or some other related service.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@weizhouapache @NVShawn whether this is Global, Zone or even Domain or Account scope is a design decision to be made. At the moment I do not have an opinion (yet ;) ).
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #12410 +/- ##
============================================
- Coverage 17.76% 17.76% -0.01%
Complexity 15863 15863
============================================
Files 5923 5923
Lines 530558 530577 +19
Branches 64830 64831 +1
============================================
Hits 94262 94262
- Misses 425749 425768 +19
Partials 10547 10547
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Description
This PR enhances the feature added in #3662 to allow for non-infinite dhcp lease times.
So we do not break current installs, we keep the 'infinite' lease time as default but allow for a config to change the lease times to non-infinite values.
We've had issues with VMs with the lease set to infinite where they never check to see if their IP is still available, so we get IP conflicts. In our environment, with a combination of very short lived and long lived VMs and constrained IP address, we have to rely on IP address re-use and lease timeouts. This change allows us to configure our IP lease timeouts without modifying default behavior.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity