DHCP plugin updates #296
DHCP plugin updates #296
Conversation
Add tests, adapted from aii-core. Replace bare open() with CAF::FileReader to make the tests work. Include some improvements which accumulated on our side.
|
Are there any examples of |
|
@jrha: We use it exactly like the tests I've added since 2009 :-) |
| @@ -312,12 +310,24 @@ sub Configure | |||
| } | |||
| my $ip = $self->get_ip($bootable, $tree); | |||
|
|
|||
| my $server_ip = gethostbyname(hostname()); | |||
| $server_ip = inet_ntoa($server_ip) if defined($server_ip); | |||
| variable AII_DHCP_ADDOPTIONS ?= null; | ||
| "/system/aii/dhcp/options/addoptions" ?= AII_DHCP_ADDOPTIONS; | ||
| "options" ?= AII_DHCP_ADDOPTIONS; |
There was a problem hiding this comment.
shoudl be "options/addoptions"
There was a problem hiding this comment.
No, it should not :-) The original aii-dhcp command had a command-line option called --addoptions - which, due to being a command line option, needed to be a single string. But the plugin version never had "addoptions".
In other words - originally, the DHCP plugin did not come with templates, which was unfortunate. When templates were eventually added, they did not match what the code did. This change makes the templates match the code.
There was a problem hiding this comment.
we use the options/addtoptions value; at the very least this change is backwards incompatible
same for the relocation of the tftpserver from options/tftpserver to simple tftpserver.
in the Shellfe.pm is a methods called dhcp that uses this. please also fix that code if you plan to cleanup the schema.
There was a problem hiding this comment.
No, it is not backwards incompatible - using addoptions in the plugin's templates would be backwards incompatible :-)
The underlying issue is, aii-core/src/main/perl/DHCP.pm is not compatible with the DHCP plugin - and therefore, you cannot use the the same templates. Which is unfortunate, but this is the case ever since the plugin exists. aii-dhcp/src/main/pan/quattor contains templates for the plugin - so it must be compatible with what the plugin is doing.
This PR does not try to address the incompatibilities between the two implementations. But I think it is successfully pointing out where the incompatibilities are :-)
| @documentation{ | ||
| Enable the plugin | ||
| } | ||
| "enabled" = true; |
There was a problem hiding this comment.
seriously backwards incompatible
i propose to set "enabled" ?= false;
There was a problem hiding this comment.
I disagree :-) When quattor/aii/ks/config is included, it makes sure /system/aii/osinstall/ks is not empty, so the plugin will actually be used.
This template does exactly the same. If I write include 'quattor/aii/dhcp/config', I want that to cause the plugin to be enabled. But, since the plugin does not have any plugin-wide configuration options at the moment, the enabled flag is used to serve this purpose.
You could call enabled as an_arbitrary_value_to_ensure_the_path_exists, but that is much harder to type :-)
There was a problem hiding this comment.
with current code, including quattor/aii/dhcp/config sets the discovery plugin, and effectively disables any dhcp configuration. i'm not sure it's the intention, at the very very least is backwards incompatible
| @@ -537,6 +538,10 @@ sub dhcp | |||
| { | |||
| my ($self, $node, $st, $cmd, $dhcpmgr) = @_; | |||
|
|
|||
| # If the profile has a discovery plugin configured, then don't second-guess | |||
| # it - it may or may not be ISC DHCP | |||
| return if $st->{configuration}->elementExists (DISCOVERY); | |||
There was a problem hiding this comment.
add a $self->verbose("discovery plugin configured, no dhcp configuration") or something like that
There was a problem hiding this comment.
and the return condition should be that $st->{configuration}->...->{enabled} is true (so that one can disable it; and it should be disabled by default)
There was a problem hiding this comment.
I have no problem with adding a diag message, however, the code above is correct. The use of plugins is triggered by the existence of the plugin name - there does not have to be any specific keys under the name; see iter_plugins(). Neither nbp nor osinstall plugins are required to have a property called enabled - I don't see why discovery plugins should have that.
As I wrote above, enabled is really just a placeholder here - since the DHCP plugin does not have any plugin-wide options, it needs something to force the path to exist.
There was a problem hiding this comment.
ok, if you really only need/want path existance, get rid of the enabled; the name suggests that you can also disable it.
you can just do
discovery : dict = dict() with length(SELF) == 0
and get the same result.
| # | ||
| # "/system/aii/dhcp/options/tftpserver" = "tftp.mydomain.org" | ||
| # | ||
| bind "/system/aii/discovery/dhcp" = structure_dhcp_module_info; |
There was a problem hiding this comment.
fyi, since you do this, there's no way to unbind so the code has to be able to disable the discovery
There was a problem hiding this comment.
That's why the bind is in config.pan and not in schema.pan - you generally include .../config.pan when you want to use something. Again, this is exactly how quattor/aii/ks/config.pan and quattor/aii/pxelinux/config.pan behaves. Which is btw. nicer than the previous practice of having the bind statement in .../schema.pan.
There was a problem hiding this comment.
euhm, my point is that this bind disables dhcp configuration and only configures discovery; and there's no way to disable it.
so the template with this defined should just be called dhcp/discovery.pan, dhcp/config.pan at least suggest it does something with dhcp.
There was a problem hiding this comment.
Well, ks/config.pan and pxelinux/config.pan are not called ks/osinstall.pan and pxelinux/nbp.pan either :-)
The current template layout is inconsistent - it mixes core bits and plugins together. Maybe the templates of real plugins should be moved to a plugins subdirectory?
There was a problem hiding this comment.
my point wrt renaming dhcp/config to dhcp/discvovery is that the current template and code does not configure dhcp since it enables the discovery plugin/whatever.
i'm aware that it's a mess (and i clearly haven't looked at the code base for a while), but this PR breaks our setup, so i'm trying to figure out where it goes wrong and we can get out of this.
Add tests, adapted from aii-core. Replace bare open() with
CAF::FileReader to make the tests work. Include some improvements which
accumulated on our side.