Fix Discovery provisioning tests - #20638
Conversation
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Using a bare
assertonsat.execute('systemctl restart dhcpd').status == 0will raise an uninformative AssertionError; consider checking the status and raising/logging a more descriptive error (including stdout/stderr) to ease debugging when the restart fails. - If there is (or might be) a shared helper for managing systemd services in tests, consider calling that instead of invoking
systemctldirectly here to keep service management behavior consistent across fixtures.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Using a bare `assert` on `sat.execute('systemctl restart dhcpd').status == 0` will raise an uninformative AssertionError; consider checking the status and raising/logging a more descriptive error (including stdout/stderr) to ease debugging when the restart fails.
- If there is (or might be) a shared helper for managing systemd services in tests, consider calling that instead of invoking `systemctl` directly here to keep service management behavior consistent across fixtures.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| # Restart dhcpd to pick up new PXE configuration for discovery | ||
| assert sat.execute('systemctl restart dhcpd').status == 0 |
There was a problem hiding this comment.
Why would one need to restart dhcpd? the proxy should only write to existing paths and those should be dynamically loaded by dhcpd
|
|
PRT Result |
|
|
PRT Result |
9604670 to
df70b14
Compare
|
So, the issue is stale DHCP lease state, not PXE template changes, and this seems the same issue as SAT-28381, stale dynamic leases interfering with provisioning robottelo/robottelo/host_helpers/satellite_mixins.py Lines 386 to 389 in 036a711 |
|
Okay, that's a much better explanation. Thanks for digging it up! I still don't like the solution much, but at least there is a "why" now :) |
well, i'm open to revisiting this, if you have any alternative approach |
|
Not really, no :( |
|
PRT Result |
|
Should PRT have passed with this change? |
it should, but lemme dig more |
df70b14 to
1d3ba53
Compare
|
|
PRT Result |
|
|
PRT Result |
|
Below two tests are failing for a specific reason, and a Jira ticket has already been attached to below link. I would suggest using these changes with the given tests so that at least three tests will pass, as one of the tests uses parameterization. Right now, 12 tests are failing, and 1 failure is expected. This fix will resolve 3 additional issues, so after the re-run, there should be 8 failures remaining. |
940581c to
d841f20
Compare
|
1 similar comment
|
e24f3ee to
dbb127b
Compare
|
|
PRT Result |
|
UI tests are failing mainly failing for Navigation and Non existent element. @archanaserver you should check corresponding code at airgun repo for
It should solve failing tests. |
I have already discussed the above issue on Slack, and I am currently looking into it. |
@nacoool this seems like a different failure, can you clarify are these airgun failures pre-existing, or were they exposed by my changes? If yes then I believe fixing this airgun issue is not part of this PR scope? and we can handle this separately if this same behavior not applying to the 2 of the API failures we can see right now? @evgeni @amolpati30 thoughts on this? |
The failure you are referring to was encountered recently and is not caused by your changes. We can address it separately. The remaining failures can be handled later, and I do not believe they are related to your changes. Most of the tests are passing now, so I am giving my ACK. |
dbb127b to
6fe1c92
Compare
|
This test test_positive_reboot_all_pxe_hosts is just testing reboot functionality. and It doesn't need to set hostgroup/location/organization/build. Those lines are causing the host to auto-provision before the test can reboot it, so dropping it. |
|
|
PRT Result |
|
I see failures related to Manifester and RHSM which are unrelated to this change so merging as is |
* Fix Discovery provisioning tests * Apply SAT-33477 workaround to UI tests (cherry picked from commit 9ff92d0)
* Fix Discovery provisioning tests * Apply SAT-33477 workaround to UI tests (cherry picked from commit 9ff92d0)
* Fix Discovery provisioning tests * Apply SAT-33477 workaround to UI tests (cherry picked from commit 9ff92d0)
Problem Statement
Discovery provisioning tests are failing because
dhcpdis not restarted after building PXE templates in themodule_discovery_satfixture. Without the restart, dhcpd serves old cached configuration.Solution
Added
dhcpdservice restart in themodule_discovery_satfixture after all discovery configuration is complete. This ensures dhcpd picks up the updated PXE boot configuration and serves the correct discovery boot files to VMs.Related Issues
Fixes: SAT-39920