diff --git a/xCAT-server/lib/xcat/plugins/dhcp.pm b/xCAT-server/lib/xcat/plugins/dhcp.pm index cfe7b9879..bafa6cd0e 100644 --- a/xCAT-server/lib/xcat/plugins/dhcp.pm +++ b/xCAT-server/lib/xcat/plugins/dhcp.pm @@ -1003,66 +1003,17 @@ sub addnode if ($nrhash) { $nrent = $nrhash->{$node}->[0]; - if ($nrent and $nrent->{tftpserver} and $nrent->{tftpserver} ne '') - { - #check the value of inet_ntoa(inet_aton("")),if the hostname cannot be resolved, - #the value of inet_ntoa() will be "undef", which will cause fatal error - my $tmp_name = inet_aton($nrent->{tftpserver}); - unless ($tmp_name) { - - #tell the reason to the user - $callback->( - { error => ["Unable to resolve the tftpserver for node"], errorcode => [1] } - ); - return; - } - $tftpserver = inet_ntoa($tmp_name); - $nxtsrv = $tftpserver; - $lstatements = _omapi_next_server_statement($tftpserver) . $statements; - } - else - { - $guess_next_server = 1; - } - if ($nrent->{netboot} and ($nrent->{netboot} eq 'petitboot' or $nrent->{netboot} eq 'onie' )) { - if ($guess_next_server) { - my $node_server = undef; - if ($nrent->{xcatmaster}) { - $node_server = $nrent->{xcatmaster}; - } - unless ($node_server) { - my @nxtsrvd = xCAT::NetworkUtils->my_ip_facing($node); - unless ($nxtsrvd[0]) { $nxtsrv = $nxtsrvd[1]; } - elsif ($nxtsrvd[0] == 1) { $callback->({ error => [ $nxtsrvd[1] ] }); } - else { - $callback->({ error => ["Unable to determine the tftpserver for $node, verify \"xcatmaster\" is set correctly"], errorcode => [1] }); - return; - } - } else { - my $tmp_server = inet_aton($node_server); - unless ($tmp_server) { - $callback->({ error => ["Unable to resolve the tftpserver for $node, verify \"xcatmaster\" is set correctly"], errorcode => [1] }); - return; - } - $nxtsrv = inet_ntoa($tmp_server); - } - unless ($nxtsrv) { - $callback->({ error => ["Unable to determine the tftpserver for $node, verify \"xcatmaster\" is set correctly"], errorcode => [1] }); - return; - } - $guess_next_server = 0; - } - } - - #else { - # $nrent = $nrtab->getNodeAttribs($node,['servicenode']); - # if ($nrent and $nrent->{servicenode}) { - # $statements = 'next-server = \"'.inet_ntoa(inet_aton($nrent->{servicenode})).'\";'.$statements; - # } - #} } - else - { + + # Which server this node is sent to. Both backends read the same answer + # out of next_server_for_node; an undefined tftpserver is the one case + # nothing here can name, and the subnet's own value carries it. + ( $nxtsrv, $tftpserver ) = next_server_for_node( $node, $nrent ); + return unless defined $nxtsrv; + if ( defined $tftpserver ) { + $lstatements = _omapi_next_server_statement($tftpserver) . $statements; + } + else { $guess_next_server = 1; } unless ($machash) @@ -3659,7 +3610,7 @@ sub kea_node_reservations return []; } - my ( $nxtsrv, $tftpserver ) = kea_next_server_for_node($node, $nrent); + my ( $nxtsrv, $tftpserver ) = next_server_for_node($node, $nrent); my @reservations; my @macs = split(/\|/, $macent->{mac}); foreach my $mace (@macs) { @@ -3851,7 +3802,7 @@ sub kea_node_client_classes_for_nodes my $macent = $macents && $macents->{$node} ? $macents->{$node}->[0] : undef; next unless $macent && $macent->{mac}; - my ( $nxtsrv ) = kea_next_server_for_node($node, $nrent); + my ( $nxtsrv ) = next_server_for_node($node, $nrent); my $ient = $ients && $ients->{$node} ? $ients->{$node}->[0] : undef; my $iname = ( $ient and $ient->{server} and $ient->{target} ) ? $ient->{iname} : undef; @@ -4081,35 +4032,76 @@ sub kea_query_node } } -sub kea_next_server_for_node +#: A netboot method that builds a URL needs the server's address in hand, so +#: for those the search cannot end at "whatever the subnet says" -- there is +#: nothing to interpolate a subnet value into. +sub _needs_absolute_next_server +{ + my ($nrent) = @_; + + my $netboot = $nrent ? $nrent->{netboot} : undef; + return 0 unless $netboot; + return ( $netboot eq 'petitboot' or $netboot eq 'onie' ) ? 1 : 0; +} + +#: Which server a node is sent to, in the order noderes states it: the node's +#: own tftpserver, then its xcatmaster, then -- only for the methods above -- +#: the interface of this machine that faces the node. A node that named none of +#: them inherits the subnet's value, which is what '${next-server}' stands for +#: and why the second return value is undefined there: there is no per-node +#: address to write down. +#: +#: Returns ( next-server, tftpserver ) or the empty list, having already told +#: the caller's callback why. +#: +#: Both backends read this. They had a copy each and the copies had drifted: +#: ISC honoured xcatmaster only for petitboot and onie, so every other node in +#: a hierarchical cluster was sent to the management node instead of to its +#: service node; and Kea fell back to my_ip_facing for every node, so a subnet +#: whose tftpserver named some third machine was overruled on one backend and +#: obeyed on the other. +sub next_server_for_node { my ( $node, $nrent ) = @_; - if ($nrent and $nrent->{tftpserver} and $nrent->{tftpserver} ne '') { - my $tmp_name = inet_aton($nrent->{tftpserver}); + #check the value of inet_ntoa(inet_aton("")),if the hostname cannot be resolved, + #the value of inet_ntoa() will be "undef", which will cause fatal error + if ( $nrent and $nrent->{tftpserver} and $nrent->{tftpserver} ne '' ) { + my $tmp_name = inet_aton( $nrent->{tftpserver} ); unless ($tmp_name) { + + #tell the reason to the user $callback->({ error => ["Unable to resolve the tftpserver for node"], errorcode => [1] }); return; } my $server = inet_ntoa($tmp_name); - return ($server, $server); + return ( $server, $server ); } - my $node_server = $nrent && $nrent->{xcatmaster} ? $nrent->{xcatmaster} : undef; - if ($node_server) { - my $tmp_server = inet_aton($node_server); - if ($tmp_server) { - my $server = inet_ntoa($tmp_server); - return ($server, $server); + if ( $nrent and $nrent->{xcatmaster} ) { + my $tmp_server = inet_aton( $nrent->{xcatmaster} ); + unless ($tmp_server) { + $callback->({ error => ["Unable to resolve the tftpserver for $node, verify \"xcatmaster\" is set correctly"], errorcode => [1] }); + return; } + my $server = inet_ntoa($tmp_server); + return ( $server, $server ); } - my @nxtsrvd = xCAT::NetworkUtils->my_ip_facing($node); - unless ($nxtsrvd[0]) { - return ($nxtsrvd[1], $nxtsrvd[1]); + if ( _needs_absolute_next_server($nrent) ) { + my @facing = xCAT::NetworkUtils->my_ip_facing($node); + unless ( $facing[0] ) { + return ( $facing[1], $facing[1] ); + } + my $why = + ( $facing[0] == 1 ) + ? $facing[1] + : "Unable to determine the tftpserver for $node, verify \"xcatmaster\" is set correctly"; + $callback->({ error => [$why], errorcode => [1] }); + return; } - return ('${next-server}', undef); + return ( '${next-server}', undef ); } sub kea_boot_for_node diff --git a/xCAT-test/dhcptest/spec.md b/xCAT-test/dhcptest/spec.md index a96b91ed4..337ed1414 100644 --- a/xCAT-test/dhcptest/spec.md +++ b/xCAT-test/dhcptest/spec.md @@ -401,7 +401,7 @@ Scenario: The node is told its own name A service node serves the racks behind it. Which server a node is sent to is what makes a hierarchical cluster work, and it is visible in one field. -Source: `dhcp.pm:1000-1045`, `kea_next_server_for_node`, `dhcp.pm:3441` +Source: `next_server_for_node`, which both backends read, and `dhcp.pm:3441` ```gherkin @S-36 @@ -421,7 +421,9 @@ Scenario: next-server otherwise comes from the subnet Given a node with neither tftpserver nor xcatmaster When it discovers Then siaddr is the tftpserver of the subnet it discovered on - # dhcp.pm:1125 -- '${next-server}' defers to the network-level value. + # '${next-server}' is what a node that named no server of its own is + # given: ISC leaves the subnet statement to answer it, and Kea says + # nothing in the reservation, which comes to the same thing. @S-39 Scenario: A node's URLs point at the same server as its next-server @@ -794,7 +796,8 @@ Stated so that a green run is not read as more than it is: Every row is a place where ISC dhcpd and Kea answered the same frame differently, or where only one of them answered it at all. They were found by reading this document against `dhcp.pm` and `BootPolicy.pm` and are enumerated -in VersatusHPC/xcat-internal#175. +in VersatusHPC/xcat-internal#175. Rows 26 and 27 are not in that issue: they +were found by the wire cases, which is what the wire cases are for. The decision column is now the specification: the scenarios above state it unconditionally, and the wire cases assert it against both backends. "Already @@ -828,6 +831,8 @@ the drift was in the spec text, not in xCAT. | 23 | Adoption without a daemon restart: an ISC/OMAPI property | No restart, both | Restarting mid-discovery drops every other machine being discovered | S-58 | already asserted on both by `run-adoption` | | 24 | `authoritative`: an ISC directive, nothing cited for Kea | DHCPNAK rather than silence, both | A node that moved rack must be told to start over | S-53 | Kea: `authoritative` on | | 25 | The whole common-option block sourced only from the ISC generator | Parity, asserted not assumed | An installer that loses its resolver, route, clock or MTU fails late and obscurely | S-44 to S-52 | neither, so far -- now asserted on the wire on both | +| 26 | `noderes.xcatmaster`: read for every node (Kea) vs only for `petitboot` and `onie` (ISC) | Read for every node, both | An operator sets it per node and deliberately; a hierarchical cluster's compute nodes were being sent to the management node on ISC | S-37 | ISC: honour it whatever the netboot method, and put it in siaddr as well as in the URLs built from it | +| 27 | No server named at all: subnet value (ISC) vs `my_ip_facing` (Kea) | The subnet's value, both | The two agree only while the subnet's tftpserver is this machine; `networks.tftpserver` exists precisely to say otherwise | S-38 | Kea: leave `next-server` out of the reservation and let the subnet answer | ## Appendix B: spec review diff --git a/xCAT-test/unit/dhcp_kea_plugin_intent.t b/xCAT-test/unit/dhcp_kea_plugin_intent.t index 8f5867ffc..6bbae8ca7 100644 --- a/xCAT-test/unit/dhcp_kea_plugin_intent.t +++ b/xCAT-test/unit/dhcp_kea_plugin_intent.t @@ -549,8 +549,14 @@ foreach my $case (@sysconfig_policy_cases) { # get a Kea host reservation exactly like a regular compute node. The Kea # reservation builder loops over every requested node without filtering on # service-node membership, so kea_build_node_reservations must emit an - # ip/mac/hostname reservation whose next-server is resolved (via - # my_ip_facing) to the management server that serves the node's subnet. + # ip/mac/hostname reservation for it. + # + # This node names no server of its own -- its tftpserver is the + # placeholder and it has no xcatmaster -- so the address it is + # sent to is the subnet's, which a reservation states by saying nothing. + # Kea used to fall back to my_ip_facing here, which is a different answer + # from the one ISC gives the same node whenever networks.tftpserver names + # some third machine. package DHCPKeaResTable; sub new { my ( $class, $rows ) = @_; return bless { rows => $rows }, $class; } sub getNodesAttribs { @@ -608,7 +614,69 @@ foreach my $case (@sysconfig_policy_cases) { is( $r->{'ip-address'}, '192.168.201.21', 'service node reservation carries the node IP' ); is( $r->{'hw-address'}, '42:d7:c0:a8:c9:15', 'service node reservation carries the node MAC' ); is( $r->{hostname}, 'svc01', 'service node reservation carries the hostname' ); - is( $r->{'next-server'}, '192.168.201.20', 'service node reservation next-server resolves to the serving management IP' ); + ok( !exists $r->{'next-server'}, + 'a node that names no server of its own leaves next-server to the subnet' ); +} + +{ + # Where a node is sent, in the order noderes states it. Both backends read + # this one answer; they used to have a copy each and the copies disagreed + # about every row but the first. + no warnings 'redefine'; + local *xCAT::NetworkUtils::my_ip_facing = sub { return ( 0, '10.0.0.1' ); }; + my @errors; + local $xCAT_plugin::dhcp::callback = sub { + my $resp = shift; + push @errors, @{ $resp->{error} || [] }; + }; + + my @cases = ( + [ { tftpserver => '192.0.2.10', xcatmaster => '192.0.2.20' }, + [ '192.0.2.10', '192.0.2.10' ], + 'the node\'s own tftpserver outranks its xcatmaster', + ], + [ { tftpserver => '', xcatmaster => '192.0.2.20' }, + [ '192.0.2.20', '192.0.2.20' ], + 'the placeholder defers to the xcatmaster attribute', + ], + [ { netboot => 'xnba', xcatmaster => '192.0.2.20' }, + [ '192.0.2.20', '192.0.2.20' ], + 'xcatmaster is honoured for every netboot method, not only petitboot and onie', + ], + [ { netboot => 'xnba' }, + [ '${next-server}', undef ], + 'a node naming neither inherits the subnet\'s value', + ], + [ {}, + [ '${next-server}', undef ], + 'and so does a node with no noderes entry to speak of', + ], + [ { netboot => 'petitboot' }, + [ '10.0.0.1', '10.0.0.1' ], + 'petitboot needs an address to build its URL with, so it falls back to the facing interface', + ], + [ { netboot => 'onie' }, + [ '10.0.0.1', '10.0.0.1' ], + 'and so does onie', + ], + ); + + foreach my $case (@cases) { + my ( $nrent, $want, $why ) = @{$case}; + is_deeply( [ xCAT_plugin::dhcp::next_server_for_node( 'n1', $nrent ) ], $want, $why ); + } + + is_deeply( [ xCAT_plugin::dhcp::next_server_for_node( 'n1', undef ) ], + [ '${next-server}', undef ], 'a node with no noderes row at all inherits the subnet too' ); + + is( scalar(@errors), 0, 'none of those are an error the operator has to read about' ); + + # An xcatmaster nobody can resolve is a misconfiguration, and silently + # sending the node somewhere else hides it. + @errors = (); + my @unresolvable = xCAT_plugin::dhcp::next_server_for_node( 'n1', { xcatmaster => 'no.such.host.invalid' } ); + is( scalar(@unresolvable), 0, 'an unresolvable xcatmaster yields no address' ); + like( ( $errors[0] || '' ), qr/xcatmaster/, 'and says which attribute to look at' ); } my @normalized_mac_cases = ( @@ -681,7 +749,7 @@ foreach my $case (@invalid_mac_cases) { return '192.0.2.25'; }; local *xCAT_plugin::dhcp::ipIsDynamic = sub { return 0; }; - local *xCAT_plugin::dhcp::kea_next_server_for_node = sub { return ( '192.0.2.1', '192.0.2.1' ); }; + local *xCAT_plugin::dhcp::next_server_for_node = sub { return ( '192.0.2.1', '192.0.2.1' ); }; local *xCAT_plugin::dhcp::kea_boot_for_node = sub { return {}; }; my $backend = bless {}, 'DHCPKeaMacBackend'; @@ -787,7 +855,7 @@ foreach my $case (@invalid_mac_cases) { return $host eq 'valid01' ? '192.0.2.30' : undef; }; local *xCAT_plugin::dhcp::ipIsDynamic = sub { return 0; }; - local *xCAT_plugin::dhcp::kea_next_server_for_node = sub { return ( '192.0.2.1', '192.0.2.1' ); }; + local *xCAT_plugin::dhcp::next_server_for_node = sub { return ( '192.0.2.1', '192.0.2.1' ); }; local *xCAT_plugin::dhcp::kea_boot_for_node = sub { return {}; }; local *xCAT::MsgUtils::message = sub { return; }; local *xCAT::MsgUtils::trace = sub { return; }; @@ -856,7 +924,7 @@ foreach my $case (@invalid_mac_cases) { my ( $class, $name ) = @_; return $xnba_tables{$name}; }; - local *xCAT_plugin::dhcp::kea_next_server_for_node = sub { return ( '192.0.2.1', '192.0.2.1' ); }; + local *xCAT_plugin::dhcp::next_server_for_node = sub { return ( '192.0.2.1', '192.0.2.1' ); }; my $classes = xCAT_plugin::dhcp::kea_node_client_classes_for_nodes(['xnba01'])->{classes}; my ($bios_class) = grep { $_->{name} =~ /-bios\z/ } @$classes; @@ -904,7 +972,7 @@ foreach my $case (@invalid_mac_cases) { local *xCAT::NetworkUtils::getipaddr = $noip_getipaddr; local *xCAT_plugin::dhcp::getipaddr = $noip_getipaddr; local *xCAT_plugin::dhcp::ipIsDynamic = sub { return 0; }; - local *xCAT_plugin::dhcp::kea_next_server_for_node = sub { return ( '192.0.2.1', '192.0.2.1' ); }; + local *xCAT_plugin::dhcp::next_server_for_node = sub { return ( '192.0.2.1', '192.0.2.1' ); }; local *xCAT_plugin::dhcp::kea_boot_for_node = sub { return {}; }; my @errors; @@ -963,7 +1031,7 @@ foreach my $case (@invalid_mac_cases) { my ($ip) = @_; return $ip eq '192.0.2.150' || $ip eq '2001:db8::150'; }; - local *xCAT_plugin::dhcp::kea_next_server_for_node = sub { return; }; + local *xCAT_plugin::dhcp::next_server_for_node = sub { return; }; local *xCAT_plugin::dhcp::kea_boot_for_node = sub { return {}; }; local *xCAT::MsgUtils::message = sub { return; }; local *xCAT::MsgUtils::trace = sub { return; }; @@ -1255,7 +1323,7 @@ foreach my $case (@invalid_mac_cases) { my ( $class, $name ) = @_; return $tables{$name}; }; - local *xCAT_plugin::dhcp::kea_next_server_for_node = sub { return ( '192.0.2.1', '192.0.2.1' ); }; + local *xCAT_plugin::dhcp::next_server_for_node = sub { return ( '192.0.2.1', '192.0.2.1' ); }; my $classes = xCAT_plugin::dhcp::kea_node_client_classes_for_nodes( [ 'smp01', 'san01' ] )->{classes}; my %by_name = map { $_->{name} => $_ } @$classes; @@ -1321,7 +1389,7 @@ foreach my $case (@invalid_mac_cases) { my ( $class, $name ) = @_; return $tables{$name}; }; - local *xCAT_plugin::dhcp::kea_next_server_for_node = sub { return ( '192.0.2.1', '192.0.2.1' ); }; + local *xCAT_plugin::dhcp::next_server_for_node = sub { return ( '192.0.2.1', '192.0.2.1' ); }; my $config = { Dhcp4 => { 'client-classes' => [] } }; ok( xCAT_plugin::dhcp::kea_sync_node_client_classes( $config, [ 'cn01', 'cn02' ] ), @@ -1432,7 +1500,7 @@ foreach my $case (@invalid_mac_cases) { my ( $class, $name ) = @_; return $tables{$name}; }; - local *xCAT_plugin::dhcp::kea_next_server_for_node = sub { return ( '192.0.2.1', '192.0.2.1' ); }; + local *xCAT_plugin::dhcp::next_server_for_node = sub { return ( '192.0.2.1', '192.0.2.1' ); }; local *xCAT_plugin::dhcp::proxydhcp = sub { return 1; }; my $classes = xCAT_plugin::dhcp::kea_node_client_classes_for_nodes( [ 'booted', 'win01' ] )->{classes};