Add Virtual Machine name label to virt-launcher pod #7102

machadovilaca · 2022-01-18T17:38:57Z

Adding this label will allow monitoring systems to correlate the virt-launcher pods running the VMs and the VMs themselves.

We should guarantee that if the user is not using an internal template with predefined labels, we will still ensure we have this information

/cc @sradco

Signed-off-by: João Vilaça jvilaca@redhat.com

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Release note:

Add Virtual Machine name label to virt-launcher pod

sradco · 2022-01-18T18:10:02Z

/retest

orelmisan · 2022-01-18T22:02:49Z

Hi @machadovilaca, can you please explain the motivation behind this change? (in the PR description)

machadovilaca · 2022-01-19T10:22:10Z

@orelmisan description updated

orelmisan · 2022-01-19T10:47:09Z

@orelmisan description updated

@machadovilaca Thank you!

iholder101

I like this idea a lot. Thanks, great job!

iholder101 · 2022-01-20T10:19:17Z

pkg/virt-controller/services/template_test.go

+				Expect(len(pod.Spec.Containers)).To(Equal(1))
+				vmNameLabel, ok := pod.Labels[v1.VirtualMachineNameLabel]
+				Expect(ok).To(BeTrue())
+				Expect(vmNameLabel).To(Equal("testvmi"))


nit: I would use vmi.Name(): Expect(vmNameLabel).To(Equal(vmi.Name()))

iholder101 · 2022-01-20T10:19:43Z

pkg/virt-controller/services/template_test.go

+
+				pod, err := svc.RenderLaunchManifest(&vmi)
+				Expect(err).ToNot(HaveOccurred())
+				Expect(len(pod.Spec.Containers)).To(Equal(1))


I think this line is redundant

iholder101 · 2022-01-20T10:20:48Z

pkg/virt-controller/services/template_test.go

+				pod, err := svc.RenderLaunchManifest(&vmi)
+				Expect(err).ToNot(HaveOccurred())
+				Expect(len(pod.Spec.Containers)).To(Equal(1))
+				vmNameLabel, ok := pod.Labels[v1.VirtualMachineNameLabel]
+				Expect(ok).To(BeTrue())
+				Expect(vmNameLabel).To(Equal("testvmi"))


same as above

iholder101 · 2022-01-20T10:22:09Z

pkg/virt-controller/services/template_test.go

@@ -3200,6 +3203,49 @@ var _ = Describe("Template", func() {
 				Expect(pod.Spec.Containers[0].Resources.Requests.Memory().Value()).To(Equal(expectedMemory.Value()))
 			})
 		})
+
+		Context("with Virtual Machine name label in the template", func() {


I think that using "template" in here is confusing and out of context.
Maybe "with Virtual Machine name label annotation" would be better.

removed in the template

iholder101 · 2022-01-20T10:22:16Z

pkg/virt-controller/services/template_test.go

+			})
+		})
+
+		Context("without Virtual Machine name label in the template", func() {


same as above

removed in the template

iholder101

Great job!
/lgtm

One nit below, you don't have to use it - up to you.

iholder101 · 2022-01-20T12:11:38Z

pkg/virt-controller/services/template_test.go

+
+				pod, err := svc.RenderLaunchManifest(&vmi)
+				Expect(err).ToNot(HaveOccurred())
+				Expect(len(pod.Spec.Containers)).To(Equal(1))


nit: You can change it to Expect(pod.Spec.Containers).To(HaveLen(1))

sradco · 2022-01-20T13:46:15Z

/lgtm

machadovilaca · 2022-01-21T11:20:08Z

/retest

sradco · 2022-01-23T14:35:15Z

/retest

sradco · 2022-01-24T10:06:23Z

/retest

sradco · 2022-01-31T13:25:39Z

/retest

machadovilaca · 2022-02-02T20:39:07Z

/retest

sradco · 2022-02-03T10:34:51Z

/lgtm

machadovilaca · 2022-02-03T10:38:26Z

/assign @stu-gott

Signed-off-by: João Vilaça <jvilaca@redhat.com>

iholder101 · 2022-02-22T13:06:24Z

/lgtm
Thanks!

iholder101 · 2022-02-23T15:48:16Z

/ok-to-test

kubevirt-bot · 2022-02-23T17:49:52Z

@machadovilaca: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubevirt-e2e-k8s-1.20-cgroupsv2	`b053f39`	link	true	`/test pull-kubevirt-e2e-k8s-1.20-cgroupsv2`
pull-kubevirt-e2e-k8s-1.20-sig-compute-nonroot	`b053f39`	link	true	`/test pull-kubevirt-e2e-k8s-1.20-sig-compute-nonroot`

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. I understand the commands that are listed here.

machadovilaca · 2022-02-23T19:11:29Z

/retest

stu-gott

/approve

kubevirt-bot · 2022-02-23T20:12:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: stu-gott

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~~ [stu-gott]

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

machadovilaca · 2022-04-22T13:35:50Z

/cherrypick release-0.49
/cherrypick release-0.50

kubevirt-bot · 2022-04-22T13:36:28Z

@machadovilaca: #7102 failed to apply on top of branch "release-0.49":

Applying: Add Virtual Machine name label to virt-launcher pod
Using index info to reconstruct a base tree...
M	pkg/virt-controller/services/template.go
M	pkg/virt-controller/services/template_test.go
M	staging/src/kubevirt.io/api/core/v1/types.go
Falling back to patching base and 3-way merge...
Auto-merging staging/src/kubevirt.io/api/core/v1/types.go
CONFLICT (content): Merge conflict in staging/src/kubevirt.io/api/core/v1/types.go
Auto-merging pkg/virt-controller/services/template_test.go
Auto-merging pkg/virt-controller/services/template.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Add Virtual Machine name label to virt-launcher pod
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherrypick release-0.49
/cherrypick release-0.50

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.

machadovilaca · 2022-04-22T13:40:20Z

/cherrypick release-0.50

kubevirt-bot · 2022-04-22T13:40:55Z

@machadovilaca: #7102 failed to apply on top of branch "release-0.50":

Applying: Add Virtual Machine name label to virt-launcher pod
Using index info to reconstruct a base tree...
M	pkg/virt-controller/services/template.go
M	staging/src/kubevirt.io/api/core/v1/types.go
Falling back to patching base and 3-way merge...
Auto-merging staging/src/kubevirt.io/api/core/v1/types.go
CONFLICT (content): Merge conflict in staging/src/kubevirt.io/api/core/v1/types.go
Auto-merging pkg/virt-controller/services/template.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Add Virtual Machine name label to virt-launcher pod
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherrypick release-0.50

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.

kubevirt-bot requested a review from sradco January 18, 2022 17:38

kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/M labels Jan 18, 2022

machadovilaca force-pushed the add-vm-name-label-to-virt-launcher branch from c0551a7 to 66a2ba9 Compare January 19, 2022 10:19

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 19, 2022

machadovilaca force-pushed the add-vm-name-label-to-virt-launcher branch from 66a2ba9 to b053f39 Compare January 19, 2022 10:21

kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 19, 2022

iholder101 reviewed Jan 20, 2022

View reviewed changes

machadovilaca force-pushed the add-vm-name-label-to-virt-launcher branch from b053f39 to 2504610 Compare January 20, 2022 11:34

iholder101 reviewed Jan 20, 2022

View reviewed changes

kubevirt-bot assigned iholder101 Jan 20, 2022

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 20, 2022

machadovilaca force-pushed the add-vm-name-label-to-virt-launcher branch from 2504610 to 3f59394 Compare January 20, 2022 12:43

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 20, 2022

kubevirt-bot assigned sradco Jan 20, 2022

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 20, 2022

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 2, 2022

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Feb 3, 2022

kubevirt-bot assigned stu-gott Feb 3, 2022

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 22, 2022

machadovilaca added 2 commits February 22, 2022 11:22

Add Virtual Machine name label to virt-launcher pod

6f94d0b

Signed-off-by: João Vilaça <jvilaca@redhat.com>

Use DNS sanitized VM name as VM name label

a11c1e2

Signed-off-by: João Vilaça <jvilaca@redhat.com>

machadovilaca force-pushed the add-vm-name-label-to-virt-launcher branch from 39b3e9b to a11c1e2 Compare February 22, 2022 11:26

kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 22, 2022

kubevirt-bot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Feb 23, 2022

stu-gott reviewed Feb 23, 2022

View reviewed changes

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 23, 2022

kubevirt-bot merged commit 88ef459 into kubevirt:main Feb 23, 2022

machadovilaca deleted the add-vm-name-label-to-virt-launcher branch February 23, 2022 20:14

This was referenced Apr 22, 2022

[release-0.49] Add Virtual Machine name label to virt-launcher pod #7617

Merged

[release-0.50] Add Virtual Machine name label to virt-launcher pod #7619

Merged

Add Virtual Machine name label to virt-launcher pod #7102

Add Virtual Machine name label to virt-launcher pod #7102

Conversation

machadovilaca commented Jan 18, 2022 • edited

sradco commented Jan 18, 2022

orelmisan commented Jan 18, 2022

machadovilaca commented Jan 19, 2022

orelmisan commented Jan 19, 2022 • edited

iholder101 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

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

Choose a reason for hiding this comment

iholder101 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sradco commented Jan 20, 2022

machadovilaca commented Jan 21, 2022

sradco commented Jan 23, 2022

sradco commented Jan 24, 2022

sradco commented Jan 31, 2022

machadovilaca commented Feb 2, 2022

sradco commented Feb 3, 2022

machadovilaca commented Feb 3, 2022

iholder101 commented Feb 22, 2022

iholder101 commented Feb 23, 2022

kubevirt-bot commented Feb 23, 2022 • edited

machadovilaca commented Feb 23, 2022

stu-gott left a comment

Choose a reason for hiding this comment

kubevirt-bot commented Feb 23, 2022

machadovilaca commented Apr 22, 2022

kubevirt-bot commented Apr 22, 2022

machadovilaca commented Apr 22, 2022

kubevirt-bot commented Apr 22, 2022

machadovilaca commented Jan 18, 2022 •

edited

orelmisan commented Jan 19, 2022 •

edited

kubevirt-bot commented Feb 23, 2022 •

edited