Switch to Policy API for Segment and Segment Ports #260
Conversation
pkg/types/utils.go
Outdated
| converter := bindings.NewTypeConverter() | ||
| obj, errs := converter.ConvertToGolang(dataValue, destBindingType) | ||
| if len(errs) > 0 { | ||
| t, _ := obj.(T) |
There was a problem hiding this comment.
Is there any need for assigning t here? I think you can probably just return nil, errs[0]
There was a problem hiding this comment.
can't return nil as per the compiler
There was a problem hiding this comment.
but I found a better way to do this
| cachedNodeSet[nodeName] = &statusmanager.NodeStatus{ | ||
| Addresses: nodeAddresses, | ||
| Success: false, | ||
| Reason: fmt.Sprintf("Error while achieving port with specific address for node %s: %v", nodeName, err), |
There was a problem hiding this comment.
I know this error message has been like this for year, do you mind improving it?
It goes into the node status and not just in the logs.
Maybe just use "retrieving" instead of "achieving" (and do this for all the place where we ser this error in status)
| if inSlice(address, nodeAddresses) && !inSlice(address, collectedAddresses) { | ||
| log.Info(fmt.Sprintf("Node address %s matches port %s", address, port.Id)) | ||
| log.Info(fmt.Sprintf("Node address %s matches port %s", address, *port.Id)) |
There was a problem hiding this comment.
nit: change this line to node address %s matches segment port %s.
In this way, when we will check logs for troubleshooting, we will know that the operator is executing the code for policy API.
Also, I think the linter suggests to start log lines with a lowercase letter, so it might be another good thing to do.
| portState, err := segPortStateClient.Get(segId, *port.Id, nil, nil) | ||
| if err != nil { | ||
| return filteredPorts, err | ||
| } |
There was a problem hiding this comment.
For this check and the one at line 210 I'm thinking that perhaps we want to do thing a little differently:
Instead of returning at the first error, keep iterating over ports, and then return slices for FilteredPorts and errors.
The thing is that we want to find "the" port with the right IP, so even if we encounter issues while fetching or parsing state for other ports, we might still be able to find the port we are looking for.
There was a problem hiding this comment.
Shouldn't we do this only if there was only 1 Segment Port for the node? From the code, we can expect multiple Segment Ports matching the same IP. If that's the case, we'll end up doing partial update which may actually go unnoticed.
There was a problem hiding this comment.
what we can do alternatively is finish the loop collecting errors. If there are any ports which couldn't be read from NSX, we return 1 error providing the info of all erroneous ports
As NSX 9.0 intends to remove MP APIs, we need to switch to Policy API. This patch makes the changes necessary for node controller Logical Switch related APIs The patch removes some stale code which is no longer used Testing Done ------------ - Verified that the Segment Ports for nodes are updated by the operator - Verified that the operator is running fine Jira: NCP-601
| portState, err := segPortStateClient.Get(segId, *port.Id, nil, nil) | ||
| if err != nil { | ||
| return filteredPorts, err | ||
| } |
There was a problem hiding this comment.
I think we should remove using SDK:
nsxt "github.com/vmware/go-vmware-nsxt"
So, we should not use nsx manger client at code line#L106
ManagerClient *nsxt.APIClient
There was a problem hiding this comment.
hey Yun, the change you suggest is doable but I am not sure if it matches the criteria for this patch as the intention here is to switch those MP api calls to Policy api calls which are going to be removed. The suggested change switches the sdk but still uses mp api and these api calls are not planned for removal.
There was a problem hiding this comment.
we need to use some inventory API but they are supported as well to list vif and vms
Here, we can use this client from sdk:
At code line#L124
vms, _, err := nsxClient.FabricApi.ListVirtualMachines
we can use vm client from sdk:
https://github.com/vmware/vsphere-automation-sdk-go/blob/services/nsxt/v0.12.0/services/nsxt-mp/nsx/fabric/VirtualMachinesClient.go
At code line#L151
vifs, _, err := nsxClient.FabricApi.ListVifs(nsxClient.Context, localVarOptionals)
we can use vif licent from sdk
https://github.com/vmware/vsphere-automation-sdk-go/blob/services/nsxt/v0.12.0/services/nsxt-mp/nsx/fabric/VifsClient.go
As NSX 9.0 intends to remove MP APIs, we need to switch to Policy API. This patch makes the changes necessary for node controller Logical Switch related APIs
The patch removes some stale code which is no longer used
Testing Done
Jira: NCP-601