hook: add PVC option #10598
hook: add PVC option #10598
Conversation
kubevirt-bot
added
do-not-merge/work-in-progress
|
@alicefr: GitHub didn't allow me to request PR reviews from the following users: zippy2. Note that only kubevirt members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this: 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. |
|
@vasiliy-ul I'd love also to have your opinion on this feature, would you mind taking a look at this guide when you have a bit of time |
There was a problem hiding this comment.
@alicefr, very interesting approach. I particularly liked the detailed guide and the demo 👍
Regarding the
I'd love also to integrate the documentation and demo somehow upstream and suggestions are welcome.
I think this can be part of the user guide. A section about troubleshooting the VMs and debug techniques would be very useful. Additionally, didn't you think about a blog post?
Use a PVC for storing and sharing files between the compute and the sidecar containers. Signed-off-by: Alice Frosi <afrosi@redhat.com>
kubevirt-bot
removed
the
do-not-merge/work-in-progress
|
I update the PR with a unit test for the hook option and I think now this is ready for review |
Signed-off-by: Alice Frosi <afrosi@redhat.com>
| if requestedHookSidecar.PVC.SharedComputePath != "" { | ||
| containers[0].VolumeMounts = append(containers[0].VolumeMounts, | ||
| k8sv1.VolumeMount{ | ||
| Name: requestedHookSidecar.PVC.Name, | ||
| MountPath: requestedHookSidecar.PVC.SharedComputePath, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Could this be a condition of its own at the same level as if requestedHookSidecar.PVC != nil. Mainly to reduce the cyclomatic complexity.
There was a problem hiding this comment.
It could reduce the nested if but I need to recheck if the PVC is nil and it would add another condition to the if. Hence, I'm not sure if this is more readable
There was a problem hiding this comment.
Totally missed the possibility of nil here. 🤦🏽♂️
| } | ||
| sidecarVolumes = append(sidecarVolumes, vol) | ||
| if requestedHookSidecar.PVC.SharedComputePath != "" { | ||
| containers[0].VolumeMounts = append(containers[0].VolumeMounts, |
There was a problem hiding this comment.
WDYT about using compute.VolumeMounts instead of containers[0].VolumeMounts because, IIUC, we're appending to compute container's volume mounts here and the variable compute is a pointer:
There was a problem hiding this comment.
The containers aren't pointers, so, I don't think that modifying the compute object will be reflected in the containers array. AFAIU, they are full copies
|
I have nothing to add, I think the code looks fine and the feature is very very helpful. |
|
/test pull-kubevirt-e2e-k8s-1.27-sig-storage |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vasiliy-ul 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:
Approvers can indicate their approval by writing |
kubevirt-bot
added
the
approved
Use a PVC for storing and sharing files between the compute and the sidecar containers.
What this PR does / why we need it:
This PR adds the possibility of using a PVC with the KubeVirt hook sidecar and the compute container.
One usage of this enhancement is for debugging purposes and a full guide with a demo is available here. This option allows to supply of additional debug tools without the need to rebuild the virt-launcher image and explain how to launch QEMU with a debug tool like strace.
A concrete example of when this can be useful is when QEMU faces some permission issues, like in this case. For those types of failures, starting QEMU with strace can save you a lot of time.
Special notes for your reviewer:
Please check the guide, otherwise, these changes standalone don't make much sense. I'd love also to integrate the documentation and demo somehow upstream and suggestions are welcome.
Additionally, we could provide an additional image as part of KubeVIrt release with the most used debugging tool that could be used to populate the PVC.