[PATCH/foreman 1/1] Fixes #944 - clear network conflicts

Development
1

Alter error handling

Ensure that conflict errors are bound to the rails.errors[:conflict] key.
This is so that we can detect the "all errors are conflict errors" condition.

Change the update/create page to display "overwrite"

If a collision-only situation is detected then change the GUI to
indicate that the next save will be forced.

Order the hostname and IP based upon DNS record type

Handle conflicts with a lease

Display conflict messages in the GUI

Signed-off-by: Paul Kelly <paul.ian.kelly@googlemail.com>

··· --- app/controllers/application_controller.rb | 2 +- app/controllers/hosts_controller.rb | 10 +++++ app/helpers/layout_helper.rb | 6 ++- app/models/host.rb | 9 +++++ app/models/orchestration.rb | 15 +++++--- app/models/orchestration/dhcp.rb | 55 +++++++++++++++++++++++++--- app/models/orchestration/dns.rb | 37 +++++++++++++++++-- app/views/hosts/_form.html.erb | 4 +- lib/core_extensions.rb | 6 +++ lib/net.rb | 5 +++ lib/net/dhcp/record.rb | 2 +- lib/net/dns.rb | 6 +--- lib/net/dns/a_record.rb | 6 +++- lib/net/dns/ptr_record.rb | 6 +++- test/test_helper.rb | 1 + test/unit/orchestration/dhcp_test.rb | 4 +- 16 files changed, 143 insertions(+), 31 deletions(-)

diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index 29d1570…9411c4a 100644
— a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -206,7 +206,7 @@ class ApplicationController < ActionController::Base
else
hash[:redirect] ||= eval("#{controller_name}_url")
end

  • hash[:error_msg] ||= hash[:object].errors[:base]

  • hash[:error_msg] ||= hash[:object].errors[:base] + hash[:object].errors[:conflict].each{|e| “Conflict - #{e}”}

    hash[:json_code] ||= :unprocessable_entity
    logger.info “Failed to save: #{hash[:object].errors.full_messages.join(”, ")}"
    diff --git a/app/controllers/hosts_controller.rb b/app/controllers/hosts_controller.rb
    index bd848c2…037a7c1 100644
    — a/app/controllers/hosts_controller.rb
    +++ b/app/controllers/hosts_controller.rb
    @@ -91,6 +91,7 @@ class HostsController < ApplicationController
    process_success :success_redirect => @host
    else
    load_vars_for_ajax

  •  offer_to_overwrite_conflicts
 process_error

    end
    end
    @@ -105,6 +106,7 @@ class HostsController < ApplicationController
    process_success :success_redirect => @host
    else
    load_vars_for_ajax

  •  offer_to_overwrite_conflicts
 process_error

    end
    end
    @@ -539,4 +541,12 @@ class HostsController < ApplicationController
    @host.request_url = request.host_with_port if @host.respond_to?(:request_url)
    end

  • if a save failed and the only reason was network conflicts then flag this so that the view

  • is rendered differently and the next save operation will be forced

  • def offer_to_overwrite_conflicts

  • if !@host.errors.empty? and @host.errors.are_all_conflicts?

  •  @host.overwrite = "true"

  • end

  • end

end
diff --git a/app/helpers/layout_helper.rb b/app/helpers/layout_helper.rb
index 8057c88…5349de4 100644
— a/app/helpers/layout_helper.rb
+++ b/app/helpers/layout_helper.rb
@@ -75,10 +75,12 @@ module LayoutHelper
end
end

  • def submit_or_cancel f
  • def submit_or_cancel f, overwrite = false
    "
    ".html_safe + content_tag(:p, :class => “ra”) do
  •  text    = overwrite ? "Overwrite" : "Submit"

  •  options = overwrite ? {:class => "btn danger"} : {:class => "btn primary"}
 link_to("Cancel", eval("#{controller_name}_path"), :class => "btn") + " " +

  •  f.submit("Submit", :class => "btn primary")

  •  f.submit(text, options)
    end
    end

diff --git a/app/models/host.rb b/app/models/host.rb
index 64b1a12…0033ea3 100644
— a/app/models/host.rb
+++ b/app/models/host.rb
@@ -601,6 +601,15 @@ class Host < Puppet::Rails::Host
{}
end

  • def overwrite?

  • @overwrite ||= false

  • end

  • We have to coerce the value back to boolean. It is not done for us by the framework.

  • def overwrite=(value)

  • @overwrite = value == “true”

  • end

  • private

    align common mac and ip address input

    def normalize_addresses
    diff --git a/app/models/orchestration.rb b/app/models/orchestration.rb
    index 5d4d0b3…6e6e5b8 100644
    — a/app/models/orchestration.rb
    +++ b/app/models/orchestration.rb
    @@ -11,8 +11,8 @@ module Orchestration
    before_validation :setup_clone

     # extend our Host model to know how to handle subsystems

  •  include Orchestration::DHCP
 include Orchestration::DNS

  •  include Orchestration::DHCP
 include Orchestration::TFTP
 include Orchestration::Puppetca
 include Orchestration::Libvirt

@@ -40,9 +40,9 @@ module Orchestration
end

 # log and add to errors
  • def failure msg, backtrace=nil
  • def failure msg, backtrace=nil, dest = :base
    logger.warn(backtrace ? msg + backtrace.join("\n") : msg)
  •  errors.add :base, msg

  •  errors.add dest, msg
 false
    end

@@ -53,7 +53,7 @@ module Orchestration
# not care about their return status.
def valid?(context = nil)
super

  •  errors.empty?

  •  overwrite? ? errors.are_all_conflicts? : errors.empty?

    end

    we override the destroy method, in order to ensure our queue exists before other callbacks

@@ -88,10 +88,13 @@ module Orchestration
rescue Net::Conflict => e
task.status = “conflict”
@record_conflicts << e

  •      failure e.message

  •      failure e.message, nil, :conflict
   rescue RestClient::Exception => e
     task.status = "failed"
     failure "#{task.name} task failed with the following error: #{e.response}"

  •    rescue Net::LeaseConflict => e

  •      task.status = "failed"

  •      failure "DHCP has a lease at #{e}"
   rescue => e
     task.status = "failed"
     failure "#{task.name} task failed with the following error: #{e}"

@@ -99,7 +102,7 @@ module Orchestration
end

   # if we have no failures - we are done

  •  return true if q.failed.empty? and q.pending.empty? and q.conflict.empty? and errors.empty?

  •  return true if q.failed.empty? and q.pending.empty? and q.conflict.empty? and (overwrite? ? errors.are_all_conflicts? : errors.empty?)

 logger.warn "Rolling back due to a problem: #{q.failed + q.conflict}"
 # handle errors

diff --git a/app/models/orchestration/dhcp.rb b/app/models/orchestration/dhcp.rb
index ac07ad3…6a56087 100644
— a/app/models/orchestration/dhcp.rb
+++ b/app/models/orchestration/dhcp.rb
@@ -2,7 +2,7 @@ module Orchestration::DHCP
def self.included(base)
base.send :include, InstanceMethods
base.class_eval do

  •  after_validation :queue_dhcp

  •  after_validation :validate_dhcp, :queue_remove_dhcp_conflicts, :queue_dhcp
 before_destroy :queue_dhcp_destroy
 validate :ip_belongs_to_subnet?, :valid_jumpstart_model

    end
    @@ -11,11 +11,11 @@ module Orchestration::DHCP
    module InstanceMethods

    def dhcp?

  •  !subnet.nil? and subnet.dhcp? and errors.empty?

  •  !subnet.nil? and subnet.dhcp?

    end

    def sp_dhcp?

  •  sp_valid? and !sp_subnet.nil? and sp_subnet.dhcp? and errors.empty?

  •  sp_valid? and !sp_subnet.nil? and sp_subnet.dhcp?

    end

    def dhcp_record
    @@ -30,22 +30,55 @@ module Orchestration::DHCP

    protected

  • def validate_dhcp

  •  # This is an expensive operation and we will only do it if the DNS validation failed. This will ensure

  •  # that we report on both DNS and DHCP conflicts when we offer to remove collisions. It retrieves and

  •  # caches the conflicting records so we must always do it when overwriting

  •  return unless (!errors.empty? and errors.are_all_conflicts?) or overwrite?

  •  return unless dhcp?

  •  status = true

  •  status = failure("- #{dhcp_record.conflicts[0]} exists in DHCP", nil, :conflict) if dhcp_record.conflicting?

  •  if sp_dhcp?

  •    (status = failure("- #{sp_dhcp_record.conflicts[0]} exists in DHCP", nil, :conflict) and status) if sp_dhcp_record.conflicting?

  •  end

  •  overwrite? ? errors.are_all_conflicts? : status

  • rescue Net::LeaseConflict => e

  •  failure("DHCP has a lease at #{e}")

  • end

  • def set_dhcp
    dhcp_record.create
    end

  • def set_dhcp_conflicts

  •  dhcp_record.conflicts.each{|conflict| conflict.create}

  • end

  • def set_sp_dhcp
    sp_dhcp_record.create
    end

  • def set_sp_dhcp_conflicts

  •  sp_dhcp_record.conflicts.each{|conflict| conflict.create}

  • end

  • def del_dhcp
    dhcp_record.destroy
    end

  • def del_dhcp_conflicts

  •  dhcp_record.conflicts.each{|conflict| conflict.destroy}

  • end

  • def del_sp_dhcp
    sp_dhcp_record.destroy
    end

  • def del_sp_dhcp_conflicts

  •  sp_dhcp_record.conflicts.each{|conflict| conflict.destroy}

  • end

  • private

    returns a hash of dhcp record settings

    def dhcp_attrs
    @@ -84,13 +117,12 @@ module Orchestration::DHCP
    end

    def queue_dhcp

  •  return unless (dhcp? or (old and old.dhcp?) or sp_dhcp? or (old and old.sp_dhcp?)) and errors.empty?

  •  logger.debug "inspecting changes that are required for DHCP infrastructure"

  •  return unless (dhcp? or (old and old.dhcp?) or sp_dhcp? or (old and old.sp_dhcp?)) and (overwrite? ? errors.are_all_conflicts? : errors.empty?)
 new_record? ? queue_dhcp_create : queue_dhcp_update

    end

    def queue_dhcp_create

  •  logger.debug "Adding new DHCP reservations"

  •  logger.debug "Scheduling new DHCP reservations"
 queue.create(:name   => "DHCP Settings for #{self}", :priority => 10,
              :action => [self, :set_dhcp]) if dhcp?
 queue.create(:name   => "DHCP Settings for #{sp_name}", :priority => 15,

@@ -143,6 +175,17 @@ module Orchestration::DHCP
true
end

  • def queue_remove_dhcp_conflicts
  •  return unless dhcp? and (!errors.empty? and errors.are_all_conflicts?)

  •  return unless overwrite?

  •  logger.debug "Scheduling DHCP conflicts removal"

  •  queue.create(:name   => "DHCP conflicts removal for #{self}", :priority => 5,

  •               :action => [self, :del_dhcp_conflicts]) if dhcp_record.conflicting?

  •  queue.create(:name   => "DHCP conflicts removal for #{sp_name}", :priority => 5,

  •               :action => [self, :del_sp_dhcp_conflicts]) if sp_valid? and sp_dhcp_record.conflicting?

  •  true
  • end
  • def ip_belongs_to_subnet?
    return if subnet.nil? or ip.nil?
    return unless dhcp?
    diff --git a/app/models/orchestration/dns.rb b/app/models/orchestration/dns.rb
    index 1c91d26…a42fe5a 100644
    — a/app/models/orchestration/dns.rb
    +++ b/app/models/orchestration/dns.rb
    @@ -2,7 +2,7 @@ module Orchestration::DNS
    def self.included(base)
    base.send :include, InstanceMethods
    base.class_eval do
  •  after_validation :validate_dns, :queue_dns

  •  after_validation :validate_dns, :queue_remove_dns_conflicts, :queue_dns
 before_destroy :queue_dns_destroy

    end
    end
    @@ -10,7 +10,7 @@ module Orchestration::DNS
    module InstanceMethods

    def dns?

  •  !domain.nil? and !domain.proxy.nil? and errors.empty?

  •  !domain.nil? and !domain.proxy.nil?

    end

    def dns_a_record
    @@ -29,23 +29,41 @@ module Orchestration::DNS
    dns_a_record.create
    end

  • def set_conflicting_dns_a_record

  •  dns_a_record.conflicts[0].create

  • end

  • def set_dns_ptr_record
    dns_ptr_record.create
    end

  • def set_conflicting_dns_ptr_record

  •  dns_ptr_record.conflicts[0].create

  • end

  • def del_dns_a_record
    dns_a_record.destroy
    end

  • def del_conflicting_dns_a_record

  •  dns_a_record.conflicts[0].destroy

  • end

  • def del_dns_ptr_record
    dns_ptr_record.destroy
    end

  • def del_conflicting_dns_ptr_record

  •  dns_ptr_record.conflicts[0].destroy

  • end

  • def validate_dns
    return unless dns?
    return unless new_record?

  •  failure("#{hostname} A record is already in DNS") if dns_a_record.conflicting?

  •  failure("#{ip} PTR is already in DNS")            if dns_ptr_record.conflicting?

  •  return if overwrite?

  •  status = true

  •  (status =  failure("- #{dns_a_record.conflicts[0]} exists in DNS ", nil, :conflict))  if dns_a_record.conflicting?

  •  (status = (failure("- #{dns_ptr_record.conflicts[0]} exists in DNS", nil, :conflict)) and status) if dns_ptr_record.conflicting?

    end

    def dns_record_attrs
    @@ -60,6 +78,7 @@ module Orchestration::DNS
    end

    def queue_dns_create

  •  logger.debug "Scheduling new DNS entries"
 queue.create(:name   => "DNS record for #{self}", :priority => 3,
              :action => [self, :set_dns_a_record])
 queue.create(:name   => "Reverse DNS record for #{self}", :priority => 3,

@@ -86,5 +105,15 @@ module Orchestration::DNS
:action => [self, :del_dns_ptr_record])
end

  • def queue_remove_dns_conflicts
  •  return unless dns? and errors.empty?

  •  return unless overwrite?

  •  logger.debug "Scheduling DNS conflict removal"

  •  queue.create(:name   => "Remove conflicting DNS record for #{self}", :priority => 1,

  •               :action => [self, :del_conflicting_dns_a_record])

  •  queue.create(:name   => "Remove conflicting Reverse DNS record for #{self}", :priority => 1,

  •               :action => [self, :del_conflicting_dns_ptr_record])
  • end
    end
    end
    diff --git a/app/views/hosts/_form.html.erb b/app/views/hosts/_form.html.erb
    index 8041c74…5e8bf9b 100644
    — a/app/views/hosts/_form.html.erb
    +++ b/app/views/hosts/_form.html.erb
    @@ -66,6 +66,6 @@
    <%= textarea_f f, :comment, :help_block => “Additional information about this host”, :class => “xxlarge”, :rows => “3” %>

  • <%= submit_or_cancel f %>
  • <%= f.hidden_field :overwrite? %>
  • <%= submit_or_cancel f, @host.overwrite? %>
    <% end %>
    diff --git a/lib/core_extensions.rb b/lib/core_extensions.rb
    index 12e6fd6…926e726 100644
    — a/lib/core_extensions.rb
    +++ b/lib/core_extensions.rb
    @@ -86,3 +86,9 @@ class String
    end
    end
    end

+class ActiveModel::Errors

  • def are_all_conflicts?
  • self[:conflict].count == self.count
  • end
    +end
    \ No newline at end of file
    diff --git a/lib/net.rb b/lib/net.rb
    index 698010f…6b0e8ac 100644
    — a/lib/net.rb
    +++ b/lib/net.rb
    @@ -10,6 +10,9 @@ module Net
    opts.each do |k,v|
    eval(“self.#{k}= v”) if self.respond_to?("#{k}=")
    end if opts
  •  raise Net::LeaseConflict.new("#{self.mac}/#{self.ip}") if opts['state']

  •  self.logger ||= Rails.logger
 raise "Must define a hostname" if hostname.blank?
 raise "Must define a proxy"    if proxy.nil?

@@ -42,4 +45,6 @@ module Net
class Conflict < Exception
attr_accessor :type, :expected, :actual, :message
end
+

  • class LeaseConflict < RuntimeError; end
    end
    diff --git a/lib/net/dhcp/record.rb b/lib/net/dhcp/record.rb
    index d6d1df0…1abbecc 100644
    — a/lib/net/dhcp/record.rb
    +++ b/lib/net/dhcp/record.rb
    @@ -31,7 +31,7 @@ module Net::DHCP
    e.type = "dhcp"
    e.expected = to_s
    e.actual = conflicts
  •    e.message  = "DHCP conflict detected - expected #{to_s}, found #{conflicts.map(&:to_s).join(', ')}"

  •    e.message  = "in DHCP detected - expected #{to_s}, found #{conflicts.map(&:to_s).join(', ')}"
   raise e
 end
    end
    diff --git a/lib/net/dns.rb b/lib/net/dns.rb
    index 5184459…7d8b2b1 100644
    — a/lib/net/dns.rb
    +++ b/lib/net/dns.rb
    @@ -45,10 +45,6 @@ module Net
    raise “Must provide a DNS resolver” unless resolver.is_a?(Resolv::DNS)
    end
  •  def to_s

  •    "#{hostname}/#{ip}"

  •  end

  •  def destroy
   logger.info "Delete the DNS #{type} record for #{to_s}"
 end

@@ -73,7 +69,7 @@ module Net
e.type = "dns"
e.expected = to_s
e.actual = conflicts

  •    e.message  = "DNS conflict detected - expected #{to_s}, found #{conflicts.map(&:to_s).join(', ')}"

  •    e.message  = "in DNS detected - expected #{to_s}, found #{conflicts.map(&:to_s).join(', ')}"
   e
 end

diff --git a/lib/net/dns/a_record.rb b/lib/net/dns/a_record.rb
index bb61119…98e1a45 100644
— a/lib/net/dns/a_record.rb
+++ b/lib/net/dns/a_record.rb
@@ -7,6 +7,10 @@ module Net
@type = "A"
end

  •  def to_s

  •    "#{hostname}/#{ip}"

  •  end

  •  def destroy
   super
   proxy.delete(hostname)

@@ -24,7 +28,7 @@ module Net
@conflicts ||= [dns_lookup(hostname)].delete_if { |c| c == self }.compact
end

  •  # Verifies that are record already exists on the dhcp server

  •  # Verifies that a record already exists on the dns server
 def valid?
   self == dns_lookup(hostname)
 end

diff --git a/lib/net/dns/ptr_record.rb b/lib/net/dns/ptr_record.rb
index a9a8099…44b5ad1 100644
— a/lib/net/dns/ptr_record.rb
+++ b/lib/net/dns/ptr_record.rb
@@ -6,6 +6,10 @@ module Net
@type = "PTR"
end

  •  def to_s

  •    "#{ip}/#{hostname}"

  •  end

  •  def destroy
   super
   proxy.delete(to_arpa)

@@ -23,7 +27,7 @@ module Net
@conflicts ||= [dns_lookup(ip)].delete_if{|c| c == self}.compact
end

  •  # Verifies that are record already exists on the dhcp server

  •  # Verifies that a record already exists on the dns server
 def valid?
   logger.debug "Fetching DNS reservation for #{to_s}"
   self == dns_lookup(ip)

diff --git a/test/test_helper.rb b/test/test_helper.rb
index 615ad5f…fb3130e 100644
— a/t