Bug in the load balancing exercise

[adityanathan]

In the load balancing exercise (exercises/load_balance), the ecmp_group table should drop any packets that do not match any of the destination IP addresses contained in the table (which only contains 10.0.0.1 in the exercise). This is achieved through mark_to_drop(), which modifies the standard_metadata.egress_spec field to a special value (-1 in BMv2). However, the solution applies the ecmp_nhop table immediately after the ecmp_group table. Even if the ecmp_group table does not populate the meta.ecmp_select field, the field will contain the default value of 0, which results in a hit in the ecmp_nhop table. This results in the ecmp_nhop table altering the egress_spec field and other fields to valid values. Instead of dropping the packet, the packet is forwarded to the host corresponding to meta.ecmp_select = 0.

This could be fixed by setting meta.ecmp_select to be -1 or greater than ecmp_count in the apply body of the ingress pipeline before either table of the ingress pipeline is applied.

[jafingerhut]

Do you think this would make a good item to consider in a "Food for thought" section of the load_balance exercise's README file? Note that many exercises have such a section in their README describing other possible enhancements that could be made to the program, but have no solution in the solutions directory.

If you think it is better as part of the P4 programs provided, do you think it would be better as part of the exercise to solve, so it is in solutions/load_balance.p4 but not load_balance.p4 code, and there are additional steps in the README guiding someone to the solution?

The last alternative would be that it is simply part of the load_balance.p4 and solutions/load_balance.p4 programs, already solved.

[adityanathan]

I think it would be better as part of the exercise to solve with additional steps in the README. I was similarly stuck in Multi-Hop Route Inspection where one of the subtleties is that simply pushing a switch_t header to the swtraces header stack does not make the header valid and that the header has to be explicitly set as valid (link). Spending some time to understand why my solution wasn't working for MRI and then looking at the solution helped me appreciate this subtlety.

[jafingerhut]

Are you interested in writing a PR that adds more instructions to the load_balance exercise README.md that would explain what more steps the person doing the exercise should do?

That would also require updating the solutions/load_balance.p4 program with a solution for it, and perhaps adding a "TODO" comment in load_balance.p4 starting point to give the person doing the exercise a clue where to add the required code.

[adityanathan]

Yes. I can do this.