Hi Chris
Presumably this same logic flaw exists in the version landed to 2.7? IIRC this patch was one first pushed against b2_5 and was then upstreamed to master after extensive testing by ORNL.
Amir
can you comment on any differences between the 2.5.x and master versions of this patch?
Thanks
Peter
Yes, I would guess that 2.7 and master have the same problem.
ORNL uses fine grained routing, and as far as I know does not do any multihop routing, so I wouldn't claim their testing was extensive. I believe that it fixed their issue, but that does not cover all cases. We have a few clusters that have multiple hops to go from IB <> Ethernet <> IB, and I expect that number to grown in the future. We don't use fine grained routing at all, so hopefully this illustrates Chris' point, that reviews are really needed, and with a broader focus of the big picture.
Actually, ORNL probably does have multiple hops as part of their fine grain routing. The fine grain routing is about (as far as I understand it), having multiple possible routes to a source of differing distance/performance, and prioritizing the best routes by default. So most of the time they are probably prioritizing the routes with the smallest number of hops. Because they use fine grain routing, they are probably using the optional hop and/or priority settings.
It is likely that there was no testing done with the default values.
Actually for our test bed systems we have no fine grain routing. In fact in my smallest non-cray setup I don't have any routers, straight server to client, and I did test it there with no problems. Our next machine up for testing does have routers but it is not a fine grain routing setup. Its a pretty basic setup and it also worked for that configuration. For our production system yes we do use fine grain routing and I believe we tweak both hops and priority settings. Can't say 100% since I don't set up the production machines. I just break them 
Concern about multi-hop systems was brought up (see comment 3 of http://review.whamcloud.com/#/c/13162/), but we don't use it at ORNL. Since our fine-grained routing configs existed before the priority parameter even existed, we leave priority at the default value. Our hops parameter does not specify the actual network hop count, it's really used as a priority. We should eventually switch to using priority instead of hops for FGR.
The logic here (as I understand it) is as follows:
1. ping information is returned to the pinger
2. for each route on the gateway on the pinger
3. walk through all the NIs which the router returns to a max of 16
4. for each NI if any of its nids are down then consider that router down unless (5) evaluates to true
5. if the NI status is up then make sure that this NI is on the same network as the destination net defined in the route. If so then router is up, since we're able to get to the destination
6. if you finish going through all the NIs and none of them are down and none of them are on the same network as the destination net defined in the route,
6a. then consider the router down if the number of hops for this router is exactly 1 (which it is by default)
Problem is that there are two scenarios where the check in (6) will evaluate to true:
1. This is a multihop system where the destination net is not on the first hop and the # of hops is not defined in the route (or it's wrong IE: 1)
2. LNet on the router came up without the interface which is on the destination network (case in 6060).
From the point of view of the pinger, there is no way to distinguish between these two scenarios without knowing whether you're dealing with multihop route or not.
The assumption (as far as I can tell) which was made in the fix, is that if you're in a multihop system then you'll always define the number of hops in the route to > 1. If you don't define the number of hops (then you default to 1) and therefore the first hop has to have an interface on the destination network.
From what I could gather from the bug description, is that there exists a system, which uses multihops, but the routes don't define the # of hops value, since it is optional, and therefore on pinging the router, the logic determines that it's down, and causes traffic to stop flowing. Is that true?
INHO, I think that 6060 should be the responsibility of the network admin to determine and fix (technically it's a network issue), since there is no way to differentiate between the two scenarios I identified above. The patch for 6060 would work if the the # of hops in the route was mandatory, but it isn't.
thoughts?
INHO, I think that 6060 should be the responsibility of the network admin to determine and fix (technically it's a network issue), since there is no way to differentiate between the two scenarios I identified above. The patch for 6060 would work if the the # of hops in the route was mandatory, but it isn't.
That would be a fair statement, except that the hop parameter has always been optional. LNET isn't like a traditional network with a default route. It is a fire-and-forget network. You hand the traffic to the router and hope that it has a route to get your data where you want it to go. The hop parameter was typically intended to provide backup routes that were further away, such as the way ORNL is using it for fine grained routing (before the priority value was added).
It is okay to change the default behavior, but not in a maintenance release, and certainly not without changing the documentation. I have no problem making this a system admin decision and require the use of the hops parameter when setting up your network, but when a minor release breaks your existing installation, that is simply not the right thing to do.
A better bit of logic, would do the opposite of 4. If the NI status of all of its NIDS are up, then set all the routes to up. We handle that in our init scripts. We use nodediag to check that all of our interfaces are up and at the proper speed based on our config files. If the running config does not match the expected config, we stop LNET to prevent this from happening.
As I indicated in my previous comment the patch pushed in for 6060 is not the correct approach due to the fact that # of hops is not a mandatory parameters. So I agree that changing the behavior in a maintenance release is not a good idea.
I think the logic you're proposing is identical to simply removing this code bit:
/* if @down is zero and this route is single-hop, it means * we can't find NI for target network */ if (down == 0 && rte->lr_hops == 1) down = 1;
The result is that the code will loop through all the gateway's NI statuses and stop either:
1. no more NIs
2. found the NI with net == to destination net.
rte->lr_downis will be set to the number of down NIs. If all are up (or 2 above is true), then it would be zero. This should resolve the issue LLNL is having.
However, this will effectively role back the change done for ORNL, so I'd like to get agreement from ORNL on this change since it'll impact them. Basically, there will be no way to determine if a router has no way to get a message to its final destination in the case when a router comes up without a particular interface.
This can be resolved as you mentioned through scripts that check running config against expected config.
With regards to LNet being a fire and forget network, it seems like having the router pinger feature and avoid_asym_router_failure option contradicts this view.
lnet's whole approach to routes and asymmetric router failure detection seems pretty convoluted at this point. The approach in steps 4 and 5 might basically work, but they make my head hurt. I don't think we care what interfaces our peer node has; we care about what networks are reachable from our peer.
It seems like reachable networks and hop counts could have been done completely automatically. Instead of specifying "routes" on nodes, we should really be calling them "gateways". We have sysadmins list "gateway" nodes, and then all of the routing should be pretty reasonable to build up using code rather than relying on error-prone humans to get it right.
But fixing all of that is too involved for this ticket. Sigh.
In the short term, you will need to revert the patch and work on a different approach to LU-6060.
We will discuss the impact of reverting LU-6060.
Amir Shehata (amir.shehata@intel.com) uploaded a new patch: http://review.whamcloud.com/15719
Subject: LU-6851 lnet: revert LU-6060 patch
Project: fs/lustre-release
Branch: master
Current Patch Set: 1
Commit: 77a00f064086cfa9114822def1aceb8847325524
Chris, is there a significant complexity to change the LLNL LNET config to have a hop count > 1? My understanding is that as long as the hop count for each route is the same on any client it should not affect the routing priority, so it would be possible to change your config to specify the "correct" hop count of 2 or 3 for each remote network without affecting the operation with or without the LU-6060 patch applied.
Another alternative is to either revert the patch only from the LLNL CHAOS tree, or only apply it to the ORNL tree until a long-term solution can be found. Are there other sites besides LLNL that are using multi-hop routing? I agree that changing the configuration requirements for multi-hop networks in a maintenance release is unpleasant, but I'm trying to find a path forward that allows proper functionality for both sites with a single code base.
I don't understand why you guys are so reluctant to revert an obviously buggy patch. Revert the buggy patch and work on a less buggy approach to ORNL's problem. Period. I'll even suggest one way to fix it once you do the Right Thing here.
I think the reason is reverting will also impact people. We don't know how many sites would be impacted by the revert compared to the current condition you appear to be the only one. ORNL has been in that situation where we were the only site having a problem so I know what it is like 
The most important thing now is It to develop a solution to really fix the problem for everyone ASAP.
If the patch is reverted from b2_5_fe (since this is a maintenance release and shouldn't change behaviour) then that will give LLNL lots of time to fix up their routing configs to list multi-hop routes with hop count > 1, and time to update the documentation for the "hop" parameter (or ideally set hop > 1 automatically for such routes). Since that shouldn't affect current behaviour either way, it would be possible to change that well in advance of LLNL updating to a 2.7 or later release?
Whether or not LLNL or any other customer knows about a fundamental new configuration burden will depend on how you go about documenting the change and communicating with your customers.
But I would argue that this is the wrong approach. Lustre is already more difficult than it needs to be to configure and get working; this makes that even worse. People will misconfigure the hop count, and we don't have adequate tools in place to make clear why things are not working when that happens. Are you comfortable with the additional support burden? I am not.
Here is my alternate suggestion: Instead of having lr_hops default to 1, have it default to a magic number that means that the value is not configured (e.g. -1). Then we can test whether the value has been configured before using it anywhere. Additionally, make clear in the documentation that voluntarily setting the hop count on routes to the correct value will unlock additional benefits.
That would preserve the long standing lnet behavior (over a decade I presume), and allow those who want to go to the extra configuration effort to take advantage of additional benefits.
And maybe someday someone will make lnet figure out hop counts automatically.
And maybe someday someone will make lnet figure out hop counts automatically.
lnet_ni_status_t has an unused 32-bit unsigned member that could be used to store a hop-count. The router pings can send up to 16 NI statuses, so it could theoretically send all local NIs with hop count 0 and peer NIs with hop count 1+(value returned from peer). After several pings, it should be possible for LNET to know about all NIs on the connected fabrics and the distance to them.
In our center, we have more than 16 NIs. So we would not be able to rely on this unless we increase the maximum ping RPC size. That doesn't sound desirable.
I had ideas to use the the unused member for a load metric. ns_status uses hex magic numbers, so encoding it in there with a simple OR might not be straightforward. I guess hop count is more important.
As I was thinking about using that field to support a versioning system for LNet.
It would be nice to settle on the action for this patch. Is it ok with everyone to revert this on master and 2.5fe, and then come up with a way for LNet to find out the number of hops on a specific path as a separate feature? In my opinion, that's the way to go.
If there are no objections I will update the patch and push in gerrit later today, so we can include this in 2.8.
Reverting still leaves it in a broken state. We really need a fix more than anything.
Are you looking for a fix in 2.8? or can we track it through a different LU?
James, I had a question, what does ORNL use the # hops for?
There is no way that an automatic method to identify reachable networks and their associated hops counts can reasonably be introduced in time for 2.8. That needs careful design and implementation work. I think it would be excellent to work on that for a subsequent release.
Changing the default to be "value not set" instead of 1 seems fairly reasonable to me in the 2.8 time frame.
I can make the change that Chris suggested, but I'm unsure how that'll affect ORNL. I don't have clarity on how they are using the hop count. Do they assume that the default is 1 and they expect the behavior to be the same as 6060, if so then they'll still have the same problem with Chris' suggested solution.
And also I think the use of the hop count should be considered. If ORNL is using the hop count as some sort of priority, rather than an actual hop count, then it's my opinion, that it's not the intended usage of the hop count and we shouldn't be making changes to appease a case which is outside the normal usage scenario. There exists a priority parameter which can be used instead.
I can make the change that Chris suggested, but I'm unsure how that'll affect ORNL. I don't have clarity on how they are using the hop count. Do they assume that the default is 1 and they expect the behavior to be the same as 6060, if so then they'll still have the same problem with Chris' suggested solution.
And also I think the use of the hop count should be considered. If ORNL is using the hop count as some sort of priority, rather than an actual hop count, then it's my opinion, that it's not the intended usage of the hop count and we shouldn't be making changes to appease a case which is outside the normal usage scenario. There exists a priority parameter which can be used instead.
I don't have much background on the original intent of the hop count parameter. The comment by the struct is just "how far I am" which is certainly open to interpretation. One interpretation is the number of LNET hops (can the router talk to the destination NI directly, or might I pass through multiple routers to get there), which LNET could theoretically figure out automatically. A different interpretation is the number of hops within the underlying network, which we don't want LNET trying to figure out automatically. Consider an o2ib network: a router on the same leaf blade might have hop count 1, but a router on a different leaf blade that has to hop through a spine module might have hop count 3. Then consider something like Cray Gemini, where there might be many hops between a client and a router.
Our implementation on Titan is kind of a hybrid between the two. Our primary routes, which clients should always use in absence of failure, use hop count 1. The backup routers are farther away (more Gemini network hops), but we use an arbitrary hop count of 10.
This means that in the current state, our primary routers are checked to make sure all the required interfaces are present and working. Our backup routers will be unused if the interface is down, but it will still attempt to use them if LNET came up without a required interface. That's non-ideal, but that double failure pattern is extremely rare.
Priority didn't exist when we implemented this. We can (and should) move to priority in the future. We need to pick what "hops" is going to mean going forward and make sure the code and documentation all match that assumption.
Our production routers have external monitoring to make sure our IB interfaces are healthy, so it's not the end of the world if LU-6060 went away. I'm afraid that would be a step in the wrong direction, though. I think Chris' suggestion is reasonable for 2.8. I prefer that over reverting LU-6060.
Matt, thanks for the explanation.
I believe hop was added with the routing feature. The Lustre Manual has this on it: "The hop parameter specifies the number of hops to the destination". My assumption here is that it refers to the number of LNet hops to get to the destination, based on the explanation in the manual and the fact that hops parameter was added as part of the routing feature.
However, as far as I understand from your explanation, it appears that the hop count is explicitly set to 1 in ORNL configuration. Given that then switching the default to -1 to identify whether the hop count was explicitly set or not, should work for ORNL as well.
I'll update a patch with the suggested solution.
So I pushed in a patch to default the number of hops to -1.
However, I'm not happy with this solution.
Currently the hops is used to decide on which route to select and to calculate the distance to the nearest peer. If the default is -1 for routes which don't explicitly specify the # of hops, how should these routes be considered in the comparison? Should they be preferred? or should they be least preferred? To maintain the existing behavior (which is essential in order not to cause existing sites that use routes to start to misbehave) the logic I implemented makes a route with a default hop count of -1 equivalent to a route with a hop count of 1.
The benefit of the solution is that it avoids the 6060 issue given that routes are explicitly declared with hop count of 1.
I still maintain from my reading of the code that hop count is not being used as it was intended to be used. The intention is for it to define the number of LNet hops to the destination. It was implemented as an optional parameter to give the user the flexibility to determine the route a message takes based on how far it is from the destination.
From my understanding its usage has evolved to be more of a priority level. Ideally, hop counts should not be specified by the user because that gives an inaccurate view of the network. hop counts should be measured as traffic flows through the network and then routing is adjusted according to hop counts discovered. However, this is not a simple feature to implement and I'm not sure of the need for it.
Furthermore, the way we do asymmetric router failure discovery seems wrong to me, which is essentially the core of the issue in 6060.
We depend on the ping message, and when we get the ping info from the router we make a decision locally whether that gateway can get us to the destination network. However this is error prone as we can see in 6060 and compounding the issue is our dependance on the hop count which is in itself user specified and error prone.
The semantics we currently use is that we ask the gateway, tell me all your network interfaces (but dont' tell me more than 16 of them - which is in itself a problem), and when we get the information we requested we try to determine if the gateway in question can get us to the destination network.
The semantics of the query to the gateway should rather be: "Can you get to network x "
This way the gateway is the one that's providing us with the answer
The answer can be
1. yes I have a route to the network
2. yes I am on that network
3. no I don't have a route to that network and Im not on that network
This way we free ourselves from the 16 network limitation in ping, and we never have to depend on the number of hops. And way we avoid convoluted logic to try and figure out whether the gateway is up or not
We just query each gateway we have on our list with the destination network
For example, if a route is defined as tcp1 <ip>@tcp2, then we query <ip>@tcp2 with tcp1 and let the gateway tell me if it can get to tcp1.
I think that this is the best we can do to restore basic functionality in time for Lustre 2.8.0. I mean, seriously, our admins couldn't even mount Lustre any more due to the bug introduced by LU-6060. That is a far bigger problem than moving to (unsigned int)-1 as a default and potentially having the routing be sub-optimal for 0.01% of Lustre users. At least it will still mount and work for them.
Yes, the hops value was originally used by ORNL as more of a priority than an lnet hop count. Later on the priority value was added and they probably should have shifted to using that, but old habits are hard to break. I completely understand how hard it can be to overcome inertia in a production setting.
I agree that the asymmetric router failure detection design has issues. I am happy to talk about how to make it better, and how to perhaps make automate hop count and/or automate detection of reachable lnet networks. But I think that you should start a separate ticket or lustre-devel mailing list thread for that discussion.
If you look at the Lustre Manual section: 36.2.1.3
"If the target network string contains no expansions, then the hopcount defaults to 1 and may be omitted (that is, the remote network is adjacent). In practice, this is true for most multi-network configurations. It is an error to specify an inconsistent hop count for a given target network. This is why an explicit hopcount is required if the target network string specifies more than one network."
The default value is documented. So other sites could already be depending on the fact that hopcount is being defaulted to 1 for correct operations of their clustre. Given that I don't have enough data to determine how many sites that is, I think deviating from documented behavior is dangerous.
To me, out of the solutions the least dangerous is to simply revert 6060 (which doesn't impact ORNL that much since they already have scripts which check the health of their interfaces).
The issue here is to correctly get asymmetric router failure detection to work and not to change the existing documented LNet behavior.
I think the best that we can do with this messy situation is as follows:
- apply the http://review.whamcloud.com/15719 patch for 2.8.0 to make the default behaviour safer
- revert the
LU-6060patch for 2.5.4 (or apply 15719, if that is acceptable to LLNL) - LLNL should change their LNet config to properly specify the hop count on their multi-hop networks (this can work for any version of Lustre in use at LLNL)
- ORNL should change their LNet config to use priority instead of hop count to prioritize fine-grained routing (this can work for recent versions of Lustre in use at ORNL)
- file a separate feature ticket for improving LNet-level route detection
We had a discussion about the options Andreas. We have no problem now with the revert of LU-6060 for b2_5 since all our non cray clients are 2.7+. We will no longer be receiving newer Cray 2.5 clients so we will never run into dealing with an LU-6060 revert. We plan to port our non-cray clusters that are fine grain routed to priority and we hope to have it completed after Dec 1. As for a revert on master well I don't believe keeping that change in will impact LLNL Since this is 2.8 we could just document the behavior change since it is a new release. Would that be okay?
I would like to see the 15719 change finished and landed for 2.8.0.
Just documenting the change is not sufficient for LLNL for 2.8. We do not think that added requirement is a particularly wise configuration design choice. If you make it optional with 15719, then it won't bother us.
For the Intel's 2.5 FE branch, yes please revert the LU-6060 patch.
I would like to see the 15719 change finished and landed for 2.8.0.
Just documenting the change is not sufficient for LLNL for 2.8. We do not think that added requirement is a particularly wise configuration design choice. If you make it optional with 15719, then it won't bother us.
For the Intel's 2.5 FE branch, yes please revert the LU-6060 patch.
Am I reading this right but basically you are stating that Andreas suggestion to fix up your scripts for the hop count is the wrong thing since it is not what you do. Earlier for the LU-6060 patch your argument was revert the bogus patch and fix your scripts to us. Now its my turn to say the same thing. Do the right thing by fixing your scripts and forget about 15719 since it is not a proper fix.
We compromise on the 2.5 LU-6060 revert and we are spending valuable time and resource to move to priority FGR as a gesture of good will to your needs. It is not cool if you refused to the do the same for us for the 2.8 release.
We don't have scripts to generate lnet configurations per say, just configuration files.
I don't think you are quite reading it right. I am suggesting that LU-6060 is the wrong approach for lustre, and that is why it shouldn't be in the code. Lustre is a filesystem designed for massive scalability. We need to keep in mind that scalability is not only about scaling data/metadata performance, is it also about scalability of configuration. Lustre is already more difficult to configure and administrate then it needs to be, and LU-6060 made configuration in large installations even harder.
LU-6060 introduced a new requirement to LNet that all hops counts in a routed configuration have to be correct or LNet flat out does not work. I am saying that is a poor design for scalable configuration, and that is the reason it didn't belong in Lustre/LNet. Fortunately, the benefits of LU-6060 could be entirely optional! We can allow LNet to continue to be configured the same way it has been since inception LNet routers, while also providing additional checks and benefits to the users that are willing to go the extra mile during configuration.
The changes that you made to use priority didn't effect LLNL in any way that I can see, and doesn't have all that much to do with LU-6060 either. If you made changes to use priority to express networking priority you were simply updating your scripts that originated back in the old days when there was no priority option, and you therefore needed to use hops to express priority. That was a reasonable decision at the time. I also understand how old configurations can linger for quite some time. I didn't care one way or another if you changed you scripts. It has no effect on LLNL. I think it was reasonable to upgrade those scripts at some point to use the priority option to express your route priority, but that change didn't have to happen in the context of this ticket or LU-6060. That didn't help anyone outside of ORNL. Since it provided no external benefit, I don't think that can be included in any perceived compromise.
I apologize if something I said caused misunderstanding that resulted in you doing work earlier than you had planned.
Landing change 15719 is the compromise position. It allows the configuration lnet routing to continue to work as it has since its inception by default, while also at the same time providing the benefits of LU-6060 to those persons who choose to take on the additional effort of setting hops counts everywhere.
Change 15719 won't harm you, and it won't require us to accept new configuration burdens. Change 15719 is what the LU-6060 patch should have contained before we allowed it to land.
Change 15719 puts right what once went wrong.
Since the hops configuration requirements far exceeds the effort level an system administration can handle and according to your experience with multi hop the best configuration is to ignore the hops setting then why don't we just rip out the hops code then.
Because you and Cray wanted to build improvements to ARF on top of the hop count, and I'm all about compromise.
But seriously, that is an interesting thought...I'm not entirely sure what other purpose it serves now that we have priority. Is there a real use case for having hops on top of priority for routing purposes? Can the ARF be made to work under the additional required failure modes without using hops? Somebody would have to give that more thought.
15719 would still be reasonable to land in the mean time, even if we decided to take hops out at some point.
My attempt to summarize, based on the thread above and the 15719 review comments.
- ORNL has an LNet setup with single-hop routes and fine-grained routing. The configuration specifies different hopcounts because route priority had not yet been introduced when this was set up. The code from
LU-6060works for ORNL for their primary (hopcount 1) routers, but not for their secondaries (configured hopcount > 1).
- LLNL has an LNet setup with multi-hop routes. They do not specify the hopcount in their routes. The code for
LU-6060breaks this setup, because no traffic flows across multi-hop routes.
In 15719 there are three proposals on the table for fixing this:
- Have a special "not set" value sent to the kernel when the hopcount wasn't specified.
- lnet_compare_routes() treats unset as hopcount 1.
- LNetDist() treats unset as hopcount 1.
- lnet_parse_rc_info() treats unset to mean a route may be multi-hop.
- Have a special "not set" value sent to the kernel when the hopcount wasn't specified.
- lnet_compare_routes() does not compare hopcounts if one of them was unset.
- LNetDist() has to return something, unclear what, which cannot be an error.
- lnet_parse_rc_info() treats unset to mean a route may be multi-hop.
- Change the default for unset hopcount to 255 (largest hopcount), so effectively:
- lnet_compare_routes() treats unset as hopcount 255.
- LNetDist() treats unset as hopcount 255.
- lnet_parse_rc_info() treats unset as hopcount 255.
Option (3) is the simplest to implement. It is also guaranteed to break multi-hop configurations where only the non-default hopcounts have been specified.
Option (2) requires a decision to be made on what LNetDist() should do when it sees an undefined hopcount.
Option (1) minimizes the change to behavior. I count that as an advantage because an unanticipated change in behavior is what landed us in this mess in the first place. For that reason I strongly recommend going with this option.
Thanks, Olaf, that is a very nice summary.
I'm not sure about the accuracy of this sentence: "The code from LU-6060 works for ORNL for their primary (hopcount 1) routers, but not for their secondaries (configured hopcount > 1)." The LU-6060 change was primarily to improve detection of a fault that ORNL was experiencing. But that probably isn't too important for the discussion.
Option 3 was an interesting idea, but you are correct that there is a guaranteed breakage. I also worry a bit about locking in the idea that a number in the valid hop count range doesn't actually represent the real lnet hop count.
Options 1 and 2 have the same primary solution, and just differ on the secondary implementation details. I have been suggesting that primary solution all along, so I am certainly still in favor of it. I, of course recommended the secondary details in Option 2. But I'm not wed to them.
I'll throw my support behind Option 1 as well.
Oleg Drokin (oleg.drokin@intel.com) merged in patch http://review.whamcloud.com/15719/
Subject: LU-6851 lnet: Ignore hops if not explicitly set
Project: fs/lustre-release
Branch: master
Current Patch Set:
Commit: cdc97ad749b541ea2e2f1c0b5b9bc35c37762877
http://review.whamcloud.com/#/c/15719/ has landed for 2.8.0