weizhouapache commented on code in PR #7799:
URL: https://github.com/apache/cloudstack/pull/7799#discussion_r1290982022
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/guest/AddGuestOsCmd.java:
##########
@@ -16,6 +16,7 @@
// under the License.
package org.apache.cloudstack.api.command.admin.guest;
+import org.apache.cloudstack.acl.RoleType;
Review Comment:
this can be moved to line 22 for alignment
##########
engine/schema/src/main/java/com/cloud/storage/dao/GuestOSDao.java:
##########
@@ -19,9 +19,16 @@
import com.cloud.storage.GuestOSVO;
import com.cloud.utils.db.GenericDao;
+import java.util.List;
+import java.util.Set;
+
public interface GuestOSDao extends GenericDao<GuestOSVO, Long> {
- GuestOSVO listByDisplayName(String displayName);
+ GuestOSVO listOneByDisplayName(String displayName);
Review Comment:
use `find` instead of `list` in the method name?
##########
server/src/main/java/com/cloud/server/ManagementServerImpl.java:
##########
@@ -2798,16 +2798,24 @@ public GuestOS addGuestOs(final AddGuestOsCmd cmd) {
guestOsVo.setIsUserDefined(true);
final GuestOS guestOsPersisted = _guestOSDao.persist(guestOsVo);
- if (cmd.getDetails() != null && !cmd.getDetails().isEmpty()) {
- Map<String, String> detailsMap = cmd.getDetails();
- for (Object key : detailsMap.keySet()) {
- _guestOsDetailsDao.addDetail(guestOsPersisted.getId(),
(String)key, detailsMap.get(key), false);
- }
- }
+ Map<String, String> details = cmd.getDetails();
+ Boolean forDisplay = cmd.getForDisplay();
Review Comment:
there are following columns in tables
`display_vm` in `vm_instance`
`display_volume` in `volumes`
`display_network` in `networks`
`display_nic` in `nics`
`display` in `vpc`
`display` in `firewall_rules`
can we add a new column `display` to `guest_os` table as well ?
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/guest/UpdateGuestOsCmd.java:
##########
@@ -16,6 +16,7 @@
// under the License.
package org.apache.cloudstack.api.command.admin.guest;
+import org.apache.cloudstack.acl.RoleType;
Review Comment:
same as previous
##########
engine/schema/src/main/java/com/cloud/storage/dao/GuestOSDaoImpl.java:
##########
@@ -62,4 +71,41 @@ public GuestOSVO
findByCategoryIdAndDisplayNameOrderByCreatedDesc(long categoryI
}
return null;
}
+
+ /**
+ "select * from guest_os go2 where display_name in"
+ + "(select display_name from"
+ + "(select display_name, count(1) as count from guest_os
go1 group by display_name having count > 1) tab0)";
+ *
+ * @return
+ */
+ @Override
+ @DB
+ public Set<String> findDoubleNames() {
+ String selectSql = "(select display_name from (select display_name,
count(1) as count from guest_os go1 group by display_name having count > 1)
tab0)";
Review Comment:
```suggestion
String selectSql = "select display_name, count(1) as count from
guest_os where removed IS NULL group by display_name having count > 1";
```
##########
engine/schema/src/main/java/com/cloud/upgrade/GuestOsMapper.java:
##########
@@ -51,6 +53,86 @@ public GuestOsMapper() {
guestOSDao = new GuestOSDaoImpl();
}
+ public void mergeDuplicates() {
+ LOG.info("merging duplicate guest osses");
+ Set<Set<GuestOSVO>> duplicates = findDuplicates();
+ LOG.debug(String.format("merging %d sets of duplicates",
duplicates.size()));
+ for (Set<GuestOSVO> setOfGuestOSes : duplicates) {
+ // decide which to (mark as) remove(d)
+ // # highest/lowest id
+ // # or is user_defined == false
+ GuestOSVO guestOSVO = highestIdFrom(setOfGuestOSes);
+ LOG.info(String.format("merging %d duplicates for %s ",
setOfGuestOSes.size(), guestOSVO.getDisplayName()));
+ makeNormative(guestOSVO, setOfGuestOSes);
+
+ }
+ }
+
+ private void makeNormative(GuestOSVO guestOSVO, Set<GuestOSVO>
setOfGuestOSes) {
+ for (GuestOSVO oldGuestOs : setOfGuestOSes) {
+ List<GuestOSHypervisorVO> mappings =
guestOSHypervisorDao.listByGuestOsId(oldGuestOs.getId());
+ copyMappings(guestOSVO, mappings);
+ // // find VMs
+ // // // for each VM
+ // // // // set the guest_os_id to the one to keep
+ // // find templates
+ // // // for each template
+ // // // // set the guest_os_id to the one to keep
+ // // mark as removed
+
+ }
Review Comment:
after copying, should we keep only the first or last guest os as
is_display=true ?
##########
engine/schema/src/main/java/com/cloud/upgrade/GuestOsMapper.java:
##########
@@ -51,6 +53,86 @@ public GuestOsMapper() {
guestOSDao = new GuestOSDaoImpl();
}
+ public void mergeDuplicates() {
+ LOG.info("merging duplicate guest osses");
+ Set<Set<GuestOSVO>> duplicates = findDuplicates();
+ LOG.debug(String.format("merging %d sets of duplicates",
duplicates.size()));
+ for (Set<GuestOSVO> setOfGuestOSes : duplicates) {
+ // decide which to (mark as) remove(d)
+ // # highest/lowest id
+ // # or is user_defined == false
+ GuestOSVO guestOSVO = highestIdFrom(setOfGuestOSes);
+ LOG.info(String.format("merging %d duplicates for %s ",
setOfGuestOSes.size(), guestOSVO.getDisplayName()));
+ makeNormative(guestOSVO, setOfGuestOSes);
+
+ }
+ }
+
+ private void makeNormative(GuestOSVO guestOSVO, Set<GuestOSVO>
setOfGuestOSes) {
+ for (GuestOSVO oldGuestOs : setOfGuestOSes) {
+ List<GuestOSHypervisorVO> mappings =
guestOSHypervisorDao.listByGuestOsId(oldGuestOs.getId());
+ copyMappings(guestOSVO, mappings);
+ // // find VMs
+ // // // for each VM
+ // // // // set the guest_os_id to the one to keep
+ // // find templates
+ // // // for each template
+ // // // // set the guest_os_id to the one to keep
+ // // mark as removed
+
+ }
+ // set the lower id as not user defined, if that was not the premise
anyway
+
+ }
+
+ private void copyMappings(GuestOSVO guestOSVO, List<GuestOSHypervisorVO>
mappings) {
+ for (GuestOSHypervisorVO mapping : mappings) {
+ if (null ==
guestOSHypervisorDao.findByOsIdAndHypervisor(guestOSVO.getId(),
mapping.getHypervisorType(), mapping.getHypervisorVersion())) {
+ GuestOSHypervisorVO newMap = new GuestOSHypervisorVO();
+ newMap.setGuestOsId(guestOSVO.getId());
+ newMap.setGuestOsName(guestOSVO.getDisplayName());
+ newMap.setHypervisorType(mapping.getHypervisorType());
+ newMap.setHypervisorVersion(mapping.getHypervisorVersion());
+ guestOSHypervisorDao.persist(newMap);
+ }
+ }
+ }
+
+ private GuestOSVO highestIdFrom(Set<GuestOSVO> setOfGuestOSes) {
+ GuestOSVO rc = null;
+ for (GuestOSVO guestOSVO: setOfGuestOSes) {
+ if (rc == null || (guestOSVO.getId() > rc.getId() &&
!guestOSVO.getIsUserDefined())) {
+ rc = guestOSVO;
Review Comment:
just `return guestOSVO` here ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
