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 of multiple MAC addresses for host declaration and a little fix to failover #11

Merged
merged 4 commits into from
Jun 29, 2015

Conversation

redrampage
Copy link
Contributor

I'm back again with a new PR.

I've run into some problems with static DHCP reservations on bonded interfaces:

  • The resulting MAC address assigned to the bond can be inherited from any slave interface, whichever is first added to the bond.
  • When using iPXE to boot from LACP enabled interfaces it will choose any of the slave interfaces whichever discovered first.

So I wrote a small patch to support assignment of multiple MACs to a single host. It creates a separate host statements for each MAC(because dhcpd wants them to be unique).
If no 'option host-name' is specified in 'host.options' - it is added. This is made for compatibility with 'use-host-decl-names on' dhcpd option.

Also there is a small patch for failover configuration which allows usage of hostnames in inventory file.

And some fixes for group expanding macros.

Fixed primary declaration in failover statements.
Small bugfix in group expanding macros.
@@ -47,16 +47,16 @@ group {
{% endif %}
{% if group.groups is defined and group.groups %}
{% for group in group.groups %}
{{ print.group(group) | indent(8, true) }}
{{ group(group) | indent(8, true) }}
Copy link
Member

Choose a reason for hiding this comment

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

I don't think these changes should be here. print.group() is a recursive call of group() macro from dhcpd.conf.j2 template. Without this you cannot do recursive groups in the configuration file. The same goes for print.subnet() and print.hosts(). You should reverse those changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, actually, on the flat group it gave me an error 'print is undefined'.
Ok, I'll look into it more.

Copy link
Member

Choose a reason for hiding this comment

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

Check if creating nested groups works in dhcpd.conf template, using inventory variables.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, to me it looks like both variants are failing.
I've tried the following empty playbook:

---
- hosts: all
  sudo: yes
  vars:
    dhpcd_probe: False
    dhcpd_groups:
      - comment: "level one - elem one"
        groups: 
          - comment: "level two - emen one"
            options: "#testing"
          - comment: "level two - emen two"
            options: "#testing-2"
      - comment: "level one - elem two"
  roles:
    - role: debops.dhcpd

With old 'print.' statements(original contents) it fails with message:

AnsibleUndefinedVariable: One or more undefined variables: 'print' is undefined

Without it(my flawed patch) on recursive groups it fails with:

AttributeError: 'dict' object has no attribute '__call__'

I guess it's because 'groups' is already defined as local variable in the for loop.
I don't see any solutions except to rename those macros to 'print_group', 'print_host', etc... as they used to be once.

Copy link
Member

Choose a reason for hiding this comment

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

Renaming could be a solution. I'll check the issue in a day or two when I have a chance (holidays).

@redrampage
Copy link
Contributor Author

I've reverted macros names back to long and distinct ones. Also switched DHCP Probe off by default as mentioned in #12 (left lib paths as is, because don't know which ones to use). And finally added default value for default subnet(otherwise task breaks, is there is no default IPv6 defined on host), didn't thought of anything better than loopback for that.

@drybjed
Copy link
Member

drybjed commented Jun 29, 2015

Looks good. I wonder about that IPv6 default value though, how did you get the error. Currently debops.dhcpd is written to manage either DHCPv4 or DHCPv6 server, and it expects the respective network / IP addresses present and configured. In other words, if you don't have IPv6 enabled, how did you get an error from DHCPv6? Unless it's an issue related to debops.dhcpd and debops.subnetwork role order in the main playbook...

@redrampage
Copy link
Contributor Author

When I run setup module on my test host

ansible -m setup -i ./inventory test.host

I get an empty dict in "ansible_default_ipv6", so the dhcpd role basically fails with error "address not defined in ansible_default_ipv6"

@drybjed
Copy link
Member

drybjed commented Jun 29, 2015

Ah, during the Jinja templating phase then? Yeah, I suppose it would... OK, I'll accept ::1 as a default there, we'lll see if there are any unintended consequences.

Thanks for the contribution. :-) I'll update the documentation later, after I'm back from holiday.

drybjed added a commit that referenced this pull request Jun 29, 2015
Support of multiple MAC addresses for host declaration and a little fix to failover
@drybjed drybjed merged commit 6c8afc7 into debops:master Jun 29, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants