You are right, the DORA cycle in dhcp_worker::start() retries inf-times until one iface gets IP. I didn't want to just copy-paste all dhcp_worker::start() code to the ::renew() (they both should at least print error/warning if reply is not received withing acceptable time - something you mentioned a few weeks ago, when I had problems with 'osv not booting" - so additional improvement is in place).
So I renamed start() to _send_and_wait, and refactored it to accept pointer to function to be actually called. Now it is used by both start() and renew(). Justin ----- Original Message ----- > From: "Nadav Har'El" <[email protected]> > To: "Justin Cinkelj" <[email protected]> > Cc: "Osv Dev" <[email protected]> > Sent: Sunday, December 25, 2016 10:42:24 PM > Subject: Re: [PATCH] dhcp: do not release assigned IP just to update DNS name > > In general, I think this is the right direction, but there's something I am > not sure I understand: It seems to me that the old code had a loop checking > that the new DHPC request worked. The current code, doesn't it just send > the updated DHCP request once, and forgets about it? It doesn't wait for it > to take effect, and it can even be lost on the network. > Don't we need a retry loop here too? > > > -- > Nadav Har'El > [email protected] > > On Fri, Dec 23, 2016 at 12:15 AM, Justin Cinkelj <[email protected]> > wrote: > > > We want to propagate new hostname to DNS server via DHCP server. > > Previously, a DHCP release followed by a new DORA cycle was used. > > This did update DNS name, but VM was for a short period of time > > without a valid IP. > > > > Now an early lease renew (with the new hostname included) is sent. > > Thus VM does not need to stop using assigned IP address. > > > > Fixes #816 > > > > Signed-off-by: Justin Cinkelj <[email protected]> > > --- > > core/dhcp.cc | 33 +++++++++++++++++++++++++++++---- > > include/osv/dhcp.hh | 4 +++- > > modules/cloud-init/cloud-init.cc | 10 +++++----- > > 3 files changed, 37 insertions(+), 10 deletions(-) > > > > diff --git a/core/dhcp.cc b/core/dhcp.cc > > index 0436837..b0e9f1e 100644 > > --- a/core/dhcp.cc > > +++ b/core/dhcp.cc > > @@ -72,10 +72,9 @@ void dhcp_release() > > net_dhcp_worker.release(); > > } > > > > -void dhcp_restart(bool wait) > > +void dhcp_renew() > > { > > - net_dhcp_worker.release(); > > - net_dhcp_worker.start(wait); > > + net_dhcp_worker.renew(); > > } > > > > namespace dhcp { > > @@ -221,6 +220,9 @@ namespace dhcp { > > pkt->secs = 0; > > pkt->flags = 0; > > memcpy(pkt->chaddr, IF_LLADDR(ifp), ETHER_ADDR_LEN); > > + ulong yip_n = htonl(yip.to_ulong()); > > + ulong sip_n = htonl(sip.to_ulong()); > > + memcpy(&pkt->ciaddr.s_addr, &yip_n, 4); > > > > // Options > > u8* options_start = reinterpret_cast<u8*>(pkt+1); > > @@ -242,7 +244,7 @@ namespace dhcp { > > *options++ = DHCP_OPTION_END; > > > > dhcp_len += options - options_start; > > - build_udp_ip_headers(dhcp_len, INADDR_ANY, INADDR_BROADCAST); > > + build_udp_ip_headers(dhcp_len, yip_n, sip_n); > > } > > > > void dhcp_mbuf::compose_release(struct ifnet* ifp, > > @@ -541,6 +543,22 @@ namespace dhcp { > > _client_addr = _server_addr = ipv4_zero; > > } > > > > + void dhcp_interface_state::renew() > > + { > > + // Update state > > + _state = DHCP_REQUEST; > > + > > + // Compose a dhcp request packet > > + dhcp_mbuf dm(false); > > + _xid = rand(); > > + dm.compose_request(_ifp, > > + _xid, > > + _client_addr, _server_addr); > > + > > + // Send > > + _sock->dhcp_send(dm); > > + } > > + > > void dhcp_interface_state::process_packet(struct mbuf* m) > > { > > dhcp_mbuf dm(true, m); > > @@ -724,6 +742,13 @@ namespace dhcp { > > usleep(1000); > > } > > > > + void dhcp_worker::renew() > > + { > > + for (auto &it: _universe) { > > + it.second->renew(); > > + } > > + } > > + > > void dhcp_worker::dhcp_worker_fn() > > { > > while (true) { > > diff --git a/include/osv/dhcp.hh b/include/osv/dhcp.hh > > index b286727..166df48 100644 > > --- a/include/osv/dhcp.hh > > +++ b/include/osv/dhcp.hh > > @@ -25,7 +25,7 @@ > > extern "C" { > > void dhcp_start(bool wait); > > void dhcp_release(); > > -void dhcp_restart(bool wait); > > +void dhcp_renew(); > > } > > > > namespace dhcp { > > @@ -226,6 +226,7 @@ namespace dhcp { > > > > void discover(); > > void release(); > > + void renew(); > > void process_packet(struct mbuf*); > > void state_discover(dhcp_mbuf &dm); > > void state_request(dhcp_mbuf &dm); > > @@ -256,6 +257,7 @@ namespace dhcp { > > void start(bool wait); > > // Send release packet for all DHCP IPs. > > void release(); > > + void renew(); > > > > void dhcp_worker_fn(); > > void queue_packet(struct mbuf* m); > > diff --git a/modules/cloud-init/cloud-init.cc b/modules/cloud-init/cloud- > > init.cc > > index e3ec8f8..f9b35de 100644 > > --- a/modules/cloud-init/cloud-init.cc > > +++ b/modules/cloud-init/cloud-init.cc > > @@ -17,17 +17,17 @@ > > #include <osv/hypervisor.hh> > > > > // we cannot include osv/dhcp.hh, hence direct declaration. > > -extern "C" void dhcp_restart(bool wait); > > +extern "C" void dhcp_renew(); > > > > // 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) { > > +void set_hostname_renew_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); > > + dhcp_renew(); > > } > > } > > } > > @@ -245,7 +245,7 @@ void hostname_module::handle(const YAML::Node& doc) > > { > > auto hostname = doc.as<string>(); > > debug("cloudinit hostname: %s\n", hostname.c_str()); > > - set_hostname_restart_dhcp(hostname); > > + set_hostname_renew_dhcp(hostname); > > } > > > > void osvinit::add_module(std::shared_ptr<config_module> module) > > @@ -280,7 +280,7 @@ void osvinit::load_from_cloud(bool > > ignore_missing_source) > > auto& ds = get_data_source(); > > > > // Set the hostname from given data source, if it exists. > > - set_hostname_restart_dhcp(ds.external_hostname()); > > + set_hostname_renew_dhcp(ds.external_hostname()); > > > > // Load user data. > > user_data = ds.get_user_data(); > > -- > > 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 [email protected]. > > 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 [email protected]. For more options, visit https://groups.google.com/d/optout.
