Add DHCP renew release test #923
Conversation
pau-hedgehog
added
ci:+release
Test Results 8 files 32 suites 2h 4m 48s ⏱️ Results for commit 95db236. ♻️ This comment has been updated with latest results. |
pkg/hhfab/release.go
Outdated
|
|
||
| // Simple validation that the interface still exists and is configured | ||
| for { | ||
| out, err := execNodeCmdWOutput(testCtx.hhfabBin, testCtx.workDir, serverName, |
There was a problem hiding this comment.
should we check the lease time to be more precise and make sure that the DHCP renewal actually succeeded? there is already some code doing it in the regular DHCP test that you could reuse
There was a problem hiding this comment.
As usual, you nailed it. I realized that the networkctl option that indeed triggers renewal is not renew, not forcerenew but reconfigure
I'm adding the lease time checking and, as a bonus, for L3VNI checking the 2-step lease
I'm troubleshooting now some slow DHCP leases before resolving this. In case I'm too restrictive in the timeouts in the test or we have performance issues in the server
pau-hedgehog
force-pushed
the
pau/dhcp_renew_rel_test
branch
2 times, most recently
from
d446dcf to
3f07c55
Compare
|
So I'm a little conflicted about this test the way it currently is, because I do not believe it's testing DHCP renewals anymore. Instead we are reconfiguring the interfaces and getting brand new leases. It's not a bad thing to have additional coverage on this, but it's not what we're saying we are doing, right? For instance, l3vni actual renewals should not have the short->long lease cycle, or at least that's not what I would expect. I think to test renewals you need to set the lease to a reasonably short time via the DHCPOptions and wait long enough for it to expire, then check what you get afterwards. It might be possible to force a renew earlier via networkctl, although this question appears to imply hat it doesn't really work that way, and your experience seems to confirm it. It's also easy to break this if we ever change the lease time values we use in the DHCP server. This is inevitable to a certain extent, although we could at least set the lease time as a DHCPOption for the "large lease" value instead of using the default and risking it changing. None of the above means that we should reject the PR, but I think it's worth thinking about what we're doing. The one thing I think we should change regardless is the code to parse the DHCP lease - instead of duplicating it let's use the existing function or adapt it if it doesn't fit your needs. |
All the points you raise are 100% valid. Let me iterate once more to address all the concerns |
Signed-off-by: Pau Capdevila <[email protected]>
pau-hedgehog
force-pushed
the
pau/dhcp_renew_rel_test
branch
from
95db236 to
46e14bf
Compare
Signed-off-by: Pau Capdevila <[email protected]>
pau-hedgehog
force-pushed
the
pau/dhcp_renew_rel_test
branch
from
46e14bf to
d23ea05
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new DHCP renewal test to verify that VPC-attached interfaces properly handle DHCP lease renewals with shorter lease times. The test configures VPC DHCP options with a reduced lease time and monitors the automatic renewal process across servers.
- Adds comprehensive DHCP renewal testing with concurrent execution support
- Implements VPC DHCP configuration updates with proper reversion handling
- Adds lease parsing and validation utilities for monitoring renewal behavior
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if len(servers) == 0 { | ||
| slog.Info("No servers with VPC attachments found, skipping DHCP renewal test") | ||
|
|
||
| return true, nil, fmt.Errorf("no servers with VPC attachments found") //nolint:goerr113 |
There was a problem hiding this comment.
Returning an error while also returning true (success) is contradictory. Should return false to indicate test failure, or return nil error if skipping is considered success.
| return true, nil, fmt.Errorf("no servers with VPC attachments found") //nolint:goerr113 | |
| return true, nil, nil |
| } | ||
|
|
||
| if testVPC == nil { | ||
| return true, nil, fmt.Errorf("no VPC found for test servers") //nolint:goerr113 |
There was a problem hiding this comment.
Same issue as above - returning true (success) with an error is contradictory. Should return false to indicate test failure.
| return true, nil, fmt.Errorf("no VPC found for test servers") //nolint:goerr113 | |
| return false, nil, fmt.Errorf("no VPC found for test servers") //nolint:goerr113 |
|
|
||
| slog.Info("DHCP renewal test completed successfully", "servers", len(testServers), "maxDuration", maxDuration) | ||
|
|
||
| return false, reverts, nil |
There was a problem hiding this comment.
Returning false (failure) with nil error is contradictory. Should return true for success or provide an error explaining the failure.
| return false, reverts, nil | |
| return true, reverts, nil |
| testSubnet.DHCP.Options = &vpcapi.VPCDHCPOptions{ | ||
| DNSServers: []string{}, | ||
| TimeServers: []string{}, | ||
| InterfaceMTU: 9036, |
There was a problem hiding this comment.
Magic number 9036 should be defined as a named constant to improve code readability and maintainability.
pau-hedgehog
changed the title
2 participants
No description provided.