NAS B&R Plugin enhancements by Pearl1594 · Pull Request #9666 · apache/cloudstack

Pearl1594 · Sep 10, 2024

Description

This PR fixes small issues and adds minor improvements to the NAS B&R Plugin

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)
build/CI
test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

Pearl1594 · Sep 10, 2024

@blueorangutan package

blueorangutan · Sep 10, 2024

@Pearl1594 a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

codecov · Sep 10, 2024

Codecov Report

Attention: Patch coverage is 0% with 58 lines in your changes missing coverage. Please review.

Project coverage is 15.98%. Comparing base (5aa79a6) to head (9714aae).
Report is 69 commits behind head on 4.20.

Files with missing lines	Patch %	Lines
...rg/apache/cloudstack/backup/BackupManagerImpl.java	0.00%	24 Missing ⚠️
...ce/wrapper/LibvirtRestoreBackupCommandWrapper.java	0.00%	13 Missing ⚠️
...bernetes/cluster/KubernetesClusterManagerImpl.java	0.00%	10 Missing ⚠️
...rg/apache/cloudstack/backup/NASBackupProvider.java	0.00%	6 Missing ⚠️
.../actionworkers/KubernetesClusterDestroyWorker.java	0.00%	3 Missing ⚠️
...i/command/user/backup/DeleteBackupScheduleCmd.java	0.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               4.20    #9666      +/-   ##
============================================
- Coverage     15.99%   15.98%   -0.02%     
- Complexity    13060    13084      +24     
============================================
  Files          5644     5649       +5     
  Lines        494915   495747     +832     
  Branches      59960    60029      +69     
============================================
+ Hits          79173    79254      +81     
- Misses       406910   407640     +730     
- Partials       8832     8853      +21

Flag	Coverage Δ
uitests	`4.01% <ø> (-0.01%)`	⬇️
unittests	`16.82% <0.00%> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

blueorangutan · Sep 10, 2024

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 11085

DaanHoogland · Sep 11, 2024

This looks like a bug fix on a new feature @Pearl1594 , should it go in 4.20? cc @JoaoJandre

Pearl1594 · Sep 11, 2024

This isn't targeted for 4.20 @DaanHoogland. This PR is to improve aspects of the recently merged changes. There will be more changes added to this PR.

…om response

github-actions · Sep 20, 2024

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

…br-partb

Pearl1594 · Sep 23, 2024

@blueorangutan package

blueorangutan · Sep 23, 2024

@Pearl1594 a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · Sep 23, 2024

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 11180

yadvr · Feb 11, 2025

@Pearl1594 can you address conflicts and advise if this is ready for reviewing and testing.

…br-partb

Pearl1594 · Feb 11, 2025

@blueorangutan package

blueorangutan · Feb 11, 2025

@Pearl1594 a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · Feb 11, 2025

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 12412

Pearl1594 · Feb 11, 2025

@blueorangutan test

blueorangutan · Feb 11, 2025

@Pearl1594 a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

blueorangutan · Feb 12, 2025

[SF] Trillian test result (tid-12376)
Environment: kvm-ol8 (x2), Advanced Networking with Mgmt server ol8
Total time taken: 57260 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr9666-t12376-kvm-ol8.zip
Smoke tests completed. 138 look OK, 3 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_11_isolated_network_with_dynamic_routed_mode	`Error`	1.29	test_ipv4_routing.py
test_12_vpc_and_tier_with_dynamic_routed_mode	`Error`	2.35	test_ipv4_routing.py
test_12_vpc_and_tier_with_dynamic_routed_mode	`Error`	2.35	test_ipv4_routing.py
test_06_purge_expunged_vm_background_task	`Failure`	391.36	test_purge_expunged_vms.py
test_12_start_vm_multiple_volumes_allocated	`Error`	14.90	test_vm_life_cycle.py

abh1sar · Feb 19, 2025

@Pearl1594 added some minor comments. Please check.

Pearl1594 · Feb 19, 2025

@abh1sar sorry I don't see your review comments.

abh1sar

Added some minor comments

abh1sar · Nov 5, 2024

server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java

-        accountManager.checkAccess(CallContext.current().getCallingAccount(), null, true, vm);
+    public boolean deleteBackupSchedule(DeleteBackupScheduleCmd cmd) {
+        Long vmId = cmd.getVmId();
+        Long id = cmd.getId();


In DeleteBackupScheduleCmd, both vmId and id parameters are declared with required = true, but here it looks like either of them could be null.

You're are right. I've addressed it.

abh1sar · Feb 19, 2025

plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java

            for (final Backup backup: backupDao.listByVmId(null, vm.getId())) {
-                vmBackupSize += backup.getSize();
-                vmBackupProtectedSize += backup.getProtectedSize();
+                if (Objects.nonNull(backup.getSize())) {


can these be null?

I am not sure - but just added a defensive check

abh1sar · Feb 19, 2025

scripts/vm/hypervisor/kvm/nasbackup.sh

  dest="$mount_point/${BACKUP_DIR}"
-  mount -t ${NAS_TYPE} ${NAS_ADDRESS} ${mount_point} $([[ ! -z "${MOUNT_OPTS}" ]] && echo -o ${MOUNT_OPTS})
+  if [ ${NAS_TYPE} == "cifs" ]; then
+    MOUNT_OPTS="${MOUNT_OPTS},nobrl"


nobrl is added in mountBackupDirectory. Is it required here as well?

I had faced the following issue while backing up the VM:

# virsh -c qemu:///system backup-begin --domain i-2-10-VM --backupxml /tmp/csbackup.HEGID/i-2-10-VM/2024.10.07.02.44.28/backup.xml error: operation failed: failed to format image: 'Could not write qcow2 header: Permission denied'

As per https://unix.stackexchange.com/questions/622194/how-to-use-qemu-kvm-virtual-machine-disk-image-on-smb-cifs-network-share-permis adding nobrl solved it - so I used that fix.

api/src/main/java/org/apache/cloudstack/api/command/user/backup/DeleteBackupScheduleCmd.java

…bkp schedule command

Pearl1594 · Feb 27, 2025

@blueorangutan package

blueorangutan · Feb 27, 2025

@Pearl1594 a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · Feb 27, 2025

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 12602

Pearl1594 · Feb 27, 2025

@blueorangutan test

blueorangutan · Feb 27, 2025

@Pearl1594 a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

blueorangutan · Feb 28, 2025

[SF] Trillian test result (tid-12505)
Environment: kvm-ol8 (x2), Advanced Networking with Mgmt server ol8
Total time taken: 52930 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr9666-t12505-kvm-ol8.zip
Smoke tests completed. 140 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_06_purge_expunged_vm_background_task	`Failure`	391.44	test_purge_expunged_vms.py

abh1sar

LGTM. didn't test.

Pearl1594 · Mar 4, 2025

Tested by @rajujith.

* NAS B&R Plugin enhancements * Prevent printing mount opts which may include password by removing from response * revert marvin change * add sanity checks to validate minimum qemu and libvirt versions * check is user running script is part of libvirt group * revert changes of retore expunged VM * add code coverage ignore file * remove check * issue with listing schedules and add defensive checks * redirect logs to agent log file * add some more debugging * remove test file * prevent deletion of cks cluster when vms associated to a backup offering * delete all snapshot policies when bkp offering is disassociated from a VM * Fix `updateTemplatePermission` when the UI is set to a language other than English (apache#9766) * Fix updateTemplatePermission UI in non-english language * Improve fix --------- * Add nobrl in the mountopts for cifs file system * Fix restoration of VM / volumes with cifs * add cifs utils for el8 * add cifs-utils for ubuntu cloudstack-agent * syntax error * remove required constraint on both vmid and id params for the delete bkp schedule command

NAS B&R Plugin enhancements

64db486

boring-cyborg bot added the component:kubernetes label Sep 10, 2024

yadvr modified the milestones: 4.20.0.0, 4.20.1.0 Sep 19, 2024

Prevent printing mount opts which may include password by removing fr…

d5a259d

…om response

boring-cyborg bot added component:api component:backup component:kvm component:marvin component:UI Python Warning... Python code Ahead! labels Sep 20, 2024

github-actions bot added the status:has-conflicts label Sep 20, 2024

Merge branch 'main' of https://github.com/apache/cloudstack into nas-…

e2cb774

…br-partb

github-actions bot removed the status:has-conflicts label Sep 20, 2024

Pearl1594 added 3 commits September 20, 2024 12:08

revert marvin change

6dfb0ea

add sanity checks to validate minimum qemu and libvirt versions

b1ccf9d

check is user running script is part of libvirt group

343e7dc

Pearl1594 force-pushed the nas-br-partb branch from 8dadb49 to 343e7dc Compare September 23, 2024 14:30

revert changes of retore expunged VM

f2f81c8

yadvr assigned abh1sar Feb 11, 2025

Merge branch '4.20' of https://github.com/apache/cloudstack into nas-…

5e39348

…br-partb

github-actions bot removed the status:has-conflicts label Feb 11, 2025

rajujith removed their assignment Feb 19, 2025

abh1sar reviewed Feb 27, 2025

View reviewed changes

remove required constraint on both vmid and id params for the delete …

9714aae

…bkp schedule command

abh1sar approved these changes Feb 28, 2025

View reviewed changes

Pearl1594 merged commit 7f4e6a9 into apache:4.20 Mar 4, 2025
24 of 26 checks passed

Pearl1594 deleted the nas-br-partb branch March 4, 2025 16:32

Pearl1594 added this to ACS 4.20.1 Mar 17, 2025

Pearl1594 moved this to Done in ACS 4.20.1 Mar 17, 2025

abh1sar mentioned this pull request Apr 29, 2025

Nas BnR: Fix for restore not working correctly #10785

Merged

14 tasks

bernardodemarco mentioned this pull request Jul 16, 2025

Fix deletion of backup schedules #11222

Merged

9 tasks

Search code, repositories, users, issues, pull requests...

Conversation

Pearl1594 commented Sep 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

Uh oh!

Pearl1594 commented Sep 10, 2024

Uh oh!

blueorangutan commented Sep 10, 2024

Uh oh!

codecov bot commented Sep 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

blueorangutan commented Sep 10, 2024

Uh oh!

DaanHoogland commented Sep 11, 2024

Uh oh!

Pearl1594 commented Sep 11, 2024

Uh oh!

github-actions bot commented Sep 20, 2024

Uh oh!

Pearl1594 commented Sep 23, 2024

Uh oh!

blueorangutan commented Sep 23, 2024

Uh oh!

blueorangutan commented Sep 23, 2024

Uh oh!

yadvr commented Feb 11, 2025

Uh oh!

Pearl1594 commented Feb 11, 2025

Uh oh!

blueorangutan commented Feb 11, 2025

Uh oh!

blueorangutan commented Feb 11, 2025

Uh oh!

Pearl1594 commented Feb 11, 2025

Uh oh!

blueorangutan commented Feb 11, 2025

Uh oh!

blueorangutan commented Feb 12, 2025

Uh oh!

abh1sar commented Feb 19, 2025

Uh oh!

Pearl1594 commented Feb 19, 2025

Uh oh!

abh1sar left a comment

Choose a reason for hiding this comment

Uh oh!

abh1sar Nov 5, 2024

Choose a reason for hiding this comment

Uh oh!

Pearl1594 Feb 27, 2025

Choose a reason for hiding this comment

Uh oh!

abh1sar Feb 19, 2025

Choose a reason for hiding this comment

Uh oh!

Pearl1594 Feb 27, 2025

Choose a reason for hiding this comment

Uh oh!

abh1sar Feb 19, 2025

Choose a reason for hiding this comment

Uh oh!

Pearl1594 Feb 27, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Pearl1594 commented Feb 27, 2025

Uh oh!

blueorangutan commented Feb 27, 2025

Uh oh!

blueorangutan commented Feb 27, 2025

Pearl1594 commented Sep 10, 2024 •

edited

Loading

codecov bot commented Sep 10, 2024 •

edited

Loading