-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add support for disabling serivce / system and disk offerings #9546
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
base: main
Are you sure you want to change the base?
Changes from all commits
6932fb0
cfe4c6a
ea1c41d
6601b38
6ed1b60
fdb1f45
3785bab
bd7ff55
9040055
d34a916
1f3d2c9
29eea23
bb8a660
31f5c9b
a9f1b0a
590dc48
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -45,6 +45,17 @@ | |
import javax.inject.Inject; | ||
import javax.naming.ConfigurationException; | ||
|
||
import com.cloud.dc.VlanDetailsVO; | ||
import com.cloud.dc.dao.VlanDetailsDao; | ||
import com.cloud.hypervisor.HypervisorGuru; | ||
import com.cloud.network.dao.NsxProviderDao; | ||
import com.cloud.network.element.NsxProviderVO; | ||
import com.cloud.utils.crypt.DBEncryptionUtil; | ||
import com.cloud.host.HostTagVO; | ||
import com.cloud.storage.StoragePoolTagVO; | ||
import com.cloud.storage.VolumeApiServiceImpl; | ||
import com.cloud.vm.VMInstanceVO; | ||
import com.googlecode.ipv6.IPv6Address; | ||
import org.apache.cloudstack.acl.SecurityChecker; | ||
import org.apache.cloudstack.affinity.AffinityGroup; | ||
import org.apache.cloudstack.affinity.AffinityGroupService; | ||
|
@@ -161,7 +172,6 @@ | |
import com.cloud.dc.PodVlanMapVO; | ||
import com.cloud.dc.Vlan; | ||
import com.cloud.dc.Vlan.VlanType; | ||
import com.cloud.dc.VlanDetailsVO; | ||
import com.cloud.dc.VlanVO; | ||
import com.cloud.dc.dao.AccountVlanMapDao; | ||
import com.cloud.dc.dao.ClusterDao; | ||
|
@@ -175,7 +185,6 @@ | |
import com.cloud.dc.dao.HostPodDao; | ||
import com.cloud.dc.dao.PodVlanMapDao; | ||
import com.cloud.dc.dao.VlanDao; | ||
import com.cloud.dc.dao.VlanDetailsDao; | ||
import com.cloud.dc.dao.VsphereStoragePolicyDao; | ||
import com.cloud.deploy.DataCenterDeployment; | ||
import com.cloud.deploy.DeploymentClusterPlanner; | ||
|
@@ -194,12 +203,10 @@ | |
import com.cloud.exception.ResourceAllocationException; | ||
import com.cloud.exception.ResourceUnavailableException; | ||
import com.cloud.gpu.GPU; | ||
import com.cloud.host.HostTagVO; | ||
import com.cloud.host.HostVO; | ||
import com.cloud.host.dao.HostDao; | ||
import com.cloud.host.dao.HostTagsDao; | ||
import com.cloud.hypervisor.Hypervisor.HypervisorType; | ||
import com.cloud.hypervisor.HypervisorGuru; | ||
import com.cloud.hypervisor.kvm.dpdk.DpdkHelper; | ||
import com.cloud.network.IpAddress; | ||
import com.cloud.network.IpAddressManager; | ||
|
@@ -222,13 +229,11 @@ | |
import com.cloud.network.dao.Ipv6GuestPrefixSubnetNetworkMapDao; | ||
import com.cloud.network.dao.NetworkDao; | ||
import com.cloud.network.dao.NetworkVO; | ||
import com.cloud.network.dao.NsxProviderDao; | ||
import com.cloud.network.dao.PhysicalNetworkDao; | ||
import com.cloud.network.dao.PhysicalNetworkTrafficTypeDao; | ||
import com.cloud.network.dao.PhysicalNetworkTrafficTypeVO; | ||
import com.cloud.network.dao.PhysicalNetworkVO; | ||
import com.cloud.network.dao.UserIpv6AddressDao; | ||
import com.cloud.network.element.NsxProviderVO; | ||
import com.cloud.network.rules.LoadBalancerContainer.Scheme; | ||
import com.cloud.network.vpc.VpcManager; | ||
import com.cloud.offering.DiskOffering; | ||
|
@@ -256,9 +261,7 @@ | |
import com.cloud.storage.Storage; | ||
import com.cloud.storage.Storage.ProvisioningType; | ||
import com.cloud.storage.StorageManager; | ||
import com.cloud.storage.StoragePoolTagVO; | ||
import com.cloud.storage.Volume; | ||
import com.cloud.storage.VolumeApiServiceImpl; | ||
import com.cloud.storage.VolumeVO; | ||
import com.cloud.storage.dao.DiskOfferingDao; | ||
import com.cloud.storage.dao.StoragePoolTagsDao; | ||
|
@@ -278,7 +281,6 @@ | |
import com.cloud.utils.Pair; | ||
import com.cloud.utils.UriUtils; | ||
import com.cloud.utils.component.ManagerBase; | ||
import com.cloud.utils.crypt.DBEncryptionUtil; | ||
import com.cloud.utils.db.DB; | ||
import com.cloud.utils.db.EntityManager; | ||
import com.cloud.utils.db.Filter; | ||
|
@@ -303,7 +305,6 @@ | |
import com.google.common.base.MoreObjects; | ||
import com.google.common.base.Preconditions; | ||
import com.google.common.collect.Sets; | ||
import com.googlecode.ipv6.IPv6Address; | ||
import com.googlecode.ipv6.IPv6Network; | ||
|
||
import static com.cloud.configuration.Config.SecStorageAllowedInternalDownloadSites; | ||
|
@@ -4367,8 +4368,11 @@ | |
} | ||
|
||
annotationDao.removeByEntityType(AnnotationService.EntityType.DISK_OFFERING.name(), offering.getUuid()); | ||
offering.setState(DiskOffering.State.Inactive); | ||
if (_diskOfferingDao.update(offering.getId(), offering)) { | ||
List<VolumeVO> volumesUsingOffering = _volumeDao.findByDiskOfferingId(diskOfferingId); | ||
if (!volumesUsingOffering.isEmpty()) { | ||
throw new InvalidParameterValueException(String.format("Unable to delete disk offering: %s [%s] because there are volumes using it", offering.getUuid(), offering.getName())); | ||
} | ||
Comment on lines
+4371
to
+4374
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How is this related to the main change? Why should we start blocking this now? |
||
if (_diskOfferingDao.remove(offering.getId())) { | ||
CallContext.current().setEventDetails("Disk offering id=" + diskOfferingId); | ||
return true; | ||
} else { | ||
|
@@ -4440,15 +4444,17 @@ | |
throw new InvalidParameterValueException(String.format("Unable to delete service offering: %s by user: %s because it is not root-admin or domain-admin", offering.getUuid(), user.getUuid())); | ||
} | ||
|
||
List<VMInstanceVO> vmsUsingOffering = _vmInstanceDao.listByOfferingId(offeringId); | ||
if (!vmsUsingOffering.isEmpty()) { | ||
throw new CloudRuntimeException(String.format("Unable to delete service offering %s as it is in use", offering.getUuid())); | ||
} | ||
Comment on lines
+4447
to
+4450
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How is this related to the main change? Why should we start blocking this now? |
||
annotationDao.removeByEntityType(AnnotationService.EntityType.SERVICE_OFFERING.name(), offering.getUuid()); | ||
if (diskOffering.isComputeOnly()) { | ||
diskOffering.setState(DiskOffering.State.Inactive); | ||
if (!_diskOfferingDao.update(diskOffering.getId(), diskOffering)) { | ||
if (!_diskOfferingDao.remove(diskOffering.getId())) { | ||
throw new CloudRuntimeException(String.format("Unable to delete disk offering %s mapped to the service offering %s", diskOffering.getUuid(), offering.getUuid())); | ||
} | ||
} | ||
offering.setState(ServiceOffering.State.Inactive); | ||
if (_serviceOfferingDao.update(offeringId, offering)) { | ||
if (_serviceOfferingDao.remove(offeringId)) { | ||
CallContext.current().setEventDetails("Service offering id=" + offeringId); | ||
return true; | ||
} else { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1388,19 +1388,17 @@ def setUpClass(cls): | |
cls.services["service_offering"] | ||
) | ||
|
||
cls.cleanup = [ | ||
cls._cleanup = [ | ||
cls.service_offering, | ||
] | ||
return | ||
|
||
@classmethod | ||
def tearDownClass(cls): | ||
try: | ||
#Cleanup resources used | ||
cleanup_resources(cls.apiclient, cls.cleanup) | ||
except Exception as e: | ||
raise Exception("Warning: Exception during cleanup : %s" % e) | ||
return | ||
super(TestNOWithOnlySourceNAT, cls).tearDownClass() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Pearl1594 , the class level cleanup array should be called class scope methods should use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry @DaanHoogland , not sure I understand what's the problem here? As suggested, I've updated the tearDownClass methods to invoke super() There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it is in the names of the variables |
||
|
||
def tearDown(self): | ||
super(TestNOWithOnlySourceNAT, self).tearDown() | ||
|
||
@attr(tags=["advanced", "advancedns"], required_hardware="false") | ||
def test_create_network_with_snat(self): | ||
|
@@ -1446,17 +1444,20 @@ def test_create_network_with_snat(self): | |
zoneid=self.zone.id | ||
) | ||
self.debug("Created guest network with ID: %s within account %s" % (self.network.id, self.account.name)) | ||
self.cleanup.append(self.network) | ||
|
||
self.debug("Deploying VM in account: %s on the network %s" % (self.account.name, self.network.id)) | ||
# Spawn an instance in that network | ||
VirtualMachine.create( | ||
self.vm1 = VirtualMachine.create( | ||
self.apiclient, | ||
self.services["virtual_machine"], | ||
accountid=self.account.name, | ||
domainid=self.account.domainid, | ||
serviceofferingid=self.service_offering.id, | ||
networkids=[str(self.network.id)] | ||
) | ||
self.cleanup.append(self.vm1) | ||
|
||
self.debug("Deployed VM in network: %s" % self.network.id) | ||
|
||
src_nat_list = PublicIPAddress.list( | ||
|
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.
Isn't this a query that is executed if vmid != null? if so, should we list inactive offerings?
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.
Since this PR aims to support disabling service offerings - to make it similar to the way network offerings work, it would make sense to list active and inactive offerings
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.
This method is too long (500+ lines...) for me to judge what it is exactly doing and where it is being done.
I just want to make sure that when creating VMs we don't list inactive offerings. If this change does not cause that, then I'm ok.
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.
Al, that's a fair point.. Probably reverting to it listing Active by default would be a better option. Thanks.