-
Notifications
You must be signed in to change notification settings - Fork 897
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
Perfomance fix for Object Storage Manager deletion #14009
Perfomance fix for Object Storage Manager deletion #14009
Conversation
de92bfa
to
9215caa
Compare
Checked commit xlab-si@9215caa with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
@aiperon @roliveri so, this has a funky side effect for the refresh. If there is :dependent => :destroy missing, the refresh saving code does not delete the missing objects. It uses association.delete(deletes), where association is ems.cloud_object_store_objects This affect only old refresh, not graph refresh |
Disabling a broken spec caused by ManageIQ/manageiq#14009
@Ladas Should I fix this spec? |
@aiperon @roliveri disabling the spec in ManageIQ/manageiq-providers-amazon#148, could you guys figure out the way to pass the spec? This might be even a rails bug, cause I would not expect this would be interconnected like this. |
@aiperon if you know how? :-) I am disabling it for now, so it doesn't block the AWS, with the knowledge that the refresh will create zombies for OpenStack and AWS old refresh |
So, we (or I) need to fix the old refresh. |
It might be, it's something already fixed in Rails master :-) The hack that would not require changing of the saving_code (not sure if we can, it's the shared one) would be doing disconnect_inv method that would call destroy, on the cloud_object_store_object model. Then it will not be calling the association.delete |
the code is here https://github.com/Ladas/manageiq/blob/48aa323551c8825e02170e251afa717ca807d2ed/app/models/ems_refresh/save_inventory_helper.rb#L65-L65 so we need disconnect_inv method and passing disconnect = true here https://github.com/Ladas/manageiq/blob/1bbb4fcef58c1f852f7e983c43153b0457cf214c/app/models/ems_refresh/save_inventory_object_storage.rb#L80-L80 |
it's ugly, but acceptable for me, given all ^ code will hopefully be deleted soon :-D @roliveri is that fine with you? |
@Ladas That's OK with me. |
@aiperon should I send the fix, or you are working on that? |
@Ladas, please do it. I haven't worked on that yet. |
Recently, the inventorying of S3 objects was merged to the master.
ManageIQ/manageiq-providers-amazon#123
Since a lot of objects could be stored on S3, user wouldn't be able to delete Amazon provider.
This fix allows destroying of storage manager with 40K objects in 40 seconds.
@roliveri please, review
@miq-bot add_label euwe/no, providers, providers/storage, performance