Fixes #16105 - Force DNS rebuild when provisioning discovered host #297

imriz · 2016-08-16T08:22:59Z

No description provided.

imriz · 2016-08-16T11:08:01Z

[test]

lzap · 2016-08-18T16:20:12Z

Will take a look tomorrow, thanks!

lzap · 2016-08-18T16:26:26Z

app/controllers/discovered_hosts_controller.rb

@@ -73,6 +73,7 @@ def update
  def perform_update(host, success_message = nil)
    Taxonomy.no_taxonomy_scope do
      ::ForemanDiscovery::HostConverter.set_build_clean_facts(host)
+      @host.primary_interface.discovery_queue_rebuild_dns unless @host.primary_interface.pending_dns_record_changes?


This code will not work with auto-provisioning, make sure it work in both ways. Anyways.

I think instead of calling this explicitly, we should create new after_validation :queue_dns_discovery hook. In the hook method, check if we are converting from Discovered, like we do in other hooks, then queue it when necessary.

Add tests for normal and autoprovisioning please. And make the commit message shorter :-)

lzap · 2016-08-18T16:31:09Z

Example:

module MyExtensions
  extend ActiveSupport::Concern

  included do
    after_validation :queue_dns_rebuild
  end

  def queue_dns_rebuild
    return unless errors.empty? && !Setting[:discovery_always_rebuild_dns]
    return if new_record? # Discovered Hosts already exist, and new_records will break `find`
    return unless type_changed? and ::Host::Base.find(self.id).type == "Host::Discovered"

    # do your stuff here
  end

lzap · 2016-08-18T16:33:33Z

The code could be perhaps part of discovery's Host::ManagedExtensions I think. Just another method I believe.

imriz · 2016-08-19T09:11:25Z

The code could be perhaps part of discovery's Host::ManagedExtensions I think. Just another method I believe.

Mmm, maybe the same convention as the other orchestration concerns? Orchestration::Discovery::DnsExtensions or something like that, or, since it belongs to the Nic model, Nic::ManagedExtensions?

imriz · 2016-08-21T08:06:50Z

@lzap updated the PR.

lzap · 2016-08-24T14:30:39Z

app/models/setting/discovered.rb

@@ -31,6 +31,7 @@ def self.load_defaults
        self.set('discovery_facts_ipmi', N_("Regex to organize facts for ipmi section"), "", N_("IPMI facts")),
        self.set('discovery_lock', N_("Automatically generate PXE configuration to pin a newly discovered host to discovery"), false, N_("Lock PXE")),
        self.set('discovery_lock_template', N_("PXE template to be used when pinning a host to discovery"), 'pxelinux_discovery', N_("Locked template name"),nil,{ :collection => Proc.new {Hash[ProvisioningTemplate.all.map{|template| [template[:name], template[:name]]}]} }),
+        self.set('discovery_always_rebuild_dns', N_("Force DNS entries creation when provisioning discovered host"), true, N_("Force DNS")),


Looks great. Why this is enabled by default? Normally, discovered hosts should not be added to DNS, thus I'd set this to false.

This only affects when you provision the host, not when it enters discovery, and in that case you always expect DNS to be created.

This only affects when you provision the host, not when it enters discovery, and in that case you always expect DNS to be created.

Disregard my comment, this should be turned on by default.

Later,
Lukas #lzap Zapletal

lzap · 2016-08-24T14:44:29Z

What a coincidence, I just accepted my very first MyHeritage Instant Discovery e-mail when I noticed your domain. I am newcomer there. I was wondering if you could update http://projects.theforeman.org/projects/foreman/wiki/Who_Uses_Foreman (if that's valid and appropriate).

Very nice patch indeed. Please do my smallish comments and I will test and merge it later this week.

imriz · 2016-08-24T15:19:56Z

I was wondering if you could update http://projects.theforeman.org/projects/foreman/wiki/Who_Uses_Foreman (if that's valid and appropriate).

Sure! will do :)

imriz · 2016-08-25T09:25:09Z

@lzap PR updated per your comments, and I've updated the "Who_Uses_Foreman" :)

Thanks!!

lzap · 2016-08-26T12:57:06Z

It work fine, in my environment I see:

2016-08-26T14:38:42 2576109c [app] [I] Create DHCP reservation for x525400e94080.local.lan-52:54:00:e9:40:80/192.168.122.101
2016-08-26T14:38:42 2576109c [app] [D] DHCP reservation on net 192.168.122.0 with attrs: {:hostname=>"x525400e94080.local.lan", :mac=>"52:54:00:e9:40:80", :ip=>"192.168.122.101", :network=>"192.168.122.0", :nextServer=>"192.168.122.1", :filename=>"pxelinux.0"}
2016-08-26T14:38:42 2576109c [app] [I] Delete the DNS A record for x525400e94080.local.lan/192.168.122.101
2016-08-26T14:38:42 2576109c [app] [I] Add DNS A record for x525400e94080.local.lan/192.168.122.101
2016-08-26T14:38:42 2576109c [app] [I] Delete the DNS PTR record for 192.168.122.101/x525400e94080.local.lan
2016-08-26T14:38:42 2576109c [app] [D] Fetching DNS reservation for 192.168.122.101/x525400e94080.local.lan
2016-08-26T14:38:42 2576109c [app] [I] Add DNS PTR record for 192.168.122.101/x525400e94080.local.lan

The initial DNS deletion failed on proxy and returned 404 but this was not reason for rollback and the transaction continued. Which brings this question - do we want really to rebuild DNS (delete/add operations) or simply build DNS (add operation only)? I lean towards the latter to prevent such 404 errors. Or both maybe (make it a three state flag), because I would like to implement naming discovered hosts using the "deacon" gem we recently added (http://projects.theforeman.org/issues/16330).

Explain me your use case - why you prefer rebuilding instead of building DNS entry? Do you create DNS name during discovery phase somehow?

imriz · 2016-08-26T13:07:43Z

No specific reason. I used it because it is safe, and can overcome stale
records, if such exists. I don't mind changing to add only if you prefer.

On Aug 26, 2016 3:57 PM, "Lukáš Zapletal" notifications@github.com wrote:

It work fine, in my environment I see:

2016-08-26T14:38:42 2576109c [app] [I] Create DHCP reservation for x525400e94080.local.lan-52:54:00:e9:40:80/192.168.122.101
2016-08-26T14:38:42 2576109c [app] [D] DHCP reservation on net 192.168.122.0 with attrs: {:hostname=>"x525400e94080.local.lan", :mac=>"52:54:00:e9:40:80", :ip=>"192.168.122.101", :network=>"192.168.122.0", :nextServer=>"192.168.122.1", :filename=>"pxelinux.0"}
2016-08-26T14:38:42 2576109c [app] [I] Delete the DNS A record for x525400e94080.local.lan/192.168.122.101
2016-08-26T14:38:42 2576109c [app] [I] Add DNS A record for x525400e94080.local.lan/192.168.122.101
2016-08-26T14:38:42 2576109c [app] [I] Delete the DNS PTR record for 192.168.122.101/x525400e94080.local.lan
2016-08-26T14:38:42 http://192.168.122.101/x525400e94080.local.lan2016-08-26T14:38:42 2576109c [app] [D] Fetching DNS reservation for 192.168.122.101/x525400e94080.local.lan
2016-08-26T14:38:42 http://192.168.122.101/x525400e94080.local.lan2016-08-26T14:38:42 2576109c [app] [I] Add DNS PTR record for 192.168.122.101/x525400e94080.local.lan

The initial DNS deletion failed on proxy and returned 404 but this was not
reason for rollback and the transaction continued. Which brings this
question - do we want really to rebuild DNS (delete/add operations) or
simply build DNS (add operation only)? I lean towards the latter to
prevent such 404 errors. Or both maybe (make it a three state flag),
because I would like to implement naming discovered hosts using the
"deacon" gem we recently added (http://projects.theforeman.
org/issues/16330).

Explain me your use case - why you prefer rebuilding instead of building
DNS entry? Do you create DNS name during discovery phase somehow?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#297 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFcUWyDCZY2J40bu-1HE-M-CBFHwK5iLks5qjuKjgaJpZM4JlJXe
.

lzap · 2016-08-29T14:09:25Z

Merged as fe52972, thank you!

theforeman-bot added Needs testing Not yet reviewed labels Aug 16, 2016

imriz force-pushed the 16105 branch 3 times, most recently from 2cc8bc6 to 4f76415 Compare August 17, 2016 09:40

lzap reviewed Aug 18, 2016
View reviewed changes

lzap changed the title ~~Fixes #16105 - Force DNS rebuild when provisioning discovered host, even if it appears to have no DNS changes pending~~ Fixes #16105 - Force DNS rebuild when provisioning discovered host Aug 18, 2016

imriz force-pushed the 16105 branch from 4f76415 to d18b44a Compare August 21, 2016 08:05

imriz force-pushed the 16105 branch from d18b44a to 76d8f71 Compare August 21, 2016 09:58

lzap reviewed Aug 24, 2016
View reviewed changes

Fixes #16105 - Force DNS rebuild when provisioning discovered host

1db5533

imriz force-pushed the 16105 branch from 76d8f71 to 1db5533 Compare August 25, 2016 09:22

lzap closed this Aug 29, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #16105 - Force DNS rebuild when provisioning discovered host #297

Fixes #16105 - Force DNS rebuild when provisioning discovered host #297

imriz commented Aug 16, 2016

imriz commented Aug 16, 2016

lzap commented Aug 18, 2016

lzap Aug 18, 2016

lzap commented Aug 18, 2016

lzap commented Aug 18, 2016

imriz commented Aug 19, 2016 •

edited

Loading

imriz commented Aug 21, 2016

lzap Aug 24, 2016

imriz Aug 24, 2016

lzap Aug 25, 2016

lzap commented Aug 24, 2016

imriz commented Aug 24, 2016

imriz commented Aug 25, 2016

lzap commented Aug 26, 2016

imriz commented Aug 26, 2016

lzap commented Aug 29, 2016

Fixes #16105 - Force DNS rebuild when provisioning discovered host #297

Fixes #16105 - Force DNS rebuild when provisioning discovered host #297

Conversation

imriz commented Aug 16, 2016

imriz commented Aug 16, 2016

lzap commented Aug 18, 2016

lzap Aug 18, 2016

Choose a reason for hiding this comment

lzap commented Aug 18, 2016

lzap commented Aug 18, 2016

imriz commented Aug 19, 2016 • edited Loading

imriz commented Aug 21, 2016

lzap Aug 24, 2016

Choose a reason for hiding this comment

imriz Aug 24, 2016

Choose a reason for hiding this comment

lzap Aug 25, 2016

Choose a reason for hiding this comment

lzap commented Aug 24, 2016

imriz commented Aug 24, 2016

imriz commented Aug 25, 2016

lzap commented Aug 26, 2016

imriz commented Aug 26, 2016

lzap commented Aug 29, 2016

imriz commented Aug 19, 2016 •

edited

Loading