You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2023/01/05 19:44:28 UTC
[GitHub] [cloudstack] weizhouapache commented on a diff in pull request #6979: Guest os mappings improvements
weizhouapache commented on code in PR #6979:
URL: https://github.com/apache/cloudstack/pull/6979#discussion_r1062822604
##########
vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/ClusterMO.java:
##########
@@ -69,7 +69,7 @@
//
public class ClusterMO extends BaseMO implements VmwareHypervisorHost {
private static final Logger s_logger = Logger.getLogger(ClusterMO.class);
- private ManagedObjectReference _environmentBrowser = null;
+ public ManagedObjectReference _environmentBrowser = null;
Review Comment:
maybe protected is enough.
##########
plugins/hypervisors/xenserver/src/main/java/com/cloud/hypervisor/xenserver/resource/wrapper/xenbase/CitrixCheckGuestOsMappingCommandWrapper.java:
##########
@@ -0,0 +1,67 @@
+//
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+//
+
+package com.cloud.hypervisor.xenserver.resource.wrapper.xenbase;
+
+import java.util.Set;
+
+import org.apache.log4j.Logger;
+
+import com.cloud.agent.api.Answer;
+import com.cloud.agent.api.CheckGuestOsMappingAnswer;
+import com.cloud.agent.api.CheckGuestOsMappingCommand;
+import com.cloud.hypervisor.xenserver.resource.CitrixResourceBase;
+import com.cloud.resource.CommandWrapper;
+import com.cloud.resource.ResourceWrapper;
+import com.xensource.xenapi.Connection;
+import com.xensource.xenapi.VM;
+
+@ResourceWrapper(handles = CheckGuestOsMappingCommand.class)
+public final class CitrixCheckGuestOsMappingCommandWrapper extends CommandWrapper<CheckGuestOsMappingCommand, Answer, CitrixResourceBase> {
+
+ private static final Logger s_logger = Logger.getLogger(CitrixCheckGuestOsMappingCommandWrapper.class);
+
+ @Override
+ public Answer execute(final CheckGuestOsMappingCommand command, final CitrixResourceBase citrixResourceBase) {
+ final Connection conn = citrixResourceBase.getConnection();
+ String guestOsName = command.getGuestOsName();
+ String guestOsMappingName = command.getGuestOsHypervisorMappingName();
+ try {
+ s_logger.info("Checking guest os mapping name: " + guestOsMappingName + " for the guest os: " + guestOsName + " in the hypervisor");
+ final Set<VM> vms = VM.getAll(conn);
+ if (vms == null || vms.isEmpty()) {
+ return new CheckGuestOsMappingAnswer(command, "Unable to match guest os mapping name: " + guestOsMappingName + " in the hypervisor");
+ }
+ for (VM vm : vms) {
+ if (vm != null && vm.getIsATemplate(conn) && guestOsMappingName.equalsIgnoreCase(vm.getNameLabel(conn))) {
+ if (guestOsName.equalsIgnoreCase(vm.getNameLabel(conn))) {
+ // Exact matching may fail, try with regex?
Review Comment:
is this a TODO ?
##########
api/src/main/java/org/apache/cloudstack/api/response/HypervisorGuestOsNamesResponse.java:
##########
@@ -0,0 +1,76 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.cloudstack.api.response;
+
+import java.util.List;
+
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.BaseResponse;
+
+import com.cloud.serializer.Param;
+import com.google.gson.annotations.SerializedName;
+
+public class HypervisorGuestOsNamesResponse extends BaseResponse {
+ @SerializedName(ApiConstants.HYPERVISOR)
+ @Param(description = "the hypervisor")
+ private String hypervisor;
+
+ @SerializedName(ApiConstants.HYPERVISOR_VERSION)
+ @Param(description = "version of the hypervisor for guest os names")
+ private String hypervisorVersion;
+
+ @SerializedName(ApiConstants.GUEST_OSES)
+ @Param(description = "the guest OSes of the hypervisor", responseObject = HypervisorGuestOsResponse.class)
+ private List<HypervisorGuestOsResponse> guestOSes;
+
+ @SerializedName(ApiConstants.GUEST_OSES_COUNT)
+ @Param(description = "the count of guest OSes of the hypervisor")
+ private Integer guestOSesCount;
Review Comment:
is this being used somewhere ?
##########
server/src/main/java/com/cloud/api/ApiResponseHelper.java:
##########
@@ -3611,6 +3614,28 @@ public GuestOsMappingResponse createGuestOSMappingResponse(GuestOSHypervisor gue
return response;
}
+ @Override
+ public HypervisorGuestOsNamesResponse createHypervisorGuestOSNamesResponse(List<Pair<String, String>> hypervisorGuestOsNames) {
+ HypervisorGuestOsNamesResponse response = new HypervisorGuestOsNamesResponse();
+ List<HypervisorGuestOsResponse> hypervisorGuestOsResponses = new ArrayList<>();
+ for (Pair<String, String> hypervisorGuestOsName : hypervisorGuestOsNames) {
+ HypervisorGuestOsResponse hypervisorGuestOsResponse = createHypervisorGuestOsResponse(hypervisorGuestOsName);
+ hypervisorGuestOsResponses.add(hypervisorGuestOsResponse);
+ }
+ response.setGuestOSes(hypervisorGuestOsResponses);
+ response.setGuestOSesCount(hypervisorGuestOsResponses.size());
+ response.setObjectName("hypervisorguestosnames");
+ return response;
+ }
+
+ private HypervisorGuestOsResponse createHypervisorGuestOsResponse(Pair<String, String> hypervisorGuestOsName) {
+ HypervisorGuestOsResponse hypervisorGuestOsResponse = new HypervisorGuestOsResponse();
+ hypervisorGuestOsResponse.setOsStdName(hypervisorGuestOsName.first());
+ hypervisorGuestOsResponse.setOsNameForHypervisor(hypervisorGuestOsName.second());
+ hypervisorGuestOsResponse.setObjectName("guestoses");
Review Comment:
@harikrishna-patnala
normally we do not add "s" or "es' in the object name.
##########
server/src/main/java/com/cloud/server/ManagementServerImpl.java:
##########
@@ -2627,6 +2641,12 @@ public Pair<List<? extends GuestOSHypervisor>, Integer> listGuestOSMappingByCrit
sc.addAnd("hypervisorVersion", SearchCriteria.Op.EQ, hypervisorVersion);
}
+ if (osDisplayName != null) {
+ List<GuestOSVO> guestOSVOS = _guestOSDao.listLikeDisplayName(osDisplayName);
+ List<Long> guestOSids = guestOSVOS.stream().map(mo -> mo.getId()).collect(Collectors.toList());
+ sc.addAnd("guestOsId", SearchCriteria.Op.IN, guestOSids.toArray());
Review Comment:
@harikrishna-patnala
you may need to check if guestOSids is empty here.
I have seen some mysql errors like "name in ( )" in the past.
##########
ui/public/locales/en.json:
##########
@@ -1905,6 +1920,7 @@
"label.vms": "VMs",
"label.vmstate": "VM state",
"label.vmtotal": "Total of VMs",
+"label.vmware": "VMWare",
Review Comment:
VMware (w in lower case)
##########
plugins/hypervisors/xenserver/src/main/java/com/cloud/hypervisor/xenserver/resource/wrapper/xenbase/CitrixCheckGuestOsMappingCommandWrapper.java:
##########
@@ -0,0 +1,67 @@
+//
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+//
+
+package com.cloud.hypervisor.xenserver.resource.wrapper.xenbase;
+
+import java.util.Set;
+
+import org.apache.log4j.Logger;
+
+import com.cloud.agent.api.Answer;
+import com.cloud.agent.api.CheckGuestOsMappingAnswer;
+import com.cloud.agent.api.CheckGuestOsMappingCommand;
+import com.cloud.hypervisor.xenserver.resource.CitrixResourceBase;
+import com.cloud.resource.CommandWrapper;
+import com.cloud.resource.ResourceWrapper;
+import com.xensource.xenapi.Connection;
+import com.xensource.xenapi.VM;
+
+@ResourceWrapper(handles = CheckGuestOsMappingCommand.class)
+public final class CitrixCheckGuestOsMappingCommandWrapper extends CommandWrapper<CheckGuestOsMappingCommand, Answer, CitrixResourceBase> {
+
+ private static final Logger s_logger = Logger.getLogger(CitrixCheckGuestOsMappingCommandWrapper.class);
+
+ @Override
+ public Answer execute(final CheckGuestOsMappingCommand command, final CitrixResourceBase citrixResourceBase) {
+ final Connection conn = citrixResourceBase.getConnection();
+ String guestOsName = command.getGuestOsName();
+ String guestOsMappingName = command.getGuestOsHypervisorMappingName();
+ try {
+ s_logger.info("Checking guest os mapping name: " + guestOsMappingName + " for the guest os: " + guestOsName + " in the hypervisor");
+ final Set<VM> vms = VM.getAll(conn);
+ if (vms == null || vms.isEmpty()) {
Review Comment:
@harikrishna-patnala
this is marked as resolved, but I do not see any change.
have you pushed the latest commit ?
##########
plugins/hypervisors/xenserver/src/main/java/com/cloud/hypervisor/xenserver/resource/wrapper/xenbase/CitrixGetHypervisorGuestOsNamesCommandWrapper.java:
##########
@@ -0,0 +1,75 @@
+//
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+//
+
+package com.cloud.hypervisor.xenserver.resource.wrapper.xenbase;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Set;
+
+import org.apache.commons.lang3.StringUtils;
+import org.apache.log4j.Logger;
+
+import com.cloud.agent.api.Answer;
+import com.cloud.agent.api.GetHypervisorGuestOsNamesAnswer;
+import com.cloud.agent.api.GetHypervisorGuestOsNamesCommand;
+import com.cloud.hypervisor.xenserver.resource.CitrixResourceBase;
+import com.cloud.resource.CommandWrapper;
+import com.cloud.resource.ResourceWrapper;
+import com.cloud.utils.Pair;
+import com.xensource.xenapi.Connection;
+import com.xensource.xenapi.VM;
+
+@ResourceWrapper(handles = GetHypervisorGuestOsNamesCommand.class)
+public final class CitrixGetHypervisorGuestOsNamesCommandWrapper extends CommandWrapper<GetHypervisorGuestOsNamesCommand, Answer, CitrixResourceBase> {
+
+ private static final Logger s_logger = Logger.getLogger(CitrixGetHypervisorGuestOsNamesCommandWrapper.class);
+
+ @Override
+ public Answer execute(final GetHypervisorGuestOsNamesCommand command, final CitrixResourceBase citrixResourceBase) {
+ final Connection conn = citrixResourceBase.getConnection();
+ String keyword = command.getKeyword();
+ try {
+ s_logger.info("Getting guest os names in the hypervisor");
+ final Set<VM> vms = VM.getAll(conn);
Review Comment:
@harikrishna-patnala
what's the return of VM.getAll ? can you give an example ?
##########
api/src/main/java/org/apache/cloudstack/api/response/GuestOSResponse.java:
##########
@@ -35,13 +35,17 @@ public class GuestOSResponse extends BaseResponse {
@Param(description = "the ID of the OS category")
private String osCategoryId;
- @SerializedName(ApiConstants.DESCRIPTION)
- @Param(description = "the name/description of the OS type")
- private String description;
Review Comment:
for backwards compatibility, would it be better to keep "description" ?
it can be marked as deprecated.
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/guest/AddGuestOsMappingCmd.java:
##########
@@ -82,6 +88,17 @@ public String getOsNameForHypervisor() {
return osNameForHypervisor;
}
+ public Boolean getOsMappingCheckEnabled() {
+ if (osMappingCheckEnabled == null) {
+ return false;
+ }
+ return osMappingCheckEnabled;
Review Comment:
@harikrishna-patnala
Can you please consider @GutoVeronezi 's suggestion ?
--
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-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org