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.

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

Support KubeVirt provider #197

Merged
merged 1 commit into from
Jan 17, 2018
Merged

Conversation

masayag
Copy link
Contributor

@masayag masayag commented Dec 31, 2017

The kubevirt provider will be managed as a virtualization manager under
the container managers.

The endpoint and authentication of kubevirt will be selected by
:kubevirt role and authkey.

This PR depends on ManageIQ/manageiq#16721.

Additional related PRs:

UI PR:
ManageIQ/manageiq-ui-classic#3143

kubevirt provider PR:
ManageIQ/manageiq-providers-kubevirt#6

@masayag
Copy link
Contributor Author

masayag commented Jan 1, 2018

@miq-bot add_labels enhancement, gaprindashvili/no

@@ -0,0 +1,39 @@
module ManageIQ::Providers::Kubernetes::VirtualizationManagerMixin

Choose a reason for hiding this comment

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

I can't seem to find where this is used?
(except for this)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@moolitayer I've updated the PR message to reference the other related PRs (ui-classic and kubevirt provider)

@moolitayer moolitayer requested review from cben and zeari January 8, 2018 09:55
@moolitayer moolitayer self-assigned this Jan 8, 2018
@moolitayer
Copy link

@cben @zeari need your review on this one

@masayag
Copy link
Contributor Author

masayag commented Jan 8, 2018

@moolitayer I need to update the current version of the PR. Will nag when ready for review.

@moolitayer
Copy link

@moolitayer I need to update the current version of the PR. Will nag when ready for review.

thanks

it "Creates a virtualization manager when container manager is updated with a kubevirt endpoint" do
ems = FactoryGirl.create(:ems_kubernetes)
kubevirt_endpoint = FactoryGirl.create(:endpoint, :role => 'kubevirt', :hostname => 'host', :resource => ems)
ems.endpoints << kubevirt_endpoint
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zeari @moolitayer this specific test fails with:

/home/masayag/work/manageiq-master/app/models/ext_management_system.rb:95:in hostname_format_valid?'
...
# /home/masayag/.rvm/gems/ruby-2.3.3/gems/activerecord-5.0.6/lib/active_record/suppressor.rb:41:in save' # /home/masayag/work/manageiq-master/app/models/mixins/has_virtualization_manager_mixin.rb:8:in virtualization_endpoint_created'
# /home/masayag/work/manageiq-master/app/models/manageiq/providers/container_manager.rb:104:in endpoint_created' # /home/masayag/work/manageiq-master/app/models/endpoint.rb:17:in endpoint_created'

It appears that even though the endpoint_created was called twice (for 'default' and 'kubevirt'), before saving the virtualization manager, there is 'default' endpoint.

Any ideas ?

Choose a reason for hiding this comment

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

There is an error related to the validity of the hostname?

Copy link

@zeari zeari Jan 14, 2018

Choose a reason for hiding this comment

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

Try

it "Creates a virtualization manager when container manager is updated with a kubevirt endpoint" do
  ems = FactoryGirl.create(:ems_kubernetes, :endpoints => [FactoryGirl.build(:endpoint, :role => 'default', :hostname => 'iamahostname'])
  kubevirt_endpoint = FactoryGirl.create(:endpoint, :role => 'kubevirt', :hostname => 'host', :resource => ems)
   # ems.endpoints << kubevirt_endpoint  ----- you dont need this because of :resource => ems
  expect(ems.virtualization_manager).not_to be_nil
  expect(ems.virtualization_manager.parent_manager).to eq(ems)
  expect(ems.virtualization_manager.default_endpoint.role).to eq("kubevirt")
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zeari that didn't do the trick.

I ended up with creating an endpoint (via FactoryGirl.build) and in the next line added it to the ems.endpoints

That seems to satisfy the spec.

@masayag masayag force-pushed the virtualization_manager branch 3 times, most recently from 31323c0 to c65977a Compare January 14, 2018 18:48
The kubevirt provider will be managed as a virtualization manager under
the container managers.

The endpoint and authentication of kubevirt will be selected by
:kubevirt role and authkey.
@miq-bot
Copy link
Member

miq-bot commented Jan 16, 2018

Checked commit masayag@c16eb0d with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 1 offense detected

app/models/manageiq/providers/kubernetes/container_manager_mixin.rb

@masayag
Copy link
Contributor Author

masayag commented Jan 16, 2018

@zeari I've updated the PR according to Adam's comments on the core PR (renamed the virtualization_manger to infra_manager).

@agrare agrare closed this Jan 17, 2018
@agrare agrare reopened this Jan 17, 2018
@agrare
Copy link
Member

agrare commented Jan 17, 2018

@masayag looks like some valid test failures

@masayag masayag closed this Jan 17, 2018
@masayag masayag reopened this Jan 17, 2018
@agrare
Copy link
Member

agrare commented Jan 17, 2018

Travis isn't running and the test suite passes locally, just going to merge this to fix the errors on master

@agrare agrare merged commit c16eb0d into ManageIQ:master Jan 17, 2018
agrare added a commit that referenced this pull request Jan 17, 2018
@agrare agrare added this to the Sprint 78 Ending Jan 29, 2018 milestone Jan 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants