Re: cleaning DHCP iface address/routes
On Wed, Nov 23, 2016 at 9:52 PM, Justin Cinkelj
wrote:
> I wish to ask for comment or two about how to clean DHCP interface routes
> after sending DHCP release.
> It is one thing Nadav suggested (if I understood him correctly) to add.
>
I didn't ask for this, but you're right, it does make sense to stop using
the IP address after we release it.
>
> In DORA cycle, after receiving ACK, the following things are called from
> dhcp_interface_state::state_request
> osv::start_if
> if /32 netmask:
> osv_route_add_interface
> osv_route_add_network( def_route )
> std::for_each(dm.get_routes()
> osv_route_add_interface (via cur_route.gw or via
> cur_route.iface)
> set_dns_config
>
> It seems to me that osv_route_add_{interface|network} only send special
> packet out of interface.
> If it is so, there is nothing to cleanup.
>
This code doesn't "send a packet".
The idea is to add this interface to the routing table (which in Linux you
see with the "route" command). The "default route" determines which
interface the packets go out from, by default. The additional /32 (a
256-address subnet) route is useful so if you have several interfaces, so
only one can be the default, but you still want packets to each subnet to
go out their respective interfaces.
The API to change the routing table is through a "routing socket", a socket
on which you send special routing messages, or its Linux expanded version,
the "netlink". Yucky, but that's the API invented at a dark time where 1.
there was a desire to avoid new system calls, and 2. /proc was not yet
invented.
So this code is not really sending a packet, it's modifying the routing
table.
I think that when you later mark the interface down, this should
automatically remove the routes which mention it because those are no
longer possible routes. You won't need to do this explicitly. But I never
checked this.
> set_dns_config - well, that should be inverted too. Current set_dns_config
> code seems to be ok for a single interface only.
> https://github.com/cloudius-systems/osv/blob/master/libc/
> network/__dns.cc#L33
> So if say clear_dns_config() is added, I don't need to bother about more
> than one interface case, where same DNS resolver can be reached from
> multiple interfaces.
>
The DNS configuration is at a whole different layer than the IP address
configuration. It's just the IP address (or list of those) of name servers.
It can be reached through any relevant interface (through the routing
table).
On release we can clear the list of DNS servers, or do nothing.
The whole DNS server setting through DHCP thing doesn't make sense if you
use DHCP on several interfaces, because which one is supposed to set the
DNS server. I don't know what we're supposed to do if only one of those
interfaces are released. Perhaps the best thing to do regarding the DNS is
simply nothing. It won't work anyway if you no longer have an IP address.
>
> The osv::start_if main part is in_control(NULL, SIOCAIFADDR, ...)
> https://github.com/cloudius-systems/osv/blob/master/bsd/
> porting/networking.cc#L113
>
> Simply adding osv::stop_if, which does SIOCDIFADDR seems to be enough.
>
I don't remember if changing the IP address to 0 is enough, or you need to
switch the IFF_UP flag down, but yes, one of those should be enough - it
should also remove the routes automatically, and it's fine not to touch the
DNS configuration, I think.
> With delay added between DHCP release and next DORA cycle, ifconfig.so and
> lsroute.so showed that IP and routes were removed, and VM didn't respond to
> ping any more (until DORA cycle set IP again).
>
Excellent.
>
> Did I miss/misunderstood anything significant?
>
I don't think so.
--
You received this message because you are subscribed to the Google Groups "OSv
Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to osv-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
Re: [PATCH 0/2] dhcp: remove IP from interface when DHCP release is send
I didn't seem to get these patches... Can you please send them again? Thanks. -- Nadav Har'El n...@scylladb.com On Wed, Nov 23, 2016 at 11:07 PM, Justin Cinkelj wrote: > The first patch removes IP from iface when DHCP release is sent. > This one is needed, because otherwise we send release, but keep using the > IP address. > > The second one clears DNS configuration at DHCP release event. > > Justin Cinkelj (2): > dhcp: remove IP from interface when DHCP release is send > dhcp: remove DNS resolvers when DHCP release is send > > bsd/porting/networking.cc | 29 +++-- > bsd/porting/networking.hh | 2 ++ > core/dhcp.cc | 22 +- > include/osv/dhcp.hh | 1 + > libc/network/__dns.cc | 8 > libc/network/__dns.hh | 2 ++ > 6 files changed, 49 insertions(+), 15 deletions(-) > > -- > 2.9.3 > > -- > You received this message because you are subscribed to the Google Groups > "OSv Development" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to osv-dev+unsubscr...@googlegroups.com. > For more options, visit https://groups.google.com/d/optout. > -- You received this message because you are subscribed to the Google Groups "OSv Development" group. To unsubscribe from this group and stop receiving emails from it, send an email to osv-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[PATCH 0/2] dhcp: remove IP from interface when DHCP release is send
The first patch removes IP from iface when DHCP release is sent. This one is needed, because otherwise we send release, but keep using the IP address. The second one clears DNS configuration at DHCP release event. Justin Cinkelj (2): dhcp: remove IP from interface when DHCP release is send dhcp: remove DNS resolvers when DHCP release is send bsd/porting/networking.cc | 29 +++-- bsd/porting/networking.hh | 2 ++ core/dhcp.cc | 22 +- include/osv/dhcp.hh | 1 + libc/network/__dns.cc | 8 libc/network/__dns.hh | 2 ++ 6 files changed, 49 insertions(+), 15 deletions(-) -- 2.9.3 -- You received this message because you are subscribed to the Google Groups "OSv Development" group. To unsubscribe from this group and stop receiving emails from it, send an email to osv-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
cleaning DHCP iface address/routes
I wish to ask for comment or two about how to clean DHCP interface routes after
sending DHCP release.
It is one thing Nadav suggested (if I understood him correctly) to add.
In DORA cycle, after receiving ACK, the following things are called from
dhcp_interface_state::state_request
osv::start_if
if /32 netmask:
osv_route_add_interface
osv_route_add_network( def_route )
std::for_each(dm.get_routes()
osv_route_add_interface (via cur_route.gw or via cur_route.iface)
set_dns_config
It seems to me that osv_route_add_{interface|network} only send special packet
out of interface.
If it is so, there is nothing to cleanup.
set_dns_config - well, that should be inverted too. Current set_dns_config code
seems to be ok for a single interface only.
https://github.com/cloudius-systems/osv/blob/master/libc/network/__dns.cc#L33
So if say clear_dns_config() is added, I don't need to bother about more than
one interface case, where same DNS resolver can be reached from multiple
interfaces.
The osv::start_if main part is in_control(NULL, SIOCAIFADDR, ...)
https://github.com/cloudius-systems/osv/blob/master/bsd/porting/networking.cc#L113
Simply adding osv::stop_if, which does SIOCDIFADDR seems to be enough.
With delay added between DHCP release and next DORA cycle, ifconfig.so and
lsroute.so showed that IP and routes were removed, and VM didn't respond to
ping any more (until DORA cycle set IP again).
Did I miss/misunderstood anything significant?
Justin
--
You received this message because you are subscribed to the Google Groups "OSv
Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to osv-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
[COMMIT osv master] cloud-init: trigger dynamic DNS update on hostname change
From: Justin Cinkelj
Committer: Nadav Har'El
Branch: master
cloud-init: trigger dynamic DNS update on hostname change
When cloud-init changes VM hostname, VM DNS name should be updated too.
We try to do this by releasing DHCP lease and requesting new lease
with new name. This instructs DHCP server to perform dynamic DNS update.
DNS update will/should succeed if
1) new name is not already claimed by some other VM/IP and
2) obviously, DHCP and DNS server need to support dynamic DNS updates.
The dhcp_worker::init was split into two parts (::init and ::start), so
that can be discovery-resend logic can be reused.
Signed-off-by: Justin Cinkelj
Message-Id: <20161123144311.29393-1-justin.cink...@xlab.si>
---
diff --git a/core/dhcp.cc b/core/dhcp.cc
--- a/core/dhcp.cc
+++ b/core/dhcp.cc
@@ -66,7 +66,8 @@ int dhcp_hook_rx(struct mbuf* m)
void dhcp_start(bool wait)
{
// Initialize the global DHCP worker
-net_dhcp_worker.init(wait);
+net_dhcp_worker.init();
+net_dhcp_worker.start(wait);
}
// Send DHCP release, for example at shutdown.
@@ -75,6 +76,12 @@ void dhcp_release()
net_dhcp_worker.release();
}
+void dhcp_restart(bool wait)
+{
+net_dhcp_worker.release();
+net_dhcp_worker.start(wait);
+}
+
namespace dhcp {
const char * dhcp_options_magic = "\x63\x82\x53\x63";
@@ -504,8 +511,6 @@ namespace dhcp {
void dhcp_interface_state::discover()
{
-// FIXME: send release packet in case the interface has an address
-
// Update state
_state = DHCP_DISCOVER;
@@ -665,12 +670,10 @@ namespace dhcp {
// FIXME: free packets and states
}
-void dhcp_worker::init(bool wait)
+void dhcp_worker::init()
{
struct ifnet *ifp = nullptr;
-// FIXME: clear routing table (use case run dhclient 2nd time)
-
// Allocate a state for each interface
IFNET_RLOCK();
TAILQ_FOREACH(ifp, &V_ifnet, if_link) {
@@ -686,7 +689,11 @@ namespace dhcp {
_dhcp_thread = sched::thread::make([&] { dhcp_worker_fn(); });
_dhcp_thread->set_name("dhcp");
_dhcp_thread->start();
+}
+void dhcp_worker::start(bool wait)
+{
+// FIXME: clear routing table (use case run dhclient 2nd time)
do {
// Send discover packets!
for (auto &it: _universe) {
@@ -711,6 +718,7 @@ namespace dhcp {
for (auto &it: _universe) {
it.second->release();
}
+_have_ip = false;
// Wait a bit, so hopefully UDP release packets will be actually
put on wire.
usleep(1000);
}
diff --git a/include/osv/dhcp.hh b/include/osv/dhcp.hh
--- a/include/osv/dhcp.hh
+++ b/include/osv/dhcp.hh
@@ -25,6 +25,7 @@
extern "C" {
void dhcp_start(bool wait);
void dhcp_release();
+void dhcp_restart(bool wait);
}
namespace dhcp {
@@ -249,8 +250,11 @@ namespace dhcp {
dhcp_worker();
~dhcp_worker();
-// Initializing a state per interface, sends discover packets
-void init(bool wait);
+// Initializing a state per interface
+void init();
+// Send discover packets
+void start(bool wait);
+// Send release packet for all DHCP IPs.
void release();
void dhcp_worker_fn();
diff --git a/modules/cloud-init/cloud-init.cc
b/modules/cloud-init/cloud-init.cc
--- a/modules/cloud-init/cloud-init.cc
+++ b/modules/cloud-init/cloud-init.cc
@@ -16,6 +16,22 @@
#include
#include
+// we cannot include osv/dhcp.hh, hence direct declaration.
+extern "C" void dhcp_restart(bool wait);
+
+// Set the hostname to given string.
+// If hostname changes, try to propagate the change to DHCP server too.
+void set_hostname_restart_dhcp(std::string hostname) {
+if (hostname.length() > 0) {
+char old_hostname[256] = "";
+gethostname(old_hostname, sizeof(old_hostname));
+sethostname(hostname.c_str(), hostname.length());
+if (hostname != old_hostname) {
+dhcp_restart(true);
+}
+}
+}
+
namespace init {
using namespace std;
@@ -229,9 +245,7 @@ void hostname_module::handle(const YAML::Node& doc)
{
auto hostname = doc.as();
debug("cloudinit hostname: %s\n", hostname.c_str());
-if (hostname.size()) {
-sethostname(hostname.c_str(), hostname.size());
-}
+set_hostname_restart_dhcp(hostname);
}
void osvinit::add_module(std::shared_ptr module)
@@ -265,11 +279,8 @@ void osvinit::load_from_cloud(bool
ignore_missing_source)
try {
auto& ds = get_data_source();
-std::string hostname = ds.external_hostname();
-if (hostname.length() > 0) {
-// Set the hostname from given data source, if it exists.
-sethostname(hostname.c_str(), hostname.length());
-}
+// Set the hostname from given data source, if it exists.
+set_hostname_restart_dhcp(ds.external_hostname());
// Load user data.
user_d
[PATCH v3] cloud-init: trigger dynamic DNS update on hostname change
When cloud-init changes VM hostname, VM DNS name should be updated too.
We try to do this by releasing DHCP lease and requesting new lease
with new name. This instructs DHCP server to perform dynamic DNS update.
DNS update will/should succeed if
1) new name is not already claimed by some other VM/IP and
2) obviously, DHCP and DNS server need to support dynamic DNS updates.
The dhcp_worker::init was split into two parts (::init and ::start), so
that can be discovery-resend logic can be reused.
Signed-off-by: Justin Cinkelj
---
core/dhcp.cc | 20 ++--
include/osv/dhcp.hh | 8 ++--
modules/cloud-init/cloud-init.cc | 27 +++
3 files changed, 39 insertions(+), 16 deletions(-)
diff --git a/core/dhcp.cc b/core/dhcp.cc
index 27d7aef..add540a 100644
--- a/core/dhcp.cc
+++ b/core/dhcp.cc
@@ -66,7 +66,8 @@ int dhcp_hook_rx(struct mbuf* m)
void dhcp_start(bool wait)
{
// Initialize the global DHCP worker
-net_dhcp_worker.init(wait);
+net_dhcp_worker.init();
+net_dhcp_worker.start(wait);
}
// Send DHCP release, for example at shutdown.
@@ -75,6 +76,12 @@ void dhcp_release()
net_dhcp_worker.release();
}
+void dhcp_restart(bool wait)
+{
+net_dhcp_worker.release();
+net_dhcp_worker.start(wait);
+}
+
namespace dhcp {
const char * dhcp_options_magic = "\x63\x82\x53\x63";
@@ -504,8 +511,6 @@ namespace dhcp {
void dhcp_interface_state::discover()
{
-// FIXME: send release packet in case the interface has an address
-
// Update state
_state = DHCP_DISCOVER;
@@ -665,12 +670,10 @@ namespace dhcp {
// FIXME: free packets and states
}
-void dhcp_worker::init(bool wait)
+void dhcp_worker::init()
{
struct ifnet *ifp = nullptr;
-// FIXME: clear routing table (use case run dhclient 2nd time)
-
// Allocate a state for each interface
IFNET_RLOCK();
TAILQ_FOREACH(ifp, &V_ifnet, if_link) {
@@ -686,7 +689,11 @@ namespace dhcp {
_dhcp_thread = sched::thread::make([&] { dhcp_worker_fn(); });
_dhcp_thread->set_name("dhcp");
_dhcp_thread->start();
+}
+void dhcp_worker::start(bool wait)
+{
+// FIXME: clear routing table (use case run dhclient 2nd time)
do {
// Send discover packets!
for (auto &it: _universe) {
@@ -711,6 +718,7 @@ namespace dhcp {
for (auto &it: _universe) {
it.second->release();
}
+_have_ip = false;
// Wait a bit, so hopefully UDP release packets will be actually put
on wire.
usleep(1000);
}
diff --git a/include/osv/dhcp.hh b/include/osv/dhcp.hh
index 30ca86f..b286727 100644
--- a/include/osv/dhcp.hh
+++ b/include/osv/dhcp.hh
@@ -25,6 +25,7 @@
extern "C" {
void dhcp_start(bool wait);
void dhcp_release();
+void dhcp_restart(bool wait);
}
namespace dhcp {
@@ -249,8 +250,11 @@ namespace dhcp {
dhcp_worker();
~dhcp_worker();
-// Initializing a state per interface, sends discover packets
-void init(bool wait);
+// Initializing a state per interface
+void init();
+// Send discover packets
+void start(bool wait);
+// Send release packet for all DHCP IPs.
void release();
void dhcp_worker_fn();
diff --git a/modules/cloud-init/cloud-init.cc b/modules/cloud-init/cloud-init.cc
index 67f477e..e3ec8f8 100644
--- a/modules/cloud-init/cloud-init.cc
+++ b/modules/cloud-init/cloud-init.cc
@@ -16,6 +16,22 @@
#include
#include
+// we cannot include osv/dhcp.hh, hence direct declaration.
+extern "C" void dhcp_restart(bool wait);
+
+// Set the hostname to given string.
+// If hostname changes, try to propagate the change to DHCP server too.
+void set_hostname_restart_dhcp(std::string hostname) {
+if (hostname.length() > 0) {
+char old_hostname[256] = "";
+gethostname(old_hostname, sizeof(old_hostname));
+sethostname(hostname.c_str(), hostname.length());
+if (hostname != old_hostname) {
+dhcp_restart(true);
+}
+}
+}
+
namespace init {
using namespace std;
@@ -229,9 +245,7 @@ void hostname_module::handle(const YAML::Node& doc)
{
auto hostname = doc.as();
debug("cloudinit hostname: %s\n", hostname.c_str());
-if (hostname.size()) {
-sethostname(hostname.c_str(), hostname.size());
-}
+set_hostname_restart_dhcp(hostname);
}
void osvinit::add_module(std::shared_ptr module)
@@ -265,11 +279,8 @@ void osvinit::load_from_cloud(bool ignore_missing_source)
try {
auto& ds = get_data_source();
-std::string hostname = ds.external_hostname();
-if (hostname.length() > 0) {
-// Set the hostname from given data source, if it exists.
-sethostname(hostname.c_str(), hostname.length());
-}
+// Set the hostname f
Re: [PATCH v2 2/2] cloud-init: trigger dynamic DNS update on hostname change
On Wed, Nov 23, 2016 at 3:42 PM, Justin Cinkelj
wrote:
>
>
> On 11/23/2016 02:32 PM, Nadav Har'El wrote:
>
>
> On Wed, Nov 23, 2016 at 3:27 PM, Justin Cinkelj
> wrote:
>
>> When cloud-init changes VM hostname, VM DNS name should be updated too.
>> We try to do this by releasing DHCP lease and requesting new lease
>> with new name. This instructs DHCP server to perform dynamic DNS update.
>> DNS update will/should succeed if
>> 1) new name is not already claimed by some other VM/IP and
>> 2) obviously, DHCP and DNS server need to support dynamic DNS updates.
>>
>> The dhcp_worker::init was split into two parts (::init and ::start), so
>> that can be discovery-resend logic can be reused.
>>
>> Signed-off-by: Justin Cinkelj
>> ---
>> core/dhcp.cc | 23 ++-
>> include/osv/dhcp.hh | 8 ++--
>> modules/cloud-init/cloud-init.cc | 27 +++
>> 3 files changed, 43 insertions(+), 15 deletions(-)
>>
>> diff --git a/core/dhcp.cc b/core/dhcp.cc
>> index 27d7aef..d89eb46 100644
>> --- a/core/dhcp.cc
>> +++ b/core/dhcp.cc
>> @@ -66,7 +66,8 @@ int dhcp_hook_rx(struct mbuf* m)
>> void dhcp_start(bool wait)
>> {
>> // Initialize the global DHCP worker
>> -net_dhcp_worker.init(wait);
>> +net_dhcp_worker.init();
>> +net_dhcp_worker.start(wait);
>> }
>>
>> // Send DHCP release, for example at shutdown.
>> @@ -75,6 +76,12 @@ void dhcp_release()
>> net_dhcp_worker.release();
>> }
>>
>> +void dhcp_restart(bool wait)
>> +{
>> +net_dhcp_worker.release();
>> +net_dhcp_worker.start(wait);
>> +}
>> +
>> namespace dhcp {
>>
>> const char * dhcp_options_magic = "\x63\x82\x53\x63";
>> @@ -504,7 +511,10 @@ namespace dhcp {
>>
>> void dhcp_interface_state::discover()
>> {
>> -// FIXME: send release packet in case the interface has an
>> address
>> +// send release packet in case the interface has an address
>> +if (_client_addr != ipv4_zero) {
>> +release();
>> +}
>>
>
> I don't know why this FIXME was correct, or why this extra code was
> needed: In both the dhcp_start() and dhcp_restart() cases, we start with
> zero address (in dhcp_restart() you call release() already). So why do we
> need to check it again here?
>
> I just noted the fixme line, and then added the if sentence for
> just-in-case.
> I don't see how current code could send release via this 3 lines; maybe if
> at some later time
> we/I forget to call release before discover;
>
> Maybe I should just remove the fixme comment?
>
Yes, I think that would make sense.
By the way, I already committed the second patch in this series.
>
> Justin
>
>
> // Update state
>> _state = DHCP_DISCOVER;
>> @@ -665,12 +675,10 @@ namespace dhcp {
>> // FIXME: free packets and states
>> }
>>
>> -void dhcp_worker::init(bool wait)
>> +void dhcp_worker::init()
>> {
>> struct ifnet *ifp = nullptr;
>>
>> -// FIXME: clear routing table (use case run dhclient 2nd time)
>> -
>> // Allocate a state for each interface
>> IFNET_RLOCK();
>> TAILQ_FOREACH(ifp, &V_ifnet, if_link) {
>> @@ -686,7 +694,11 @@ namespace dhcp {
>> _dhcp_thread = sched::thread::make([&] { dhcp_worker_fn(); });
>> _dhcp_thread->set_name("dhcp");
>> _dhcp_thread->start();
>> +}
>>
>> +void dhcp_worker::start(bool wait)
>> +{
>> +// FIXME: clear routing table (use case run dhclient 2nd time)
>> do {
>> // Send discover packets!
>> for (auto &it: _universe) {
>> @@ -711,6 +723,7 @@ namespace dhcp {
>> for (auto &it: _universe) {
>> it.second->release();
>> }
>> +_have_ip = false;
>> // Wait a bit, so hopefully UDP release packets will be actually
>> put on wire.
>> usleep(1000);
>> }
>> diff --git a/include/osv/dhcp.hh b/include/osv/dhcp.hh
>> index 30ca86f..b286727 100644
>> --- a/include/osv/dhcp.hh
>> +++ b/include/osv/dhcp.hh
>> @@ -25,6 +25,7 @@
>> extern "C" {
>> void dhcp_start(bool wait);
>> void dhcp_release();
>> +void dhcp_restart(bool wait);
>> }
>>
>> namespace dhcp {
>> @@ -249,8 +250,11 @@ namespace dhcp {
>> dhcp_worker();
>> ~dhcp_worker();
>>
>> -// Initializing a state per interface, sends discover packets
>> -void init(bool wait);
>> +// Initializing a state per interface
>> +void init();
>> +// Send discover packets
>> +void start(bool wait);
>> +// Send release packet for all DHCP IPs.
>> void release();
>>
>> void dhcp_worker_fn();
>> diff --git a/modules/cloud-init/cloud-init.cc
>> b/modules/cloud-init/cloud-init.cc
>> index 67f477e..e3ec8f8 100644
>> --- a/modules/cloud-init/cloud-init.cc
>> +++ b/modules/cloud-init/cloud-init.cc
>> @@ -16,6 +16,22 @@
>> #include
>> #include
>>
>> +// we cannot include o
[COMMIT osv master] dhcp: set src and dest IP in DHCP release packet
From: Justin Cinkelj
Committer: Nadav Har'El
Branch: master
dhcp: set src and dest IP in DHCP release packet
DHCP release packet should have valid IP header. There is no need
to use INADDR_ANY/INADDR_BROADCAST as src/dest address.
Signed-off-by: Justin Cinkelj
Message-Id: <20161123132746.27570-2-justin.cink...@xlab.si>
---
diff --git a/core/dhcp.cc b/core/dhcp.cc
--- a/core/dhcp.cc
+++ b/core/dhcp.cc
@@ -197,7 +197,7 @@ namespace dhcp {
*options++ = DHCP_OPTION_END;
dhcp_len += options - options_start;
-build_udp_ip_headers(dhcp_len);
+build_udp_ip_headers(dhcp_len, INADDR_ANY, INADDR_BROADCAST);
}
void dhcp_mbuf::compose_request(struct ifnet* ifp,
@@ -239,7 +239,7 @@ namespace dhcp {
*options++ = DHCP_OPTION_END;
dhcp_len += options - options_start;
-build_udp_ip_headers(dhcp_len);
+build_udp_ip_headers(dhcp_len, INADDR_ANY, INADDR_BROADCAST);
}
void dhcp_mbuf::compose_release(struct ifnet* ifp,
@@ -275,7 +275,7 @@ namespace dhcp {
*options++ = DHCP_OPTION_END;
dhcp_len += options - options_start;
-build_udp_ip_headers(dhcp_len);
+build_udp_ip_headers(dhcp_len, yip_n, sip_n);
}
u32 dhcp_mbuf::get_xid()
@@ -456,7 +456,7 @@ namespace dhcp {
return pos + 2 + len;
}
-void dhcp_mbuf::build_udp_ip_headers(size_t dhcp_len)
+void dhcp_mbuf::build_udp_ip_headers(size_t dhcp_len, in_addr_t
src_addr, in_addr_t dest_addr)
{
struct ip* ip = pip();
struct udphdr* udp = pudp();
@@ -473,8 +473,8 @@ namespace dhcp {
ip->ip_ttl = 128;
ip->ip_p = IPPROTO_UDP;
ip->ip_sum = 0;
-ip->ip_src.s_addr = INADDR_ANY;
-ip->ip_dst.s_addr = INADDR_BROADCAST;
+ip->ip_src.s_addr = src_addr;
+ip->ip_dst.s_addr = dest_addr;
ip->ip_sum = in_cksum(_m, min_ip_len);
// UDP
diff --git a/include/osv/dhcp.hh b/include/osv/dhcp.hh
--- a/include/osv/dhcp.hh
+++ b/include/osv/dhcp.hh
@@ -168,7 +168,7 @@ namespace dhcp {
u8* add_option(u8* pos, u8 type, u8 len, u8 data); // memset
// Packet assembly
-void build_udp_ip_headers(size_t dhcp_len);
+void build_udp_ip_headers(size_t dhcp_len, in_addr_t src_addr,
in_addr_t dest_addr);
// mbuf related
void allocate_mbuf();
--
You received this message because you are subscribed to the Google Groups "OSv
Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to osv-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
Re: [PATCH v2 2/2] cloud-init: trigger dynamic DNS update on hostname change
On 11/23/2016 02:32 PM, Nadav Har'El wrote:
On Wed, Nov 23, 2016 at 3:27 PM, Justin Cinkelj
mailto:justin.cink...@xlab.si>> wrote:
When cloud-init changes VM hostname, VM DNS name should be updated
too.
We try to do this by releasing DHCP lease and requesting new lease
with new name. This instructs DHCP server to perform dynamic DNS
update.
DNS update will/should succeed if
1) new name is not already claimed by some other VM/IP and
2) obviously, DHCP and DNS server need to support dynamic DNS
updates.
The dhcp_worker::init was split into two parts (::init and
::start), so
that can be discovery-resend logic can be reused.
Signed-off-by: Justin Cinkelj mailto:justin.cink...@xlab.si>>
---
core/dhcp.cc | 23 ++-
include/osv/dhcp.hh | 8 ++--
modules/cloud-init/cloud-init.cc | 27 +++
3 files changed, 43 insertions(+), 15 deletions(-)
diff --git a/core/dhcp.cc b/core/dhcp.cc
index 27d7aef..d89eb46 100644
--- a/core/dhcp.cc
+++ b/core/dhcp.cc
@@ -66,7 +66,8 @@ int dhcp_hook_rx(struct mbuf* m)
void dhcp_start(bool wait)
{
// Initialize the global DHCP worker
-net_dhcp_worker.init(wait);
+net_dhcp_worker.init();
+net_dhcp_worker.start(wait);
}
// Send DHCP release, for example at shutdown.
@@ -75,6 +76,12 @@ void dhcp_release()
net_dhcp_worker.release();
}
+void dhcp_restart(bool wait)
+{
+net_dhcp_worker.release();
+net_dhcp_worker.start(wait);
+}
+
namespace dhcp {
const char * dhcp_options_magic = "\x63\x82\x53\x63";
@@ -504,7 +511,10 @@ namespace dhcp {
void dhcp_interface_state::discover()
{
-// FIXME: send release packet in case the interface has
an address
+// send release packet in case the interface has an address
+if (_client_addr != ipv4_zero) {
+release();
+}
I don't know why this FIXME was correct, or why this extra code was
needed: In both the dhcp_start() and dhcp_restart() cases, we start
with zero address (in dhcp_restart() you call release() already). So
why do we need to check it again here?
I just noted the fixme line, and then added the if sentence for
just-in-case.
I don't see how current code could send release via this 3 lines; maybe
if at some later time
we/I forget to call release before discover;
Maybe I should just remove the fixme comment?
Justin
// Update state
_state = DHCP_DISCOVER;
@@ -665,12 +675,10 @@ namespace dhcp {
// FIXME: free packets and states
}
-void dhcp_worker::init(bool wait)
+void dhcp_worker::init()
{
struct ifnet *ifp = nullptr;
-// FIXME: clear routing table (use case run dhclient 2nd
time)
-
// Allocate a state for each interface
IFNET_RLOCK();
TAILQ_FOREACH(ifp, &V_ifnet, if_link) {
@@ -686,7 +694,11 @@ namespace dhcp {
_dhcp_thread = sched::thread::make([&] {
dhcp_worker_fn(); });
_dhcp_thread->set_name("dhcp");
_dhcp_thread->start();
+}
+void dhcp_worker::start(bool wait)
+{
+// FIXME: clear routing table (use case run dhclient 2nd
time)
do {
// Send discover packets!
for (auto &it: _universe) {
@@ -711,6 +723,7 @@ namespace dhcp {
for (auto &it: _universe) {
it.second->release();
}
+_have_ip = false;
// Wait a bit, so hopefully UDP release packets will be
actually put on wire.
usleep(1000);
}
diff --git a/include/osv/dhcp.hh b/include/osv/dhcp.hh
index 30ca86f..b286727 100644
--- a/include/osv/dhcp.hh
+++ b/include/osv/dhcp.hh
@@ -25,6 +25,7 @@
extern "C" {
void dhcp_start(bool wait);
void dhcp_release();
+void dhcp_restart(bool wait);
}
namespace dhcp {
@@ -249,8 +250,11 @@ namespace dhcp {
dhcp_worker();
~dhcp_worker();
-// Initializing a state per interface, sends discover packets
-void init(bool wait);
+// Initializing a state per interface
+void init();
+// Send discover packets
+void start(bool wait);
+// Send release packet for all DHCP IPs.
void release();
void dhcp_worker_fn();
diff --git a/modules/cloud-init/cloud-init.cc
b/modules/cloud-init/cloud-init.cc
index 67f477e..e3ec8f8 100644
--- a/modules/cloud-init/cloud-init.cc
+++ b/modules/cloud-init/cloud-init.cc
@@ -16,6 +16,22 @@
#include
#include
+// we cannot include osv/dhcp.hh, hence di
Re: [PATCH v2 2/2] cloud-init: trigger dynamic DNS update on hostname change
On Wed, Nov 23, 2016 at 3:27 PM, Justin Cinkelj
wrote:
> When cloud-init changes VM hostname, VM DNS name should be updated too.
> We try to do this by releasing DHCP lease and requesting new lease
> with new name. This instructs DHCP server to perform dynamic DNS update.
> DNS update will/should succeed if
> 1) new name is not already claimed by some other VM/IP and
> 2) obviously, DHCP and DNS server need to support dynamic DNS updates.
>
> The dhcp_worker::init was split into two parts (::init and ::start), so
> that can be discovery-resend logic can be reused.
>
> Signed-off-by: Justin Cinkelj
> ---
> core/dhcp.cc | 23 ++-
> include/osv/dhcp.hh | 8 ++--
> modules/cloud-init/cloud-init.cc | 27 +++
> 3 files changed, 43 insertions(+), 15 deletions(-)
>
> diff --git a/core/dhcp.cc b/core/dhcp.cc
> index 27d7aef..d89eb46 100644
> --- a/core/dhcp.cc
> +++ b/core/dhcp.cc
> @@ -66,7 +66,8 @@ int dhcp_hook_rx(struct mbuf* m)
> void dhcp_start(bool wait)
> {
> // Initialize the global DHCP worker
> -net_dhcp_worker.init(wait);
> +net_dhcp_worker.init();
> +net_dhcp_worker.start(wait);
> }
>
> // Send DHCP release, for example at shutdown.
> @@ -75,6 +76,12 @@ void dhcp_release()
> net_dhcp_worker.release();
> }
>
> +void dhcp_restart(bool wait)
> +{
> +net_dhcp_worker.release();
> +net_dhcp_worker.start(wait);
> +}
> +
> namespace dhcp {
>
> const char * dhcp_options_magic = "\x63\x82\x53\x63";
> @@ -504,7 +511,10 @@ namespace dhcp {
>
> void dhcp_interface_state::discover()
> {
> -// FIXME: send release packet in case the interface has an address
> +// send release packet in case the interface has an address
> +if (_client_addr != ipv4_zero) {
> +release();
> +}
>
I don't know why this FIXME was correct, or why this extra code was needed:
In both the dhcp_start() and dhcp_restart() cases, we start with zero
address (in dhcp_restart() you call release() already). So why do we need
to check it again here?
>
> // Update state
> _state = DHCP_DISCOVER;
> @@ -665,12 +675,10 @@ namespace dhcp {
> // FIXME: free packets and states
> }
>
> -void dhcp_worker::init(bool wait)
> +void dhcp_worker::init()
> {
> struct ifnet *ifp = nullptr;
>
> -// FIXME: clear routing table (use case run dhclient 2nd time)
> -
> // Allocate a state for each interface
> IFNET_RLOCK();
> TAILQ_FOREACH(ifp, &V_ifnet, if_link) {
> @@ -686,7 +694,11 @@ namespace dhcp {
> _dhcp_thread = sched::thread::make([&] { dhcp_worker_fn(); });
> _dhcp_thread->set_name("dhcp");
> _dhcp_thread->start();
> +}
>
> +void dhcp_worker::start(bool wait)
> +{
> +// FIXME: clear routing table (use case run dhclient 2nd time)
> do {
> // Send discover packets!
> for (auto &it: _universe) {
> @@ -711,6 +723,7 @@ namespace dhcp {
> for (auto &it: _universe) {
> it.second->release();
> }
> +_have_ip = false;
> // Wait a bit, so hopefully UDP release packets will be actually
> put on wire.
> usleep(1000);
> }
> diff --git a/include/osv/dhcp.hh b/include/osv/dhcp.hh
> index 30ca86f..b286727 100644
> --- a/include/osv/dhcp.hh
> +++ b/include/osv/dhcp.hh
> @@ -25,6 +25,7 @@
> extern "C" {
> void dhcp_start(bool wait);
> void dhcp_release();
> +void dhcp_restart(bool wait);
> }
>
> namespace dhcp {
> @@ -249,8 +250,11 @@ namespace dhcp {
> dhcp_worker();
> ~dhcp_worker();
>
> -// Initializing a state per interface, sends discover packets
> -void init(bool wait);
> +// Initializing a state per interface
> +void init();
> +// Send discover packets
> +void start(bool wait);
> +// Send release packet for all DHCP IPs.
> void release();
>
> void dhcp_worker_fn();
> diff --git a/modules/cloud-init/cloud-init.cc b/modules/cloud-init/cloud-
> init.cc
> index 67f477e..e3ec8f8 100644
> --- a/modules/cloud-init/cloud-init.cc
> +++ b/modules/cloud-init/cloud-init.cc
> @@ -16,6 +16,22 @@
> #include
> #include
>
> +// we cannot include osv/dhcp.hh, hence direct declaration.
> +extern "C" void dhcp_restart(bool wait);
> +
> +// Set the hostname to given string.
> +// If hostname changes, try to propagate the change to DHCP server too.
> +void set_hostname_restart_dhcp(std::string hostname) {
> +if (hostname.length() > 0) {
> +char old_hostname[256] = "";
> +gethostname(old_hostname, sizeof(old_hostname));
> +sethostname(hostname.c_str(), hostname.length());
> +if (hostname != old_hostname) {
> +dhcp_restart(true);
> +}
> +}
> +}
> +
> namespace init {
> using namespace std;
>
> @@ -229,9 +245,7 @@ void hostname_mo
[PATCH v2 2/2] cloud-init: trigger dynamic DNS update on hostname change
When cloud-init changes VM hostname, VM DNS name should be updated too.
We try to do this by releasing DHCP lease and requesting new lease
with new name. This instructs DHCP server to perform dynamic DNS update.
DNS update will/should succeed if
1) new name is not already claimed by some other VM/IP and
2) obviously, DHCP and DNS server need to support dynamic DNS updates.
The dhcp_worker::init was split into two parts (::init and ::start), so
that can be discovery-resend logic can be reused.
Signed-off-by: Justin Cinkelj
---
core/dhcp.cc | 23 ++-
include/osv/dhcp.hh | 8 ++--
modules/cloud-init/cloud-init.cc | 27 +++
3 files changed, 43 insertions(+), 15 deletions(-)
diff --git a/core/dhcp.cc b/core/dhcp.cc
index 27d7aef..d89eb46 100644
--- a/core/dhcp.cc
+++ b/core/dhcp.cc
@@ -66,7 +66,8 @@ int dhcp_hook_rx(struct mbuf* m)
void dhcp_start(bool wait)
{
// Initialize the global DHCP worker
-net_dhcp_worker.init(wait);
+net_dhcp_worker.init();
+net_dhcp_worker.start(wait);
}
// Send DHCP release, for example at shutdown.
@@ -75,6 +76,12 @@ void dhcp_release()
net_dhcp_worker.release();
}
+void dhcp_restart(bool wait)
+{
+net_dhcp_worker.release();
+net_dhcp_worker.start(wait);
+}
+
namespace dhcp {
const char * dhcp_options_magic = "\x63\x82\x53\x63";
@@ -504,7 +511,10 @@ namespace dhcp {
void dhcp_interface_state::discover()
{
-// FIXME: send release packet in case the interface has an address
+// send release packet in case the interface has an address
+if (_client_addr != ipv4_zero) {
+release();
+}
// Update state
_state = DHCP_DISCOVER;
@@ -665,12 +675,10 @@ namespace dhcp {
// FIXME: free packets and states
}
-void dhcp_worker::init(bool wait)
+void dhcp_worker::init()
{
struct ifnet *ifp = nullptr;
-// FIXME: clear routing table (use case run dhclient 2nd time)
-
// Allocate a state for each interface
IFNET_RLOCK();
TAILQ_FOREACH(ifp, &V_ifnet, if_link) {
@@ -686,7 +694,11 @@ namespace dhcp {
_dhcp_thread = sched::thread::make([&] { dhcp_worker_fn(); });
_dhcp_thread->set_name("dhcp");
_dhcp_thread->start();
+}
+void dhcp_worker::start(bool wait)
+{
+// FIXME: clear routing table (use case run dhclient 2nd time)
do {
// Send discover packets!
for (auto &it: _universe) {
@@ -711,6 +723,7 @@ namespace dhcp {
for (auto &it: _universe) {
it.second->release();
}
+_have_ip = false;
// Wait a bit, so hopefully UDP release packets will be actually put
on wire.
usleep(1000);
}
diff --git a/include/osv/dhcp.hh b/include/osv/dhcp.hh
index 30ca86f..b286727 100644
--- a/include/osv/dhcp.hh
+++ b/include/osv/dhcp.hh
@@ -25,6 +25,7 @@
extern "C" {
void dhcp_start(bool wait);
void dhcp_release();
+void dhcp_restart(bool wait);
}
namespace dhcp {
@@ -249,8 +250,11 @@ namespace dhcp {
dhcp_worker();
~dhcp_worker();
-// Initializing a state per interface, sends discover packets
-void init(bool wait);
+// Initializing a state per interface
+void init();
+// Send discover packets
+void start(bool wait);
+// Send release packet for all DHCP IPs.
void release();
void dhcp_worker_fn();
diff --git a/modules/cloud-init/cloud-init.cc b/modules/cloud-init/cloud-init.cc
index 67f477e..e3ec8f8 100644
--- a/modules/cloud-init/cloud-init.cc
+++ b/modules/cloud-init/cloud-init.cc
@@ -16,6 +16,22 @@
#include
#include
+// we cannot include osv/dhcp.hh, hence direct declaration.
+extern "C" void dhcp_restart(bool wait);
+
+// Set the hostname to given string.
+// If hostname changes, try to propagate the change to DHCP server too.
+void set_hostname_restart_dhcp(std::string hostname) {
+if (hostname.length() > 0) {
+char old_hostname[256] = "";
+gethostname(old_hostname, sizeof(old_hostname));
+sethostname(hostname.c_str(), hostname.length());
+if (hostname != old_hostname) {
+dhcp_restart(true);
+}
+}
+}
+
namespace init {
using namespace std;
@@ -229,9 +245,7 @@ void hostname_module::handle(const YAML::Node& doc)
{
auto hostname = doc.as();
debug("cloudinit hostname: %s\n", hostname.c_str());
-if (hostname.size()) {
-sethostname(hostname.c_str(), hostname.size());
-}
+set_hostname_restart_dhcp(hostname);
}
void osvinit::add_module(std::shared_ptr module)
@@ -265,11 +279,8 @@ void osvinit::load_from_cloud(bool ignore_missing_source)
try {
auto& ds = get_data_source();
-std::string hostname = ds.external_hostname();
-if (hostname.length() > 0) {
-// Set the hos
[PATCH v2 0/2] cloud-init: trigger dynamic DNS update on hostname change
Additional patch 1/2 added, to set src and dest IP in DHCP release packet. The 2/2 patch is updated. Also on second DORA try is the discovery packet resend if server doesn't respond in time. The dhcp_worker and dhcp_interface_state restart function was removed - as Nadav noted, the were not really needed. The routing table is not cleared on DHCP release. I just have to look a bit more at this, and still wish to send patches early. Justin Cinkelj (2): dhcp: set src and dest IP in DHCP release packet cloud-init: trigger dynamic DNS update on hostname change core/dhcp.cc | 35 --- include/osv/dhcp.hh | 10 +++--- modules/cloud-init/cloud-init.cc | 27 +++ 3 files changed, 50 insertions(+), 22 deletions(-) -- 2.9.3 -- You received this message because you are subscribed to the Google Groups "OSv Development" group. To unsubscribe from this group and stop receiving emails from it, send an email to osv-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[PATCH v2 1/2] dhcp: set src and dest IP in DHCP release packet
DHCP release packet should have valid IP header. There is no need
to use INADDR_ANY/INADDR_BROADCAST as src/dest address.
Signed-off-by: Justin Cinkelj
---
core/dhcp.cc| 12 ++--
include/osv/dhcp.hh | 2 +-
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/core/dhcp.cc b/core/dhcp.cc
index 689a387..27d7aef 100644
--- a/core/dhcp.cc
+++ b/core/dhcp.cc
@@ -197,7 +197,7 @@ namespace dhcp {
*options++ = DHCP_OPTION_END;
dhcp_len += options - options_start;
-build_udp_ip_headers(dhcp_len);
+build_udp_ip_headers(dhcp_len, INADDR_ANY, INADDR_BROADCAST);
}
void dhcp_mbuf::compose_request(struct ifnet* ifp,
@@ -239,7 +239,7 @@ namespace dhcp {
*options++ = DHCP_OPTION_END;
dhcp_len += options - options_start;
-build_udp_ip_headers(dhcp_len);
+build_udp_ip_headers(dhcp_len, INADDR_ANY, INADDR_BROADCAST);
}
void dhcp_mbuf::compose_release(struct ifnet* ifp,
@@ -275,7 +275,7 @@ namespace dhcp {
*options++ = DHCP_OPTION_END;
dhcp_len += options - options_start;
-build_udp_ip_headers(dhcp_len);
+build_udp_ip_headers(dhcp_len, yip_n, sip_n);
}
u32 dhcp_mbuf::get_xid()
@@ -456,7 +456,7 @@ namespace dhcp {
return pos + 2 + len;
}
-void dhcp_mbuf::build_udp_ip_headers(size_t dhcp_len)
+void dhcp_mbuf::build_udp_ip_headers(size_t dhcp_len, in_addr_t src_addr,
in_addr_t dest_addr)
{
struct ip* ip = pip();
struct udphdr* udp = pudp();
@@ -473,8 +473,8 @@ namespace dhcp {
ip->ip_ttl = 128;
ip->ip_p = IPPROTO_UDP;
ip->ip_sum = 0;
-ip->ip_src.s_addr = INADDR_ANY;
-ip->ip_dst.s_addr = INADDR_BROADCAST;
+ip->ip_src.s_addr = src_addr;
+ip->ip_dst.s_addr = dest_addr;
ip->ip_sum = in_cksum(_m, min_ip_len);
// UDP
diff --git a/include/osv/dhcp.hh b/include/osv/dhcp.hh
index 55e0b16..30ca86f 100644
--- a/include/osv/dhcp.hh
+++ b/include/osv/dhcp.hh
@@ -168,7 +168,7 @@ namespace dhcp {
u8* add_option(u8* pos, u8 type, u8 len, u8 data); // memset
// Packet assembly
-void build_udp_ip_headers(size_t dhcp_len);
+void build_udp_ip_headers(size_t dhcp_len, in_addr_t src_addr,
in_addr_t dest_addr);
// mbuf related
void allocate_mbuf();
--
2.9.3
--
You received this message because you are subscribed to the Google Groups "OSv
Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to osv-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
Re: [PATCH] cloud-init: trigger dynamic DNS update on hostname change
On 11/23/2016 11:39 AM, Nadav Har'El wrote: On Wed, Nov 23, 2016 at 11:20 AM, Justin Cinkelj mailto:justin.cink...@xlab.si>> wrote: - Original Message - > From: "Nadav Har'El" mailto:n...@scylladb.com>> > To: "Justin Cinkelj" mailto:justin.cink...@xlab.si>> > Cc: "Osv Dev" mailto:osv-dev@googlegroups.com>> > Sent: Wednesday, November 23, 2016 8:31:01 AM > Subject: Re: [PATCH] cloud-init: trigger dynamic DNS update on hostname change > > Hi Justin, some comments inline below: > > On Tue, Nov 22, 2016 at 11:37 PM, Justin Cinkelj mailto:justin.cink...@xlab.si>> > wrote: > > > When cloud-init changes VM hostname, VM DNS name should be updated too. > > We try to do this by releasing DHCP lease and requesting new lease > > > > Is the "release" part actually necessary? Can't we start the initialization > again without a release? (and even more optimized version would be to > contact the server directly, but let's not try to optimize this case too > much)? Hm, I didn't think about not doing release. One potential problem - old name might not get released. Optimized version would be to contact the server directly - which server? The DHCP server. I thought that instead of sending a new broadcast (which one does before he knows what is the DHCP server) maybe you can send the message directly to the server, like we do in the RELEASE case. But this optimization is probably not worth it, and I don't know if it's even possible. You are right, sending to server IP is better. I would like to think that when a DHCP server gets a request from a MAC address which it already gave an IP address and/or dns entry for, it will remove the old entries before giving out new ones. I didn't test this, however. Again, because the "RELEASE" operation is considered, also by the DHCP docs, as "optional" because of the fact it doesn't have an ack. So I doubt the correct operationof any DHCP server can rely on release taking place. Anyway, I'm not against this release() - it will have a tiny performance impact. Feel free to keep it. -- You received this message because you are subscribed to the Google Groups "OSv Development" group. To unsubscribe from this group and stop receiving emails from it, send an email to osv-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
Re: [PATCH] cloud-init: trigger dynamic DNS update on hostname change
On Wed, Nov 23, 2016 at 11:20 AM, Justin Cinkelj wrote: > > > - Original Message - > > From: "Nadav Har'El" > > To: "Justin Cinkelj" > > Cc: "Osv Dev" > > Sent: Wednesday, November 23, 2016 8:31:01 AM > > Subject: Re: [PATCH] cloud-init: trigger dynamic DNS update on hostname > change > > > > Hi Justin, some comments inline below: > > > > On Tue, Nov 22, 2016 at 11:37 PM, Justin Cinkelj > > > wrote: > > > > > When cloud-init changes VM hostname, VM DNS name should be updated too. > > > We try to do this by releasing DHCP lease and requesting new lease > > > > > > > Is the "release" part actually necessary? Can't we start the > initialization > > again without a release? (and even more optimized version would be to > > contact the server directly, but let's not try to optimize this case too > > much)? > > Hm, I didn't think about not doing release. One potential problem - old > name > might not get released. > > Optimized version would be to contact the server directly - which server? > The DHCP server. I thought that instead of sending a new broadcast (which one does before he knows what is the DHCP server) maybe you can send the message directly to the server, like we do in the RELEASE case. But this optimization is probably not worth it, and I don't know if it's even possible. I would like to think that when a DHCP server gets a request from a MAC address which it already gave an IP address and/or dns entry for, it will remove the old entries before giving out new ones. I didn't test this, however. Again, because the "RELEASE" operation is considered, also by the DHCP docs, as "optional" because of the fact it doesn't have an ack. So I doubt the correct operationof any DHCP server can rely on release taking place. Anyway, I'm not against this release() - it will have a tiny performance impact. Feel free to keep it. -- You received this message because you are subscribed to the Google Groups "OSv Development" group. To unsubscribe from this group and stop receiving emails from it, send an email to osv-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
Re: [PATCH] cloud-init: trigger dynamic DNS update on hostname change
- Original Message -
> From: "Nadav Har'El"
> To: "Justin Cinkelj"
> Cc: "Osv Dev"
> Sent: Wednesday, November 23, 2016 8:31:01 AM
> Subject: Re: [PATCH] cloud-init: trigger dynamic DNS update on hostname change
>
> Hi Justin, some comments inline below:
>
> On Tue, Nov 22, 2016 at 11:37 PM, Justin Cinkelj
> wrote:
>
> > When cloud-init changes VM hostname, VM DNS name should be updated too.
> > We try to do this by releasing DHCP lease and requesting new lease
> >
>
> Is the "release" part actually necessary? Can't we start the initialization
> again without a release? (and even more optimized version would be to
> contact the server directly, but let's not try to optimize this case too
> much)?
Hm, I didn't think about not doing release. One potential problem - old name
might not get released.
Optimized version would be to contact the server directly - which server?
Do you mean that DHCP client would directly talk to DNS server? That cannot
work, DHCP server usually has trusted key allowing it to update the DNS server.
>
> The reason I'm asking this is that release() is unreliable and cannot be
> assumed to do anything (since it has no acknowledgement). So I wonder if
> it's worth calling at all. Maybe it can't hurt, but it just seems weird to
> me to do something which isn't guaranteed to work.
Its all on UDP anyway. In practice, as we just finished one DORA cycle, network
is working, packet drop is hopefully low, and release will work. If release gets
lost, then there is not much we can do. But I think it is worth to try to
release, just to be kind against DHCP server.
>
>
> > with new name. This instructs DHCP server to perform dynamic DNS update.
> > DNS update will/should succeed if
> > 1) new name is not already claimed by some other VM/IP and
> > 2) obviously, DHCP and DNS server need to support dynamic DNS updates.
> >
> > Signed-off-by: Justin Cinkelj
> > ---
> > core/dhcp.cc | 17 +
> > include/osv/dhcp.hh | 7 +++
> > modules/cloud-init/cloud-init.cc | 27 +++
> > 3 files changed, 43 insertions(+), 8 deletions(-)
> >
> > diff --git a/core/dhcp.cc b/core/dhcp.cc
> > index 689a387..f33a091 100644
> > --- a/core/dhcp.cc
> > +++ b/core/dhcp.cc
> > @@ -75,6 +75,11 @@ void dhcp_release()
> > net_dhcp_worker.release();
> > }
> >
> > +void dhcp_restart()
> > +{
> > +net_dhcp_worker.restart();
> > +}
> > +
> > namespace dhcp {
> >
> > const char * dhcp_options_magic = "\x63\x82\x53\x63";
> > @@ -648,6 +653,11 @@ namespace dhcp {
> > //couple of time
> > }
> >
> > +void dhcp_interface_state::restart() {
> > +release();
> > +discover();
> >
>
>
> Is discover() really what we want to do here?
> It's been a while since I looked at this code, but it seems to me that
> discover() is just an internal function to send a discover packet, once.
> the init() code does more than this - it also waits for an ack, and retries
> if an ack doesn't arrive. Don't we need to call that?
>
>
> If we do, then probably we don't need a new restart() function - we could
> justl the existing init() function. I see in init() some FIXME about
> running for a second time, maybe something needs fixing there.
> See also above my question on whether it's worth bothering with the
> release() before this init().
>
You are right - the second DORA cycle should retry sending discover too.
I will split dhcp_interface_state::init() into two functions (internal
structures allocation + DORA cycle with retry), so second part can be reused.
FIXME: clear routing table (use case run dhclient 2nd time)
dhcp_interface_state::state_request adds them, so
dhcp_interface_state::release should remove them.
Thank you for review - most helpful :)
Justin
>
> > +}
> > +
> >
> > ///
> >
> > dhcp_worker::dhcp_worker()
> > @@ -706,6 +716,13 @@ namespace dhcp {
> > } while (!_have_ip && wait);
> > }
> >
> > +void dhcp_worker::restart()
> > +{
> > +for (auto &it: _universe) {
> > +it.second->restart();
> > +}
> > +}
> > +
> > void dhcp_worker::release()
> > {
> > for (auto &it: _universe) {
> > diff --git a/include/osv/dhcp.hh b/include/osv/dhcp.hh
> > index 55e0b16..374b82f 100644
> > --- a/include/osv/dhcp.hh
> > +++ b/include/osv/dhcp.hh
> > @@ -25,6 +25,7 @@
> > extern "C" {
> > void dhcp_start(bool wait);
> > void dhcp_release();
> > +void dhcp_restart();
> > }
> >
> > namespace dhcp {
> > @@ -225,6 +226,7 @@ namespace dhcp {
> >
> > void discover();
> > void release();
> > +void restart();
> > void process_packet(struct mbuf*);
> > void state_discover(dhcp_mbuf &dm);
> > void state_request(dhcp_mbuf &dm);
> > @@ -251,7 +253,12 @@ namespace dhcp {
> >
> > // Ini
