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

Fixes #23654 - update even virtual interface facts #5597

Merged
merged 1 commit into from Sep 9, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions app/models/host/base.rb
Expand Up @@ -208,6 +208,7 @@ def set_interfaces(parser)
if !self.managed? && self.primary_interface.mac.blank? && self.primary_interface.identifier.blank?
identifier, values = parser.suggested_primary_interface(self)
if values.present?
logger.debug "Suggested #{identifier} NIC as a primary interface."
self.primary_interface.mac = Net::Validations.normalize_mac(values[:macaddress])
interface_klass = interface_class(identifier)
# bridge interfaces are not attached to parent interface so save would not be possible
Expand Down Expand Up @@ -479,7 +480,7 @@ def get_interface_scope(name, attributes, base = self.interfaces)
# we search bonds based on identifiers, e.g. ubuntu sets random MAC after each reboot se we can't
# rely on mac
when 'Nic::Bond', 'Nic::Bridge'
base.virtual.where(:identifier => name)
base.where(:identifier => name)
Copy link
Member

Choose a reason for hiding this comment

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

This allows to update physical interface to bond/bridge, which is virtual. Reading #23654 it seems that you defined the br_private interface defined already and not marked as bridge. This does not seem to be a bug but rather a misconfiguration. Or could you explain pls, why we need to search also physical interfaces?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ares the issue with primary bridge interface because in this case the information already in the DB with incorrect :virual set to false. BTW the :identifier is unique so my proposed changes should work in all cases.

# for other interfaces we distinguish between virtual and physical interfaces
# for virtual devices we don't check only mac address since it's not unique,
# if we want to update the device it must have same identifier
Expand Down Expand Up @@ -528,7 +529,7 @@ def save_updated_bond(bond)
bond.save!
rescue StandardError => e
logger.warn "Saving #{bond.identifier} NIC for host #{self.name} failed, skipping because #{e.message}:"
bond.errors.full_messages.each { |e| logger.warn " #{e}" }
bond.errors.full_messages.each { |message| logger.warn " #{message}" }
end

def set_interface(attributes, name, iface)
Expand Down
17 changes: 13 additions & 4 deletions test/models/host_test.rb
Expand Up @@ -937,7 +937,7 @@ def teardown

@host.environment = env_with_other_tax
refute @host.valid?
assert_match /is not assigned/, @host.errors[:environment_id].first
assert_match(/is not assigned/, @host.errors[:environment_id].first)
end
end

Expand Down Expand Up @@ -1611,6 +1611,15 @@ def teardown
assert host.interfaces.where(:mac => '00:00:00:11:22:33').first.link
end

test "#set_interfaces updates existing bridge interface suggested as primary" do
host, parser = setup_host_with_nic_parser({:identifier => 'br-ex', :macaddress => '00:00:00:11:22:33', :bridge => true, :virtual => true, :ipaddress => '10.10.0.1'})
host.managed = false
host.primary_interface.update(:identifier => 'br-ex', :mac => '00:00:00:11:22:33', :ip => '10.10.0.1')

host.set_interfaces(parser)
assert host.interfaces.where(:identifier => 'br-ex').first.virtual
end

test "#set_interfaces creates new interface even if primary interface has same MAC" do
host, parser = setup_host_with_nic_parser({:macaddress => '00:00:00:11:22:33', :virtual => true, :ipaddress => '10.10.0.1', :attached_to => 'eth0', :identifier => 'eth0_0'})
host.update_attribute :mac, '00:00:00:11:22:33'
Expand Down Expand Up @@ -2119,7 +2128,7 @@ def teardown
host.built(true)
email = ActionMailer::Base.deliveries.detect { |mail| mail.subject =~ /Host #{host} is built/ }
assert email
assert_match /Your host has finished/, email.body.encoded
assert_match(/Your host has finished/, email.body.encoded)
end

test "is not sent when not installed" do
Expand Down Expand Up @@ -3164,7 +3173,7 @@ def to_managed!

host.operatingsystem.media = []
refute host.valid?
assert_match /must belong/, host.errors[:medium_id].first
assert_match(/must belong/, host.errors[:medium_id].first)
end

context "lookup value attributes" do
Expand Down Expand Up @@ -3797,7 +3806,7 @@ def to_managed!
test "host have a media provider providing a valid URL" do
hostgroup = FactoryBot.create(:hostgroup, :with_domain, :with_os)
host = FactoryBot.create(:host, :managed, :hostgroup => hostgroup)
assert_match /^http:/, host.medium_provider.medium_uri.to_s
assert_match(/^http:/, host.medium_provider.medium_uri.to_s)
end

test "should find interface by attached mac" do
Expand Down