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