Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cloud dibbler #26

Closed
wants to merge 15 commits into from

Conversation

@baodongli
Copy link

commented Feb 19, 2015

Hi Tomasz, we are trying to use dibbler client to implement PD for openstack neutron. A few changes in the linux port that we have to make in order to meet our needs:
-- to be able to run multiple instances of the dibbler client in a single node. To do that, we localized most of the files used by the dibbler client, duid, pid, log, .xml etc.
-- to be able to run multiple instances of the dibbler client on a single interface. To do that, we bound the socket of each client to a unique LLA address. Each client is responsible for a single PD request. We thought of dynamically changing the client's configuration in order to request/release multiple PD leases dynamically. However, we felt that would require more work/change, and time is not on our side. Further change may be needed so that the unicast socket is also bound to a unique source LLA.
-- a little change on the server side so that questions and answers are matched based on not just transaction id, but also the sender's (peer's) address of the request. In our test, we also used Dibbler server, and the change was needed so that the server wouldn't get confused when receiving multiple requests from multiple clients at the same time.

We need this change ASAP in order to meet our neutron PD development deadline for Kilo release (https://wiki.openstack.org/wiki/Kilo_Release_Schedule). Therefore, we are hoping that this change can be reviewed and revised if needed and pushed upstream ASAP.

Your response will be highly appreciated!

Thanks,
Robert, John

johndavidge and others added 11 commits Jan 23, 2015
Robert Li
Add client working area, etc
Each client will have its own working area to store things like client-duid,
pid file, log, and DBs.

There is a change in the server side to make sure that answers and requests
are matched not only on the transaction id basis, but also on the Peer
address. It was found that the same transaction id may be used by multiple
clients at the same time.
@tomaszmrugalski

This comment has been minimized.

Copy link
Owner

commented Feb 19, 2015

Thanks for using Dibbler and submitting this patch. Can you express ASAP in days? I looked at your Kilo release schedule, but I'm confused which deadlines are you trying to make.

Without looking at the code, I have couple comments. Your first bullet (running multiple instances) will definitely go through (unless the patch is really borked). The second item (binding socket of each client to unique LLA) will be ok as long as the basic usage (running a single instance) is not affected. Note that dibbler must still be able to run on other OSes, including the weird ones, like windows. I don't understand either the problem or the solution in the third bullet. The server will never get confused when receiving muliple requests from multiple clients at the same time. The kernel will sort that out and the packets will be waiting in the socket. Are you talking about the code for cached replies? If that is so, the whole caching mechanism should be removed altogether.

Anyway, I'm about to leave for a business trip and will return next Thursday. The earliest I could look at this patch would be the next weekend (Feb. 28/Mar.1). Assuming I'll review the code, you should get my comments by March 1st. Let's further assume that within a couple days you send updated patch and I'll merge it. So your code could be on master maybe on March 4? 5?

Would having your code on git master be sufficient? I didn't plan to do any releases anytime soon. If you take a look at the CHANGELOG there's almost nothing changed since 1.0.0.

Cheers,
Tomek

@baodongli

This comment has been minimized.

Copy link
Author

commented Feb 19, 2015

Thanks Tomek for the quick response! Definitely we can work towards the plan you proposed. Your review and support will make it happen! While you are traveling, we can make the patch a little better.

We will try to limit the patch to linux only without affecting other OSes.
Regarding the third bullet, yes I think I'm referring to the cached replies.
I'll consult openstack community regarding whether or not it's sufficient to have the code on the master without an official release.

Thanks again
Robert

@booxter

This comment has been minimized.

Copy link

commented Feb 20, 2015

Hi,

I'm one of openstack packagers for Red Hat. Ideal way is to have an official release with the patches included. Of course we can fetch patches from git, but it will add more work for distributions. It's also safer for us to rely on what upstream considers a good package instead of fetching from git.

Do you have any problems with releasing an official package with the features included? It seems to me those constitute quite significant change in the code, something that is worth a release.

Thoughts?

@tomaszmrugalski

This comment has been minimized.

Copy link
Owner

commented Feb 20, 2015

Hi. I suppose it would be easier for everyone to just do a release. Well, everyone except me :) I'll do my best to help. The only constraint here is time. I'm about to leave on a business trip and when I get back, I'll need to prepare for the upcoming IETF meeting (I'm also co-chair of DHC working group). Dibbler is a multi-platform software, so I need to make sure that it builds on Linux, FreeBSD, NetBSD, OpenBSD, Mac OS and Windows. Dibbler is essentially one man hobby project, so I need to do this myself on my non-work time. Sadly, my continous integration setup covers linux builds only.

But let's say I can do a release after the patch is merged. Assuming the timeline I proposed holds, the 1.0.1 release would go out on March 7? Maybe 8?

On a completely unrelated note, have you guys looked at Kea (http://kea.isc.org)? That's the other DHCPv4/DHCPv6 implementation I'm working on. There's pretty good chance it can address all your server needs. Anyway, that's something for a different discussion.

@johndavidge

This comment has been minimized.

Copy link

commented Feb 23, 2015

Hi Tomek,

That timeline sounds great to me. Thanks for your help going forward to get these changes out.

@booxter

This comment has been minimized.

Copy link

commented Feb 23, 2015

Hi Tomek,
thanks a lot. We're looking forward to test the new client.

@baodongli

This comment has been minimized.

Copy link
Author

commented Feb 24, 2015

Hi Tomek,

To facilitate your review, I'm going to explain the changes in each of the files in below:

-- Misc/Portable.h and Misc/Portable.h.in:
The changes here are constrained to LINUX only. The main purpose is to be able to provide per-client configuration, working directory, pid file, log file. Instead of constants, variables are used for LINUX

-- Port-linux/dibbler-client.cpp:
Added two command line switches -W and -A. The -W switch allows the user to enter a client specific working directory. The -A switch allows the user to enter a client specific LLA address that is used as the source address for multicast communication with the server. Variables WORKDIR, CLNTPID_FILE, CLNTLOG_FILE, CLNTCONF_FILE are initialized with their original values, and properly populated if user has entered -W. Therefore, the original behavior is kept intact.

-- Port-linux/dibbler-server.cpp and Port-linux/dibbler-relay.cpp
The change is necessary due to redefinition of WORKDIR.

-- Port-linux/daemon.cpp
The change is necessary due to redefinition of CLNTPID_FILE

-- ClntTransMgr/ClntTransMgr.cpp
The change here is to use the client specific LLA address. This will only be useful for Linux-port now. Other ports don't support the -A switch. Note that the unicast socket is not changed. Therefore, limitation with this LLA is that it only supports multicast.

-- ClntIfaceMgr/ClntIfaceIface.cpp
Due to redefinition of WORKDIR, this file has to be changed. I'm not quite sure about the changes in this file, though. Due to the set of working dir, WORKDIR doesn't seem to be needed when operating on files. Some of files, for example the .xml files, are operated without specifying their path in the code. Not sure if files being manipulated here are different from those. But if it indeed requires WORKDIR in here, I can make changes accordingly.

-- SrvTransMgr/SrvTransMgr.cpp
A little change on the server side so that questions and answers are matched based on not just transaction id, but also the sender's (peer's) address of the request. In our test, we also used Dibbler server, and the change was needed so that the server wouldn't get confused when receiving multiple requests from multiple clients at the same time.

@tomaszmrugalski

This comment has been minimized.

Copy link
Owner

commented Mar 1, 2015

Ok, I managed to get through the patch. I accepted parts of it and rewrote the rest. The code is now on dibbler master. Can you check whether the code does what you expected it to do. Note that I changed your patch significantly and tweaked several other things, so please re-test.

Changes:

  • server caching removed (no need to check trans-id or client-id)
  • There's new optional parameter bind-to-address that you can specify on a per interface basis. That takes one parameter that specify the link-local address that the client will bind to. If not specified, it will use the old behavior (bind to the first LL address reported)
  • The optional -w parameter on command line now specified the work directory.

I did casual checks and all of them seem to be working. Please retest is more thoroughly (and send patch if necessary).

I typically mention contributors in AUTHORS file. How would you like to be credited? Would first name, last name, affilation be ok or do you prefer something different?

Once I get your confirmation that the changes are ok, I'll make sure that other platforms compile ok (I'm pretty sure that they are broken after the const renames in Portable.h) and then release dibbler 1.0.1RC1.

@baodongli

This comment has been minimized.

Copy link
Author

commented Mar 2, 2015

Hi Tomek, we've just tested the change, and it's working!!! thanks for your quick and timely support, and we really appreciate it! Thank you for making an official release and it will really help us a lot.

Thank you for offering to mention our names in AUTHORs file, please add John Davidge and me.

There is one intermittent issue that I saw and may not be related to the current change since I also saw it with our original patch. Occasionally one of the clients received Advertise, and somehow didn't send the request. We'll keep an eye on it and hopefully chase it down before the official release. If we found the fix after the official release, what would be the process to patch it in the release?

@tomaszmrugalski

This comment has been minimized.

Copy link
Owner

commented Mar 2, 2015

Cool, I just updated AUTHORS. I'm sorry to hear about the occasional lack of request. That sounds pretty serious. If that happens, can you provide log files and possibly traffic capture? The only thing that immediately comes to mind is that the Advertise was rejected for certain reason, e.g. it didn't contain any addresses.

As for the fix appearing after release, I'll just include it in the next release. Typically I do RC1 (release candidate) first, then after some time (which ranges from couple days to couple months if I'm super busy) the final is released.

If the fix is super important (it's a judgement call. I'll try to assess how likely is that the underlying issue would bother users), then I may do another release for it.

@johndavidge

This comment has been minimized.

Copy link

commented Mar 2, 2015

Thanks Tomek, this is great! Just took a look at the authors file update and it looks like you've credited Robert as 'John Robert' - which coincidentally is my middle name! If you'd like to include my email address as well (jodavidg@cisco.com) that would be great.

Thanks!

@baodongli

This comment has been minimized.

Copy link
Author

commented Mar 2, 2015

Hi Tomek, my name is Baodong (Robert) Li and my email is baoli@cisco.com. Thanks!

@tomaszmrugalski

This comment has been minimized.

Copy link
Owner

commented Mar 2, 2015

Oh, I'm terribly sorry. John specified his name in his profile, so I managed to get it right. I was trying to do the same for you, Baodong, but couldn't find it in your profile. I (incorrectly) guessed that the signature from your first comment is your first and last name. Anyway, I have it corrected now. Does it look ok now?

@johndavidge

This comment has been minimized.

Copy link

commented Mar 2, 2015

Looks great, thanks Tomek.

@baodongli

This comment has been minimized.

Copy link
Author

commented Mar 2, 2015

Thanks Tomek!
We'll keep you updated with the 'client not sending request' issue.

johndavidge added a commit to Tehsmash/neutron that referenced this pull request Mar 3, 2015
Implement Neutron IPv6 Prefix Delegation
This patch implements IPv6 Prefix Delegation for automatic allocation
of subnet CIDRs in Neutron. This is achieved using a new PrefixDelegation
class which provides an interface to a DHCPv6 client which handles PD
messages to and from a PD Server administered by the OpenStack deployer.

To enable this feature, subnets have been given a new property,
ipv6_pd_enabled. This property is hidden and does not affect the Neutron API.

WORKFLOW:

1. Create a new IPv6 subnet with CIDR ::/64
2. Create an interface between this subnet and a router with an existing
external interface

NOTE: The current packaged version of Dibbler (1.0.0) does not have the
nessessary features to make this possible. A new version (1.0.1) is
currently undergoing code review with the aim to release shortly after
the Kilo-3 feature freeze (March 7th/8th). See:

tomaszmrugalski/dibbler#26

To review the new functionality before this release please build from
source at:

https://github.com/tomaszmrugalski/dibbler

A PD Server reachable via the br-ex interface is needed to use this
feature. A dibbler-server configured to listen on br-ex is sufficient.

Co-Authored-By: Baodong (Robert) Li <baoli@cisco.com>

Change-Id: Ic0c6ed4dba74da94a75838178a1837f93d2d0885
Implements: blueprint ipv6-prefix-delegation
@baodongli

This comment has been minimized.

Copy link
Author

commented Mar 3, 2015

Hi Tomek, I finally hit the issue again, and I loaded the logs in here:
client log: http://pastebin.com/UAnfZUwG. The log shows that the client got the advertise, and then went to sleep.
server log: http://pastebin.com/1CN6LxtF. I couldn't find a way to highlight the relevant lines related to this particular client, but you can just look at lines with timestamp 40:11. I included logs for other clients that might be useful.

I was running multiple clients (5 actually). 4 of them got their prefixes, and the other one just didn't send the request for some reason.

I will look into the code as well. I'm also looking to see if it's our driver code that caused this issue.

thanks.

@baodongli

This comment has been minimized.

Copy link
Author

commented Mar 4, 2015

Hi Tomek:

I was able to chase down a couple of issues and come up with a tentative patch in below.
-- incorrect timeout value as calculated by "unsigned long TClntMsg::getTimeout()". Due to the use of the cast (uint32_t) in the calculation, in a 64 bit system, a supposed-to-be value of -1 would be considered a positive value. This result in the solicit msg never being processed.

-- is_addr_tentative() check would cause the client to quit if the first LLA (which may not be used by the client) happens to be undergoing DAD check. There is another place in the code with the same check. But I didn't make change over there.

diff --git a/ClntCfgMgr/ClntCfgMgr.cpp b/ClntCfgMgr/ClntCfgMgr.cpp
index 5d9ed6d..99b48a3 100644
--- a/ClntCfgMgr/ClntCfgMgr.cpp
+++ b/ClntCfgMgr/ClntCfgMgr.cpp
@@ -220,8 +220,12 @@ bool TClntCfgMgr::matchParsedSystemInterfaces(ClntParser *parser) {
// Check if the interface is during bring-up phase
// (i.e. DAD procedure for link-local addr is not complete yet)
char tmp[64];

  •        ifaceIface->firstLLAddress();
    
  •        inet_ntop6(ifaceIface->getLLAddress(), tmp);
    
  •        if (cfgIface->getBindToAddr()) {
    
  •            inet_ntop6(cfgIface->getBindToAddr()->getPlain(), tmp);
    
  •        } else {
    
  •            ifaceIface->firstLLAddress();
    
  •            inet_ntop6(ifaceIface->getLLAddress(), tmp);
    
  •        }
         if (is_addr_tentative(ifaceIface->getName(), ifaceIface->getID(), tmp)
             == LOWLEVEL_TENTATIVE_YES) {
             Log(Notice) << "Interface " << ifaceIface->getFullName()
    

    diff --git a/ClntMessages/ClntMsg.cpp b/ClntMessages/ClntMsg.cpp
    index 7e2a814..803427a 100644
    --- a/ClntMessages/ClntMsg.cpp
    +++ b/ClntMessages/ClntMsg.cpp
    @@ -320,8 +320,8 @@ TClntMsg::~TClntMsg() {

    void TClntMsg::setDefaults()
    {

  • FirstTimeStamp = (uint32_t)time(NULL);

  • LastTimeStamp = (uint32_t)time(NULL);

  • FirstTimeStamp = time(NULL);

  • LastTimeStamp = time(NULL);

RC = 0;
RT = 0;
@@ -340,7 +340,7 @@ void TClntMsg::setDefaults()

unsigned long TClntMsg::getTimeout()
{

  • long diff = (LastTimeStamp+RT) - (uint32_t)time(NULL);
  • long diff = (LastTimeStamp+RT) - time(NULL);
    return (diff<0) ? 0 : diff;
    }

@@ -394,7 +394,7 @@ void TClntMsg::send()
<< "/" << Iface << " to multicast." << LogEnd;
ClntIfaceMgr().sendMulticast(Iface, pkt, getSize());
}

  • LastTimeStamp = (uint32_t)time(NULL);
  • LastTimeStamp = time(NULL);
    delete [] pkt;
    }

Please incorporate the above change in the upcoming release as you see fit.

Another question is about the use of the BindToAddr. In our use case, we'd have multiple dibbler clients running on a single upstream interface, would the client be limited to use multicast only, and therefore the server shouldn't enable unicast at all? As I understand it, the unicast socket is bound to the any :: address (ClntTransMgr.cpp).

thanks.

johndavidge added a commit to Tehsmash/neutron that referenced this pull request Mar 5, 2015
Implement Neutron IPv6 Prefix Delegation
This patch implements IPv6 Prefix Delegation for automatic allocation
of subnet CIDRs in Neutron. This is achieved using a new PrefixDelegation
class which provides an interface to a DHCPv6 client which handles PD
messages to and from a PD Server administered by the OpenStack deployer.

To enable this feature, subnets have been given a new property,
ipv6_pd_enabled. This property is hidden and does not affect the Neutron API.

WORKFLOW:

1. Create a new IPv6 subnet with CIDR ::/64
2. Create an interface between this subnet and a router with an existing
external interface

NOTE: The current packaged version of Dibbler (1.0.0) does not have the
nessessary features to make this possible. A new version (1.0.1) is
currently undergoing code review with the aim to release shortly after
the Kilo-3 feature freeze (March 7th/8th). See:

tomaszmrugalski/dibbler#26

To review the new functionality before this release please build from
source at:

https://github.com/tomaszmrugalski/dibbler

A PD Server reachable via the br-ex interface is needed to use this
feature. A dibbler-server configured to listen on br-ex is sufficient.

Co-Authored-By: Baodong (Robert) Li <baoli@cisco.com>

Change-Id: Ic0c6ed4dba74da94a75838178a1837f93d2d0885
Implements: blueprint ipv6-prefix-delegation
johndavidge added a commit to Tehsmash/neutron that referenced this pull request Mar 6, 2015
Implement Neutron IPv6 Prefix Delegation
This patch implements IPv6 Prefix Delegation for automatic allocation
of subnet CIDRs in Neutron. This is achieved using a new PrefixDelegation
class which provides an interface to a DHCPv6 client which handles PD
messages to and from a PD Server administered by the OpenStack deployer.

To enable this feature, subnets have been given a new property,
ipv6_pd_enabled. This property is hidden and does not affect the Neutron API.

WORKFLOW:

1. Create a new IPv6 subnet with CIDR ::/64
2. Create an interface between this subnet and a router with an existing
external interface

NOTE: The current packaged version of Dibbler (1.0.0) does not have the
nessessary features to make this possible. A new version (1.0.1) is
currently undergoing code review with the aim to release shortly after
the Kilo-3 feature freeze (March 7th/8th). See:

tomaszmrugalski/dibbler#26

To review the new functionality before this release please build from
source at:

https://github.com/tomaszmrugalski/dibbler

A PD Server reachable via the br-ex interface is needed to use this
feature. A dibbler-server configured to listen on br-ex is sufficient.

Co-Authored-By: Baodong (Robert) Li <baoli@cisco.com>

Change-Id: Ic0c6ed4dba74da94a75838178a1837f93d2d0885
Implements: blueprint ipv6-prefix-delegation
@tomaszmrugalski

This comment has been minimized.

Copy link
Owner

commented Mar 7, 2015

Thanks a lot for the patch. The bind-to-address fix is good, but the lack of cast in address is only a workaround. The whole timing implementation seems to be poorly implemented, unfortunately. Note that the FirstTimeStamp and LastTimeStamp are signed, not unsigned...

Anyway, I'm disappearing on vacation tomorrow. I had only short period of time today, so I decided to apply your patch as is and release 1.0.1RC1. Hopefully, it'll be ok for your needs. If yes - great. If not - I'll try to sort that out in early April. I'll get back home on 18th, but will then go to IETF meeting, which will take 100% of my time.

Thanks again for your patches. If you have other fixes, please open new pull requests. Also, I'd love to talk with you about OpenStack deployment to understand it better. Dibbler is a nice piece of software that is easy to install, but it has scalability issues that are difficult to overcome once you reach certain number of leases. Kea may be better solution if you need scalability, but I don't know enough about OpenStack environment to make such a recommendation yet.

@baodongli

This comment has been minimized.

Copy link
Author

commented Mar 9, 2015

Hi Tomek, thank you very much for making the release for us!
The current plan is to use dibbler client for PD purpose, and there is no plan in openstack to use dibbler server yet. We'll continue to evaluate the alternatives as openstack evolves, and I'm sure we'll need your help down the road. Thanks again for making the release happen!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.