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 2020/08/24 14:58:24 UTC
[GitHub] [cloudstack] RodrigoDLopez opened a new pull request #4283: Removes unnecessary validations
RodrigoDLopez opened a new pull request #4283:
URL: https://github.com/apache/cloudstack/pull/4283
## Description
<!--- Describe your changes in detail -->
On [PR#2848](https://github.com/apache/cloudstack/pull/2848) the author added the method `preStorageMigrationStateCheck` that will check tags when the operator runs `migrateVirtualMachine` command. We do not need this kind of check, because when running migrations, the operator must have the ability to migrate volumes to the destination pool, and assuming the risk/responsibilities of migrations to different QoS primary storage systems as proposed and implemented in [Fix limitation on tag matching in 'migrateVolume' with disk offering replacement](https://github.com/apache/cloudstack/pull/2636)
Therefore, this PR removes the unnecessary method and permits the operator to run migrate commands without any kind of tags check, as it should be.
<!-- For new features, provide link to FS, dev ML discussion etc. -->
<!-- In case of bug fix, the expected and actual behaviours, steps to reproduce. -->
<!-- When "Fixes: #<id>" is specified, the issue/PR will automatically be closed when this PR gets merged -->
<!-- For addressing multiple issues/PRs, use multiple "Fixes: #<id>" -->
<!-- Fixes: # -->
## Types of changes
<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] Enhancement (improves an existing feature and functionality)
- [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
## Screenshots (if appropriate):
## How Has This Been Tested?
To test this, I used the `cloudmonkey` to run `migrateVirtualMachine` command
<!-- Please describe in detail how you tested your changes. -->
<!-- Include details of your testing environment, and the tests you ran to -->
<!-- see how your change affects other areas of the code, etc. -->
<!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/master/CONTRIBUTING.md) document -->
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4283: Removes unnecessary validations
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4283:
URL: https://github.com/apache/cloudstack/pull/4283#issuecomment-679264278
@RodrigoDLopez a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rafaelweingartner commented on pull request #4283: Removes unnecessary validations
Posted by GitBox <gi...@apache.org>.
rafaelweingartner commented on pull request #4283:
URL: https://github.com/apache/cloudstack/pull/4283#issuecomment-696455059
> @RodrigoDLopez this removes tags checking altogether? This will allow migrating volumes to storages which are not even suitable wrt tags?
Exactly that. This was discussed and developed within a series of PRs I created some time ago, and it was broken while merging forward. Therefore, @RodrigoDLopez is fixing part of the broken code with this PR. These are the following PRs that were used to deliver this feature:
* https://github.com/apache/cloudstack/pull/2425
* https://github.com/apache/cloudstack/pull/2486
* https://github.com/apache/cloudstack/pull/2607
* https://github.com/apache/cloudstack/pull/2612
* https://github.com/apache/cloudstack/pull/2761
* https://github.com/apache/cloudstack/pull/2636
The implementation was broken down into several different PRs to easy development and testing. Moreover, the feature was already discussed, approved, and merged a long time ago and there are already people using it. What @RodrigoDLopez is doing is just fixing this feature that was broken during a merge forward from 4.11 that was executed on 4.12.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] RodrigoDLopez closed pull request #4283: Removes unnecessary validations
Posted by GitBox <gi...@apache.org>.
RodrigoDLopez closed pull request #4283:
URL: https://github.com/apache/cloudstack/pull/4283
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] RodrigoDLopez commented on pull request #4283: Removes unnecessary validations
Posted by GitBox <gi...@apache.org>.
RodrigoDLopez commented on pull request #4283:
URL: https://github.com/apache/cloudstack/pull/4283#issuecomment-679263677
@blueorangutan package
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rafaelweingartner commented on pull request #4283: Removes unnecessary validations
Posted by GitBox <gi...@apache.org>.
rafaelweingartner commented on pull request #4283:
URL: https://github.com/apache/cloudstack/pull/4283#issuecomment-754136739
just one thing needs to be clear, it does not corrupt the DB. The volume table shows the correct storage pool. The only thing is that this storage pool might not have have tags of the volume's disk offering (in case of an override by the operator). However, this can be fixed if the operator uses the parameter `newDiskOffering`.
**No DB will ever be corrupted by this feature**. There is only the possibility to apply some override in case of an emergency or some necessity by the operator. **Nothing will break**! Tags on pools and volume are merely logical constraints applied by ACS, and nothing more.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] RodrigoDLopez commented on pull request #4283: Removes unnecessary validations
Posted by GitBox <gi...@apache.org>.
RodrigoDLopez commented on pull request #4283:
URL: https://github.com/apache/cloudstack/pull/4283#issuecomment-859588534
@PaulAngus @shwstppr
This one is not needed anymore.
Those that made trouble to merge this, has merged another branch that do exactly the same result.
Many thanks to you all, prevents mine ideas to go through. but implements those ideas in another way and merge it without testes. you rock
@weizhouapache @rafaelweingartner @GabrielBrascher
thanks for the review
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rafaelweingartner commented on pull request #4283: Removes unnecessary validations
Posted by GitBox <gi...@apache.org>.
rafaelweingartner commented on pull request #4283:
URL: https://github.com/apache/cloudstack/pull/4283#issuecomment-754130004
One alternative would then be to forbid the override placement. And then, we allow changing the target storage pool, as long as the new disk offering matches it (or the current disk offering, if there is no new disk offering).
What do you think? Then, the DB would always be consistent with the storage where the volume resides.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] RodrigoDLopez edited a comment on pull request #4283: Removes unnecessary validations
Posted by GitBox <gi...@apache.org>.
RodrigoDLopez edited a comment on pull request #4283:
URL: https://github.com/apache/cloudstack/pull/4283#issuecomment-859588534
@PaulAngus @shwstppr
This one is not needed anymore.
Those that made the trouble to merge this, has merged another branch that does exactly the same result.
Many thanks to you all, prevents my ideas to go through, but still implement those ideas in another way and merge it without tests. You rock!!!!
@weizhouapache @rafaelweingartner @GabrielBrascher
thanks for the review
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher commented on pull request #4283: Removes unnecessary validations
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on pull request #4283:
URL: https://github.com/apache/cloudstack/pull/4283#issuecomment-791675371
The way I see there is no risk. The migration command is clear about overriding offerings.
1. if the ADMIN does not want to override the tags validation will happen as expected.
2. otherwise, if the ADMIN wants to do such migration, the offerings are updated accordingly to one of the offerings that comply with the target storage pool. Thus, allowing the volume migration to the target pool with no harm.
**NOTE:** I would like to stress how important it is to allow migrating volumes between storage pools as well as overriding disk offerings. We have some annoying issues in production due to such restrictions. For now, we have to manually run MySQL queries any time that such a migration operation is necessary; these, betting that there will be no human mistaken and compromise the DB consistency.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] RodrigoDLopez commented on pull request #4283: Removes unnecessary validations
Posted by GitBox <gi...@apache.org>.
RodrigoDLopez commented on pull request #4283:
URL: https://github.com/apache/cloudstack/pull/4283#issuecomment-792884105
> @RodrigoDLopez can you please resolve the conflicts?
Thanks for the review.
I carried out the correction of conflicts as requested.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] RodrigoDLopez commented on pull request #4283: Removes unnecessary validations
Posted by GitBox <gi...@apache.org>.
RodrigoDLopez commented on pull request #4283:
URL: https://github.com/apache/cloudstack/pull/4283#issuecomment-762960273
> for vms, we use "cmk find hostsformigration" to list all hosts, it also mentions if the hosts are suitable or not for migration.
> if users try to migrate vm to a unsuitable host, cloudstack still allows it.
>
> for volumes, it looks like "find storagepoolsformigration" also mark storage pools with unmatched tag as "unsuitable".
> https://github.com/apache/cloudstack/blob/master/engine/storage/src/main/java/org/apache/cloudstack/storage/allocator/ZoneWideStoragePoolAllocator.java#L72
> https://github.com/apache/cloudstack/blob/master/engine/storage/src/main/java/org/apache/cloudstack/storage/allocator/ClusterScopeStoragePoolAllocator.java#L81
> https://github.com/apache/cloudstack/blob/master/engine/storage/src/main/java/org/apache/cloudstack/storage/allocator/LocalStoragePoolAllocator.java#L105
>
> @rafaelweingartner @RodrigoDLopez could you please confirm it ?
> if so, I think we could remove the check.
@weizhouapache Exactly! Moreover, even though the override is a possibility, if the operator defines the "newDiskOffering" option, the tags are checked (tags of the new target storage pool with the new disk offering being set). The only situation that the tags are not checked is during a disk placement override that operators might execute in case of emergency or maybe for testing purposes.
The goal here is to enable/empower/provide flexibility for root Admins. They are the root admins of the cloud infrastructure, and they probably know best (better than me at least) how to use/manage their cloud infrastructure.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rafaelweingartner commented on pull request #4283: Removes unnecessary validations
Posted by GitBox <gi...@apache.org>.
rafaelweingartner commented on pull request #4283:
URL: https://github.com/apache/cloudstack/pull/4283#issuecomment-696455059
> @RodrigoDLopez this removes tags checking altogether? This will allow migrating volumes to storages which are not even suitable wrt tags?
Exactly that. This was discussed and developed within a series of PRs I created some time ago, and it was broken while merging forward. Therefore, @RodrigoDLopez is fixing part of the broken code with this PR. These are the following PRs that were used to deliver this feature:
* https://github.com/apache/cloudstack/pull/2425
* https://github.com/apache/cloudstack/pull/2486
* https://github.com/apache/cloudstack/pull/2607
* https://github.com/apache/cloudstack/pull/2612
* https://github.com/apache/cloudstack/pull/2761
* https://github.com/apache/cloudstack/pull/2636
The implementation was broken down into several different PRs to easy development and testing. Moreover, the feature was already discussed, approved, and merged a long time ago and there are already people using it. What @RodrigoDLopez is doing is just fixing this feature that was broken during a merge forward from 4.11 that was executed on 4.12.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rafaelweingartner commented on pull request #4283: Removes unnecessary validations
Posted by GitBox <gi...@apache.org>.
rafaelweingartner commented on pull request #4283:
URL: https://github.com/apache/cloudstack/pull/4283#issuecomment-754104827
**Please, check the code, and the cited PRs,** you can check there in the PR I created there is a method called `doesTargetStorageSupportNewDiskOffering`. I have not checked if somebody else also broke this check on master, but the code I created (**please, refer to the PR, you can see the code there**) checks if the new disk offering matches the target storage. However, that is not mandatory, as I said, it can be used as a means to facilitate operational tasks, and it must be used wisely by operators. **Moreover, it has been merged, and it is in use by people.** I really do not see why so much resistance from you guys 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rafaelweingartner commented on pull request #4283: Removes unnecessary validations
Posted by GitBox <gi...@apache.org>.
rafaelweingartner commented on pull request #4283:
URL: https://github.com/apache/cloudstack/pull/4283#issuecomment-696455059
> @RodrigoDLopez this removes tags checking altogether? This will allow migrating volumes to storages which are not even suitable wrt tags?
Exactly that. This was discussed and developed within a series of PRs I created some time ago, and it was broken while merging forward. Therefore, @RodrigoDLopez is fixing part of the broken code with this PR. These are the following PRs that were used to deliver this feature:
* https://github.com/apache/cloudstack/pull/2425
* https://github.com/apache/cloudstack/pull/2486
* https://github.com/apache/cloudstack/pull/2607
* https://github.com/apache/cloudstack/pull/2612
* https://github.com/apache/cloudstack/pull/2761
* https://github.com/apache/cloudstack/pull/2636
The implementation was broken down into several different PRs to easy development and testing. Moreover, the feature was already discussed, approved, and merged a long time ago and there are already people using it. What @RodrigoDLopez is doing is just fixing this feature that was broken during a merge forward from 4.11 that was executed on 4.12.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher edited a comment on pull request #4283: Removes unnecessary validations
Posted by GitBox <gi...@apache.org>.
GabrielBrascher edited a comment on pull request #4283:
URL: https://github.com/apache/cloudstack/pull/4283#issuecomment-791675371
The way I see there is no risk. The migration command is clear about overriding offerings.
1. if the ADMIN does not want to override the tags validation will happen as expected.
2. otherwise, if the ADMIN wants to do such migration, the offerings are updated accordingly to one of the offerings that comply with the target storage pool. Thus, allowing the volume migration to the target pool with no harm.
**NOTE:** I would like to stress how important it is to allow migrating volumes between storage pools as well as overriding disk offerings. We have some annoying issues in production due to such restrictions. For now, we have to manually run MySQL queries any time that such a migration operation is necessary; thus, betting that there will be no human mistakes compromising the DB consistency.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rafaelweingartner commented on pull request #4283: Removes unnecessary validations
Posted by GitBox <gi...@apache.org>.
rafaelweingartner commented on pull request #4283:
URL: https://github.com/apache/cloudstack/pull/4283#issuecomment-754881532
@PaulAngus it is your opinion. In my opinion, if the root wants to do a disk placement override, she/he should be able to do so. That is how it was presented for the community, when this feature was first introduced via https://github.com/apache/cloudstack/pull/2425, then we had a series of other patches to improve this feature, one of the PRs introduced the new option in the API to update the DB (https://github.com/apache/cloudstack/pull/2486), if the root admin wants to make a permanent change. The feature was presented to the community as a method to empower/enable root admins to do disk placement override in their daily tasks. Then, to normalize this situation, the root admin can use the option to replace the disk offering.
I really see no harm in this API.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] PaulAngus commented on pull request #4283: Removes unnecessary validations
Posted by GitBox <gi...@apache.org>.
PaulAngus commented on pull request #4283:
URL: https://github.com/apache/cloudstack/pull/4283#issuecomment-754113136
@rafaelweingartner
In your response, you say that the disk offering check is not mandatory, therefore the database integrity problem still exists.
I can't speak for anyone else, but _my_ objection is the perpetuation of the database integrity issue. I only comment now because I wasn't pinged on the other PRs, I was on this one. If I'd seen this before, I would have objected before.
Also, there is no double jeopardy, there is nothing to stop any piece of code being revisited at a later date.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher commented on pull request #4283: Removes unnecessary validations
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on pull request #4283:
URL: https://github.com/apache/cloudstack/pull/4283#issuecomment-791661210
@RodrigoDLopez can you please resolve the conflicts?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rafaelweingartner commented on pull request #4283: Removes unnecessary validations
Posted by GitBox <gi...@apache.org>.
rafaelweingartner commented on pull request #4283:
URL: https://github.com/apache/cloudstack/pull/4283#issuecomment-754119754
@PaulAngus I disagree. Your concern is only valid if (and only if) the operator is careless, which they should not be. This API is only available (should only be) to root admin.
If we revise this piece of code, we break API compatibility, which has already been broken as a matter of fact. Right now, it is considered as a bug, but if you want to remove this feature, then this has many other implications such as versioning.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rafaelweingartner edited a comment on pull request #4283: Removes unnecessary validations
Posted by GitBox <gi...@apache.org>.
rafaelweingartner edited a comment on pull request #4283:
URL: https://github.com/apache/cloudstack/pull/4283#issuecomment-754104827
**Please, check the code, and the cited PRs,** you can check there in the PR I created there is a method called `doesTargetStorageSupportNewDiskOffering` (https://github.com/apache/cloudstack/blob/261750c191f36ca93f0b8a030b00730b4a448c29/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java#L2272). I have not checked if somebody else also broke this check on master, but the code I created (**please, refer to the PR, you can see the code there**) checks if the new disk offering matches the target storage. However, that is not mandatory, as I said, it can be used as a means to facilitate operational tasks, and it must be used wisely by operators. **Moreover, it has been merged, and it is in use by people.** I really do not see why so much resistance from you guys 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] PaulAngus commented on pull request #4283: Removes unnecessary validations
Posted by GitBox <gi...@apache.org>.
PaulAngus commented on pull request #4283:
URL: https://github.com/apache/cloudstack/pull/4283#issuecomment-754037964
@RodrigoDLopez
1. the tests above are regression tests. They therefore cannot check whether the feature works when used as they can only look backwards. They are also only one small part of the testing rigour that should be applied to any new feature.
2. the community is working RCs for the 4.15 release, and certainly, I have not heard that master is available to add new features.
3. The tag validations are there to stop the service offerings from being broken - if you allow **anyone** to move VMs or storage contrary to the applied tags they are highly likely to pollute the database with resources in places that their service offerings say that they can't be.
4. If the problem is with moving between conflicting tags, then there needs to be a mechanism to facilitate changing the service offerings applied to a VM/volume, where the VM/storage is automatically migrated to matching hosts/storage; keeping the integrity of the database intact.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4283: Removes unnecessary validations
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4283:
URL: https://github.com/apache/cloudstack/pull/4283#issuecomment-726083402
@sureshanaparti @shwstppr @rhtyd can we merge? Or do you want to re-start the discussion?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] PaulAngus commented on pull request #4283: Removes unnecessary validations
Posted by GitBox <gi...@apache.org>.
PaulAngus commented on pull request #4283:
URL: https://github.com/apache/cloudstack/pull/4283#issuecomment-754097398
thank you I didn't see the email this morning.
I still disagree with the premise; and having the facility to change the disk offering at the same time gives no guarantee that the disk offering and the new storage placement will match.
**I cannot see how an API that has the ability to break the integrity of the database is justifiable.**
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] RodrigoDLopez removed a comment on pull request #4283: Removes unnecessary validations
Posted by GitBox <gi...@apache.org>.
RodrigoDLopez removed a comment on pull request #4283:
URL: https://github.com/apache/cloudstack/pull/4283#issuecomment-859588534
@PaulAngus @shwstppr
This one is not needed anymore.
Those that made the trouble to merge this, has merged another branch that does exactly the same result.
Many thanks to you all, prevents my ideas to go through, but still implement those ideas in another way and merge it without tests. You rock!!!!
@weizhouapache @rafaelweingartner @GabrielBrascher
thanks for the review
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher edited a comment on pull request #4283: Removes unnecessary validations
Posted by GitBox <gi...@apache.org>.
GabrielBrascher edited a comment on pull request #4283:
URL: https://github.com/apache/cloudstack/pull/4283#issuecomment-791675371
The way I see there is no risk. The migration command is clear about overriding offerings.
1. if the ADMIN does not want to override the tags validation will happen as expected.
2. otherwise, if the ADMIN wants to do such migration, the offerings are updated accordingly to one of the offerings that comply with the target storage pool. Thus, allowing the volume migration to the target pool with no harm.
**NOTE:** I would like to stress how important it is to allow migrating volumes between storage pools as well as overriding disk offerings. We have some annoying issues in production due to such restrictions. For now, we have to manually run MySQL queries any time that such a migration operation is necessary; thus, betting that there will be no human mistaken and compromise the DB consistency.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rafaelweingartner commented on pull request #4283: Removes unnecessary validations
Posted by GitBox <gi...@apache.org>.
rafaelweingartner commented on pull request #4283:
URL: https://github.com/apache/cloudstack/pull/4283#issuecomment-754057520
@PaulAngus it is not that complicated. @RodrigoDLopez is not introducing a feature, but rather fixing a bug. The feature was introduced via https://github.com/apache/cloudstack/pull/2636. It was first released on ACS 4.12. If you check the title of the PR that introduced it "Fix limitation on tag matching in 'migrateVolume' with disk offering replacement", and its code, you will see that there is a mechanism to normalize/update the disk offering in the DB to match the target storage. You can even check this option in the API documentation (https://cloudstack.apache.org/api/apidocs-4.14/apis/migrateVolume.html).
The feature was introduced to allow operators to have means to move the volume around, even if the tags do not match. This allows tasks such as troubleshooting performance problems, or validating if a storage tier upgrade would improve application performance. There was a series of PRs that I created to address these needs two years ago or so. They all have been reviewed, approved and merged, and were released. However, it happens to be that this feature was broken at some point in time, and @RodrigoDLopez is fixing this.
Also, there was an e-mail today stating "[ANNOUNCE] master and 4.15 branches open for merge". Therefore, we expected that to be open for everybody, especially for a bug fix such as this one, that was not even raised as a blocker. We know people using this feature, which required some internal patching to make it work on 4.13 for instance.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on pull request #4283: Removes unnecessary validations
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on pull request #4283:
URL: https://github.com/apache/cloudstack/pull/4283#issuecomment-762895687
for vms, we use "cmk find hostsformigration" to list all hosts, it also mentions if the hosts are suitable or not for migration.
if users try to migrate vm to a unsuitable host, cloudstack still allows it.
for volumes, it looks like "find storagepoolsformigration" also mark storage pools with unmatched tag as "unsuitable".
https://github.com/apache/cloudstack/blob/master/engine/storage/src/main/java/org/apache/cloudstack/storage/allocator/ZoneWideStoragePoolAllocator.java#L72
https://github.com/apache/cloudstack/blob/master/engine/storage/src/main/java/org/apache/cloudstack/storage/allocator/ClusterScopeStoragePoolAllocator.java#L81
https://github.com/apache/cloudstack/blob/master/engine/storage/src/main/java/org/apache/cloudstack/storage/allocator/LocalStoragePoolAllocator.java#L105
@rafaelweingartner @RodrigoDLopez could you please confirm it ?
if so, I think we could remove the check.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] shwstppr commented on a change in pull request #4283: Removes unnecessary validations
Posted by GitBox <gi...@apache.org>.
shwstppr commented on a change in pull request #4283:
URL: https://github.com/apache/cloudstack/pull/4283#discussion_r508333691
##########
File path: engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java
##########
@@ -2293,21 +2290,6 @@ private void migrateThroughHypervisorOrStorage(StoragePool destPool, VMInstanceV
}
}
- private void preStorageMigrationStateCheck(StoragePool destPool, VMInstanceVO vm) {
- if (destPool == null) {
- throw new CloudRuntimeException("Unable to migrate vm: missing destination storage pool");
- }
-
- checkDestinationForTags(destPool, vm);
- try {
- stateTransitTo(vm, Event.StorageMigrationRequested, null);
- } catch (final NoTransitionException e) {
- String msg = String.format("Unable to migrate vm: %s", vm.getUuid());
- s_logger.debug(msg);
- throw new CloudRuntimeException(msg, e);
- }
Review comment:
@rafaelweingartner @RodrigoDLopez do we need to remove this part of code as well? This will be changing VM state.
Regarding previously agreed behaviour I'm not completely aware of them. I'll go through older discussions regarding the same. It will be better if others can comment. +1 from me if this is what we already agreed and implemented.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] shwstppr commented on pull request #4283: Removes unnecessary validations
Posted by GitBox <gi...@apache.org>.
shwstppr commented on pull request #4283:
URL: https://github.com/apache/cloudstack/pull/4283#issuecomment-762758477
@RodrigoDLopez I understand this could be a use-case for operators as explained in previous comments but I cannot agree to it in its current form due to the reasons highlighted in Paul's comment. Also, completely doing away tag matching for a particular use-case is not appropriate in my opinion. If this is done on one hand we will have migrateVirtualMachineWithVolumes API which depends on storage tag matching for finding suitable storage pools for volume (when pools for a volume is not passed by the user) and on the other migrateVirtualMachine API which completely ignores storage tags
https://github.com/apache/cloudstack/blob/master/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java#L2824
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #4283: Removes unnecessary validations
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4283:
URL: https://github.com/apache/cloudstack/pull/4283#issuecomment-697868413
@rafaelweingartner what branch should this go on (since it was broken in 4.12 you say)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] PaulAngus commented on pull request #4283: Removes unnecessary validations
Posted by GitBox <gi...@apache.org>.
PaulAngus commented on pull request #4283:
URL: https://github.com/apache/cloudstack/pull/4283#issuecomment-859691225
I don’t know anything about another version of your idea, no one has pinged me to comment on it. I would be sad if the author of the other version hadn’t pinged you for your comments.
Certainly nothing should be merged without tests. If there aren’t tests you should bring it up with whoever merged it and if you get no joy there, the community.
Kind Regards
Paul Angus
From: Rodrigo D. Lopez ***@***.***>
Sent: Friday, June 11, 2021 2:39 PM
To: apache/cloudstack ***@***.***>
Cc: Paul Angus ***@***.***>; Mention ***@***.***>
Subject: Re: [apache/cloudstack] Removes unnecessary validations (#4283)
@PaulAngus<https://github.com/PaulAngus> @shwstppr<https://github.com/shwstppr>
This one is not needed anymore.
Those that made trouble to merge this, has merged another branch that do exactly the same result.
Many thanks to you all, prevents mine ideas to go through. but implements those ideas in another way and merge it without testes. you rock
@weizhouapache<https://github.com/weizhouapache> @rafaelweingartner<https://github.com/rafaelweingartner> @GabrielBrascher<https://github.com/GabrielBrascher>
thanks for the review
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<https://github.com/apache/cloudstack/pull/4283#issuecomment-859588534>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABEWL3DS5YIZWSNORY7PMFTTSIGV7ANCNFSM4QJS6M7Q>.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher edited a comment on pull request #4283: Removes unnecessary validations
Posted by GitBox <gi...@apache.org>.
GabrielBrascher edited a comment on pull request #4283:
URL: https://github.com/apache/cloudstack/pull/4283#issuecomment-791675371
The way I see there is no risk. The migration command is clear about overriding offerings.
1. if the ADMIN does not want to override the tags, validation will happen as expected.
2. otherwise, if the ADMIN wants to do such migration, the offerings are updated accordingly to one of the offerings that comply with the target storage pool. Thus, allowing the volume migration to the target pool with no harm.
**NOTE:** I would like to stress how important it is to allow migrating volumes between storage pools as well as overriding disk offerings. We have some annoying issues in production due to such restrictions. For now, we have to manually run MySQL queries any time that such a migration operation is necessary; thus, betting that there will be no human mistakes compromising the DB consistency.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] RodrigoDLopez closed pull request #4283: Removes unnecessary validations
Posted by GitBox <gi...@apache.org>.
RodrigoDLopez closed pull request #4283:
URL: https://github.com/apache/cloudstack/pull/4283
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] PaulAngus commented on pull request #4283: Removes unnecessary validations
Posted by GitBox <gi...@apache.org>.
PaulAngus commented on pull request #4283:
URL: https://github.com/apache/cloudstack/pull/4283#issuecomment-754134211
There are many ways an admin can wreck their environment which are out of our control. This **is** within our control. And i don't believe there is another API that allows the database to be corrupted in this way.
Thats my objection. IMO these checks are not unnecessary, but vital.
I haven't -1'd this.
So if there enough others who aren't worried, I'm not stopping anyone.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rafaelweingartner commented on pull request #4283: Removes unnecessary validations
Posted by GitBox <gi...@apache.org>.
rafaelweingartner commented on pull request #4283:
URL: https://github.com/apache/cloudstack/pull/4283#issuecomment-712506439
@shwstppr to me it is very simple. It is a feature to provide flexibility and power to operators. It was already discussed with the community, accepted, merged, and released to people. Moreover, there are CloudStack deployments that take advantage of that. Operators must be responsible and aware of the actions they take. Therefore, ACS must fix it (which is what @RodrigoDLopez is doing). If we want to remove this feature, we need a new discussion, and this will break API compatibility
@DaanHoogland I do not have any preference on what version this fix must go in. However, I see it as I explained. It is a feature that was proposed, discussed, and accepted. People use it. And therefore, we should maintain it as designed and sold to the community and documented.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4283: Removes unnecessary validations
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4283:
URL: https://github.com/apache/cloudstack/pull/4283#issuecomment-679280228
Packaging result: ✔centos7 ✔centos8 ✔debian. JID-1814
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] RodrigoDLopez commented on pull request #4283: Removes unnecessary validations
Posted by GitBox <gi...@apache.org>.
RodrigoDLopez commented on pull request #4283:
URL: https://github.com/apache/cloudstack/pull/4283#issuecomment-700748842
Hi @DaanHoogland
I think it's okay to merge this branch just on the master.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rafaelweingartner edited a comment on pull request #4283: Removes unnecessary validations
Posted by GitBox <gi...@apache.org>.
rafaelweingartner edited a comment on pull request #4283:
URL: https://github.com/apache/cloudstack/pull/4283#issuecomment-754136739
just one thing needs to be clear, it does not corrupt the DB. The volume table shows the correct storage pool. The only thing is that this storage pool might not have have tags of the volume's disk offering (in case of disk placement override by the operator). However, this can be fixed if the operator uses the parameter `newDiskOffering`.
**No DB will ever be corrupted by this feature**. There is only the possibility to apply some override in case of an emergency or some necessity by the operator. **Nothing will break**! Tags on pools and volume are merely logical constraints applied by ACS, and nothing more.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] RodrigoDLopez commented on pull request #4283: Removes unnecessary validations
Posted by GitBox <gi...@apache.org>.
RodrigoDLopez commented on pull request #4283:
URL: https://github.com/apache/cloudstack/pull/4283#issuecomment-754023088
@rhtyd, @DaanHoogland, @PaulAngus, @shwstppr
Hello guys, happy new year.
Is there something missing here? Everything seems to be ok with the patch. it has no errors, and tests are passing.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] RodrigoDLopez commented on pull request #4283: Removes unnecessary validations
Posted by GitBox <gi...@apache.org>.
RodrigoDLopez commented on pull request #4283:
URL: https://github.com/apache/cloudstack/pull/4283#issuecomment-762374558
@rhtyd @shwstppr @sureshanaparti @PaulAngus
Hello to everyone, can I have more reviews/tests here?
I think this is a good one. And I'm just fixing something that at some point was discussed with the community and it was working as @rafaelweingartner said and showed in previous comments and that can also be seen in my description of this PR.
I don't know why is taking so long to analyze and merge this PR
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org