kubevirt
kubevirt copied to clipboard
kubevirt
WIP: new interface for mount operations and code refactor in virt handler
What this PR does / why we need it: This PR introduces a high-level interface for mount operations. The goal is to abstract the mounting operations in order to facilitate the introduction of a new type of mount. An example is the mounts of the socket of an externally managed daemon like the case of the SCSI persistent reservation (see WIP PR: https://github.com/kubevirt/kubevirt/pull/8210 ). Container disks and hotplugged disks have a lot of duplication for recording the mount. This PR introduces a common utils for recording the mount. This also simplifies the test for hotplugged as it separates the logic of recording and the hotplug volume operations.
There are some further improvements we can focus on in this PR (or a following one):
- create a common mechanism to set the kubelet pods dir for all the mounting mechanism (see comment: https://github.com/kubevirt/kubevirt/pull/8029#issuecomment-1173878316)
- create common functions to identify the volume on the node (eg for container disks and for hotplugged volumes)
- my hope is that we could use the same mount function for all the cases (at least for filesystem mounts) but I haven't gone so far with my findings
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:
NONE
@rmohr @awels @ShellyKa13 feedback from your side will be highly appreciated :)
@rmohr @awels @ShellyKa13 feedback from your side will be highly appreciated :)
A little bit short on time at the moment, but I will.
Well I just read the description and I already like it +1
+1
- my hope is that we could use the same mount function for all the cases (at least for filesystem mounts) but I haven't gone so far with my findings
At least having unmount centralized may be very beneficial. Unmounts on cleanup are the most critical part of all this, because we pollute the nodes if we don't do it properly.
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign vladikr for approval by writing /assign @vladikr in a comment. For more information see:The Kubernetes Code Review Process.
The full list of commands accepted by this bot can be found here.
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
@alicefr: 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.23-sig-compute-migrations-nonroot | a4987f06801cb023b0d3aa9d08a415413f48e645 | link | true | /test pull-kubevirt-e2e-k8s-1.23-sig-compute-migrations-nonroot |
| pull-kubevirt-e2e-k8s-1.23-sig-network-nonroot | 8b5cf94cbaa7017eddef49e2a06552d3ffb3979c | link | true | /test pull-kubevirt-e2e-k8s-1.23-sig-network-nonroot |
| pull-kubevirt-e2e-k8s-1.23-operator-nonroot | 8b5cf94cbaa7017eddef49e2a06552d3ffb3979c | link | true | /test pull-kubevirt-e2e-k8s-1.23-operator-nonroot |
| pull-kubevirt-e2e-k8s-1.23-sig-storage-nonroot | 8b5cf94cbaa7017eddef49e2a06552d3ffb3979c | link | true | /test pull-kubevirt-e2e-k8s-1.23-sig-storage-nonroot |
| pull-kubevirt-e2e-k8s-1.24-sig-storage-dv-gc | 54d7bd854836e9669a40d415255a4b51f257bcdf | link | true | /test pull-kubevirt-e2e-k8s-1.24-sig-storage-dv-gc |
| pull-kubevirt-goveralls | 862b90e59c594ad4625aa45b08e80fc2f64bd146 | link | false | /test pull-kubevirt-goveralls |
| pull-kubevirt-unit-test | 862b90e59c594ad4625aa45b08e80fc2f64bd146 | link | true | /test pull-kubevirt-unit-test |
| pull-kubevirt-e2e-k8s-1.22-sig-compute | 862b90e59c594ad4625aa45b08e80fc2f64bd146 | link | true | /test pull-kubevirt-e2e-k8s-1.22-sig-compute |
| pull-kubevirt-e2e-kind-1.22-sriov | 862b90e59c594ad4625aa45b08e80fc2f64bd146 | link | true | /test pull-kubevirt-e2e-kind-1.22-sriov |
| pull-kubevirt-e2e-k8s-1.22-sig-performance | 862b90e59c594ad4625aa45b08e80fc2f64bd146 | link | false | /test pull-kubevirt-e2e-k8s-1.22-sig-performance |
| pull-kubevirt-e2e-k8s-1.23-operator | 862b90e59c594ad4625aa45b08e80fc2f64bd146 | link | true | /test pull-kubevirt-e2e-k8s-1.23-operator |
| pull-kubevirt-e2e-k8s-1.22-operator | 862b90e59c594ad4625aa45b08e80fc2f64bd146 | link | true | /test pull-kubevirt-e2e-k8s-1.22-operator |
| pull-kubevirt-e2e-k8s-1.24-operator-nonroot | 862b90e59c594ad4625aa45b08e80fc2f64bd146 | link | true | /test pull-kubevirt-e2e-k8s-1.24-operator-nonroot |
| pull-kubevirt-e2e-k8s-1.24-operator | 862b90e59c594ad4625aa45b08e80fc2f64bd146 | link | true | /test pull-kubevirt-e2e-k8s-1.24-operator |
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.
Currently, this version is breaking the operator when we upgrade because I merged the cache files for container disks and hotplug into a single file. I'm not quite sure if I should keep the old format or add a legacy function that transforms the old version of the cache file into the new one. Any good suggestions?
Old way:
/var/run/kubevirt-private/
|____ container-disk-mount-state/
| |________________ 6856c1b7-fb20-4b05-aeab-b23f211cfa76
| |________________ df2ca0e1-f48e-48d5-9ba6-50634742c9e3
|
|____ hotplug-volume-mount-state
|________________ 6856c1b7-fb20-4b05-aeab-b23f211cfa76
With the new way each file contains all the mount entries:
/var/run/kubevirt-private/
|______ 6856c1b7-fb20-4b05-aeab-b23f211cfa76
|______ df2ca0e1-f48e-48d5-9ba6-50634742c9e3
@rmohr or @vladikr when you have a bit of time, could you please have a look to this?
Currently, this version is breaking the operator when we upgrade because I merged the cache files for container disks and hotplug into a single file. I'm not quite sure if I should keep the old format or add a legacy function that transforms the old version of the cache file into the new one. Any good suggestions?
My first thought is having a function that basically reads the legacy format, and converts it to the new format, and once the conversion has been verified, removes the legacy files. And it does nothing if the legacy files don't exist.
Maybe have that run on startup of the virt-handler before it becomes ready.
Currently, this version is breaking the operator when we upgrade because I merged the cache files for container disks and hotplug into a single file. I'm not quite sure if I should keep the old format or add a legacy function that transforms the old version of the cache file into the new one. Any good suggestions?
My first thought is having a function that basically reads the legacy format, and converts it to the new format, and once the conversion has been verified, removes the legacy files. And it does nothing if the legacy files don't exist.
Maybe have that run on startup of the virt-handler before it becomes ready.
Yes, it was my idea. It just introduces additional logic that we need to test. My question is if it is worthy of doing it or we could keep the old structure
As discussed with @awels, I decided to keep on the filesystem the current version of the recording (see the comment)
@rmohr @awels @xpivarc could you take a look to this PR when you have time? Many thanks
Rebase + @vasiliy-ul suggestions (https://github.com/kubevirt/kubevirt/pull/8317#discussion_r1071096127) and unit tests fix. The upgrade test should fail as I haven't added the handling of the legacy version of the record file, but the rest should work as before
we clarified with @vasiliy-ul that this new version keeps backward compatibility with the cached records.
@vasiliy-ul thanks for the review! I have updated the tests and the print message. However, I left the naming more generic, see for more detail in each comment.
/lgtm /hold
Adding a hold, so the interested ppl can take one more look here (since the PR has slightly changed). I believe that technically the PR is correct. @alicefr, feel free to remove the hold after some time if no concerns are raised here.
Overall looks good. My main concern is about the hotplug but I might have missed something.