engine/schema: fix duplicated guest OSes in 4.18.0.0 #7799
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
weizhouapache
changed the title
|
@blueorangutan package |
|
@weizhouapache a [SF] 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. |
This was
linked to
issues
Aug 2, 2023
|
Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 6622 |
Codecov Report
@@ Coverage Diff @@
## 4.18 #7799 +/- ##
============================================
- Coverage 13.06% 13.05% -0.01%
+ Complexity 9084 9083 -1
============================================
Files 2720 2720
Lines 257289 257370 +81
Branches 40116 40124 +8
============================================
Hits 33612 33612
- Misses 219455 219537 +82
+ Partials 4222 4221 -1
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| } | ||
| guestOsId = getGuestOsId(categoryId, displayName); | ||
| } else { | ||
| // TODO: update is_user_defined to false |
There was a problem hiding this comment.
In my opinion, we better leave the user_defined flag as is to keep track of operator activities.
There was a problem hiding this comment.
we can use the user_defined flag to decide which ones to remove and only keep the system defined one. In this case we, will have to allow for the ectra definition to begin with and deduplicate it right away.
There was a problem hiding this comment.
In my opinion, we better leave the user_defined flag as is to keep track of operator activities.
If so, user will be able to remove the guest os
There was a problem hiding this comment.
How about adding a new guest OS entry if the existing one is user-defined ?
We are anyways deduplicating the list from listGuestOS command starting 4.19 version, we will show only the latest entry if there duplicates. cc @DaanHoogland
There was a problem hiding this comment.
@harikrishna-patnala is there another place the deduplication is already taking place (see my pseudo code)?
I do agree we should look at the user defined flag during deduplication. seems like the cleanest way.
There was a problem hiding this comment.
@DaanHoogland I'm thinking of not doing any merge operation as it may affect the old entries in older versions as well.
I think the problem here is only that we have duplicate guest OS entries, which is not a critical/major issue.
My proposal is to leave them as is and make fix for future entries.
Regarding the "Red Hat Enterprise Linux 9" and other similar entries, instead of doing a merge operation, I would suggest adding new mapping entries for both the names (RHEL and RHEL9*) so that if the operator uses any of the Guest OSs it won't behave differently while deploying VM (otherwise if the mapping is not found, CS may choose "default").
DaanHoogland
force-pushed
the
4.18-fix-duplicated-guest-os
branch
from
d10fa3e to
5f17012
Compare
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
DaanHoogland
force-pushed
the
4.18-fix-duplicated-guest-os
branch
from
2572ce5 to
8462915
Compare
DaanHoogland
force-pushed
the
4.18-fix-duplicated-guest-os
branch
from
c8c82a1 to
7353166
Compare
|
@blueorangutan package |
|
@DaanHoogland a [SF] 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 6739 |
weizhouapache
commented
Aug 11, 2023
api/src/main/java/org/apache/cloudstack/api/command/admin/guest/AddGuestOsCmd.java
Outdated
Show resolved
Hide resolved
engine/schema/src/main/java/com/cloud/storage/dao/GuestOSDao.java
Outdated
Show resolved
Hide resolved
engine/schema/src/main/java/com/cloud/storage/dao/GuestOSDaoImpl.java
Outdated
Show resolved
Hide resolved
api/src/main/java/org/apache/cloudstack/api/command/admin/guest/UpdateGuestOsCmd.java
Outdated
Show resolved
Hide resolved
| GuestOSVO rc = null; | ||
| for (GuestOSVO guestOSVO: setOfGuestOSes) { | ||
| if (rc == null || (guestOSVO.getId() > rc.getId() && !guestOSVO.getIsUserDefined())) { | ||
| rc = guestOSVO; |
There was a problem hiding this comment.
just return guestOSVO here ?
There was a problem hiding this comment.
no, bad pattern to have multiple exit points from a method
There was a problem hiding this comment.
ok. you can add a break; then
server/src/main/java/com/cloud/server/ManagementServerImpl.java
Outdated
Show resolved
Hide resolved
DaanHoogland
force-pushed
the
4.18-fix-duplicated-guest-os
branch
from
24f5f24 to
01ab9f3
Compare
weizhouapache
commented
Aug 11, 2023
| usage_type = 24 AND usage_display like '% io write'; | ||
|
|
||
| CALL `cloud`.`IDEMPOTENT_ADD_COLUMN`('cloud.guest_os', 'display', 'tinyint(1) DEFAULT ''1'' COMMENT ''should this guest_os be shown to the end user'' '); |
There was a problem hiding this comment.
@DaanHoogland
as we discussed, add this to 4.16.0->4.16.1->4.17.0->4.17.1->4.17.2 as well ?
There was a problem hiding this comment.
no, I checked (and will test) the guestosmapper is not yet used in those.
|
@blueorangutan package |
|
@DaanHoogland a [SF] 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 6769 |
|
@weizhouapache a [SF] 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 6827 |
|
@blueorangutan test keepEnv |
|
@weizhouapache a [SF] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-7471)
|
|
[SF] Trillian test result (tid-7479)
|
|
test1: fresh installation with this PR, all looks good |
|
test2: upgrade from 4.16.1.1 to 4.18, then to this PR
the RHEL 9 issue also exist
|
|
test3:
|
DaanHoogland
approved these changes
Aug 21, 2023
There was a problem hiding this comment.
tested and reviewed, @BryanMLima can you approve it this way?
|
@rohityadavcloud @harikrishna-patnala |
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
|
@blueorangutan package |
|
@weizhouapache a [SF] 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 6854 |
|
@blueorangutan test matrix |
|
@weizhouapache a [SF] Trillian-Jenkins matrix job (centos7 mgmt + xenserver71, rocky8 mgmt + vmware67u3, centos7 mgmt + kvmcentos7) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-7506)
|
|
[SF] Trillian test result (tid-7508)
|
|
[SF] Trillian test result (tid-7507)
|
|
@BryanMLima @weizhouapache are we ready with this? |
|
retested all steps above, got same results thanks @DaanHoogland for the fix, merging |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR fixes #7783 and #7781
(Need to merge duplicated guest OSes, and keep only one of them)
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?