Conversation
vsphere idle fix
Contributor
Member
|
/review |
|
|
||
| func (c *VsphereConnection) Close() { | ||
| if c.client != nil { | ||
| c.client.Logout(context.Background()) |
There was a problem hiding this comment.
🔵 suggestion — Logout returns an error that is silently discarded. Consider logging it so failed logouts are visible in debug output:
if err := c.client.Logout(context.Background()); err != nil {
log.Debug().Err(err).Msg("failed to logout vsphere session")
}Not a correctness issue (the Closer interface is Close() with no return), but helpful for diagnosing connection-cleanup failures.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Root Cause: Two Issues Causing Idle Connection Buildup
Issue 1 (Critical): Missing ParentConnectionId in discovered assets
Files:
providers/vsphere/resources/discovery.go:125,170When the vSphere provider discovers child assets (ESXi hosts, VMs), it clones the parent config but never sets
ParentConnectionId:Compare with the k8s provider, which does this correctly (
providers/k8s/resources/discovery.go):What happens without ParentConnectionId:
govmomi.Clientsession createdConnect()againConnect()→NewVsphereConnection()→govmomi.NewClient()→ a brand new authenticated vSphere session1 + N + Msessions instead of just1Example: For a vSphere environment with 100 VMs and 10 hosts, that's 111 connections instead of 1.
How ParentConnectionId would fix it:
When set,
plugin.Service.AddRuntime()(providers-sdk/v1/plugin/service.go:73-81) detects the parent ID, retrieves the parent runtime, and shares its resources — no newgovmomi.Clientis created.Issue 2 (High): Missing Close() / session logout
File:
providers/vsphere/connection/connection.goVsphereConnectiondoes not implement theplugin.Closerinterface. When disconnect happens,doDisconnect()(providers-sdk/v1/plugin/service.go:157) checks:The
govmomi.Clientis never logged out (client.Logout(ctx)), so even after scanning completes, the sessions remain open on the vSphere server until they time out.Summary
Proposed Fix
Add
WithParentConnectionIdto discovery cloning:Apply to both Clone() calls in
discovery.go:125,170Implement
Close()method in VsphereConnection:This satisfies the
plugin.Closerinterface and ensures proper session cleanup.