A series of post-refactoring fixes #2685
Conversation
This function no longer exists and is superseded by boot_loaders @Property.
For format strings we require BaudRates member values (int), not the member itself.
|
Huh... I believe the tests fail because the failed code was not executed prior to this PR. I'll look into that too. |
|
Huh, looks like |
|
Okay, I have a fix now. However, it is kind of masking the problem, e.g. it's now possible to cleanup up from a "broken state" (we have a symlink pointing to a non-existent file). It's not clear to me how we end up with this broken state. For some reason even if my system only has one bootloader (pxe), |
817a14e to
5b24194
Compare
|
Hmmm. I see. Sometimes arch is str, sometimes is Archs. |
|
Yeah... What happens is that format is "grub" and "boot_loaders" does not contain anything (weird, why, it should contain "pxe"). Anyway, even if boot_loaders would contain "pxe", there symlink creation should be conditional -- if |
|
Okay, works, I will port |
|
@heroin-moose Would you consider this PR as mergable now? |
|
I think I shall do the proper fix. Give me a couple of hours. |
a185beb to
1eba6e9
Compare
Fix the last remaining parts of the code where plain strings are used instead of the respective enum members. Also remove code branches that refer to x86 arch that is no longer exist.
Apart from TOCTOU races this has another implication: if a symlink is pointing to a non-existent file, os.path.exists() will return False, hence the link will never be removed. In the tftpgen case this will lead us to a deadlock if we have inconsistent Grub configuration: it's not possible to create a new link before removing the old one, and we cannot remove the old one because the existing link is broken. Both utils.rmfile() and utils.mkdir() can handle ENOENT and EEXIST respectively.
Currently the cleanup code does not remove any configuration files in TFTPD directory, because both Grub and PXELINUX filenames are wrong. Also there is no code to remove the link from system_link/<name> to system/<mac>. This commit fixes the issue. I know very little about yaboot, but the code in tftpgen.py generates some links and whatnot that obviously should be removed too. I've left a FIXME, hopefully someone will tend to that soon.
Currently it's not possible to enable iPXE without specifying DHCP Filename Override option for a system because of the weird condition in the dhcpd template. To make things worse, this option does not even affect the actual iPXE configuration, it only affects PXELINUX. Digging into Git history reveals commit cf76e29 that introduced this condition, to my understanding, to prevent sending DHCP Boot File Name option to petitboot. However, petitboot simply ignores this option, so let's make UX for iPXE users more pleasant.
After the multilevel menu patch get_config_filename() became smart in order to deal with multiple bootloaders per system, that is now a thing. This has unfortunate side-effect: simply calling system.get_config_filename() may yeild unexpected results. Consider a system that supports the following list of bootloaders: ["pxe", "ipxe", "grub"] Without the expclicit "loader" parameter, system.get_config_filename() will always return a filename in Grub format (11:22:33:44:55:66), not PXELINUX format (01-11-22-33-44-55-66), as Grub is the dominant option in the get_config_filename() lookup logic. This commit fixes the most straighforward case: $ cobbler system edit --name "test" --boot-loaders "pxe grub" $ find /var/lib/tftpboot -name 11:22:33:44:55:66 /var/lib/tftpboot/grub/system/11:22:33:44:55:66 <--- RIGHT /var/lib/tftpboot/pxelinux.cfg/11:22:33:44:55:66 <--- WRONG
The latter is gone, these are leftovers.
Codecov Report
@@ Coverage Diff @@
## master #2685 +/- ##
==========================================
+ Coverage 39.22% 39.45% +0.22%
==========================================
Files 86 86
Lines 13363 13361 -2
==========================================
+ Hits 5242 5271 +29
+ Misses 8121 8090 -31
Continue to review full report at Codecov.
|
|
@heroin-moose Is this now everything except the Inheritance? |
|
@SchoolGuy, this is the first batch :) I have more (like removing enable_ipxe), but I think it would be better to commit them separately. |
|
Fixed one more iPXE bug. |
Currently CobblerSvc does not implement settings() method. This breaks
iPXE: attempts to fetch an iPXE script from the Cobbler API result in
the following AttributeError:
File "/var/www/cobbler/svc/services.py", line 89, in application
cw = CobblerSvc(server="http://127.0.0.1:%s" % remote_port)
File "/usr/lib/python3.6/site-packages/cobbler/services.py", line 50, in __init__
self.dlmgr = download_manager.DownloadManager(self.collection_mgr)
File "/usr/lib/python3.6/site-packages/cobbler/download_manager.py", line 35, in __init__
self.settings = collection_mgr.settings()
File "/usr/lib/python3.6/site-packages/cobbler/cobbler_collections/manager.py", line 98, in settings
return self.api.settings()
AttributeError: 'CobblerSvc' object has no attribute 'settings
This commit provides the missing method.
|
I have now extensively checked this and will merge this. Automated tests are sadly not existing (that is why this PR is even needed). Since the contributor has tested this in his own infrastructure I will trust him with this. |
Highlights:
While the most of the changes are trivial, there are some pieces I'm kinda
worried about. Please take a carefull look at the patches that remove code
branches related to x86 (not x86_64) and replace global state (like
repo.arch) with local.