Skip to content

Conversation

@amastbau
Copy link
Contributor

@amastbau amastbau commented Apr 15, 2025

In order to restore one or more Virtual Machines out of many in the SAME namespace, velero offers restoring by resource label.
When using a label selector, any resource not including the restore label are excluded by velero.
That might include other resources that are quieted for the VM, such as datavolumes and PVCs.
In order to resolve this, we already include any additional required resource in a graph which is return to velero for backup/restore even if those resources do not include matching label.

With this PR we are removing a condition results in excluding PVC for datavolume type VM volume from the graph, and a the datavolume pending for ever for the existence of the associated PVC after restore.

fixes issue: https://issues.redhat.com/browse/OADP-5608

How to test?
Deploy the following VM, with DataVolume type volume and a label.

$ kubectl create namespace test-vm
apiVersion: kubevirt.io/v1
kind: VirtualMachine
metadata:
  labels:
    app: test-vm-1
  name: test-vm-1
  namespace: test-vm
spec:
  dataVolumeTemplates:
  - metadata:
      annotations:
        cdi.kubevirt.io/storage.deleteAfterCompletion: "false"
      creationTimestamp: null
      name: test-vm-1-dv
    spec:
      pvc:
        accessModes:
        - ReadWriteOnce
        resources:
          requests:
            storage: 150Mi
        volumeMode: Block
      source:
        registry:
          pullMethod: node
          url: docker://quay.io/kubevirt/cirros-container-disk-demo
  running: true
  template:
    metadata:
      creationTimestamp: null
      name: test-vm-1
    spec:
      architecture: amd64
      domain:
        devices:
          disks:
          - disk:
              bus: virtio
            name: volume0
          interfaces:
          - masquerade: {}
            name: default
          rng: {}
        machine:
          type: q35
        resources:
          requests:
            memory: 256M
      networks:
      - name: default
        pod: {}
      terminationGracePeriodSeconds: 0
      volumes:
      - dataVolume:
          name: test-vm-1-dv
        name: volume0

$ velero backup create test-vm-backup --include-namespaces=test-vm
$ kubectl delete namespace/test-vm
$ velero restore create restore --from-backup=test-vm-backup --selector=app=test-vm-1
# The DataVolume and the PVC are included in the restore:

$ velero restore describe restore --details
Name:         restore
Namespace:    openshift-adp
Labels:       <none>
Annotations:  <none>

Phase:                       Completed
Total items to be restored:  7
Items restored:              7

Started:    2025-04-15 12:47:34 +0000 UTC
Completed:  2025-04-15 12:47:45 +0000 UTC

Backup:  test-vm-backup

Namespaces:
  Included:  all namespaces found in the backup
  Excluded:  <none>

Resources:
  Included:        *
  Excluded:        nodes, events, events.events.k8s.io, backups.velero.io, restores.velero.io, resticrepositories.velero.io, csinodes.storage.k8s.io, volumeattachments.storage.k8s.io, backuprepositories.velero.io
  Cluster-scoped:  auto

Namespace mappings:  <none>

Label selector:  app=test-vm-1

Or label selector:  <none>

Restore PVs:  auto

CSI Snapshot Restores:
  test-vm/test-vm-1-dv:
    Snapshot:
      Snapshot Content Name: 869720b2434ab00b3947a8811a1d62d3c427a389ae14d6b7edc5ebf4ada6c1dc
      Storage Snapshot ID: 0001-0011-openshift-storage-0000000000000002-6e885852-eacd-4673-8c04-ed000af1ff7c
      CSI Driver: openshift-storage.rbd.csi.ceph.com

Existing Resource Policy:   <none>
ItemOperationTimeout:       4h0m0s

Preserve Service NodePorts:  auto

Uploader config:


HooksAttempted:   0
HooksFailed:      0

Resource List:
  cdi.kubevirt.io/v1beta1/DataVolume:
    - test-vm/test-vm-1-dv(created)
  kubevirt.io/v1/VirtualMachine:
    - test-vm/test-vm-1(created)
  snapshot.storage.k8s.io/v1/VolumeSnapshot:
    - test-vm/velero-test-vm-1-dv-jp4jt(created)
  snapshot.storage.k8s.io/v1/VolumeSnapshotContent:
    - snapcontent-9765a149-df9b-4882-aa46-7783b012f5c2(created)
  v1/Namespace:
    - test-vm(created)
  v1/PersistentVolume:
    - pvc-19e9056b-c772-433b-8f46-5cc3d97f65f5(skipped)
  v1/PersistentVolumeClaim:
    - test-vm/test-vm-1-dv(created)

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:

FixBug: Include PVC in restore VM graph so it will be included even when using label selector that would exclude it.

@kubevirt-bot kubevirt-bot added the dco-signoff: yes Indicates the PR's author has DCO signed all their commits. label Apr 15, 2025
@kubevirt-bot kubevirt-bot requested review from aglitke and awels April 15, 2025 09:06
@kubevirt-bot
Copy link

Hi @amastbau. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

Details

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-sigs/prow repository.

@alromeros
Copy link
Member

/cc @alromeros

@kubevirt-bot kubevirt-bot requested a review from alromeros April 15, 2025 12:59
Copy link
Contributor

@ShellyKa13 ShellyKa13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, I would say that it will be good to also explain the issue in the commit message.
Also missing a UT for this
And I believe this change justifies a release note since it fixes a bug

@weshayutin
Copy link
Contributor

test container based vm's restore via selector

All passed

Setup

BACKUP 3 cirros vm's

e.g. from vm-1

apiVersion: kubevirt.io/v1
kind: VirtualMachine
metadata:
  annotations:
  generation: 1
  labels:
    app: cirros-test-cont-1
  name: cirros-test-cont-1
  namespace: cirros-test-cont
spec:
  dataVolumeTemplates:
  - metadata:
      annotations:
        cdi.kubevirt.io/storage.deleteAfterCompletion: "false"
      creationTimestamp: null
      name: cirros-test-cont-1-dv
    spec:
      pvc:
        accessModes:
        - ReadWriteOnce
        resources:
          requests:
            storage: 150Mi
        volumeMode: Block
      source:
        registry:
          pullMethod: node
          url: docker://quay.io/kubevirt/cirros-container-disk-demo
  running: true
  template:
    metadata:
      creationTimestamp: null
      name: cirros-test-cont-1
    spec:
      architecture: amd64
      domain:
        devices:
          disks:
          - disk:
              bus: virtio
            name: volume0
          interfaces:
          - masquerade: {}
            name: default
          rng: {}
        machine:
          type: q35
        resources:
          requests:
            memory: 256M
      networks:
      - name: default
        pod: {}
      terminationGracePeriodSeconds: 0
      volumes:
      - dataVolume:
          name: cirros-test-cont-1-dv
        name: volume0

backup results

backup describe https://termbin.com/ma7l

Nuke the vm's from orbit

oc delete -f cirros-test-1.yaml -f cirros-test-2.yaml -f cirros-test-3.yaml

Restore ALL

https://termbin.com/utox

Nuke all, restore 1 vm via selector

https://termbin.com/21vg

Nuke all, restore 1 vm ( second vm) via selector

https://termbin.com/shi2

Copy link
Member

@alromeros alromeros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just couple of comments

@amastbau amastbau force-pushed the include-pvc-in-vmi-action branch from 2c1b9c1 to df0cf1f Compare April 16, 2025 08:56
@amastbau amastbau force-pushed the include-pvc-in-vmi-action branch from df0cf1f to 66774ab Compare April 16, 2025 10:12
@ShellyKa13
Copy link
Contributor

/approve
Thanks @amastbau looks good

@kubevirt-bot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ShellyKa13

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 16, 2025
@alromeros
Copy link
Member

Nit but could you fix the typos in the commit name?
Do not skip PVC when building VMIs resaources grap -> Do not skip PVC when building VMIs resources graph
Looks good otherwise, thanks!

during restore item action.

In order to restore one or more Virtual Machines out of many in the SAME namespace, velero offers restoring by resource label.
When using a label selector, any resource not including the restore label are excluded by velero.
That might include other resources that are quieted for the VM, such as datavolumes and PVCs.
In order to resolve this, we already include any additional required resource in a graph which is return to velero for backup/restore even if those resources do not include matching label.

With this PR we are removing a condition results in excluding PVC for datavolume type VM volume from the graph, and a the datavolume pending for ever for the existence of the associated PVC after restore.

Signed-off-by: Amos Mastbaum <[email protected]>
@amastbau amastbau force-pushed the include-pvc-in-vmi-action branch from 66774ab to b331f12 Compare April 17, 2025 07:27
Copy link
Contributor

@ShellyKa13 ShellyKa13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks approving with the approval of @alromeros
/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 17, 2025
@kubevirt-bot kubevirt-bot merged commit ac88b8c into kubevirt:main Apr 17, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. size/M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants