Skip to content
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.

Sign up for GitHub

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

DHCP plugin updates #296

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

DHCP plugin updates #296

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions aii-core/src/main/perl/Shellfe.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ use constant PROFILEINFO => 'profiles-info.xml';
use constant NODHCP => 'nodhcp';
use constant NONBP => 'nonbp';
use constant NOOSINSTALL=> 'noosinstall';
use constant DISCOVERY => '/system/aii/discovery';
use constant OSINSTALL => '/system/aii/osinstall';
use constant NBP => '/system/aii/nbp';
use constant CDBURL => 'cdburl';
Expand Down Expand Up @@ -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);
stdweird marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a $self->verbose("discovery plugin configured, no dhcp configuration") or something like that

Copy link
Member

@stdweird stdweird Sep 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


return unless $st->{configuration}->elementExists (DHCPPATH);

my $mac;
Expand Down
41 changes: 24 additions & 17 deletions aii-dhcp/src/main/pan/quattor/aii/dhcp/config.pan
100755 → 100644
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,24 +2,31 @@
# ${developer-info}
# ${author-info}
# ${build-info}
template quattor/aii/${project.artifactId}/config;
template quattor/aii/dhcp/config;

include 'quattor/aii/${project.artifactId}/schema';
include 'quattor/aii/dhcp/schema';

# TFTP server
# This is optional. Only it is necessary if your TFTP server is running on
# a different machine than the DHCP server
#
# "/system/aii/dhcp/options/tftpserver" = "tftp.mydomain.org"
#
bind "/system/aii/discovery/dhcp" = structure_dhcp_module_info;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fyi, since you do this, there's no way to unbind so the code has to be able to disable the discovery

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


prefix "/system/aii/discovery/dhcp";

@documentation{
Enable the plugin
}
"enabled" = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seriously backwards incompatible
i propose to set "enabled" ?= false;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :-)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


bind "/system/aii/dhcp" = structure_dhcp_dhcp_info;

prefix "/system/aii/dhcp";

@documentation{
Override the TFT server for this node
}
variable AII_DHCP_TFTPSERVER ?= null;
"/system/aii/dhcp/options/tftpserver" ?= AII_DHCP_TFTPSERVER;

# Additional DHCP options (optional).
# Warning: They will be added in the host declaration of dhcpd.conf file, so
# do not forget the ';' at the end
#
#"/system/aii/dhcp/addoptions" = "options blu-blo-bli bla;";
#
"tftpserver" ?= AII_DHCP_TFTPSERVER;

@documentation{
Additional options to include in the host definition
}
variable AII_DHCP_ADDOPTIONS ?= null;
"/system/aii/dhcp/options/addoptions" ?= AII_DHCP_ADDOPTIONS;
"options" ?= AII_DHCP_ADDOPTIONS;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shoudl be "options/addoptions"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :-)

Empty file modified aii-dhcp/src/main/pan/quattor/aii/dhcp/rpms.pan
100755 → 100644
View file
Open in desktop
Empty file.
17 changes: 13 additions & 4 deletions aii-dhcp/src/main/pan/quattor/aii/dhcp/schema.pan
100755 → 100644
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,21 @@
# ${author-info}
# ${build-info}

unique template quattor/aii/${project.artifactId}/schema;
unique template quattor/aii/dhcp/schema;

type structure_dhcp_module_info = {
"enabled" ? boolean
};

# Information needed for creating the Kickstart file
type structure_dhcp_dhcp_info = {
@{TFTP server to use for this node, instead of the host where AII runs}
"tftpserver" ? string
@{Name of the file to boot}
"filename" ? string
@{
Custom options to include in the host definition. Note: if the type
of an option requires quoting, then the quotes must be included in
the value you specify in templates.
}
"options" ? string{}
};

bind "/system/aii/${project.artifactId}" = structure_dhcp_dhcp_info;
41 changes: 26 additions & 15 deletions aii-dhcp/src/main/perl/dhcp.pm
100755 → 100644
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use Socket;
use Exporter;
use Sys::Hostname;
use CAF::Lock qw (FORCE_IF_STALE);
use CAF::FileReader;
use CAF::FileWriter;
use CAF::Process;

Expand Down Expand Up @@ -70,7 +71,7 @@ sub update_dhcp_config_file
$nodes_regexp .= $node->{NAME} . '|' . $node->{FQDN} . '|';
}
$nodes_regexp =~ s/\|$//; # remove last '|'
$nodes_regexp = '\\n\s*host\s+(' . $nodes_regexp . ')\s*\{[^}]*\}';
$nodes_regexp = '\\n\s*host\s+(' . $nodes_regexp . ')\s*(\{(?:[^{}]|(?2))*\})';
$text =~ s/$nodes_regexp//gm;

#
Expand Down Expand Up @@ -138,6 +139,11 @@ sub update_dhcp_config_file
push @newnodes, "$indent\t next-server $node->{ST_IP_TFTP};";
}

# DHCP filename option
if ($node->{FILENAME}) {
push @newnodes, "$indent\t filename \"$node->{FILENAME}\";";
}

# additional options
if ($node->{MORE_OPT}) {
push @newnodes, "$indent\t $node->{MORE_OPT}";
Expand Down Expand Up @@ -255,14 +261,14 @@ sub update_dhcp_config {
}
$self->debug(3, "Locked dhcp configuration");
$self->debug(3,"DHCP configuration file : $filename");
if (!open(FILE, "< $filename")) {
my $fh = CAF::FileReader->new($filename, log => $self);
if ($EC->error()) {
$self->error("dhcp: update configuration: ".
"file access error $filename");
return(1);
}
local $/ = undef;
$text = <FILE>;
close (FILE);
$text = "$fh";
$fh->close();

#
# Add/removal of nodes
Expand All @@ -272,18 +278,10 @@ sub update_dhcp_config {
return(1);
}

#
# Backup the old one
#
if (!rename ($filename, $filename . '.pre_aii')) {
$self->error("dhcp: error creating backup copy $filename.pre_aii");
return(1);
}

#
# Write the new dhcp configuration file
#
my $file = CAF::FileWriter->open($filename, mode => 0664, log => $this_app);
my $file = CAF::FileWriter->new($filename, mode => 0664, log => $this_app, backup => '.pre_aii');
$file->print($text);
$file->close();

Expand Down Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be Socket::inet_ntoa.

if (!defined($server_ip)) {
$self->error("aii-dhcp: failed to obtain own IP address");
return;
}

my $opts = $config->getElement("/system/aii/dhcp")->getTree();
my $tftpserver = "";
my $filename = "";
my $additional = "";
if ($opts->{tftpserver}) {
$tftpserver = $opts->{tftpserver};
}
if ($opts->{filename}) {
$filename = $opts->{filename};
$filename =~ s/BOOTSRVIP/$server_ip/;
}
if ($opts->{options}) {
foreach my $k (sort keys %{$opts->{options}}) {
$additional .= "option $k $opts->{options}->{$k};\n";
Expand All @@ -331,6 +341,7 @@ sub Configure
IP => unpack('N', Socket::inet_aton($ip)),
MAC => $cards->{$bootable}->{hwaddr},
ST_IP_TFTP => $tftpserver,
FILENAME => $filename,
MORE_OPT => $additional,
};
if ($this_app->option('use_fqdn')) {
Expand Down Expand Up @@ -373,7 +384,7 @@ sub Unconfigure
IP => $ip,
};
$self->update_dhcp_config([], [$nodeconfig]);
return 0;
return 1;
}

1;
Empty file modified aii-dhcp/src/main/perl/dhcp.pod
100755 → 100644
View file
Open in desktop
Empty file.
Loading