-
Notifications
You must be signed in to change notification settings - Fork 85
desginate: Add mdns as hidden master #2105
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
base: master
Are you sure you want to change the base?
Conversation
This reverts commit 64e7ad9. We've are very close to the release and have multiple pull requests still open for Designate: * crowbar#2095 * crowbar#2104 * crowbar#2105 * SUSE-Cloud/doc-cloud#931 There are way too many question marks on this so I am switching the barclamp back to invisible mode. Lets get these pull requests squared away carefully and without the rush of imminent release.
cmurphy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is not really required but only eases out the config from
an admin perspective.
If this isn't critical maybe it could wait till the MU?
| pools = [{ | ||
| "name" => "default-bind", | ||
| "description" => "Default BIND9 Pool", | ||
| "description" => "Sample Pool for designate (relies only 1 dns-master)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand this correctly, the user doesn't have control over this from the barclamp side so they are stuck with this "sample" in their configuration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is just a sample file, the user has to edit this file and inject this pool in designate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can the user edit this file if it's being controlled by crowbar? Chef will just overwrite it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can create your own pool rather than the default pool. I think the tricky part here is that you would need to change the pool to use in the designate config. that isn't done yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can the user edit this file if it's being controlled by crowbar? Chef will just overwrite it.
Chef will overwrite file /etc/designate/pools.crowbar.yaml designate relies on /etc/designate/pools.yaml so the user can overwrite the real file or just pass that config to the designate with command documented here SUSE-Cloud/doc-cloud#909
|
This code change has no impact on any part of crowbar apart from changing the sample file. But helps greatly in trying to understand what this file really means. |
╰─ irb
irb(main):001:0> dnsmaster='123.123.123.123'
=> "123.123.123.123"
irb(main):002:0> dns = {:fqdn => 'me.exmaple.com' }
=> {:fqdn=>"me.exmaple.com"}
irb(main):003:0> network_settings = {:mdns_bind_host => dnsmaster }
=> {:mdns_bind_host=>"123.123.123.123"}
irb(main):004:0> pools = [{
irb(main):005:2* "name" => "default-bind",
irb(main):006:2* "description" => "Sample Pool for designate (relies only 1 dns-master)",
irb(main):007:2* "id" => "794ccc2c-d751-44fe-b57f-8894c9f5c842",
irb(main):008:2* "attributes" => {},
irb(main):009:2* "ns_records" => [{ "hostname" => "#{dns[:fqdn]}.", "priority" => 1 }],
irb(main):010:2* "nameservers" => [{ "host" => dnsmaster, "port" => 53 }],
irb(main):011:2* "also_notifies" => [],
irb(main):012:2* "targets" => [{
irb(main):013:4* "type" => "bind9",
irb(main):014:4* "description" => "BIND9 Server 1",
irb(main):015:4* "masters" => [{ "host" => network_settings[:mdns_bind_host], "port" => 5354 }],
irb(main):016:4* "options" => {
irb(main):017:5* "host" => dnsmaster,
irb(main):018:5* "port" => 53,
irb(main):019:5* "rndc_host" => dnsmaster,
irb(main):020:5* "rndc_port" => 953,
irb(main):021:5* "rndc_key_file" => "/etc/designate/rndc.key"
irb(main):022:5> }
irb(main):023:4> }]
irb(main):024:2> }]
=> [{"name"=>"default-bind", "description"=>"Sample Pool for designate (relies only 1 dns-master)", "id"=>"794ccc2c-d751-44fe-b57f-8894c9f5c842", "attributes"=>{}, "ns_records"=>[{"hostname"=>"me.exmaple.com.", "priority"=>1}], "nameservers"=>[{"host"=>"123.123.123.123", "port"=>53}], "also_notifies"=>[], "targets"=>[{"type"=>"bind9", "description"=>"BIND9 Server 1", "masters"=>[{"host"=>"123.123.123.123", "port"=>5354}], "options"=>{"host"=>"123.123.123.123", "port"=>53, "rndc_host"=>"123.123.123.123", "rndc_port"=>953, "rndc_key_file"=>"/etc/designate/rndc.key"}}]}]
(failed reverse-i-search)`': import^C
irb(main):025:0> require 'yaml'
=> true
irb(main):026:0> pools.to_yaml
=> "---\n- name: default-bind\n description: Sample Pool for designate (relies only 1 dns-master)\n id: 794ccc2c-d751-44fe-b57f-8894c9f5c842\n attributes: {}\n ns_records:\n - hostname: me.exmaple.com.\n priority: 1\n nameservers:\n - host: 123.123.123.123\n port: 53\n also_notifies: []\n targets:\n - type: bind9\n description: BIND9 Server 1\n masters:\n - host: 123.123.123.123\n port: 5354\n options:\n host: 123.123.123.123\n port: 53\n rndc_host: 123.123.123.123\n rndc_port: 953\n rndc_key_file: \"/etc/designate/rndc.key\"\n" |
315171a to
2e89f84
Compare
|
this change is now needed for HA, I have added comments to the code and update the commit message to reflect why this change is required. |
guangyee
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code looks good
2e89f84 to
3fe0370
Compare
|
Rebased with HA fix |
This reverts commit 64e7ad9. We've are very close to the release and have multiple pull requests still open for Designate: * crowbar#2095 * crowbar#2104 * crowbar#2105 * SUSE-Cloud/doc-cloud#931 There are way too many question marks on this so I am switching the barclamp back to invisible mode. Lets get these pull requests squared away carefully and without the rush of imminent release.
3fe0370 to
c61534d
Compare
|
|
||
| network_settings = DesignateHelper.network_settings(node) | ||
| # hidden masters are designate-mdns services, in ha this service will be running on multiple | ||
| # hosts and any host can be asked to update the zone (when a recordset, corsspoding to a vm is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be nice to correct the typo here and in the commit message ("corsspoding")
| network_settings = DesignateHelper.network_settings(node) | ||
| # hidden masters are designate-mdns services, in ha this service will be running on multiple | ||
| # hosts and any host can be asked to update the zone (when a recordset, corsspoding to a vm is | ||
| # created) on th real-master so all have to be listed as master in the pool. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and here ("th") and in the commit message
400008a to
03a5d0b
Compare
dirkmueller
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please at least make the list of nameservers expand all hosts again. Ideally the hostname ns_record would be configurable (as it needs to point to the public hostname), but thats not a new bug introduced here
| pools = [{ | ||
| "name" => "default-bind", | ||
| "description" => "Default BIND9 Pool", | ||
| "description" => "Sample Pool for designate", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this change? IMHO it should be the default pool, this matches the name line above.
| "description" => "Sample Pool for designate", | ||
| "id" => "794ccc2c-d751-44fe-b57f-8894c9f5c842", | ||
| "attributes" => {}, | ||
| "ns_records" => [{ "hostname" => "#{dns[:fqdn]}.", "priority" => 1 }], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would need to be configurable in the barclamp.
| "ns_records" => [{ "hostname" => "#{dns[:fqdn]}.", "priority" => 1 }], | ||
| "nameservers" => dnsservers.map { |ip| { "host" => ip, "port" => 53 } }, | ||
| "also_notifies" => dnsslaves.map { |ip| { "host" => ip, "port" => 53 } }, | ||
| "nameservers" => [{ "host" => dnsmaster, "port" => 53 }], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this part doesn't make a whole lot of sense to me. this way we have only one DNS server hosting the domain, with other words no High Availability (HA is a requirement, see https://jira.suse.com/browse/SOC-6361 ). We need to list all dns-server masters here, and ideally their public IP address (see SOC-9635 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nameservers is a list where designate will check if the zone and/or ns-record is created successfully, I have limited it to 1 server and that too dns-master. because ....
The DNS barclamp has only 1 dns-server as master. All others are slaves who just forward incoming request to that master (this how bind is configured). This dns-master stores all the zone and ns-records received from designate.
These slaves could also be part of nameservers where designate validates the creation of zones and ns-records, but that wasnt very useful since they are just going to forwards query to dns-master.
| "nameservers" => dnsservers.map { |ip| { "host" => ip, "port" => 53 } }, | ||
| "also_notifies" => dnsslaves.map { |ip| { "host" => ip, "port" => 53 } }, | ||
| "nameservers" => [{ "host" => dnsmaster, "port" => 53 }], | ||
| "also_notifies" => [], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leaving this empty by default makes sense to me, however if this is integrated in another hidden master setup (like e.g. SUSE's internal DNS), you need to be able to add additional hosts here. ideally this would be a setting in the barclamp ui.
|
|
||
| # the host with VIP will use that ip as the outgoing ip when connecting to the real-master | ||
| if node[:designate][:ha][:enabled] | ||
| hiddenmasters += [{ "host" => CrowbarPacemakerHelper.cluster_vip(node, "admin"), "port" => 5354 }] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is already in the list above, right? just once via vip and once via the node ip. I don't understand why we need to add it twice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
afaik Barclamp::Inventory.get_network_by_type does not return the VIP so the list has admin address,
Also iiuc the that is different from VIP, and the host with VIP will use that ip as the outgoing ip when connecting to the real-master.
|
Addressing the open review comments in the next commit. Note, "nameservers" will be updated via SCRD-9636 to use public FQDNs. |
03a5d0b to
82ff9bb
Compare
82ff9bb to
081e89f
Compare
b90aeaf to
4757bda
Compare
ritesh216
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WIP
In crowbar's world the dns-master is the master of all and slaves forward all queries to dns-master. When designate is enabled, designate-mdns service component(s) become the hidden master(s). We leave 'also_notifies' empty. It can be configured by the users via designate-manage utility as and when needed. designate-mdns service components, in HA, will be running on multiple hosts and any host can be asked to update a zone on th real-master. We add the cluster vip as the hidden master in case HA is enabled otherwise add all servers.
4757bda to
a8f2c73
Compare
in crowbar's world the dns-master is the master of all and slaves
forward all queries to dns-master. Under such a configuration there is
no need to query other nameservers as they still forward the query to
dns-master. So designate can just verify on one nameserver(dns-master)
and dns-master will take care of passing that info to all slaves.
Same goes for also_notifies: dns-master will notify all slaves in case
of either zone or recordset is updated/deleted.
this also further simplifies the designate pool config reducing the
time required to create zone and recordsets.
This change is not really required but only eases out the config from
an admin perspective.
Also having multiple nameservers confuses designate in some times as
according to these nameserver designate is not authoritative of these
zones and recordsets.