Hi @vmaffione, do you want me to do some refactoring and cleanup before doing a code review?

Yes if you want your code to be merged. You can have a commit introducing the changes affecting existing functionalities, and another one that actually introduces the bwres mechanism. Also you need to fix integration tests (see "Details" on the travis info above).

Okay, I'll start working on it.

Hopefully this looks okay, let me know if you want any more changes.

Thanks, I'll let you know asap.

I've fixed the issues you've mentioned.
Sorry for the delay but the graph algorithm was getting stuck in a loop, as some of the semantics have changed with the removeal of .from, so I had to write the algorithm down and work it through step by step to figure out how to change it.

The pipeline fails, which seems to be related to this bug. Would it be possible to upgrade the gcc version used in the pipeline?

Thanks a lot. Take your time.
Unfortunately we don't have much control on the travis build environment. However, this is not a problem, as the {0} initializer is not in C99 (that we are using). Can you please replace that with memset(&ifr, 0, sizeof(ifr));?

I'll submit another PR after this one adding all the config-generated files to gitignore, is that okay?

Yes, feel free to open that whenever you want. That one can go in immediately.

Pushed an amended commit fixing the constructor issue.

I am done for now, but since the change is complex and I already dropped many comments, I would like to give a further pass once you address my comments.

A general question: I assume (and it looks like) the implementation follows the guidelines that we agreed on the "Bw allocation scheme" google doc. Can you confirm?

One big thing that is missing in this pull request is an integration test, maybe something similar to
tests/integration/0027-ceft-addralloc.sh. Such a test would ensure that we never break your code in the future. With so much complexity, it's very likely that a future unrelated change will break it (it happened multiple times in the past, before I introduced the integration tests framework).
Even before that, it would ensure that none of the changes you will make to address my comments will break it. Maybe you can just prepare a test that reproduces the scenario that you setup manually to test your code (I guess it's a 4 or 5 nodes scenario).

I see, in that case I will start working on the issues.

Yes, the implementation should be consistent with the final version of the scheme we agreed on in the google doc.

The test scenario used 5 nodes, I should be able to find the demonstrator config as well as possibly some scripts I used to test the implementation. I will begin by creating an integration test.

Excellent, thanks. Feel free to add the demonstrator config file to the example if you think it is in shape.

Hi Vincenzo, I hope you're doing well.

Sorry for the long delay; I've started working on the PR again. I've resolved all the comments where I worked in your changes, except the one with #if 0 - I'm not sure I understood you correctly.
I will try to create an integration test asap and after that push my commits.

Hi,
Sounds good! Please also merge the latest master in your branch. There have been some minor changes, including a bug in your unit test (triggered by a ./configure --debug build).

I've pushed a partial integration test, based on 0027 and 0028. I know it's not possible to set the interface speed on veth interfaces with ethtool, but they do report 10Gbps when created and checked manually. As you can see, dif-rib-show reports 0 for all the lower flows. Any clues why that might be? I've checked in the demonstrator with qemu and the speeds are reported correctly there.

(Ignore the build error, I'll look into that.)

I can't reproduce the compilation error with local gcc 5.5.0 (nor the system 9.2.0). I thought rebasing onto current master would help but no luck. What I find curious is that the reconfigure function is practically identical with the CentralizedFaultTolerantDFT::reconfigure() in master except policy parameter prefixes - and master builds fine.

I've pushed a partial integration test, based on 0027 and 0028. I know it's not possible to set the interface speed on veth interfaces with ethtool, but they do report 10Gbps when created and checked manually. As you can see, dif-rib-show reports 0 for all the lower flows. Any clues why that might be? I've checked in the demonstrator with qemu and the speeds are reported correctly there.

(Ignore the build error, I'll look into that.)

With QEMU interfaces are virtio-net or e1000, and not veth.
I don't know why you get 0 as interface speed. Have you tried to log the return value of ethtool_cmd_speed() to see how many times it is called, and what returns on each call? Maybe it's called too early. Have you tried to write a stand-alone program to test the uipcp_get_if_speed and see what happens with veths? In any case looking here
https://stackoverflow.com/questions/2872058/get-link-speed-programmatically
there are some minor differences with your code, and you may want to look at those. It is also possible to read from /sys/class/net/eth0/speed.

I can't reproduce the compilation error with local gcc 5.5.0 (nor the system 9.2.0). I thought rebasing onto current master would help but no luck. What I find curious is that the reconfigure function is practically identical with the CentralizedFaultTolerantDFT::reconfigure() in master except policy parameter prefixes - and master builds fine.

Look at the includes. I think you are missing at least <algorithm>. In any case, I agree the error message is really confusing....

I've added a working integration test.

However, it seems that if a node is both the endpoint of a flow and the Raft leader, it gets the OpcodeFree message twice for the same flow id, which would double free and crash the node.
For now I've added an additional check when freeing to avoid this, as I haven't managed to track down why exactly this is happening.

Additionally, due to having to test with 10Gbps interfaces, I converted the bandwidth values in the policy to 64 bits else it would overflow.
What is left unchanged is

• dtcp_config.bandwidth in rlite/common.h- I wasn't sure if this was implementation defined or spec defined so I left it alone for now
• passing policy parameter values throught rlite-ctl

I've added a working integration test.

However, it seems that if a node is both the endpoint of a flow and the Raft leader, it gets the OpcodeFree message twice for the same flow id, which would double free and crash the node.
For now I've added an additional check when freeing to avoid this, as I haven't managed to track down why exactly this is happening.

Additionally, due to having to test with 10Gbps interfaces, I converted the bandwidth values in the policy to 64 bits else it would overflow.

Yes, bandwidth in bps must be 4 bits.

What is left unchanged is

* `dtcp_config.bandwidth` in `rlite/common.h`- I wasn't sure if this was implementation defined or spec defined so I left it alone for now

No, that's simply an issue. The spec struct is already 64 bits (include/rina/api.h) Please change the bandwidth value to 64 bit in rlite/common.h. You also need to update the kernel and bump the control API version number (RL_API_VERSION), since this change breaks the ABI.

* passing policy parameter values throught `rlite-ctl`