Implement NetworkFenceClass Controller #703
Conversation
mergify
bot
added
api
mergify
bot
requested review from
nixpanic,
Rakshith-R,
ShyamsundarR,
black-dragon74,
iPraveenParihar and
yati1998
Madhu-1
force-pushed
the
implement-get-client-ip
branch
from
d1f73b2 to
83ae666
Compare
docs/networkfenceclass.md
Outdated
| kind: CSIAddonsNode | ||
| metadata: | ||
| annotations: | ||
| csiaddons.openshift.io/networkfenceclass-0: network-fence-class |
There was a problem hiding this comment.
we are going to have different postfix as the keys need to be unique in annotation
There was a problem hiding this comment.
Is there really a need for such an ugly annotation? Can't whatever is going to fence not check the CSIAddonsNode.spec.drivername and match it with whatever NetworkFenceClasses there are? The creator of a NetworkFence CR will still need to pick a NetworkFenceClass for it, right?
Looping through such annotation is not really less work than looping through the NetworkFenceClasses and finding the right driver. Making sure the annotations are always correct seems more work than useful, which can have additional bugs with severe results (unable to fence).
There was a problem hiding this comment.
@nixpanic annotations are used as a way to trigger a reconcile. i am trying to avoid list operation and also there can be causes where the NFC classes are created/delete/recreate later after the csiaddosnnode registration is done, These are only of the way to triggering the Reconcile, The User will still need to look into the driverName and the NFC name present in the status field not on the annotation to get the right client Ip cidr to fence (This can be a documentation improvement i can do). another option is to have something below but that can have length limitation in some worst cases scenarios.
metadata:
annotations:
csiaddons.openshift.io/networkfenceclassname: '[{"name":"nfcName1"},{"name":"nfcName2"}]'There was a problem hiding this comment.
I must be missing something, what is the reason a reconcile is needed? When a NetworkFenceClass has a matching drivername, it is expected that the CSIAddonsNode supports that class.
CSI Provisioners and NodePlugins also do not need to have annotations for the different StorageClasses, why is this different?
There was a problem hiding this comment.
The goal is to advertise the Ip's on the CSIAddonsNode and use the NFCClass to get the cluster details to get the client IP from. In PR description i explained the workflow. This is what we have discussed early (let me know if thats not the case).
There was a problem hiding this comment.
Okay, so what I was missing (or forgetting) is that the NetworkFenceClass-Controller is not the owner of the CSIAddonsNode CR. Therefor it should not update the status of the CSIAddonsNode with the IP-address(es) that were detected by the NetworkFenceClass trigger.
Instead, the NetworkFenceClass-Controller adds an annotation, so that the CSIAddonsNode-Controller is informed to call the GetFenceClients() CSI-Addons procedure of the CSI-driver and update the CSIAddonsNode/status after that.
Madhu-1
force-pushed
the
implement-get-client-ip
branch
3 times, most recently
from
33f75cc to
c80be56
Compare
| nfcClientDetails := make([]csiaddonsv1alpha1.ClientDetail, 0) | ||
| for _, client := range clients.Clients { | ||
| logger.Info("Adding client to status", "client", client.Id, "cidrs", client.Cidrs) | ||
| nfcClientDetails = append(nfcClientDetails, csiaddonsv1alpha1.ClientDetail{ | ||
| Id: client.Id, | ||
| Cidrs: client.Cidrs, | ||
| }) | ||
| } | ||
| nfsc = append(nfsc, | ||
| csiaddonsv1alpha1.NetworkFenceClientStatus{ | ||
| NetworkFenceClassName: nfc.Name, | ||
| ClientDetails: nfcClientDetails, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Let's use a helper function here ?
| nfcClientDetails := make([]csiaddonsv1alpha1.ClientDetail, 0) | |
| for _, client := range clients.Clients { | |
| logger.Info("Adding client to status", "client", client.Id, "cidrs", client.Cidrs) | |
| nfcClientDetails = append(nfcClientDetails, csiaddonsv1alpha1.ClientDetail{ | |
| Id: client.Id, | |
| Cidrs: client.Cidrs, | |
| }) | |
| } | |
| nfsc = append(nfsc, | |
| csiaddonsv1alpha1.NetworkFenceClientStatus{ | |
| NetworkFenceClassName: nfc.Name, | |
| ClientDetails: nfcClientDetails, | |
| }, | |
| ) | |
| nfsc = append(nfsc, | |
| extractNFClientStatus(&logger, nfc.Name, client.Clients), | |
| ) |
There was a problem hiding this comment.
IMO there is no gain in doing it because its not used in multiple places nor its a complex check, its just a for loop
There was a problem hiding this comment.
This part has a loop and itself inside a if condition, which in turn is inside a loop.
It looks messy.
There was a problem hiding this comment.
I agree with Rakshith. The function is already very large, and with this change it spans > 120 lines. Please move it out into a helper to make it a little more modular and easier to understand/maintain in the future.
There was a problem hiding this comment.
getNetworkFenceClientStatus() or something would be nice.
updating the csiaddons spec dependency to the latest main. Signed-off-by: Madhu Rajanna <[email protected]>
Added `NetworkFenceClassReconciler` to manage the reconciliation of `NetworkFenceClass` resources. Fetches `NetworkFenceClass` and lists associated `CSIAddonsNode` objects based on provisioner. Adds or removes labels on csiaddonsnodes with the `NetworkFenceClass` name based on node capabilities and deletion state. Introduced helper functions for label key retrieval and label count management. Set up field indexer for `CSIAddonsNode` to efficiently watch nodes by provisioner/driver name. Signed-off-by: Madhu Rajanna <[email protected]>
adding a sample yaml for the networkfenceclass CR. Signed-off-by: Madhu Rajanna <[email protected]>
adding a new fields to the csiaddonsnode status to represent the networkfenceclass and its client details. Signed-off-by: Madhu Rajanna <[email protected]>
generated internal proto for GetFenceClients RPC. Signed-off-by: Madhu Rajanna <[email protected]>
added GetFenceClients RPC to the sidecar service to make RPC call to the csi driver Signed-off-by: Madhu Rajanna <[email protected]>
Madhu-1
force-pushed
the
implement-get-client-ip
branch
from
c80be56 to
c3369d0
Compare
when a csiaddons is registered, List the NFC CR's matching the provisioner name and send a request to get the client address from the csi driver and update the status with the client details. Signed-off-by: Madhu Rajanna <[email protected]>
run tests with verbose flag to get more detailed output. Signed-off-by: Madhu Rajanna <[email protected]>
adding documentation for the network fence class. Signed-off-by: Madhu Rajanna <[email protected]>
if we deepcopy the metadata during the update operations all the annotations gets removed. Signed-off-by: Madhu Rajanna <[email protected]>
Madhu-1
force-pushed
the
implement-get-client-ip
branch
from
c3369d0 to
f300c30
Compare
|
@nixpanic PTAL |
This PR add/update the below functionality in the controller
NetworkFenceClass Controller
CSIAddonsNode Controller
sidecar
API
Note:-
The NetworkFenceClass-Controller is not the owner of the CSIAddonsNode CR. Therefor it should not update the status of the CSIAddonsNode with the IP-address(es) that were detected by the NetworkFenceClass trigger.
Instead, the NetworkFenceClass-Controller adds an annotation, so that the CSIAddonsNode-Controller is informed to call the GetFenceClients() CSI-Addons procedure of the CSI-driver and update the CSIAddonsNode/status after that
Test results