-
Notifications
You must be signed in to change notification settings - Fork 1.3k
edge-zone,kvm,iso,cks: allow k8s deployment with direct-download iso #8142
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
Conversation
Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>
|
@blueorangutan package |
|
@weizhouapache 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 Report
@@ Coverage Diff @@
## 4.18 #8142 +/- ##
=========================================
Coverage 13.07% 13.07%
Complexity 9111 9111
=========================================
Files 2720 2720
Lines 257568 257625 +57
Branches 40154 40167 +13
=========================================
+ Hits 33666 33677 +11
- Misses 219670 219712 +42
- Partials 4232 4236 +4
... and 5 files with indirect coverage changes 📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today! |
|
Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 7505 |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
yadvr
left a comment
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 it, @shwstppr pl also look at any smoketests/unit tests changes
|
[SF] Trillian test result (tid-8094)
|
|
@blueorangutan package |
|
@shwstppr 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]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 7516 |
|
@blueorangutan test |
|
@shwstppr a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
|
[SF] Trillian test result (tid-8100)
|
DaanHoogland
left a comment
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.
looks mostly good. one potential UI issue.
| private Long getOneMatchingPoolIdFromRefs(List<VMTemplateStoragePoolVO> existingRefs, List<StoragePoolVO> pools) { | ||
| if (pools.isEmpty()) { | ||
| throw new CloudRuntimeException("No storage pools found"); | ||
| return null; |
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.
returning null is not a good practice. Can this if (pools.isEmpty()) be handled in the calling methods and an exception be thrown here?
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.
Refactored
| * Retrieve storage pools with scope = cluster or zone or local matching clusterId or dataCenterId or hostId depending on their scope | ||
| */ | ||
| private List<StoragePoolVO> getStoragePoolsFromClusterOrZone(Long clusterId, long dataCenterId, Hypervisor.HypervisorType hypervisorType) { | ||
| private List<StoragePoolVO> getStoragePoolsFromClusterOrZoneOrLocal(Long clusterId, long dataCenterId, Hypervisor.HypervisorType hypervisorType, long hostId) { |
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.
naming suggestion
| private List<StoragePoolVO> getStoragePoolsFromClusterOrZoneOrLocal(Long clusterId, long dataCenterId, Hypervisor.HypervisorType hypervisorType, long hostId) { | |
| private List<StoragePoolVO> getStoragePoolsForScope(Long clusterId, long dataCenterId, Hypervisor.HypervisorType hypervisorType, long hostId) { |
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.
Done
...ns/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStoragePoolManager.java
Show resolved
Hide resolved
...storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateDataFactoryImpl.java
Outdated
Show resolved
Hide resolved
...ernetes-service/src/main/java/com/cloud/kubernetes/version/KubernetesVersionManagerImpl.java
Outdated
Show resolved
Hide resolved
...ernetes-service/src/main/java/com/cloud/kubernetes/version/KubernetesVersionManagerImpl.java
Outdated
Show resolved
Hide resolved
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.
Can we show direct download section in Images> Kubernetes iso details page
The detail is shown only under Images> ISO's
The detail is not shown under Images> Kubernetes ISO's
kiranchavala
left a comment
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.
@shwstppr Cloudstack should not show L2 networks in the, network dropdown create kubernetes cluster form.
Currently its allowing to create k8s cluster in l2 network, its should throw a pop up saying l2 networks are not supported
|
@kiranchavala will try to make changes for your suggestions along with Daan's review comments. If you're done with testing can you please comment on the functionality part? |
|
@shwstppr Thanks functionality wise it's working fine , was able to spin up k8s cluster on edge zones and launch vm's from iso |
Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>
Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>
Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>
Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>
|
@DaanHoogland @kiranchavala I've addressed your comments and feedback @blueorangutan package |
|
@shwstppr 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. |
DaanHoogland
left a comment
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
|
Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 7659 |
|
@blueorangutan test |
|
@shwstppr a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
@shwstppr is this ready for review now? |
|
@DaanHoogland was waiting for the test result but I guess the test result didn't come due to API limit |
|
from the backend: Smoke tests completed. 108 look OK, 1 have errors, 0 did not run
test failure:
|
|
@kiranchavala Is it okay to merge now? I addressed your comments. |
kiranchavala
left a comment
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 manually and it works
…pache#8142) Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>



Description
Fixes #7284
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?