-
Notifications
You must be signed in to change notification settings - Fork 35
Fix managed-dhcp addon tests #2443
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
Conversation
|
cc: @asc5543 |
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.
Pull request overview
This PR fixes the failing test test_verify_vm_dhcp_controller in the managed DHCP add-on tests. The test was failing because VMs were getting IP addresses on the default management network instead of the DHCP-managed network.
Changes:
- Modified the test to use an untagged network (vlan_id=0) instead of a VLAN-tagged network
- Disabled the management network on test VMs (spec.mgmt_network = False) to ensure only the DHCP network is used
- Added IP pool range configuration (192.168.0.81-192.168.0.90) in config.yml
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| harvester_e2e_tests/integrations/test_9_addons.py | Changed network creation to use untagged network (vlan_id=0), disabled mgmt_network on VMs, updated DNS to 1.1.1.1, added NTP server, fixed test dependency |
| config.yml | Added IP pool start (192.168.0.81) and end (192.168.0.90) values required for DHCP tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
irishgordo
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.
lgtm, ipxe-examples compatibility issues but not a big deal, user can always change settings, they just need to be mindful of things -> this will work however on our regular baremetal loadouts without issue
config.yml
Outdated
| ip-pool-subnet: '192.168.0.0/24' | ||
| ip-pool-start: '' | ||
| ip-pool-end: '' | ||
| ip-pool-start: '192.168.0.81' |
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.
will approve but everyone must be mindful that this conflicts with our default subnet for ipxe examples.
as in 192.168.0.131 for instance is vip, and does fall into this subnet.
when you have a subnet with two possible dhcp handlers it can be bad news
we should think maybe in the future to shift this to be more ipxe-examples compatible
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.
Agree with Mike, in ipxe-example, the dhcp_server has the range from 192.168.0.50 ~ 192.168.0.130.
Maybe we can use like 192.168.0.200 ~ 192.168.0.210 as the range here
Signed-off-by: Khushboo <[email protected]>
6afa61e
d1dedbb to
6afa61e
Compare
|
I have adjusted the |
Which issue(s) this PR fixes:
Issue # #2436
What this PR does / why we need it: