Skip to content

Commit 1aede1b

Browse files
aruneshpaaruneshpa
authored
✨ Use ConfigSpec for all tag association / removal (vmware-tanzu#1469)
Historically, we relied on a combination of ConfigSpec (for new VMs) and tagging VAPIs (for existing VMs) to associate / remove tags from VMs when a policy was matched with a VM. This was because only CreateVM supported specifying tags during VM creation. There was no way to specify tags in a ConfigSpec using the Reconfigure API call. Since then, Reconfigure API has been enhanced to support this. This change modifies our code to use that. This allows an additional optimization that a single CreateVM / Reconfigure API call can now apply the tags as well as update the ExtraConfig that we maintain for book keeping of tags applied by VM Operator. Testing Done: - Created a mandatory policy that specified a tag - Validated that in steady state when the VM is matched / unmatched with that policy tag association and removal happens via Reconfigure.
1 parent f56e1c9 commit 1aede1b

File tree

2 files changed

+64
-195
lines changed

2 files changed

+64
-195
lines changed

‎pkg/vmconfig/policy/policy_reconciler.go‎

Lines changed: 30 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@ import (
1111
"strings"
1212

1313
"github.com/vmware/govmomi/object"
14-
"github.com/vmware/govmomi/vapi/tags"
1514
"github.com/vmware/govmomi/vim25"
1615
"github.com/vmware/govmomi/vim25/mo"
1716
vimtypes "github.com/vmware/govmomi/vim25/types"
@@ -89,10 +88,6 @@ func (r reconciler) Reconcile(
8988
if configSpec == nil {
9089
panic("configSpec is nil")
9190
}
92-
restClient := pkgctx.GetRestClient(ctx)
93-
if restClient == nil {
94-
panic("restClient is nil")
95-
}
9691

9792
results, err := getPolicyEvaluationResults(
9893
ctx,
@@ -148,51 +143,40 @@ func (r reconciler) Reconcile(
148143
"toAdd", toAdd,
149144
"toRem", toRem)
150145

151-
if moVM.Config == nil {
152-
for _, tag := range toAdd {
146+
if len(toAdd) > 0 || len(toRem) > 0 {
147+
for _, tag := range toRem {
153148
configSpec.TagSpecs = append(configSpec.TagSpecs, vimtypes.TagSpec{
154149
ArrayUpdateSpec: vimtypes.ArrayUpdateSpec{
155-
Operation: vimtypes.ArrayUpdateOperationAdd,
150+
Operation: vimtypes.ArrayUpdateOperationRemove,
156151
},
157152
Id: vimtypes.TagId{
158153
Uuid: tag,
159154
},
160155
})
161156
}
162157

163-
configSpec.ExtraConfig = append(configSpec.ExtraConfig,
164-
&vimtypes.OptionValue{
165-
Key: ExtraConfigPolicyTagsKey,
166-
Value: strings.Join(toAdd, ","),
167-
},
168-
)
169-
} else {
170-
mgr := tags.NewManager(restClient)
171-
172-
if len(toRem) > 0 {
173-
if err := mgr.DetachMultipleTagsFromObject(
174-
ctx,
175-
toRem,
176-
moVM.Reference()); err != nil {
177-
178-
return fmt.Errorf("failed to detach tags from vm: %w", err)
179-
}
180-
}
181-
182-
if len(toAdd) > 0 {
183-
if err := mgr.AttachMultipleTagsToObject(
184-
ctx,
185-
toAdd,
186-
moVM.Reference()); err != nil {
187-
188-
return fmt.Errorf("failed to attach tags to vm: %w", err)
189-
}
158+
for _, tag := range toAdd {
159+
configSpec.TagSpecs = append(configSpec.TagSpecs, vimtypes.TagSpec{
160+
ArrayUpdateSpec: vimtypes.ArrayUpdateSpec{
161+
Operation: vimtypes.ArrayUpdateOperationAdd,
162+
},
163+
Id: vimtypes.TagId{
164+
Uuid: tag,
165+
},
166+
})
190167
}
191168

192-
if len(toAdd) > 0 || len(toRem) > 0 {
169+
ev := strings.Join(shouldBe, ",")
170+
if moVM.Config == nil {
171+
configSpec.ExtraConfig = append(configSpec.ExtraConfig,
172+
&vimtypes.OptionValue{
173+
Key: ExtraConfigPolicyTagsKey,
174+
Value: ev,
175+
},
176+
)
177+
} else {
193178
var (
194179
ec = object.OptionValueList(moVM.Config.ExtraConfig)
195-
ev = strings.Join(shouldBe, ",")
196180
av, _ = ec.GetString(ExtraConfigPolicyTagsKey)
197181
)
198182

@@ -204,15 +188,15 @@ func (r reconciler) Reconcile(
204188
},
205189
)
206190
}
207-
} else {
208-
// Only update the status if there are no changes.
209-
vm.Status.Policies = make([]vmopv1.PolicyStatus, len(results))
210-
for i := range results {
211-
vm.Status.Policies[i].APIVersion = results[i].APIVersion
212-
vm.Status.Policies[i].Kind = results[i].Kind
213-
vm.Status.Policies[i].Generation = results[i].Generation
214-
vm.Status.Policies[i].Name = results[i].Name
215-
}
191+
}
192+
} else {
193+
// Only update the status if there are no changes.
194+
vm.Status.Policies = make([]vmopv1.PolicyStatus, len(results))
195+
for i := range results {
196+
vm.Status.Policies[i].APIVersion = results[i].APIVersion
197+
vm.Status.Policies[i].Kind = results[i].Kind
198+
vm.Status.Policies[i].Generation = results[i].Generation
199+
vm.Status.Policies[i].Name = results[i].Name
216200
}
217201
}
218202

‎pkg/vmconfig/policy/policy_reconciler_test.go‎

Lines changed: 34 additions & 149 deletions
Original file line numberDiff line numberDiff line change
@@ -172,17 +172,6 @@ var _ = Describe("Reconcile", func() {
172172
Expect(fn).To(PanicWith("configSpec is nil"))
173173
})
174174
})
175-
When("restClient is nil", func() {
176-
JustBeforeEach(func() {
177-
ctx = context.Background()
178-
})
179-
It("should panic", func() {
180-
fn := func() {
181-
_ = vmconfpolicy.Reconcile(ctx, k8sClient, vimClient, vm, moVM, configSpec)
182-
}
183-
Expect(fn).To(PanicWith("restClient is nil"))
184-
})
185-
})
186175
})
187176

188177
When("no panic is expected", func() {
@@ -590,19 +579,10 @@ var _ = Describe("Reconcile", func() {
590579
err := vmconfpolicy.Reconcile(ctx, k8sClient, vimClient, vm, moVM, configSpec)
591580
Expect(err).ToNot(HaveOccurred())
592581

593-
// Verify that the policy tag was detached
594-
attachedTags, err := tagMgr.GetAttachedTags(ctx, moVM.Self)
595-
Expect(err).ToNot(HaveOccurred())
596-
tagIDs := make([]string, len(attachedTags))
597-
for i, tag := range attachedTags {
598-
tagIDs[i] = tag.ID
599-
}
600-
// Should only have the non-policy tag
601-
Expect(tagIDs).To(ContainElement(vcSimCtx.TagID))
602-
Expect(tagIDs).ToNot(ContainElement(policyTag3ID))
582+
// Verify configSpec has a remove TagSpec for the policy tag
583+
Expect(configSpec.TagSpecs).To(ConsistOf(
584+
tagSpec(vimtypes.ArrayUpdateOperationRemove, policyTag3ID)))
603585

604-
// BMV: I think the test setup for this is a little funky: policyTag3ID
605-
// does not look to have a PolicyEval for it, so that's why it isn't here.
606586
ecTags, ok := object.OptionValueList(configSpec.ExtraConfig).
607587
GetString(vmconfpolicy.ExtraConfigPolicyTagsKey)
608588
Expect(ok).To(BeTrue())
@@ -659,27 +639,11 @@ var _ = Describe("Reconcile", func() {
659639
})
660640

661641
It("should not modify the VM's tag associations", func() {
662-
// Record initial state
663-
initialTags, err := tagMgr.GetAttachedTags(ctx, moVM.Self)
664-
Expect(err).ToNot(HaveOccurred())
665-
initialTagCount := len(initialTags)
666-
667-
err = vmconfpolicy.Reconcile(ctx, k8sClient, vimClient, vm, moVM, configSpec)
668-
Expect(err).ToNot(HaveOccurred())
669-
670-
// Verify no changes were made
671-
finalTags, err := tagMgr.GetAttachedTags(ctx, moVM.Self)
642+
err := vmconfpolicy.Reconcile(ctx, k8sClient, vimClient, vm, moVM, configSpec)
672643
Expect(err).ToNot(HaveOccurred())
673-
Expect(len(finalTags)).To(Equal(initialTagCount))
674-
675-
// Verify specific tags still exist
676-
finalTagIDs := make([]string, len(finalTags))
677-
for i, tag := range finalTags {
678-
finalTagIDs[i] = tag.ID
679-
}
680-
Expect(finalTagIDs).To(ContainElement(vcSimCtx.TagID))
681-
Expect(finalTagIDs).To(ContainElement(policyTag3ID))
682644

645+
// No tag changes needed
646+
Expect(configSpec.TagSpecs).To(BeEmpty())
683647
Expect(configSpec.ExtraConfig).To(BeEmpty())
684648
})
685649
})
@@ -727,34 +691,14 @@ var _ = Describe("Reconcile", func() {
727691
})
728692

729693
It("should associate the policy's tags with the vm", func() {
730-
// Record initial tags
731-
initialTags, err := tagMgr.GetAttachedTags(ctx, moVM.Self)
694+
err := vmconfpolicy.Reconcile(ctx, k8sClient, vimClient, vm, moVM, configSpec)
732695
Expect(err).ToNot(HaveOccurred())
733-
initialTagIDs := make([]string, len(initialTags))
734-
for i, tag := range initialTags {
735-
initialTagIDs[i] = tag.ID
736-
}
737696

738-
err = vmconfpolicy.Reconcile(ctx, k8sClient, vimClient, vm, moVM, configSpec)
739-
Expect(err).ToNot(HaveOccurred())
740-
741-
// Verify policy tags were attached
742-
finalTags, err := tagMgr.GetAttachedTags(ctx, moVM.Self)
743-
Expect(err).ToNot(HaveOccurred())
744-
finalTagIDs := make([]string, len(finalTags))
745-
for i, tag := range finalTags {
746-
finalTagIDs[i] = tag.ID
747-
}
697+
// Verify configSpec has add TagSpecs for policy tags
698+
Expect(configSpec.TagSpecs).To(ConsistOf(
699+
tagSpec(vimtypes.ArrayUpdateOperationAdd, policyTag1ID),
700+
tagSpec(vimtypes.ArrayUpdateOperationAdd, policyTag2ID)))
748701

749-
// Should have all original tags plus new policy tags
750-
for _, tagID := range initialTagIDs {
751-
Expect(finalTagIDs).To(ContainElement(tagID))
752-
}
753-
// Should have the new policy tags
754-
Expect(finalTagIDs).To(ContainElement(policyTag1ID))
755-
Expect(finalTagIDs).To(ContainElement(policyTag2ID))
756-
757-
// Should have new policy tags added
758702
ecTags, ok := object.OptionValueList(configSpec.ExtraConfig).
759703
GetString(vmconfpolicy.ExtraConfigPolicyTagsKey)
760704
Expect(ok).To(BeTrue())
@@ -807,34 +751,14 @@ var _ = Describe("Reconcile", func() {
807751
})
808752

809753
It("should associate the policies' tags with the vm", func() {
810-
// VM currently has policyTag3ID and vcSimCtx.TagID
811-
// But policies require policyTag1ID and policyTag2ID (from computePolicy1)
812-
// So it should add the required tags
813-
814-
initialTags, err := tagMgr.GetAttachedTags(ctx, moVM.Self)
815-
Expect(err).ToNot(HaveOccurred())
816-
initialCount := len(initialTags)
817-
818-
err = vmconfpolicy.Reconcile(ctx, k8sClient, vimClient, vm, moVM, configSpec)
819-
Expect(err).ToNot(HaveOccurred())
820-
821-
finalTags, err := tagMgr.GetAttachedTags(ctx, moVM.Self)
754+
err := vmconfpolicy.Reconcile(ctx, k8sClient, vimClient, vm, moVM, configSpec)
822755
Expect(err).ToNot(HaveOccurred())
823-
finalTagIDs := make([]string, len(finalTags))
824-
for i, tag := range finalTags {
825-
finalTagIDs[i] = tag.ID
826-
}
827756

828-
// Should have more tags than initially (new policy tags added)
829-
Expect(len(finalTags)).To(BeNumerically(">=", initialCount))
830-
// Should contain non-policy tag
831-
Expect(finalTagIDs).To(ContainElement(vcSimCtx.TagID))
832-
// Should contain all required policy tags
833-
Expect(finalTagIDs).To(ContainElement(policyTag1ID))
834-
Expect(finalTagIDs).To(ContainElement(policyTag2ID))
835-
Expect(finalTagIDs).To(ContainElement(policyTag3ID))
757+
// Verify configSpec has add TagSpecs for the missing policy tags
758+
Expect(configSpec.TagSpecs).To(ConsistOf(
759+
tagSpec(vimtypes.ArrayUpdateOperationAdd, policyTag1ID),
760+
tagSpec(vimtypes.ArrayUpdateOperationAdd, policyTag2ID)))
836761

837-
// Should have new policy tags added
838762
ecTags, ok := object.OptionValueList(configSpec.ExtraConfig).
839763
GetString(vmconfpolicy.ExtraConfigPolicyTagsKey)
840764
Expect(ok).To(BeTrue())
@@ -888,30 +812,13 @@ var _ = Describe("Reconcile", func() {
888812
})
889813

890814
It("should associate the missing tags with the vm", func() {
891-
initialTags, err := tagMgr.GetAttachedTags(ctx, moVM.Self)
892-
Expect(err).ToNot(HaveOccurred())
893-
initialCount := len(initialTags)
894-
895-
err = vmconfpolicy.Reconcile(ctx, k8sClient, vimClient, vm, moVM, configSpec)
815+
err := vmconfpolicy.Reconcile(ctx, k8sClient, vimClient, vm, moVM, configSpec)
896816
Expect(err).ToNot(HaveOccurred())
897817

898-
finalTags, err := tagMgr.GetAttachedTags(ctx, moVM.Self)
899-
Expect(err).ToNot(HaveOccurred())
900-
finalTagIDs := make([]string, len(finalTags))
901-
for i, tag := range finalTags {
902-
finalTagIDs[i] = tag.ID
903-
}
904-
905-
// Should have all original tags plus missing policy tags
906-
Expect(len(finalTags)).To(BeNumerically(">=", initialCount))
907-
// Should still have the original policy tag
908-
Expect(finalTagIDs).To(ContainElement(policyTag1ID))
909-
// Should have the missing policy tags
910-
Expect(finalTagIDs).To(ContainElement(policyTag2ID))
911-
Expect(finalTagIDs).To(ContainElement(policyTag3ID))
818+
// Only missing tags should be added via TagSpecs
819+
Expect(configSpec.TagSpecs).To(ConsistOf(
820+
tagSpec(vimtypes.ArrayUpdateOperationAdd, policyTag2ID)))
912821

913-
// Should have existing policyTag1ID, and the two newly added
914-
// policyTag2ID and policyTag3ID policies
915822
ecTags, ok := object.OptionValueList(configSpec.ExtraConfig).
916823
GetString(vmconfpolicy.ExtraConfigPolicyTagsKey)
917824
Expect(ok).To(BeTrue())
@@ -1035,36 +942,25 @@ var _ = Describe("Reconcile", func() {
1035942
})
1036943

1037944
It("should associate the policy's tags with the vm", func() {
1038-
// Ensure VM starts with no tags
1039-
initialTags, err := tagMgr.GetAttachedTags(ctx, moVM.Self)
945+
err := vmconfpolicy.Reconcile(ctx, k8sClient, vimClient, vm, moVM, configSpec)
1040946
Expect(err).ToNot(HaveOccurred())
1041-
Expect(initialTags).To(BeEmpty())
1042947

1043-
err = vmconfpolicy.Reconcile(ctx, k8sClient, vimClient, vm, moVM, configSpec)
1044-
Expect(err).ToNot(HaveOccurred())
1045-
1046-
finalTags, err := tagMgr.GetAttachedTags(ctx, moVM.Self)
1047-
Expect(err).ToNot(HaveOccurred())
1048-
finalTagIDs := make([]string, len(finalTags))
1049-
for i, tag := range finalTags {
1050-
finalTagIDs[i] = tag.ID
1051-
}
948+
// Verify configSpec has add TagSpecs for policy tags
949+
Expect(configSpec.TagSpecs).To(ConsistOf(
950+
tagSpec(vimtypes.ArrayUpdateOperationAdd, policyTag1ID)))
1052951

1053952
ecTags, ok := object.OptionValueList(configSpec.ExtraConfig).
1054953
GetString(vmconfpolicy.ExtraConfigPolicyTagsKey)
1055954
Expect(ok).To(BeTrue())
1056955
Expect(strings.Split(ecTags, ",")).To(ConsistOf(policyTag1ID))
1057956

1058-
// Should now have policy tags
1059-
Expect(len(finalTags)).To(BeNumerically(">", 0))
1060-
Expect(finalTagIDs).To(ContainElement(policyTag1ID))
1061-
1062957
// Ensure the VM status was not updated yet.
1063958
Expect(vm.Status.Policies).To(HaveLen(0))
1064959

1065960
// Reconcile again so the VM status is updated.
961+
configSpec = &vimtypes.VirtualMachineConfigSpec{}
1066962
err = vmconfpolicy.Reconcile(
1067-
pkgctx.WithVMTags(ctx, finalTagIDs),
963+
pkgctx.WithVMTags(ctx, []string{policyTag1ID}),
1068964
k8sClient,
1069965
vimClient,
1070966
vm,
@@ -1142,26 +1038,14 @@ var _ = Describe("Reconcile", func() {
11421038
})
11431039

11441040
It("should associate the policies' tags with the vm", func() {
1145-
// Ensure VM starts with no tags
1146-
initialTags, err := tagMgr.GetAttachedTags(ctx, moVM.Self)
1147-
Expect(err).ToNot(HaveOccurred())
1148-
Expect(initialTags).To(BeEmpty())
1149-
1150-
err = vmconfpolicy.Reconcile(ctx, k8sClient, vimClient, vm, moVM, configSpec)
1151-
Expect(err).ToNot(HaveOccurred())
1152-
1153-
finalTags, err := tagMgr.GetAttachedTags(ctx, moVM.Self)
1041+
err := vmconfpolicy.Reconcile(ctx, k8sClient, vimClient, vm, moVM, configSpec)
11541042
Expect(err).ToNot(HaveOccurred())
1155-
finalTagIDs := make([]string, len(finalTags))
1156-
for i, tag := range finalTags {
1157-
finalTagIDs[i] = tag.ID
1158-
}
11591043

1160-
// Should have tags from multiple policies
1161-
Expect(len(finalTags)).To(BeNumerically(">", 0))
1162-
Expect(finalTagIDs).To(ContainElement(policyTag1ID))
1163-
Expect(finalTagIDs).To(ContainElement(policyTag2ID))
1164-
Expect(finalTagIDs).To(ContainElement(policyTag3ID))
1044+
// Verify configSpec has add TagSpecs for all policy tags
1045+
Expect(configSpec.TagSpecs).To(ConsistOf(
1046+
tagSpec(vimtypes.ArrayUpdateOperationAdd, policyTag1ID),
1047+
tagSpec(vimtypes.ArrayUpdateOperationAdd, policyTag2ID),
1048+
tagSpec(vimtypes.ArrayUpdateOperationAdd, policyTag3ID)))
11651049

11661050
ecTags, ok := object.OptionValueList(configSpec.ExtraConfig).
11671051
GetString(vmconfpolicy.ExtraConfigPolicyTagsKey)
@@ -1173,8 +1057,9 @@ var _ = Describe("Reconcile", func() {
11731057
Expect(vm.Status.Policies).To(HaveLen(0))
11741058

11751059
// Reconcile again so the VM status is updated.
1060+
configSpec = &vimtypes.VirtualMachineConfigSpec{}
11761061
err = vmconfpolicy.Reconcile(
1177-
pkgctx.WithVMTags(ctx, finalTagIDs),
1062+
pkgctx.WithVMTags(ctx, []string{policyTag1ID, policyTag2ID, policyTag3ID}),
11781063
k8sClient,
11791064
vimClient,
11801065
vm,

0 commit comments

Comments
 (0)