abh1sar commented on code in PR #12194:
URL: https://github.com/apache/cloudstack/pull/12194#discussion_r2620828028
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/backup/UpdateBackupOfferingCmd.java:
##########
Review Comment:
We should add getDomainIds() to the if condition otherwise update will fail
if only the domainIds parameter is getting changed.
##########
server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java:
##########
@@ -2139,16 +2237,58 @@ public BackupOffering
updateBackupOffering(UpdateBackupOfferingCmd updateBackupO
fields.add("allowUserDrivenBackups: " + allowUserDrivenBackups);
}
- if (!backupOfferingDao.update(id, offering)) {
+ if
(org.apache.commons.collections.CollectionUtils.isNotEmpty(domainIds)) {
+ for (final Long domainId: domainIds) {
+ if (domainDao.findById(domainId) == null) {
+ throw new InvalidParameterValueException("Please specify a
valid domain id");
+ }
+ }
+ }
+ List<Long> filteredDomainIds = filterChildSubDomains(domainIds);
+ Collections.sort(filteredDomainIds);
+
+ boolean success =backupOfferingDao.update(id, offering);
Review Comment:
```suggestion
boolean success = backupOfferingDao.update(id, offering);
```
##########
server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java:
##########
@@ -280,6 +283,20 @@ public BackupOffering importBackupOffering(final
ImportBackupOfferingCmd cmd) {
throw new CloudRuntimeException("A backup offering with the same
name already exists in this zone");
}
+ if
(org.apache.commons.collections.CollectionUtils.isNotEmpty(cmd.getDomainIds()))
{
Review Comment:
Not really related to this change, but can we put
org.apache.commons.collections.CollectionUtils in the imports?
line 1162 is using CollectionUtils from com.amazonaws.util which can be
changed to use org.apache.commons.collections.CollectionUtils
##########
server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java:
##########
@@ -292,15 +309,55 @@ public BackupOffering importBackupOffering(final
ImportBackupOfferingCmd cmd) {
if (savedOffering == null) {
throw new CloudRuntimeException("Unable to create backup offering:
" + cmd.getExternalId() + ", name: " + cmd.getName());
}
+ if
(org.apache.commons.collections.CollectionUtils.isNotEmpty(filteredDomainIds)) {
+ List<BackupOfferingDetailsVO> detailsVOList = new ArrayList<>();
+ for (Long domainId : filteredDomainIds) {
+ detailsVOList.add(new
BackupOfferingDetailsVO(savedOffering.getId(), ApiConstants.DOMAIN_ID,
String.valueOf(domainId), false));
+ }
+ if (!detailsVOList.isEmpty()) {
+ backupOfferingDetailsDao.saveDetails(detailsVOList);
+ }
+ }
logger.debug("Successfully created backup offering " + cmd.getName() +
" mapped to backup provider offering " + cmd.getExternalId());
return savedOffering;
}
+ private List<Long> filterChildSubDomains(final List<Long> domainIds) {
+ if (domainIds == null || domainIds.size() <= 1) {
+ return domainIds == null ? new ArrayList<>() : new
ArrayList<>(domainIds);
+ }
+ final List<Long> result = new ArrayList<>();
+ for (final Long candidate : domainIds) {
+ boolean isDescendant = false;
+ for (final Long other : domainIds) {
+ if (Objects.equals(candidate, other)) continue;
+ if (domainDao.isChildDomain(other, candidate)) {
+ isDescendant = true;
+ break;
+ }
+ }
+ if (!isDescendant) {
+ result.add(candidate);
+ }
+ }
+ return result;
+ }
+
+ @Override
+ public List<Long> getBackupOfferingDomains(Long offeringId) {
+ final BackupOffering backupOffering =
backupOfferingDao.findById(offeringId);
+ if (backupOffering == null) {
+ throw new InvalidParameterValueException("Unable to find backup
offering " + backupOffering);
Review Comment:
error message is accessing backupOffering which is null
##########
server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java:
##########
@@ -2139,16 +2237,58 @@ public BackupOffering
updateBackupOffering(UpdateBackupOfferingCmd updateBackupO
fields.add("allowUserDrivenBackups: " + allowUserDrivenBackups);
}
- if (!backupOfferingDao.update(id, offering)) {
+ if
(org.apache.commons.collections.CollectionUtils.isNotEmpty(domainIds)) {
+ for (final Long domainId: domainIds) {
+ if (domainDao.findById(domainId) == null) {
+ throw new InvalidParameterValueException("Please specify a
valid domain id");
+ }
+ }
+ }
+ List<Long> filteredDomainIds = filterChildSubDomains(domainIds);
+ Collections.sort(filteredDomainIds);
+
+ boolean success =backupOfferingDao.update(id, offering);
+ if (!success) {
logger.warn(String.format("Couldn't update Backup offering (%s)
with [%s].", backupOfferingVO, String.join(", ", fields)));
}
+ if (success) {
+ List<Long> existingDomainIds =
backupOfferingDetailsDao.findDomainIds(id);
+ Collections.sort(existingDomainIds);
+ updateBackupOfferingDomainDetails(id, filteredDomainIds,
existingDomainIds);
+ }
+
BackupOfferingVO response = backupOfferingDao.findById(id);
CallContext.current().setEventDetails(String.format("Backup Offering
updated [%s].",
ReflectionToStringBuilderUtils.reflectOnlySelectedFields(response, "id",
"name", "description", "userDrivenBackupAllowed", "externalId")));
return response;
}
+ private void updateBackupOfferingDomainDetails(Long id, List<Long>
filteredDomainIds, List<Long> existingDomainIds) {
+ if (existingDomainIds == null) {
+ existingDomainIds = new ArrayList<>();
+ }
+ List<BackupOfferingDetailsVO> detailsVO = new ArrayList<>();
+ if(!filteredDomainIds.equals(existingDomainIds)) {
+ SearchBuilder<BackupOfferingDetailsVO> sb =
backupOfferingDetailsDao.createSearchBuilder();
+ sb.and("offeringId", sb.entity().getResourceId(),
SearchCriteria.Op.EQ);
+ sb.and("detailName", sb.entity().getName(), SearchCriteria.Op.EQ);
+ sb.done();
+ SearchCriteria<BackupOfferingDetailsVO> sc = sb.create();
+ sc.setParameters("offeringId", String.valueOf(id));
+ sc.setParameters("detailName", ApiConstants.DOMAIN_ID);
Review Comment:
Probably it would be better to move this to BackupOfferingDetailsDaoImpl?
##########
server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java:
##########
@@ -292,15 +309,55 @@ public BackupOffering importBackupOffering(final
ImportBackupOfferingCmd cmd) {
if (savedOffering == null) {
throw new CloudRuntimeException("Unable to create backup offering:
" + cmd.getExternalId() + ", name: " + cmd.getName());
}
+ if
(org.apache.commons.collections.CollectionUtils.isNotEmpty(filteredDomainIds)) {
+ List<BackupOfferingDetailsVO> detailsVOList = new ArrayList<>();
+ for (Long domainId : filteredDomainIds) {
+ detailsVOList.add(new
BackupOfferingDetailsVO(savedOffering.getId(), ApiConstants.DOMAIN_ID,
String.valueOf(domainId), false));
+ }
+ if (!detailsVOList.isEmpty()) {
+ backupOfferingDetailsDao.saveDetails(detailsVOList);
+ }
+ }
logger.debug("Successfully created backup offering " + cmd.getName() +
" mapped to backup provider offering " + cmd.getExternalId());
return savedOffering;
}
+ private List<Long> filterChildSubDomains(final List<Long> domainIds) {
Review Comment:
There are 3 implementations of filterChildSubDomains. Can we do something
similar to resolveDomainIds for it?
<img width="789" height="170" alt="Image"
src="https://github.com/user-attachments/assets/3944a154-d532-445f-8377-5909fb3d1493";
/>
##########
api/src/main/java/org/apache/cloudstack/api/command/offering/DomainAndZoneIdResolver.java:
##########
@@ -0,0 +1,114 @@
+// 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.command.offering;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.function.LongFunction;
+
+import com.cloud.dc.DataCenter;
+import com.cloud.domain.Domain;
+import com.cloud.exception.InvalidParameterValueException;
+import org.apache.cloudstack.api.BaseCmd;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+
+/**
+ * Helper for commands that accept a domainIds or zoneIds string and need to
+ * resolve them to lists of IDs, falling back to an offering-specific
+ * default provider.
+ */
+public interface DomainAndZoneIdResolver {
+ /**
+ * Parse the provided domainIds string and return a list of domain IDs.
+ * If domainIds is empty, the defaultDomainsProvider will be invoked with
the
+ * provided resource id to obtain the current domains.
+ */
+ default List<Long> resolveDomainIds(final String domainIds, final Long id,
final LongFunction<List<Long>> defaultDomainsProvider, final String
resourceTypeName) {
+ final List<Long> validDomainIds = new ArrayList<>();
+ final BaseCmd base = (BaseCmd) this;
+ final Logger logger = LogManager.getLogger(base.getClass());
+
+ if (StringUtils.isEmpty(domainIds)) {
+ if (defaultDomainsProvider != null) {
+ final List<Long> defaults = defaultDomainsProvider.apply(id);
+ if (defaults != null) {
+ validDomainIds.addAll(defaults);
+ }
+ }
+ return validDomainIds;
+ }
+
+ final String[] domains = domainIds.split(",");
+ final String type = (resourceTypeName == null ||
resourceTypeName.isEmpty()) ? "offering" : resourceTypeName;
+ for (String domain : domains) {
+ final String trimmed = domain == null ? "" : domain.trim();
+ if (trimmed.isEmpty() || "public".equalsIgnoreCase(trimmed)) {
+ continue;
+ }
+
+ final Domain validDomain =
base._entityMgr.findByUuid(Domain.class, trimmed);
+ if (validDomain == null) {
+ logger.warn("Invalid domain specified for {}: {}", type,
trimmed);
+ throw new InvalidParameterValueException("Failed to create " +
type + " because invalid domain has been specified.");
Review Comment:
Does it make sense to add the invalid domainId in the exception?
##########
utils/src/main/java/com/cloud/utils/component/ManagerBase.java:
##########
@@ -25,4 +25,6 @@ public ManagerBase() {
// set default run level for manager components
setRunLevel(ComponentLifecycle.RUN_LEVEL_COMPONENT_BOOTSTRAP);
}
+
+
Review Comment:
extra lines can be removed
##########
ui/src/config/section/offering.js:
##########
Review Comment:
Is there a plan to add domainIds to updateBackupOffering UI?
##########
server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java:
##########
@@ -2139,16 +2237,58 @@ public BackupOffering
updateBackupOffering(UpdateBackupOfferingCmd updateBackupO
fields.add("allowUserDrivenBackups: " + allowUserDrivenBackups);
}
- if (!backupOfferingDao.update(id, offering)) {
+ if
(org.apache.commons.collections.CollectionUtils.isNotEmpty(domainIds)) {
+ for (final Long domainId: domainIds) {
+ if (domainDao.findById(domainId) == null) {
+ throw new InvalidParameterValueException("Please specify a
valid domain id");
+ }
+ }
+ }
+ List<Long> filteredDomainIds = filterChildSubDomains(domainIds);
+ Collections.sort(filteredDomainIds);
+
+ boolean success =backupOfferingDao.update(id, offering);
+ if (!success) {
logger.warn(String.format("Couldn't update Backup offering (%s)
with [%s].", backupOfferingVO, String.join(", ", fields)));
}
+ if (success) {
+ List<Long> existingDomainIds =
backupOfferingDetailsDao.findDomainIds(id);
+ Collections.sort(existingDomainIds);
+ updateBackupOfferingDomainDetails(id, filteredDomainIds,
existingDomainIds);
+ }
+
BackupOfferingVO response = backupOfferingDao.findById(id);
CallContext.current().setEventDetails(String.format("Backup Offering
updated [%s].",
ReflectionToStringBuilderUtils.reflectOnlySelectedFields(response, "id",
"name", "description", "userDrivenBackupAllowed", "externalId")));
return response;
}
+ private void updateBackupOfferingDomainDetails(Long id, List<Long>
filteredDomainIds, List<Long> existingDomainIds) {
+ if (existingDomainIds == null) {
Review Comment:
fix identation
##########
api/src/main/java/org/apache/cloudstack/api/response/BackupOfferingResponse.java:
##########
@@ -61,6 +61,14 @@ public class BackupOfferingResponse extends BaseResponse {
@Param(description = "zone name")
private String zoneName;
+ @SerializedName(ApiConstants.DOMAIN_ID)
+ @Param(description = "the domain ID(s) this disk offering belongs to.
Ignore this information as it is not currently applicable.")
+ private String domainId;
+
+ @SerializedName(ApiConstants.DOMAIN)
+ @Param(description = "the domain name(s) this disk offering belongs to.
Ignore this information as it is not currently applicable.")
Review Comment:
Fix the description. Also since = "4.23.0" can be added
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
