Memory dump #7517

ShellyKa13 · 2022-04-04T14:51:18Z

What this PR does / why we need it:
This PR implements virtctl command of getting memory dump to a pvc.

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 virtctl Memory Dump command

davidvossel · 2022-04-04T15:27:11Z

pkg/virt-launcher/virtwrap/manager.go

+	l.domainModifyLock.Lock()
+	defer l.domainModifyLock.Unlock()


it seems like we needed this lock whenever we're performing a get+set on the domain spec. I'm uncertain if we want to hold this lock during the duration of the core dump. This lock would actually block the virt-handler's reconcile loop potentially if the same VM is synced again while the dump is taking place.

If you think its safe to remove it I will

the way you restructured this with the lock not being held during the dump looks good now

davidvossel · 2022-04-04T15:32:15Z

pkg/virt-launcher/virtwrap/manager.go

+	memoryDumpInfo := &api.DomainMemoryDumpInfo{
+		DumpTimestamp: &now,
+	}


what happens if the memory dump fails? will that get reported?

I planned to add error handling

davidvossel · 2022-04-04T15:35:42Z

pkg/virt-launcher/notify-client/client.go

+			case domainUpdate := <-domainUpdateChan:
+				guestEvent.domainMemoryDumpInfo = domainUpdate.MemoryDumpInfo
+				eventCallback(domainConn, domainCache, libvirtEvent{}, n, deleteNotificationSent, guestEvent, vmi)


i'm unsure about how this works. It doesn't appear like the memory dump info is persisted on the Domain, so if this event doesn't get processed as expected by virt-handler, I think we'd never have a chance to get the result of the memory dump again.

The method we have for storing persistent info on the domainSpec is the domainSpec.Metadata field.

This is a really good point! I am fixing it to get if the memory dump completed from the memoryDump call by updating the info of the last memory dump on the LibvirtDomainManager struct.

mhenriks · 2022-04-04T20:41:25Z

staging/src/kubevirt.io/api/core/v1/types.go

+	// dump to the given pvc
+	// +nullable
+	// +optional
+	MemoryDumpRequest *VirtualMachineMemoryDumpRequest `json:"memoryDumpRequest,omitempty" optional:"true"`


What happens if a dump is requested while one is in progress? Should this be a list like VolumeRequests?

as long as the previous request hasn't completed the request is denied, there is not really a point to do another memory dump one after the other

mhenriks · 2022-04-04T20:41:58Z

staging/src/kubevirt.io/api/core/v1/types.go

+	// DumpTimestamp is the time when the memory dump occured
+	DumpTimestamp *metav1.Time `json:"dumpTimestamp,omitempty"`
+	// VolumeName is the name of the volume the memory was dumped to
+	VolumeName string `json:"volumeName,omitempty"`


Should this be claimName?

ShellyKa13 · 2022-04-06T14:18:16Z

@davidvossel @mhenriks I updated the PR. This code now works after some manual testing. I'll appreciate a another review

ShellyKa13 · 2022-04-11T16:56:09Z

/restest

arnongilboa · 2022-04-12T13:25:32Z

pkg/handler-launcher-com/cmd/v1/cmd.proto

+
+message MemoryDumpResponse {
+  Response response = 1;
+  bool completed = 2;


why not have the more flexible string memoryDumpResponse = 2; (like all other responses) instead of a bool?

I dont understand the question. Im using the Response like all the other Responses but add a bool that I wanted to indicate if the memory dump completed or not, in the Response there is bool for success and a string message. success is not the same as completed as the request for the dump can succeed since it was triggered successfully and after that the command will return completed if the async request completed..

All I said is that bool is limited (you will rarely see it here, or in responses in general) and you may later need a response which is more informative than false.

I see, for completion its a pretty much yes or no question, and I do updated the message in the Response part if I had some error and that maybe why the completion is false.. so I think that coveres it..

arnongilboa · 2022-04-12T13:28:43Z

pkg/hotplug-disk/hotplug-disk.go

+			return "", err
+		}
+	}
+	return directoryPath, err


why err and not nil?

This function is pretty much a copy of GetFileSystemDiskTargetPathFromHostView with a small change :) I can beautify it

arnongilboa · 2022-04-12T13:30:53Z

pkg/hotplug-disk/hotplug-disk.go

+		return "", err
+	}
+	if !exists && create {
+		err = os.Mkdir(directoryPath, 0750)


if err := os.Mkdir(directoryPath, 0750); err != nil... with local err is the preferable style for such checks.

duyanyan · 2022-04-13T02:54:24Z

tests/storage/memorydump.go

+			// Verify the content is still on the pvc
+			verifyMemoryDumpOutput(memoryDumpPVC, previousOutput, true)
+		},
+			Entry("calling endpoint directly", memoryDumpVMSubresource, removeMemoryDumpVMSubresource),


Entry("[test_id:8499]calling endpoint directly"

duyanyan · 2022-04-13T02:55:05Z

tests/storage/memorydump.go

+			verifyMemoryDumpOutput(memoryDumpPVC, previousOutput, true)
+		},
+			Entry("calling endpoint directly", memoryDumpVMSubresource, removeMemoryDumpVMSubresource),
+			Entry("using virtctl", memoryDumpVirtctl, removeMemoryDumpVirtctl),


Entry("[test_id:8500]using virtctl"

duyanyan · 2022-04-13T02:56:05Z

tests/storage/memorydump.go

+			verifyMemoryDumpOutput(memoryDumpPVC, previousOutput, true)
+		})
+
+		It("Run memory dump to a pvc, remove and run memory dump to different pvc", func() {


[test_id:8503]

duyanyan · 2022-04-13T02:56:44Z

tests/storage/memorydump.go

+			Entry("using virtctl", memoryDumpVirtctl, removeMemoryDumpVirtctl),
+		)
+
+		It("Run multiple memory dumps", func() {


[test_id:8502]

duyanyan · 2022-04-13T02:57:22Z

tests/storage/memorydump.go

+			verifyMemoryDumpOutput(memoryDumpPVC2, previousOutput, true)
+		})
+
+		It("Run memory dump, stop vm and remove memory dump", func() {


[test_id:8506]

duyanyan · 2022-04-13T02:58:03Z

tests/storage/memorydump.go

+			verifyMemoryDumpOutput(memoryDumpPVC, previousOutput, true)
+		})
+
+		It("Run memory dump, stop vm start vm", func() {


[test_id:8515]

mhenriks

Played around with this for a bit and it seems to be working very well! Great job!

Have a couple simple questions. Mostly wondering if we want to annotate/label the target PVC at all. It is possible for it to outlive the VM.

mhenriks · 2022-04-13T17:16:06Z

staging/src/kubevirt.io/api/core/v1/schema.go

+	// PersistentVolumeClaimVolumeSource represents a reference to a PersistentVolumeClaim in the same namespace.
+	// Directly attached to the virt launcher
+	// +optional
+	PersistentVolumeClaim *PersistentVolumeClaimVolumeSource `json:"persistentVolumeClaim,omitempty"`


I wonder if this struct would be better since I doubt there will be any targets to memory dump other than pvc?

type MemoryDumpVolumeSource struct { PersistentVolumeClaimVolumeSource `json:",inline"` }

yeah might be a good suggestion

mhenriks · 2022-04-13T17:44:15Z

staging/src/kubevirt.io/client-go/kubecli/vm.go

@@ -228,6 +228,23 @@ func (v *vm) Migrate(name string, migrateOptions *v1.MigrateOptions) error {
 	return v.restClient.Put().RequestURI(uri).Body(optsJson).Do(context.Background()).Error()
 }

+func (v *vm) MemoryDump(name string, memoryDumpRequest *v1.VirtualMachineMemoryDumpRequest) error {


why is this on VM and not VMI?

hmm it is true that the memory dump is basically an action that happens on the vmi and there is a check that the vmi exists and running, but the action starts by updating the vm since eventually the memory dump info will be only on the vm and not on the vmi since the output does outlives the vmi. also the counteraction of memory dump - remove-memory-dump is an action solely on the vm and can happen on the vm even if vmi doesnt exist.
You think the subresource should be on the vmi and the function that is being called will update the vm status and not the vmi?

Maybe both VM and VMI should have memorydump endpoints?

which both do the same thing of updating the vm status with the memory dump request?

If you refer for doing memory dump without the whole part of updating the VM I guess its possible but then the volume will still be mounted to the VMI. and anyways maybe I can add it later as an improvement not in this PR.

mhenriks · 2022-04-13T17:49:27Z

staging/src/kubevirt.io/api/core/v1/types.go

+	// ClaimName is the name of the pvc the memory was dumped to
+	ClaimName string `json:"claimName,omitempty"`
+	// TargetFileName is the name of the memory dump output
+	TargetFileName string `json:"targetFileName,omitempty"`


I wonder if it makes sense to somehow also encode some/all of this information as annotations on the target pvc? It doesn't appear that the pvc is annotated at all now.

That is an option. If it's alright with you, i'll add it in a different PR to make it possible for this PR to be merged soon :)

Yeah #7517 (comment) sounds nice, I recently added labels to restore PVCs so we could have some queryable metrics about them
in #7533

ShellyKa13 · 2022-05-23T12:35:56Z

/retest

The command will be called from the virt handler and will trigger core dump in libvirt with memory only flag. There will be always only one memory dump command at the same time. Once the dump is complete it will save the inforamtion of the last damp so next call if done for the same file will indicate the completion or error of the last dump. Signed-off-by: Shelly Kagan <skagan@redhat.com>

The command calls a subresource endpoint. Then the vm status is being patched with the memory dump command Signed-off-by: Shelly Kagan <skagan@redhat.com>

The pvc of the memory dump should be mounted as a directory to be able to dump to it the memory, it doesnt have a disk to mount. Signed-off-by: Shelly Kagan <skagan@redhat.com>

This volume source doesnt have a matching disk, need to take that into account in the checks verifications Signed-off-by: Shelly Kagan <skagan@redhat.com>

The vm status is updated with a memory dump request with a phase of binding. That triggers update of the vm and vmi volumes update with memory dump volume source and the phase changes to inprogress. In that time the memorydump is being mounted and done, once done or failed the volume status in the vmi is updated with timestamp or error in such case the state will be updated to unmounting which updates the vmi volume list to be without the memory dump volume source which unmounts the pvc. Once it is removed from the vmi volume status list the phase will be updated to completed. Signed-off-by: Shelly Kagan <skagan@redhat.com>

The vmi gets an update of add volume, added adjustments to identify memory dump volume source as a hot plug. The volume is add to the volume status list the same as a hotplug do. Once it is attached to the virt launcher the phase is updated to mounted. Once this Phase is updated the memory dump is triggered and the phase is updated to in progress. Then the virt handler reconcile loop will keep check for memory dump completion, once completed it will update the phase and the timestamp of the memory dump which will be recieved in the vm watch which will trigger unmount of the memory dump volume source. Signed-off-by: Shelly Kagan <skagan@redhat.com>

The comman will dissocaite the memory dump, it will remove the memory dump from the volumes list and the memory dump request from the vm status. Signed-off-by: Shelly Kagan <skagan@redhat.com>

Signed-off-by: Shelly Kagan <skagan@redhat.com>

Add a validation for the pvc size that should be enough to contain the memory dump. The size should be the vm memory plus a hard coded overhead of 100Mi - that should be enough after some checking and testing examples. Also moved the pvc validation from virtctl to subresource. Signed-off-by: Shelly Kagan <skagan@redhat.com>

…umeSource Signed-off-by: Shelly Kagan <skagan@redhat.com>

Signed-off-by: Shelly Kagan <skagan@redhat.com>

* unite memory-dump and remove-memory dump to the same command with an action argumeant "get" or "remove" * make flag claim-name optional in case we want to dump to the same pvc which is already associated with the vm * move the phase assignment from virtctl to the subresource * make sure cant trigger remove when another remove is in progress Signed-off-by: Shelly Kagan <skagan@redhat.com>

Instead of polling for the memory dump completion by the same memory dump command, now updates of completion or failure is updates on the domain. Adjusted code and unit tests accordingly. Signed-off-by: Shelly Kagan <skagan@redhat.com>

mhenriks · 2022-05-24T17:14:18Z

/lgtm

davidvossel

/approve

excellent work!

kubevirt-bot · 2022-05-27T13:20:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidvossel

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~~ [davidvossel]

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

kubevirt-commenter-bot · 2022-05-27T15:20:40Z

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

kubevirt-commenter-bot · 2022-05-27T21:20:40Z

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

kubevirt-bot requested review from maiqueb and marceloamaral April 4, 2022 14:51

kubevirt-bot added the size/XXL label Apr 4, 2022

ShellyKa13 mentioned this pull request Apr 4, 2022

Memory dump command in virt launcher #7464

Closed

davidvossel reviewed Apr 4, 2022

View reviewed changes

mhenriks reviewed Apr 4, 2022

View reviewed changes

ShellyKa13 force-pushed the memory-dump branch from 38bbff1 to 134b5e2 Compare April 6, 2022 14:16

ShellyKa13 changed the title ~~[WIP] Memory dump~~ Memory dump Apr 6, 2022

kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 6, 2022

ShellyKa13 force-pushed the memory-dump branch from 134b5e2 to d59e733 Compare April 7, 2022 15:47

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

ShellyKa13 force-pushed the memory-dump branch from d59e733 to 1a91d33 Compare April 10, 2022 15:49

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

ShellyKa13 force-pushed the memory-dump branch from 1a91d33 to a9b92d1 Compare April 11, 2022 12:36

arnongilboa reviewed Apr 12, 2022

View reviewed changes

duyanyan reviewed Apr 13, 2022

View reviewed changes

mhenriks reviewed Apr 13, 2022

View reviewed changes

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

ShellyKa13 force-pushed the memory-dump branch from 2e08ad4 to 04e1721 Compare May 23, 2022 08:41

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

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

ShellyKa13 added 15 commits May 24, 2022 16:11

Add virtcl command of memory dump

18e8da4

The command calls a subresource endpoint. Then the vm status is being patched with the memory dump command Signed-off-by: Shelly Kagan <skagan@redhat.com>

Adjust mount of hot plug of memory dump pvc

32d201b

The pvc of the memory dump should be mounted as a directory to be able to dump to it the memory, it doesnt have a disk to mount. Signed-off-by: Shelly Kagan <skagan@redhat.com>

Adjust update vmi webhooks to new memory dump volume source

c50137b

This volume source doesnt have a matching disk, need to take that into account in the checks verifications Signed-off-by: Shelly Kagan <skagan@redhat.com>

Add remove memory dump association virtctl command

0a41e57

The comman will dissocaite the memory dump, it will remove the memory dump from the volumes list and the memory dump request from the vm status. Signed-off-by: Shelly Kagan <skagan@redhat.com>

Make sure memory dump is not retriggered in vm restart

2b142db

Signed-off-by: Shelly Kagan <skagan@redhat.com>

Add memory dump functional tests

c593972

Signed-off-by: Shelly Kagan <skagan@redhat.com>

Change MemoryDumpVolumeSource to have inline PersistentVolumeClaimVol…

e59c113

…umeSource Signed-off-by: Shelly Kagan <skagan@redhat.com>

Add UTs

e97b19b

Signed-off-by: Shelly Kagan <skagan@redhat.com>

Add permissions to call memory dump

9c8774f

Signed-off-by: Shelly Kagan <skagan@redhat.com>

Change update memory dump completion mechanisem

e4d6acd

Instead of polling for the memory dump completion by the same memory dump command, now updates of completion or failure is updates on the domain. Adjusted code and unit tests accordingly. Signed-off-by: Shelly Kagan <skagan@redhat.com>

ShellyKa13 force-pushed the memory-dump branch from 04e1721 to e4d6acd Compare May 24, 2022 13:13

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

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

davidvossel reviewed May 27, 2022

View reviewed changes

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

kubevirt-bot merged commit 5bf9394 into kubevirt:main May 27, 2022

Memory dump #7517

Memory dump #7517

Conversation

ShellyKa13 commented Apr 4, 2022 • edited

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

ShellyKa13 commented Apr 6, 2022

ShellyKa13 commented Apr 11, 2022

arnongilboa Apr 12, 2022 • edited

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

duyanyan Apr 13, 2022 • edited

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

mhenriks 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

akalenyu Apr 14, 2022 • edited

Choose a reason for hiding this comment

ShellyKa13 commented May 23, 2022

mhenriks commented May 24, 2022

davidvossel left a comment

Choose a reason for hiding this comment

kubevirt-bot commented May 27, 2022

kubevirt-commenter-bot commented May 27, 2022

kubevirt-commenter-bot commented May 27, 2022

ShellyKa13 commented Apr 4, 2022 •

edited

arnongilboa Apr 12, 2022 •

edited

duyanyan Apr 13, 2022 •

edited

akalenyu Apr 14, 2022 •

edited