Skip to content

Commit 5a01462

Browse files
openshift-merge-bot[bot]openshift-merge-bot[bot]
authored
Merge pull request #1432 from jcpowermac/fix-vsphere-rest-session-caching
OCPBUGS-64937: vsphere - Cache REST API sessions to prevent excessive vCenter logouts
2 parents 33dd26d + 114d062 commit 5a01462

3 files changed

Lines changed: 178 additions & 151 deletions

File tree

‎pkg/controller/vsphere/reconciler.go‎

Lines changed: 23 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -594,15 +594,10 @@ func (r *Reconciler) reconcileRegionAndZoneLabels(vm *virtualMachine) error {
594594
regionLabel := r.vSphereConfig.Labels.Region
595595
zoneLabel := r.vSphereConfig.Labels.Zone
596596

597-
var res map[string]string
598-
599-
err := r.session.WithCachingTagsManager(vm.Context, func(c *session.CachingTagsManager) error {
600-
var err error
601-
res, err = vm.getRegionAndZone(c, regionLabel, zoneLabel)
602-
603-
return err
604-
})
605-
597+
// Use cached tag manager to avoid creating new REST sessions.
598+
// This eliminates excessive vCenter login/logout cycles.
599+
tagManager := r.session.GetCachingTagsManager()
600+
res, err := vm.getRegionAndZone(tagManager, regionLabel, zoneLabel)
606601
if err != nil {
607602
return err
608603
}
@@ -1599,32 +1594,29 @@ func (vm *virtualMachine) getPowerState() (types.VirtualMachinePowerState, error
15991594
// reconcileTags ensures that the required tags are present on the virtual machine, eg the Cluster ID
16001595
// that is used by the installer on cluster deletion to ensure ther are no leaked resources.
16011596
func (vm *virtualMachine) reconcileTags(ctx context.Context, sessionInstance *session.Session, machine *machinev1.Machine, providerSpec *machinev1.VSphereMachineProviderSpec) error {
1602-
if err := sessionInstance.WithCachingTagsManager(vm.Context, func(c *session.CachingTagsManager) error {
1603-
klog.Infof("%v: Reconciling attached tags", machine.GetName())
1604-
1605-
clusterID := machine.Labels[machinev1.MachineClusterIDLabel]
1606-
tagIDs := []string{clusterID}
1607-
tagIDs = append(tagIDs, providerSpec.TagIDs...)
1608-
klog.Infof("%v: Reconciling %s tags to vm", machine.GetName(), tagIDs)
1609-
for _, tagID := range tagIDs {
1610-
attached, err := vm.checkAttachedTag(ctx, tagID, c)
1611-
if err != nil {
1612-
return err
1613-
}
1597+
// Use cached tag manager to avoid creating new REST sessions.
1598+
// This eliminates excessive vCenter login/logout cycles.
1599+
tagManager := sessionInstance.GetCachingTagsManager()
1600+
klog.Infof("%v: Reconciling attached tags", machine.GetName())
1601+
1602+
clusterID := machine.Labels[machinev1.MachineClusterIDLabel]
1603+
tagIDs := []string{clusterID}
1604+
tagIDs = append(tagIDs, providerSpec.TagIDs...)
1605+
klog.Infof("%v: Reconciling %s tags to vm", machine.GetName(), tagIDs)
1606+
for _, tagID := range tagIDs {
1607+
attached, err := vm.checkAttachedTag(ctx, tagID, tagManager)
1608+
if err != nil {
1609+
return err
1610+
}
16141611

1615-
if !attached {
1616-
klog.Infof("%v: Attaching %s tag to vm", machine.GetName(), tagID)
1617-
// the tag should already be created by installer or the administrator
1618-
if err := c.AttachTag(ctx, tagID, vm.Ref); err != nil {
1619-
return err
1620-
}
1612+
if !attached {
1613+
klog.Infof("%v: Attaching %s tag to vm", machine.GetName(), tagID)
1614+
// the tag should already be created by installer or the administrator
1615+
if err := tagManager.AttachTag(ctx, tagID, vm.Ref); err != nil {
1616+
return err
16211617
}
16221618
}
1623-
return nil
1624-
}); err != nil {
1625-
return err
16261619
}
1627-
16281620
return nil
16291621
}
16301622

‎pkg/controller/vsphere/reconciler_test.go‎

Lines changed: 87 additions & 118 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,6 @@ import (
3030

3131
"github.com/vmware/govmomi/object"
3232
"github.com/vmware/govmomi/simulator"
33-
"github.com/vmware/govmomi/vapi/rest"
3433
"github.com/vmware/govmomi/vapi/tags"
3534
"github.com/vmware/govmomi/vim25"
3635
"github.com/vmware/govmomi/vim25/mo"
@@ -1666,45 +1665,40 @@ func TestReconcileTags(t *testing.T) {
16661665
}
16671666

16681667
if tc.attachTag {
1669-
if err := sessionObj.WithCachingTagsManager(context.TODO(), func(tagMgr *session.CachingTagsManager) error {
1668+
tagMgr := sessionObj.GetCachingTagsManager()
16701669

1671-
tags, err := tagMgr.GetAttachedTags(context.TODO(), managedObjRef)
1672-
if err != nil {
1673-
return err
1674-
}
1670+
tags, err := tagMgr.GetAttachedTags(context.TODO(), managedObjRef)
1671+
if err != nil {
1672+
t.Fatal(err)
1673+
}
16751674

1676-
if len(tags) == 0 {
1677-
t.Fatalf("Expected tags to be found")
1678-
}
1675+
if len(tags) == 0 {
1676+
t.Fatalf("Expected tags to be found")
1677+
}
16791678

1680-
expectedTags := []string{tc.tagName}
1681-
if len(providerSpec.TagIDs) > 0 {
1682-
expectedTags = append(expectedTags, providerSpec.TagIDs...)
1683-
}
1679+
expectedTags := []string{tc.tagName}
1680+
if len(providerSpec.TagIDs) > 0 {
1681+
expectedTags = append(expectedTags, providerSpec.TagIDs...)
1682+
}
16841683

1685-
for _, expectedTag := range expectedTags {
1686-
gotTag := false
1687-
for _, attachedTag := range tags {
1688-
if session.IsName(expectedTag) {
1689-
if attachedTag.Name == expectedTag {
1690-
gotTag = true
1691-
break
1692-
}
1693-
} else {
1694-
if attachedTag.ID == expectedTag {
1695-
gotTag = true
1696-
break
1697-
}
1684+
for _, expectedTag := range expectedTags {
1685+
gotTag := false
1686+
for _, attachedTag := range tags {
1687+
if session.IsName(expectedTag) {
1688+
if attachedTag.Name == expectedTag {
1689+
gotTag = true
1690+
break
1691+
}
1692+
} else {
1693+
if attachedTag.ID == expectedTag {
1694+
gotTag = true
1695+
break
16981696
}
1699-
}
1700-
if !gotTag {
1701-
t.Fatalf("Expected tag %s to be found", expectedTag)
17021697
}
17031698
}
1704-
1705-
return nil
1706-
}); err != nil {
1707-
t.Fatal(err)
1699+
if !gotTag {
1700+
t.Fatalf("Expected tag %s to be found", expectedTag)
1701+
}
17081702
}
17091703
}
17101704
}
@@ -1730,50 +1724,43 @@ func TestCheckAttachedTag(t *testing.T) {
17301724
tagName := "CLUSTERID"
17311725
nonAttachedTagName := "nonAttachedTag"
17321726

1733-
if err := sessionObj.WithRestClient(context.TODO(), func(c *rest.Client) error {
1734-
tagsMgr := tags.NewManager(c)
1727+
tagsMgr := sessionObj.TagManager
17351728

1736-
id, err := tagsMgr.CreateCategory(context.TODO(), &tags.Category{
1737-
AssociableTypes: []string{"VirtualMachine"},
1738-
Cardinality: "SINGLE",
1739-
Name: tagToCategoryName(tagName),
1740-
})
1741-
if err != nil {
1742-
return err
1743-
}
1744-
1745-
_, err = tagsMgr.CreateTag(context.TODO(), &tags.Tag{
1746-
CategoryID: id,
1747-
Name: tagName,
1748-
})
1749-
if err != nil {
1750-
return err
1751-
}
1752-
1753-
if err := tagsMgr.AttachTag(context.TODO(), tagName, vm.Ref); err != nil {
1754-
return err
1755-
}
1729+
id, err := tagsMgr.CreateCategory(context.TODO(), &tags.Category{
1730+
AssociableTypes: []string{"VirtualMachine"},
1731+
Cardinality: "SINGLE",
1732+
Name: tagToCategoryName(tagName),
1733+
})
1734+
if err != nil {
1735+
t.Fatal(err)
1736+
}
17561737

1757-
nonAttachedCategoryId, err := tagsMgr.CreateCategory(context.TODO(), &tags.Category{
1758-
AssociableTypes: []string{"VirtualMachine"},
1759-
Cardinality: "SINGLE",
1760-
Name: tagToCategoryName(nonAttachedTagName),
1761-
})
1738+
_, err = tagsMgr.CreateTag(context.TODO(), &tags.Tag{
1739+
CategoryID: id,
1740+
Name: tagName,
1741+
})
1742+
if err != nil {
1743+
t.Fatal(err)
1744+
}
17621745

1763-
if err != nil {
1764-
return err
1765-
}
1746+
if err := tagsMgr.AttachTag(context.TODO(), tagName, vm.Ref); err != nil {
1747+
t.Fatal(err)
1748+
}
17661749

1767-
_, err = tagsMgr.CreateTag(context.TODO(), &tags.Tag{
1768-
CategoryID: nonAttachedCategoryId,
1769-
Name: nonAttachedTagName,
1770-
})
1771-
if err != nil {
1772-
return err
1773-
}
1750+
nonAttachedCategoryId, err := tagsMgr.CreateCategory(context.TODO(), &tags.Category{
1751+
AssociableTypes: []string{"VirtualMachine"},
1752+
Cardinality: "SINGLE",
1753+
Name: tagToCategoryName(nonAttachedTagName),
1754+
})
1755+
if err != nil {
1756+
t.Fatal(err)
1757+
}
17741758

1775-
return nil
1776-
}); err != nil {
1759+
_, err = tagsMgr.CreateTag(context.TODO(), &tags.Tag{
1760+
CategoryID: nonAttachedCategoryId,
1761+
Name: nonAttachedTagName,
1762+
})
1763+
if err != nil {
17771764
t.Fatal(err)
17781765
}
17791766

@@ -1800,20 +1787,15 @@ func TestCheckAttachedTag(t *testing.T) {
18001787

18011788
for _, tc := range testCases {
18021789
t.Run(tc.name, func(t *testing.T) {
1803-
if err := sessionObj.WithCachingTagsManager(context.TODO(), func(c *session.CachingTagsManager) error {
1790+
c := sessionObj.GetCachingTagsManager()
18041791

1805-
attached, err := vm.checkAttachedTag(context.TODO(), tc.tagName, c)
1806-
if err != nil {
1807-
return fmt.Errorf("Not expected error %v", err)
1808-
}
1809-
1810-
if attached != tc.findTag {
1811-
return fmt.Errorf("Failed to find attached tag: got %v, expected %v", attached, tc.findTag)
1812-
}
1792+
attached, err := vm.checkAttachedTag(context.TODO(), tc.tagName, c)
1793+
if err != nil {
1794+
t.Fatalf("Not expected error %v", err)
1795+
}
18131796

1814-
return nil
1815-
}); err != nil {
1816-
t.Fatal(err)
1797+
if attached != tc.findTag {
1798+
t.Fatalf("Failed to find attached tag: got %v, expected %v", attached, tc.findTag)
18171799
}
18181800
})
18191801
}
@@ -3321,21 +3303,15 @@ func TestReconcileMachineWithCloudState(t *testing.T) {
33213303
t.Fatalf("cannot create tag and category: %v", err)
33223304
}
33233305

3324-
if err := session.WithRestClient(context.TODO(), func(c *rest.Client) error {
3325-
tagsMgr := tags.NewManager(c)
3326-
3327-
err = tagsMgr.AttachTag(context.TODO(), testZone, cluster.Reference())
3328-
if err != nil {
3329-
return err
3330-
}
3306+
tagsMgr := session.TagManager
33313307

3332-
err = tagsMgr.AttachTag(context.TODO(), testRegion, dc.Reference())
3333-
if err != nil {
3334-
return err
3335-
}
3308+
err = tagsMgr.AttachTag(context.TODO(), testZone, cluster.Reference())
3309+
if err != nil {
3310+
t.Fatal(err)
3311+
}
33363312

3337-
return nil
3338-
}); err != nil {
3313+
err = tagsMgr.AttachTag(context.TODO(), testRegion, dc.Reference())
3314+
if err != nil {
33393315
t.Fatal(err)
33403316
}
33413317

@@ -3412,29 +3388,22 @@ func TestReconcileMachineWithCloudState(t *testing.T) {
34123388
}
34133389

34143390
func createTagAndCategory(session *session.Session, categoryName, tagName string) (string, error) {
3415-
var tagID string
3416-
if err := session.WithRestClient(context.TODO(), func(c *rest.Client) error {
3417-
tagsMgr := tags.NewManager(c)
3418-
3419-
id, err := tagsMgr.CreateCategory(context.TODO(), &tags.Category{
3420-
AssociableTypes: []string{"VirtualMachine"},
3421-
Cardinality: "SINGLE",
3422-
Name: categoryName,
3423-
})
3424-
if err != nil {
3425-
return err
3426-
}
3391+
tagsMgr := session.TagManager
34273392

3428-
tagID, err = tagsMgr.CreateTag(context.TODO(), &tags.Tag{
3429-
CategoryID: id,
3430-
Name: tagName,
3431-
})
3432-
if err != nil {
3433-
return err
3434-
}
3393+
id, err := tagsMgr.CreateCategory(context.TODO(), &tags.Category{
3394+
AssociableTypes: []string{"VirtualMachine"},
3395+
Cardinality: "SINGLE",
3396+
Name: categoryName,
3397+
})
3398+
if err != nil {
3399+
return "", err
3400+
}
34353401

3436-
return nil
3437-
}); err != nil {
3402+
tagID, err := tagsMgr.CreateTag(context.TODO(), &tags.Tag{
3403+
CategoryID: id,
3404+
Name: tagName,
3405+
})
3406+
if err != nil {
34383407
return "", err
34393408
}
34403409

0 commit comments

Comments
 (0)