- 
                Notifications
    You must be signed in to change notification settings 
- Fork 1.2k
Find system VM templates for CKS clusters and SharedFS honouring the preferred architecture #10946
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
Find system VM templates for CKS clusters and SharedFS honouring the preferred architecture #10946
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM didn't test but code/path looks alright
| Codecov Report❌ Patch coverage is  Additional details and impacted files@@             Coverage Diff              @@
##               4.20   #10946      +/-   ##
============================================
+ Coverage     16.14%   16.16%   +0.01%     
- Complexity    13253    13280      +27     
============================================
  Files          5656     5656              
  Lines        497893   497941      +48     
  Branches      60374    60388      +14     
============================================
+ Hits          80405    80489      +84     
+ Misses       408529   408489      -40     
- Partials       8959     8963       +4     
 Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
 | 
| @blueorangutan package | 
        
          
                engine/schema/src/main/java/com/cloud/storage/dao/VMTemplateDaoImpl.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
      | @blueorangutan package | 
| @nvazquez 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. | 
| Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 13580 | 
| @blueorangutan package | 
| @nvazquez 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. | 
| Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 13581 | 
| @blueorangutan package | 
| @nvazquez 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. | 
| Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 13582 | 
| @nvazquez can you address the outstanding questions & remarks? | 
| Thanks @harikrishna-patnala @rohityadavcloud - I have addressed your comments, can you please re-review? @blueorangutan package | 
| @nvazquez 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. | 
| Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 13816 | 
| @rohityadavcloud @harikrishna-patnala wouldn't it be better to filter out the templates that do not match the CKS ISO architecture instead of sorting them by the prefered arch setting? Anyways, I'm adding the check for CKS ISO arch vs selected template arch and failing with a proper message in case of mismatch | 
…the selected template arch
| @blueorangutan package | 
    
      
        1 similar comment
      
    
  
    | @blueorangutan package | 
| @nvazquez 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. | 
| Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 14138 | 
| @blueorangutan test | 
| @DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests | 
| [SF] Trillian test result (tid-13765) 
 | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code lgtm
| String err = String.format("The selected Kubernetes ISO %s arch (%s) doesn't match the template %s arch (%s) " + | ||
| "to deploy the Kubernetes cluster", | ||
| clusterKubernetesVersion.getName(), cksIso.getArch(), finalTemplate.getName(), finalTemplate.getArch()); | ||
| throw new CloudRuntimeException(err); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we consider the arch of cks iso before preferred arch of zone ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it could also make sense, what do you think @harikrishna-patnala @sureshanaparti @shwstppr ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that makes sense - pl address that @nvazquez - then it's ready for merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM tested in an mixed-arch zone with NUC9 x86 hosts and RPi5/arm64 16GB hosts.
| Thanks for testing @rohityadavcloud - I have pushed a fix to address the comment @blueorangutan package | 
| @nvazquez 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. | 
| Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 14478 | 
| @blueorangutan test | 
| @nvazquez a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clgtm
| [SF] Trillian test result (tid-13980) 
 | 
…preferred architecture (apache#10946) * Find system VM templates for CKS cluster honouring the preferred architecture * Fix unit tests * Fix checkstyle * Sort instead of filtering by preferred arch * Remove unnecesary stubs * Restore java version * Address review comments * Fail and display error message in case the CKS ISO arch doesnt match the selected template arch * Prefer CKS ISO arch instead of the system VM setting
Description
This PR fixes the selection of system VM templates for CKS clusters honouring the setting
system.vm.preferred.architectureFixes: #10944
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?