Renews cached NodeInfo with new vSphere connection #57978
Conversation
|
/assign @luomiao |
| return nil, err | ||
| } | ||
| } | ||
| vm := nodeInfo.vm.RenewVM(vsphereInstance.conn.GoVmomiClient) |
There was a problem hiding this comment.
It looks like you have created a new VM object and new datacenter object and replaced the old nodeinfo. Then do you need to release/destroy the old VM and datacenter object? There is a DeleteVM method in virtualmachine.go. I am wondering if replace current NodeInfo with new objects will leave dangling objects behind.
There was a problem hiding this comment.
@luomiao the DeleteVM function does deletion of actual vSphere VM. We don't want to do that. Our intention here is just to renew the vclib VirtualMachine struct, which in turn extends govmomi VirtualMachine, by setting up new client object. The vSphere VM should remain intact.
Any on going operation that still has handle to older vclib VirtualMachine object can continue to use it without any foreseeable race condition (for now at least :)). Once those operations finish the older VirtualMachine struct instance, which is no longer being referenced by NodeInfo or any other ongoing operation, will get garbage collected.
Hope that answers your concern.
|
/approve |
|
Looks good generally. The only thing missing is some unit tests. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: luomiao, prashima Associated issue: #404 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
Automatic merge from submit-queue (batch tested with PRs 57511, 57978). If you want to cherry-pick this change to another branch, please follow the instructions here. |
|
In the rest of kubernetes code, we do not capitalize start of error messages as they may be merged together. It is not critical to fix this for cherry pick but please fix it for the main repo PR. |
…-upstream-release-1.9 Automatic merge from submit-queue. Automated cherry pick of #57978 upstream release-1.9 Cherry pick of #57978 on release-1.9 #57978: Renews cached NodeInfo with new vSphere connection **Release note:** ``` Fixes authentication problem faced during various vSphere operations. ``` cc: @prashima
What this PR does / why we need it:
This PR modifies two public functions of nodemanager.go- GetNodeInfo and GetNodeDetails. For both these functions NodeInfo object is renewed with new GoVmomiClient and new vclib VirtualMachine and Datacenter.
Which issue(s) this PR fixes :
Fixes vmware#404
Special notes for your reviewer:
Code has been structured to minimize impact on existing 1.9 release code and any side-effects due to NodeInfo modification. This is a quick solution for vSphere connection renewal problem. A more enhanced solution is target for upcoming major release.
Testing:
Release note: