Add ability to assign tags to virtual machines #1014

alexander-demicev · 2020-08-24T10:50:06Z

What this PR does / why we need it:
Add the ability to assign tags to virtual machines, similar to what is done in AWS. In order to do so, we need to have Tags field in VSphereMachineTemplate and create REST client. This change assumes that tags are already created by the user.

Release note:

-->

Add the ability to assign tags to virtual machines.

k8s-ci-robot · 2020-08-24T10:50:14Z

Hi @alexander-demichev. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

alexander-demicev · 2020-08-24T10:52:25Z

pkg/services/govmomi/create_test.go

@@ -38,6 +40,7 @@ func TestCreate(t *testing.T) {
 		t.Fatal(err)
 	}
 	model.Service.TLS = new(tls.Config)
+	model.Service.RegisterEndpoints = true


This line and _ "github.com/vmware/govmomi/vapi/simulator" are needed to make simulator register rest session endpoint or it returns an error - unable to create rest client: POST https://127.0.0.1:63223/rest/com/vmware/cis/session: 404 Not Found

yastij · 2020-08-24T11:10:13Z

/ok-to-test

yastij · 2020-08-24T11:11:14Z

@alexander-demichev - we're planning to cut v0.7.1 today, let's review this, merge it and cut a release once it's ready

randomvariable · 2020-08-24T12:15:22Z

Need to add the conversion webhooks for the tests to pass.

EleanorRigby · 2020-08-27T16:39:45Z

@alexander-demichev : I think these tags have to pre-exist in vCenter. Is there a way to dynamically create/delete them with VMs?

ncdc · 2020-08-27T20:36:04Z

@alexander-demichev would you be able to elaborate on your use case here? I recognize that other infra providers such as CAPA allow this, but you may want to read https://www.vmware.com/content/dam/digitalmarketing/vmware/en/pdf/techpaper/performance/tagging-vsphere67-perf.pdf about tag performance in larger configurations.

yastij

I didn't review the tests yet, I'll do another pass monday

controllers/vspheremachine_controller.go

controllers/vspherevm_controller.go

yastij · 2020-08-28T17:32:06Z

pkg/services/govmomi/service.go

@@ -41,7 +42,9 @@ import (
 )

 // VMService provdes API to interact with the VMs using govmomi
-type VMService struct{}
+type VMService struct {
+	Tags []string


pkg/services/govmomi/service.go

yastij · 2020-08-28T17:36:57Z

pkg/services/govmomi/service.go

@@ -273,6 +281,18 @@ func (vms *VMService) reconcilePowerState(ctx *virtualMachineContext) (bool, err
 	}
 }

+func (vms *VMService) reconcileTags(ctx *virtualMachineContext) error {
+	tagManager := tags.NewManager(ctx.Session.RestClient)
+


I'd remove this blank line

pkg/session/session.go

yastij

Another pass for the API

yastij · 2020-08-31T10:09:20Z

api/v1alpha3/vspheremachine_types.go

+
+	// Tags is an optional set of tags to add to an instance.
+	// +optional
+	Tags []string `json:"tags,omitempty"`


I'd put these in the VirtualMachineCloneSpec

can we make the tag category explicit also ? (this would enable key:value use cases)

@yastij Do you have an example of how to attach a tag from a specific category?

api/v1alpha3/vspherevm_types.go

yastij · 2020-08-31T15:01:49Z

@alexander-demichev - the scalability concerns pointed by @ncdc are standing. what is the use case we want to cover and at what scale ?

alexander-demicev · 2020-09-01T09:03:59Z

@yastij @ncdc I didn't know about performance issues with tags. My use case is similar to what tags do for other providers.

fejta-bot · 2020-11-30T09:43:17Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

vrabbi · 2020-12-02T14:40:35Z

i also have a few use cases for this. having the tags would greatly enhance the ability for extensibility using the VMware Ecosystem tools. the use cases i have seen for this so far are:

on-boarding clusters to vRealize Automation using the tag based on-boarding mechanism
using VMware Event Broker Appliance to run custom tasks when a specific event occurs. this use case is very helpful as there is no specific event in vSphere for a CAPV cluster being deployed as it is simply provisioning VMs from the vSphere perspective. by adding the tags a custom action could be built in VEBA which could run any custom logic when a node is created in a CAPV clusters. this could include DRS rules (anti-affinity or affinity), creating DNS records for the nodes, adding nodes to a CMDB , creating custom groups in VROPs for monitoring a cluster etc.

regarding the performance issues, while this is something to take into consideration, 25k tag assignments with good performance as mentioned in the article seems quite reasonable. many systems add tags in vSphere and the performance issue i dont think should be a blocker here either. just as maybe the user doesnt have enough resources isnt a blocker for automated deployments.
having CAPV add tags on its own would be an issue but if its opt-in and user defined i think it makes a lot of sense to add this functionality

embano1 · 2020-12-07T23:00:18Z

Not sure about the initial use case here from reading the PR, but I'd suggest to use an async/throttled out-of-band (ie decoupled) mechanism based on vSphere events for this if possible, eg using the event broker or similar. It helps with separation of concerns (access permissions to VC), scalability concerns, network issues and making the controller logic less vulnerable to reconciliation/blocking issues due to heavy VC RPCs. We've seen users successfully adopting this model for tag management (beyond K8s controllers) and it works nicely (orthogonal) with the async reconciliation pattern in K8s controllers.

yastij · 2020-12-08T11:31:17Z

@vrabbi - I agree that we should have this. @alexander-demichev can you address the comments and rebase ?

alexander-demicev · 2020-12-08T19:12:53Z

@yastij yes, I'll try to find time this week

gab-satchi · 2021-08-05T18:03:10Z

Summarizing what's left to be done for the PR:

rebase now that main branch has switched over to v1alpha4. The API changes will need to be modified to drop v1alpha2
switch to the batch tagging API. also implies we use tagIDs in the spec and not the tag name.
rate limiting for the tag attach call. This may not be strictly necessary if we switch to the batch tagging

@alexander-demichev apologies for the long delay with this PR. Is this still something you're interested in contributing?

vrabbi

it should be explicit we are dealing with Tag IDs and not Tag Names

vrabbi · 2021-08-17T18:18:51Z

config/crd/bases/infrastructure.cluster.x-k8s.io_vspheremachines.yaml

@@ -514,6 +514,11 @@ spec:
                  a linked clone. This field is ignored if LinkedClone is not enabled.
                  Defaults to the source's current snapshot.
                type: string
+              tags:
+                description: Tags is an optional set of tags to add to an instance.


should be "Tags is an optional set of tag IDs to add to an instance."

Suggested change

description: Tags is an optional set of tags to add to an instance.

description: Tags is an optional set of tag IDs to add to an instance.

vrabbi · 2021-08-17T18:19:36Z

config/crd/bases/infrastructure.cluster.x-k8s.io_vspheremachinetemplates.yaml

@@ -574,6 +574,12 @@ spec:
                          to create a linked clone. This field is ignored if LinkedClone
                          is not enabled. Defaults to the source's current snapshot.
                        type: string
+                      tags:
+                        description: Tags is an optional set of tags to add to an


Should be "Tags is an optional set of tag IDs to add to an instance"

vrabbi · 2021-08-17T18:20:19Z

config/crd/bases/infrastructure.cluster.x-k8s.io_vspherevms.yaml

@@ -272,6 +272,11 @@ spec:
                  a linked clone. This field is ignored if LinkedClone is not enabled.
                  Defaults to the source's current snapshot.
                type: string
+              tags:
+                description: Tags is an optional set of tags to add to an instance.


This should be "Tags is an optional set of tag IDs to add to an instance."

alexander-demicev · 2021-08-18T11:30:06Z

Hi all, I updated the PR:

bumped govmomi dependency so we can use batch tagging
changed API field name to TagIDs

I'm having an issue with generating conversions, it's failing with the following error:
E0818 13:26:54.674110 59681 conversion.go:755] Warning: could not find nor generate a final Conversion function for sigs.k8s.io/cluster-api-provider-vsphere/api/v1alpha4.VirtualMachineCloneSpec -> sigs.k8s.io/cluster-api-provider-vsphere/api/v1alpha3.VirtualMachineCloneSpec E0818 13:26:54.674407 59681 conversion.go:756] the following fields need manual conversion: E0818 13:26:54.674417 59681 conversion.go:758] - TagIDs

embano1 · 2021-08-18T11:35:03Z

pkg/services/govmomi/service.go

@@ -140,6 +140,10 @@ func (vms *VMService) ReconcileVM(ctx *context.VMContext) (vm infrav1.VirtualMac
 		return vm, err
 	}

+	if err := vms.reconcileTags(vmCtx); err != nil {


I think error handling could be slightly improved here instead of reconciling (potentially forever) on any error, including unrecoverable errors.

To quote from the docs:

AttachMultipleTagsToObject attaches multiple tag IDs to a managed object. This operation is idempotent. If a tag is already attached to the object, then the individual operation is a no-op and no error will be thrown. This operation is not atomic. If the underlying call fails with one or more tags not successfully attached to the managed object reference it might leave the managed object reference in a partially tagged state and needs to be resolved by the caller. In this case BatchErrors is returned and can be used to analyse failure reasons on each failed tag. Specified tagIDs must use URN-notation instead of display names or a generic error will be returned and no tagging operation will be performed. If the managed object reference does not exist a generic 403 Forbidden error will be returned. This operation was added in vSphere API 6.5.

We are working on improved error type handling (for assertions) but the above should give you an impression of different error scenarios.

Does it mean that there are no typed errors at the moment?

Only partially ([]BatchError) and the situation is improving as we export more errors. Please see the tests for the method which shows common error scenarios and the errors.

For now, we don't distinguish between terminal and intermittent errors and we don't need to tackle that in this PR. What could be useful here is setting a condition on the VSphereMachine for reconciling tags. Right now if there is an error, nothing will show on the VSphereMachine to help the user debug further.

gab-satchi

thanks for the changes. tested it out locally and it works. It's a slight nuisance to have to use the tag URNs instead of just the name but that's an API limitation.

gab-satchi · 2021-08-26T20:16:50Z

api/v1alpha3/conversion.go

@@ -0,0 +1,24 @@
+/*


when I ran a make generate there is a new function in the generated conversion file. Should add that in. This file/function is still needed as it requires us to do the manual conversion.

gab-satchi · 2021-08-26T20:23:14Z

pkg/services/govmomi/service.go

@@ -140,6 +140,10 @@ func (vms *VMService) ReconcileVM(ctx *context.VMContext) (vm infrav1.VirtualMac
 		return vm, err
 	}

+	if err := vms.reconcileTags(vmCtx); err != nil {


For now, we don't distinguish between terminal and intermittent errors and we don't need to tackle that in this PR. What could be useful here is setting a condition on the VSphereMachine for reconciling tags. Right now if there is an error, nothing will show on the VSphereMachine to help the user debug further.

yastij · 2021-10-06T14:59:06Z

@alexander-demichev - pending @gab-satchi's comment on adding a condition, this looks good. Once we merge the v1beta1 types could you rebase your PR ? this way it can make it into the new release

alexander-demicev · 2021-10-15T12:40:22Z

@yastij Hi, I rebased the PR and added a condition

yastij

/approve

k8s-ci-robot · 2021-10-18T13:29:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: yastij

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:

~~OWNERS~~ [yastij]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

gab-satchi · 2021-10-18T14:00:15Z

thanks for the changes

/lgtm

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 24, 2020

k8s-ci-robot requested review from ncdc and yastij August 24, 2020 10:50

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 24, 2020

alexander-demicev commented Aug 24, 2020

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 24, 2020

yastij self-assigned this Aug 24, 2020

alexander-demicev force-pushed the tags branch 2 times, most recently from 29638ff to 48bfb68 Compare August 24, 2020 12:27

yastij reviewed Aug 28, 2020

View reviewed changes

yastij reviewed Aug 31, 2020

View reviewed changes

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 30, 2020

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 7, 2020

yastij removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 8, 2020

alexander-demicev force-pushed the tags branch from 48bfb68 to e391ca1 Compare December 10, 2020 17:07

vrabbi reviewed Aug 17, 2021

View reviewed changes

vrabbi mentioned this pull request Aug 17, 2021

REQUEST: New membership for @vrabbi kubernetes/org#2896

Closed

7 tasks

alexander-demicev force-pushed the tags branch from e391ca1 to 9081de6 Compare August 18, 2021 11:23

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 18, 2021

embano1 reviewed Aug 18, 2021

View reviewed changes

alexander-demicev force-pushed the tags branch 2 times, most recently from a8ec17d to 7a2d727 Compare August 19, 2021 12:30

gab-satchi reviewed Aug 26, 2021

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 26, 2021

alexander-demicev force-pushed the tags branch from 7a2d727 to 8f12a79 Compare October 15, 2021 12:39

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 15, 2021

alexander-demicev added 4 commits October 15, 2021 15:15

Add TagsIDs to API

bf3358b

Update govmomi

a6c712b

Reconcile tags for a VM

1b1c327

Add condition for tags attachment failure

e3692e9

alexander-demicev force-pushed the tags branch from 8f12a79 to e3692e9 Compare October 15, 2021 13:15

yastij approved these changes Oct 18, 2021

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 18, 2021

k8s-ci-robot assigned gab-satchi Oct 18, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 18, 2021

k8s-ci-robot merged commit 423b6df into kubernetes-sigs:master Oct 18, 2021

alexander-demicev deleted the tags branch March 29, 2022 10:00

	description: Tags is an optional set of tags to add to an instance.
	description: Tags is an optional set of tag IDs to add to an instance.

Add ability to assign tags to virtual machines #1014

Add ability to assign tags to virtual machines #1014

Conversation

alexander-demicev commented Aug 24, 2020

k8s-ci-robot commented Aug 24, 2020

Choose a reason for hiding this comment

yastij commented Aug 24, 2020

yastij commented Aug 24, 2020

randomvariable commented Aug 24, 2020

EleanorRigby commented Aug 27, 2020

ncdc commented Aug 27, 2020

yastij left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yastij left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yastij commented Aug 31, 2020

alexander-demicev commented Sep 1, 2020

fejta-bot commented Nov 30, 2020

vrabbi commented Dec 2, 2020

embano1 commented Dec 7, 2020

yastij commented Dec 8, 2020

alexander-demicev commented Dec 8, 2020

gab-satchi commented Aug 5, 2021

vrabbi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-demicev commented Aug 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

embano1 Aug 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gab-satchi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yastij commented Oct 6, 2021

alexander-demicev commented Oct 15, 2021

yastij left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 18, 2021

gab-satchi commented Oct 18, 2021

embano1 Aug 18, 2021 •

edited

Loading