Add the APIStatus field in the Cluster ProviderStatus field #63
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sidharthsurana If they are not already assigned, you can assign the PR to them by writing The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
size/XXL
* Adds a new field "APIStatus" in the ProviderStatus for Cluster type * Adds logic in the cluster controller to keep the APIStatus up to date Resolves #62
|
/ok-to-test |
k8s-ci-robot
removed
the
needs-ok-to-test
cloud/vsphere/clusteractuator.go
Outdated
| @@ -61,6 +70,91 @@ func (vc *ClusterActuator) Reconcile(cluster *clusterv1.Cluster) error { | |||
| glog.Infof("Error setting the Load Balancer members for the cluster: %s", err) | |||
| return err | |||
| } | |||
| // Check if the target kuberenetes is ready or not, and update the ProviderStatus if change is detected | |||
There was a problem hiding this comment.
Thanks for catching 👍
| } | ||
|
|
||
| func (vc *ClusterActuator) updateK8sAPIStatus(cluster *clusterv1.Cluster) error { | ||
| currentClusterAPIStatus, err := vc.getClusterAPIStatus(cluster) |
There was a problem hiding this comment.
Is this call necessary here?
The below call to updateClusterAPIStatus will also call getClusterAPIStatus, and this will incur in a double ssh to get the kubeconfig.
Also, by returning an error here, it means the status will not be updated when it's created.
There was a problem hiding this comment.
Good catch, while refactoring this locally, I meant to not call the getClusterAPIStatus below inside the updateClusterAPIStatus method, rather call it here and return early if needed. That is why I added the newStatus vsphereconfig.APIStatus field in the updateClusterAPIStatus method. However, I forgot to remove this invocation from there. :)
I will fix the updateClusterAPIStatus method
cloud/vsphere/clusteractuator.go
Outdated
| } | ||
| currentAPIStatus, err := vc.getClusterAPIStatus(cluster) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
Do you really want to return the error here? If the status is NotReady, wouldn't it be better to update the status with this condition?
There was a problem hiding this comment.
Based on the last comment above, this code actually should not have been here in this method. Thus I will remove it from here.
| } | ||
| newProviderStatus := &vsphereconfig.VsphereClusterProviderStatus{} | ||
| // create a copy of the old status so that any other fields except the ones we want to change can be retained | ||
| if oldProviderStatus != nil { |
There was a problem hiding this comment.
Technically this is not needed, because you have already checked the oldProviderStatus a few lines before.
There was a problem hiding this comment.
Yes, here we do need to check for nil since the method vsphereutils.GetClusterProviderStatus(cluster) may return a value of (nil,nil) which would go past the previous check.
There was a problem hiding this comment.
Got it, my fault, I read the previous check as an OR, when it's an AND.
Also switch the APIStatus from an int to string based enum for better readability aspect.
|
Thanks for the review @frodenas I have addressed your comments in the new patch. Please take a look again. |
|
@sidharthsurana These changes have been merged as part of PR #64, right? |
Yes, it did. PR#64 was stacked on top of this one |
Resolves #62