Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UI improvements #9773

Open
wants to merge 58 commits into
base: 4.20
Choose a base branch
from
Open

UI improvements #9773

wants to merge 58 commits into from

Conversation

abh1sar
Copy link
Collaborator

@abh1sar abh1sar commented Oct 6, 2024

Description

This PR does multiple UI improvements

  1. Autofill vCenter host, datacenter and credentials while adding cluster using the zone wide value from vmware_data_center table
  2. Show Usage settings as a different pane in global settings
  3. Option to attach the volume to an instance during volume create.
  4. Show mgmt server's service ip in the details tab of Infrastructure -> Management server.
  5. Don't display create volume snapshot button in the Instance tab for KVM, if the instance is running and "kvm.snapshot.enabled" is false. (Also contains some code cleanup in storage.js)
  6. The name Recurring snapshots is used everywhere except for one place where 'Scheduled' snapshots is used in the Recurring Snapshots form. Change it to Recurring Snapshots.
  7. Reorder the Hypervisor list shown everywhere in order of popularity.

Items 1,3,4,5,6 are UI only changes, and can be tested with just the UI.

Doc PR : apache/cloudstack-documentation#448

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

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

Screenshot 2024-10-06 at 7 47 11 AM Screenshot 2024-10-06 at 7 47 25 AM Screenshot 2024-10-04 at 3 01 02 AM Screenshot 2024-10-04 at 3 00 38 AM Screenshot 2024-10-04 at 4 13 23 AM

How Has This Been Tested?

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

Copy link

codecov bot commented Oct 6, 2024

Codecov Report

Attention: Patch coverage is 8.33333% with 11 lines in your changes missing coverage. Please review.

Project coverage is 16.03%. Comparing base (ac19379) to head (92a3dcd).

Files with missing lines Patch % Lines
...udstack/api/response/ManagementServerResponse.java 0.00% 4 Missing ⚠️
...a/com/cloud/vm/snapshot/VMSnapshotManagerImpl.java 0.00% 4 Missing ⚠️
...com/cloud/network/vpn/Site2SiteVpnManagerImpl.java 0.00% 2 Missing ⚠️
...ain/java/com/cloud/api/query/QueryManagerImpl.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               4.20    #9773      +/-   ##
============================================
- Coverage     16.03%   16.03%   -0.01%     
  Complexity    12814    12814              
============================================
  Files          5637     5637              
  Lines        493507   493552      +45     
  Branches      59831    59842      +11     
============================================
- Hits          79131    79130       -1     
- Misses       405600   405646      +46     
  Partials       8776     8776              
Flag Coverage Δ
uitests 4.01% <ø> (-0.01%) ⬇️
unittests 16.87% <8.33%> (-0.01%) ⬇️

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.

@abh1sar abh1sar marked this pull request as draft October 6, 2024 18:14
@abh1sar
Copy link
Collaborator Author

abh1sar commented Oct 6, 2024

@blueorangutan package

@blueorangutan
Copy link

@abh1sar 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
Copy link

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

@abh1sar
Copy link
Collaborator Author

abh1sar commented Oct 7, 2024

@blueorangutan ui

@blueorangutan
Copy link

@abh1sar a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.

@blueorangutan
Copy link

UI build: ✔️
Live QA URL: https://qa.cloudstack.cloud/simulator/pr/9773 (QA-JID-461)

@DaanHoogland
Copy link
Contributor

@abh1sar , given that there are DB changes as well, I don't think UI tests will be enought, do you?

@abh1sar abh1sar added this to the 4.20.1.0 milestone Oct 8, 2024
@abh1sar
Copy link
Collaborator Author

abh1sar commented Oct 8, 2024

@abh1sar , given that there are DB changes as well, I don't think UI tests will be enought, do you?

@DaanHoogland You are right.
These items contain db changes for which full testing will be required
2. Show Usage settings as a different pane in global settings
7. Reorder the Hypervisor list shown everywhere in order of popularity.

For the rest of the items, ui testing is enough.

P.S I have added db changes to schema-41900to42000 for now. Will move it to schema-42000-42010 once CloudStack version changes to 4.20.1. Will keep this PR in draft until then.

@@ -425,3 +425,8 @@ INSERT IGNORE INTO `cloud`.`guest_os_hypervisor` (uuid, hypervisor_type, hypervi

CALL `cloud`.`IDEMPOTENT_ADD_COLUMN`('cloud.vm_instance', 'delete_protection', 'boolean DEFAULT FALSE COMMENT "delete protection for vm" ');
CALL `cloud`.`IDEMPOTENT_ADD_COLUMN`('cloud.volumes', 'delete_protection', 'boolean DEFAULT FALSE COMMENT "delete protection for volumes" ');

-- Create a new group for Usage Server related configurations
INSERT INTO `cloud`.`configuration_group` (`name`, `description`, `precedence`) VALUES ('Usage Server', 'Usage Server related configuration', 9);
Copy link
Member

Choose a reason for hiding this comment

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

cc @sureshanaparti to check if configkeys can be self-discovered without any schema paths? Otherwise, we may need to move this to a new 4.20.0->4.20.1 upgrade path.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@sureshanaparti sureshanaparti Oct 30, 2024

Choose a reason for hiding this comment

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

yes, the config keys will be discover group and sub-group if they doesn't have (so, mostly for the new configs added after this new group & subgroup). If the config keys already have (all existing configs) and need to be re-discover to this, then schema path needs to remove group and sub-group for the configs Or update configs with group and sub-group ids with the new group and sub-group ids added here.

cc @abh1sar @borisstoyanov @rohityadavcloud

Copy link
Contributor

@Pearl1594 Pearl1594 left a comment

Choose a reason for hiding this comment

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

clgtm, just a few queries.

return ((['Running'].includes(record.state) && record.hypervisor !== 'LXC') ||
(['Stopped'].includes(record.state) && !['KVM', 'LXC'].includes(record.hypervisor)))
show: (record, store) => {
return (record.hypervisor !== 'KVM') ||
Copy link
Contributor

Choose a reason for hiding this comment

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

why destroyed state?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried to keep the UI conditions same as the service layer. Please see hostSupportsSnapsthotForVolume().

But I'm ok if we don't want to allow snapshot creation from UI for destroyed VMs.

record.hypervisor === 'KVM' && record.vmstate !== 'Running')
return record.state === 'Ready' &&
(record.hypervisor !== 'KVM' ||
['Stopped', 'Destroyed'].includes(record.vmstate) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

same here, why destroyed state ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same as above

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Before this change, the button would be shown for all states part from 'Running'.
Now it will only be shown for 'Stopped' or 'Destroyed' states

record.hypervisor === 'KVM' && record.vmstate !== 'Running')
return record.state === 'Ready' &&
(record.hypervisor !== 'KVM' ||
(['Stopped', 'Destroyed'].includes(record.vmstate)) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

same doubt as above

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same as above

@blueorangutan
Copy link

[SF] Trillian Build Failed (tid-11647)

Copy link
Collaborator

@rajujith rajujith left a comment

Choose a reason for hiding this comment

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

All the mentioned fixes works fine.

@blueorangutan
Copy link

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

@DaanHoogland
Copy link
Contributor

@blueorangutan test

@blueorangutan
Copy link

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

@blueorangutan
Copy link

[SF] Trillian test result (tid-11847)
Environment: kvm-ol8 (x2), Advanced Networking with Mgmt server ol8
Total time taken: 53143 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr9773-t11847-kvm-ol8.zip
Smoke tests completed. 139 look OK, 2 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 385.48 test_purge_expunged_vms.py
test_01_secure_vm_migration Error 134.12 test_vm_life_cycle.py
test_01_secure_vm_migration Error 134.13 test_vm_life_cycle.py

@DaanHoogland
Copy link
Contributor

@andrijapanicsb @rohityadavcloud , are your concerns dealt with, here? this seems ready for merge otherwise.

@abh1sar abh1sar marked this pull request as draft December 6, 2024 04:53
@abh1sar
Copy link
Collaborator Author

abh1sar commented Dec 6, 2024

@blueorangutan package

@blueorangutan
Copy link

@abh1sar 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
Copy link

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

@abh1sar
Copy link
Collaborator Author

abh1sar commented Dec 6, 2024

@blueorangutan package

@blueorangutan
Copy link

@abh1sar 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
Copy link

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

@abh1sar
Copy link
Collaborator Author

abh1sar commented Dec 6, 2024

@blueorangutan test

@blueorangutan
Copy link

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

@blueorangutan
Copy link

[SF] Trillian test result (tid-11862)
Environment: kvm-ol8 (x2), Advanced Networking with Mgmt server ol8
Total time taken: 62011 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr9773-t11862-kvm-ol8.zip
Smoke tests completed. 139 look OK, 2 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 396.93 test_purge_expunged_vms.py
test_03_secured_to_nonsecured_vm_migration Error 357.40 test_vm_life_cycle.py

@abh1sar abh1sar marked this pull request as ready for review December 10, 2024 10:40
@abh1sar
Copy link
Collaborator Author

abh1sar commented Dec 17, 2024

@blueorangutan package

@blueorangutan
Copy link

@abh1sar 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
Copy link

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

@rohityadavcloud
Copy link
Member

@blueorangutan test

@blueorangutan
Copy link

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

@blueorangutan
Copy link

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

Test Result Time (s) Test File
test_01_browser_migrate_template Error 68.75 test_image_store_object_migration.py
test_06_purge_expunged_vm_background_task Failure 400.79 test_purge_expunged_vms.py

@abh1sar
Copy link
Collaborator Author

abh1sar commented Dec 18, 2024

@andrijapanicsb please check and let me know if you have more review comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants