Re: [PATCH v2 2/2] cloud-init: trigger dynamic DNS update on hostname change

[Nadav Har'El]

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, _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 : _universe) {
>> @@ -711,6 +723,7 @@ namespace dhcp {
>>  for (auto : _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 

Re: [PATCH v2 2/2] cloud-init: trigger dynamic DNS update on hostname change

[Justin Cinkelj]

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?

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, _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 : _universe) {
@@ -711,6 +723,7 @@ namespace dhcp {
 for (auto : _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