Network Manager

[LorenzoMoro]

One week.

Network Manager first implementation added, multicast discovery is working and provider IPs are fetched from "Cluster CR" instead of ConfigMap. TODO: implementation of CR aging control for clusters which disconnected from LAN and other cosmetic stuff, but the feature seems to work.

[fluidos-bot]

The Helm README.md appears to be out-of-date.

Please, run

make docs

Here it is an excerpt of the diff:

78c78%0A< Autogenerated from chart metadata using [helm-docs v1.14.2](https://github.com/norwoodj/helm-docs/releases/v1.14.2)%0A---%0A> Autogenerated from chart metadata using [helm-docs v1.11.0](https://github.com/norwoodj/helm-docs/releases/v1.11.0)

[fluidos-bot]

The Helm README.md appears to be out-of-date.

Please, run

make docs

Here it is an excerpt of the diff:

78c78%0A< Autogenerated from chart metadata using [helm-docs v1.14.2](https://github.com/norwoodj/helm-docs/releases/v1.14.2)%0A---%0A> Autogenerated from chart metadata using [helm-docs v1.11.0](https://github.com/norwoodj/helm-docs/releases/v1.11.0)

[fluidos-bot]

The Helm README.md appears to be out-of-date.

Please, run

make docs

Here it is an excerpt of the diff:

78c78%0A< Autogenerated from chart metadata using [helm-docs v1.14.2](https://github.com/norwoodj/helm-docs/releases/v1.14.2)%0A---%0A> Autogenerated from chart metadata using [helm-docs v1.11.0](https://github.com/norwoodj/helm-docs/releases/v1.11.0)

[frisso]

@LorenzoMoro Thanks for this PR.
I'm fine with the limitations of the current implementation for the time being, hoping to have them solved soon.
Minor suggestion (although major pain): what about renaming "Clusters" in "KnownClusters", or "NeighborClusters" or something similar? I feel that the simple name "Cluster" may bring to some confusion in the future.
Thanks!

[LorenzoMoro]

@LorenzoMoro Thanks for this PR. I'm fine with the limitations of the current implementation for the time being, hoping to have them solved soon. Minor suggestion (although major pain): what about renaming "Clusters" in "KnownClusters", or "NeighborClusters" or something similar? I feel that the simple name "Cluster" may bring to some confusion in the future. Thanks!

I'm completely fine with KnownClusters, since this CRD will work also for Catalogue-fetched Clusters from other domains, which can't be defined Neighbors.

[andreacv98]

Thanks @LorenzoMoro for your work!
LGTM!
I did a bit of testing, everything seems to work in a KIND environment, and I noticed that not only the IP address is reported, but the port as well, is this something hard coded or is the port detected as well as the IP address?

I think the CRD should be rewritten a bit with an appropriate struct for the Spec and one for the Status, but for this initial PR and feature implementation may be good anyway.

One last step before proceeding with the PR is to squash your commits into a single commit, as suggested in our Contribution Guide.

Thanks again for your contribution, really appreciated!

[LorenzoMoro]

Thanks @LorenzoMoro for your work! LGTM! I did a bit of testing, everything seems to work in a KIND environment, and I noticed that not only the IP address is reported, but the port as well, is this something hard coded or is the port detected as well as the IP address?

Thanks @andreacv98!

The port follows the IP address, so as long as it is set after the IP address in "networkManager.configMaps.nodeIdentity.ip" at installation time (lines 213 and 221 in installation.sh), it is advertised via multicast by the sender clusters and accordingly detected by the receiving clusters.

I think the CRD should be rewritten a bit with an appropriate struct for the Spec and one for the Status, but for this initial PR and feature implementation may be good anyway.

We know the CRD isn't formatted as necessary, but it will as soon as we implement the aging control, thus working as a fully compliant k8s controller (and by that time the Status will be necessary, right now it's stateless).

One last step before proceeding with the PR is to squash your commits into a single commit, as suggested in our Contribution Guide.

Sure, we'll update the naming as suggested by @frisso, squash and update the PR!

[fracappa]

Thanks @LorenzoMoro for the PR!

Overall, it looks like a good job!

I share what Fulvio says on the CRD name; let's remember that a Cluster resource already exist in KIND environment and this can lead to misbehavior (especially for Kubernetes beginners) as well as confusion. Let's take into account if it's worth it to adapt FLUIDOS terminology (e.g. FLUIDOSNode , KnownFLUIDOSNode, etc).

I also share what Andrea is saying on the overall structure of the API: let's keep it uniform so we can keep the readability of our code base.

Additionally, we should also think how we support dynamic discoveries: currently the network manager execute at the beginning but it does not reconcile (Start method rather than Reconcile); here, the idea is that each FLUIDOS Node should be listening to multicast message in order to register possible newcomers. But this could be addressed subsequently.

Only a small request for whom want to test it: could you provide an example yaml of this new CRD under deploymennt/samples ? It would be really helpful.

Only waiting for modifications and squashes for approving and merging.

Thanks!

[LorenzoMoro]

We've updated the Network Manager to implement also the aging check, so now it's full working. Also format issues in the CRD etc. have been solved. It's still structured as a Controller, though the Reconciler is never triggered right now since the functions we're running are synchronous and the controller pattern works best with asynchronous workflows: we kept the Controller structure to easily implement future functionalities.