Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add method vm_move_into_folder. #285

Merged
merged 1 commit into from
Jun 20, 2018
Merged

Add method vm_move_into_folder. #285

merged 1 commit into from
Jun 20, 2018

Conversation

lfu
Copy link
Member

@lfu lfu commented Jun 8, 2018 •
edited
Loading

Add method vm_move_into_folder to move VM to a new folder.

Part of ManageIQ/manageiq#17519.

https://bugzilla.redhat.com/show_bug.cgi?id=1090957

@miq-bot add_label gaprindashvili/no, enhancement

@lfu lfu mentioned this pull request Jun 8, 2018
Merged
@agrare
Copy link
Member

agrare commented Jun 8, 2018

@lfu you'll need to update the refresh parsers to set the type as well

Fryguy
Fryguy requested changes Jun 8, 2018
View reviewed changes
app/models/manageiq/providers/vmware/infra_manager/ems_folder.rb Outdated
@@ -0,0 +1,13 @@
class ManageIQ::Providers::Vmware::InfraManager::EmsFolder < EmsFolder
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@agrare Are we going to need a data migration for this new class? This will seriously break refresh if we don't, I think.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so. Working on it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Fryguy I think the existing folders will be found and have their type updated on the first refresh after upgrade without being destroyed but let me confirm that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah never mind, forgot we .except the :type in save_inventory_with_findkey

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to delete the existing folders then the refresher would add them back in with type populated. Seems the refresher does not update the existing ones.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lfu yeah that's because of https://github.com/ManageIQ/manageiq/blob/master/app/models/ems_refresh/save_inventory_helper.rb#L101

@lfu lfu force-pushed the vm_move_folder_1090957 branch 4 times, most recently from 639a52e to 0a48372 Compare June 11, 2018 18:03
@agrare agrare mentioned this pull request Jun 13, 2018
Closed
@agrare
Copy link
Member

agrare commented Jun 14, 2018

@lfu there are a few methods in the base EmsFolder which are VMware only, can you move those into here?

https://github.com/ManageIQ/manageiq/blob/master/app/models/ems_folder.rb#L25-L33 and https://github.com/ManageIQ/manageiq/blob/master/app/models/ems_folder.rb#L121-L151

agrare
agrare reviewed Jun 14, 2018
View reviewed changes
spec/models/manageiq/providers/vmware/infra_manager/refresher_spec.rb Outdated
@@ -763,8 +763,7 @@ def assert_specific_vm
:ems_ref_obj => VimString.new("group-d1", :Folder, :ManagedObjectReference),
:uid_ems => "group-d1",
:name => "Datacenters",
:type => nil,

:type => "ManageIQ::Providers::Vmware::InfraManager::EmsFolder",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to update to ...::InfraManager::Folder

agrare
agrare reviewed Jun 14, 2018
View reviewed changes
spec/models/manageiq/providers/vmware/infra_manager/refresher_spec.rb Outdated
@@ -773,8 +772,7 @@ def assert_specific_vm
:ems_ref_obj => VimString.new("group-v11341", :Folder, :ManagedObjectReference),
:uid_ems => "group-v11341",
:name => "JFitzgerald",
:type => nil,

:type => "ManageIQ::Providers::Vmware::InfraManager::EmsFolder",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same

agrare
agrare reviewed Jun 14, 2018
View reviewed changes
spec/models/manageiq/providers/vmware/infra_manager/refresher_spec.rb Outdated
[EmsFolder, "Datacenters", {:hidden => true}] => {
[Datacenter, "Dev"] => {
[EmsFolder, "host", {:hidden => true}] => {
[ManageIQ::Providers::Vmware::InfraManager::EmsFolder, "Datacenters", {:hidden => true}] => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same on these

agrare
agrare reviewed Jun 14, 2018
View reviewed changes
spec/models/miq_vim_broker_worker/runner_spec.rb Outdated
@@ -258,7 +258,7 @@

it "will handle create events properly" do
mor = "group-v123"
klass = "EmsFolder"
klass = "ManageIQ::Providers::Vmware::InfraManager::EmsFolder"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

@agrare
Copy link
Member

agrare commented Jun 14, 2018

@lfu couple of small things then this LGTM

@lfu lfu changed the title Add EmsFolder class to VMware provider. Add Folder class to VMware provider. Jun 14, 2018
@lfu
Copy link
Member Author

lfu commented Jun 14, 2018

Travis would fail due to the factory changes in ManageIQ/manageiq#17519.
This PR and ManageIQ/manageiq#17519 becomes dependent on each other.

@lfu lfu force-pushed the vm_move_folder_1090957 branch 2 times, most recently from 79023fb to 49396d7 Compare June 14, 2018 17:58
agrare
agrare approved these changes Jun 14, 2018
View reviewed changes
Copy link
Member

@agrare agrare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 this passes for me locally with ManageIQ/manageiq#17519 applied

@lfu lfu changed the title Add Folder class to VMware provider. Add method vm_move_into_folder. Jun 19, 2018
@miq-bot
Copy link
Member

miq-bot commented Jun 19, 2018

Checked commit lfu@2ad3dc5 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 👍

agrare
agrare reviewed Jun 20, 2018
View reviewed changes
app/models/manageiq/providers/vmware/infra_manager.rb
@@ -249,6 +249,10 @@ def vm_relocate(vm, options = {})
invoke_vim_ws(:relocateVM, vm, options[:user_event], options[:host], options[:pool], options[:datastore], options[:disk_move_type], options[:transform], options[:priority], options[:disk])
end

def vm_move_into_folder(vm, options = {})
invoke_vim_ws(:moveIntoFolder, options[:folder], options[:user_event], vm.ems_ref_obj)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this right? :moveIntoFolder is defined on MiqVimFolder and takes just the managed object ems_ref_obj. The method on MiqVim is :moveIntoFolder_Task and takes a folder ref and a vm ref.

Can you do a live test of this end to end to make sure it works?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@agrare This has been end-to-end tested and it went well.
invoke_vim_ws would convert options[:folder] to MiqVimFolder and calls moveIntoFolder on it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sorry you're completely right

agrare
agrare approved these changes Jun 20, 2018
View reviewed changes
Copy link
Member

@agrare agrare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 LGTM

@agrare agrare merged commit 9fd3c24 into ManageIQ:master Jun 20, 2018
@agrare agrare added this to the Sprint 89 Ending Jul 2, 2018 milestone Jun 20, 2018
@lfu lfu deleted the vm_move_folder_1090957 branch September 29, 2018 14:35
agrare pushed a commit to agrare/manageiq-providers-vmware that referenced this pull request Apr 15, 2019
@gmcculloug
Merge pull request ManageIQ#285 from AlexanderZagaynov/BZ-1561937
3951c8b 
handle volume snapshot status changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fryguy Fryguy Fryguy requested changes

@agrare agrare agrare approved these changes

Assignees

@agrare agrare

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lfu @agrare @miq-bot @Fryguy