On 9.12.2015 13:37, David Kupka wrote:
> On 08/12/15 15:24, Petr Spacek wrote:
>> On 8.12.2015 12:19, David Kupka wrote:
>>> On 08/12/15 08:56, Petr Spacek wrote:
>>>> On 7.12.2015 14:41, David Kupka wrote:
>>>>> +def is_host_resolvable(fqdn):
>>>>> +    if not isinstance(fqdn, DNSName):
>>>>> +        fqdn = DNSName(fqdn)
>>>>> +    for rdtype in (rdatatype.A, rdatatype.AAAA):
>>>>> +        try:
>>>>> +            resolver.query(fqdn.make_absolute(), rdtype)
>>>>> +        except DNSException:
>>>>> +            continue
>>>>> +        else:
>>>>> +            return True
>>>>> +
>>>>> +    return False
>>>>>
>>>>
>>>> NACK, you are re-introducing duplicate function which was removed in
>>>> 498471e4aed1367b72cd74d15811d0584a6ee268.
>>>>
>>>> Please amend the patch ASAP to use new verify_host_resolvable() function 
>>>> so I
>>>> can test it and get it into 4.3.
>>>>
>>> Updated patches attached.
>>
>> Hmm, my review script screams:
>>
>> Detected base branch:   origin/master
>> Checks will be made against following base commit: 848912a add missing
>> /ipaplatform/constants.py to .gitignore
>> Writing API to API.txt
>> ./ipalib/plugins/dns.py:2127:9: E124 closing bracket does not match visual
>> indentation
>> ./ipalib/plugins/dns.py:2711:13: E128 continuation line under-indented for
>> visual indent
>> ./ipalib/plugins/dns.py:2713:9: E124 closing bracket does not match visual
>> indentation
>> ./ipalib/plugins/dns.py:2716:13: E128 continuation line under-indented for
>> visual indent
>> ./ipapython/ipautil.py:928:1: E302 expected 2 blank lines, found 1
>> ./ipapython/ipautil.py:948:17: E128 continuation line under-indented for
>> visual indent
>> ./ipapython/ipautil.py:956:1: E302 expected 2 blank lines, found 1
>> ./ipapython/ipautil.py:997:80: E501 line too long (83 > 79 characters)
>> ./ipapython/ipautil.py:998:80: E501 line too long (83 > 79 characters)
>> ./ipaserver/install/bindinstance.py:49:9: E128 continuation line
>> under-indented for visual indent
>> ./ipaserver/install/bindinstance.py:292:80: E501 line too long (80 > 79
>> characters)
>> ./ipaserver/install/bindinstance.py:438:19: E128 continuation line
>> under-indented for visual indent
>> ./ipaserver/install/server/common.py:271:63: E261 at least two spaces before
>> inline comment
>> ./ipaserver/install/server/common.py:271:80: E501 line too long (90 > 79
>> characters)
>> ERROR: PEP8 --diff failed, continuing ...
>> ERROR: API.txt was changed without a change in VERSION, continuing ...
>> Summary of detected errors:
>>   ERROR: PEP8 --diff failed
>>   ERROR: API.txt was changed without a change in VERSION
>>
>> Please fix it :-)
>>
>> Make make this more pleasant for you, you can use (and review) my attached
>> patch. It adds 'review' target to Makefile, it will do the same checks as I 
>> do.
>>
> 
> Unfortunately your tool does not work for me (output w/ -o xtrace attached).
> Anyway I have incremented API version and fixed most of the pep8 errors except
> for E124 twice in ipalib/plugins/dns.py as it seems to be convention in the
> file and E501 twice in ipapython/ipautil.py because IMO breaking string
> constant into multiple lines does not help readability.
> 
> Updated patches also attached.

We are almost there, but NACK for now.

1) Following attempt to install IPA explodes. Please note that
dom-245.idm.lab.eng.brq.redhat.com DNS domain is delegated to the IPA server
before installation is started, so it gives 'timeout' or possibly SERVFAIL.

# ipa-server-install --ds-password=root4lab --admin-password=root4lab
--setup-dns --forwarder=10.38.5.26 --no-reverse --allow-zone-overlap
--domain=dom-245.idm.lab.eng.brq.redhat.com
--realm=DOM-245.IDM.LAB.ENG.BRQ.REDHAT.COM
[...]

Configuring DNS (named)
  [1/11]: generating rndc key file
  [2/11]: adding DNS container
  [3/11]: setting up our zone
  [error] InvocationError: DNS check for domain
dom-245.idm.lab.eng.brq.redhat.com. failed: The DNS operation timed out after
30.000453949 seconds. Please make sure that the domain is properly delegated
to this IPA server.
ipa.ipapython.install.cli.install_tool(Server): ERROR    DNS check for domain
dom-245.idm.lab.eng.brq.redhat.com. failed: The DNS operation timed out after
30.000453949 seconds. Please make sure that the domain is properly delegated
to this IPA server.
ipa.ipapython.install.cli.install_tool(Server): ERROR    The
ipa-server-install command failed. See /var/log/ipaserver-install.log for more
information

2015-12-09T17:15:37Z DEBUG   File
"/usr/lib/python2.7/site-packages/ipapython/admintool.py", line 171, in execute
    return_value = self.run()
  File "/usr/lib/python2.7/site-packages/ipapython/install/cli.py", line 318,
in run
    cfgr.run()
  File "/usr/lib/python2.7/site-packages/ipapython/install/core.py", line 310,
in run
    self.execute()
  File "/usr/lib/python2.7/site-packages/ipapython/install/core.py", line 332,
in execute
    for nothing in self._executor():
  File "/usr/lib/python2.7/site-packages/ipapython/install/core.py", line 372,
in __runner
    self._handle_exception(exc_info)
  File "/usr/lib/python2.7/site-packages/ipapython/install/core.py", line 394,
in _handle_exception
    six.reraise(*exc_info)
  File "/usr/lib/python2.7/site-packages/ipapython/install/core.py", line 362,
in __runner
    step()
  File "/usr/lib/python2.7/site-packages/ipapython/install/core.py", line 359,
in <lambda>
    step = lambda: next(self.__gen)
  File "/usr/lib/python2.7/site-packages/ipapython/install/util.py", line 81,
in run_generator_with_yield_from
    six.reraise(*exc_info)
  File "/usr/lib/python2.7/site-packages/ipapython/install/util.py", line 59,
in run_generator_with_yield_from
    value = gen.send(prev_value)
  File "/usr/lib/python2.7/site-packages/ipapython/install/core.py", line 571,
in _configure
    next(executor)
  File "/usr/lib/python2.7/site-packages/ipapython/install/core.py", line 372,
in __runner
    self._handle_exception(exc_info)
  File "/usr/lib/python2.7/site-packages/ipapython/install/core.py", line 449,
in _handle_exception
    self.__parent._handle_exception(exc_info)
  File "/usr/lib/python2.7/site-packages/ipapython/install/core.py", line 394,
in _handle_exception
    six.reraise(*exc_info)
  File "/usr/lib/python2.7/site-packages/ipapython/install/core.py", line 446,
in _handle_exception
    super(ComponentBase, self)._handle_exception(exc_info)
  File "/usr/lib/python2.7/site-packages/ipapython/install/core.py", line 394,
in _handle_exception
    six.reraise(*exc_info)
  File "/usr/lib/python2.7/site-packages/ipapython/install/core.py", line 362,
in __runner
    step()
  File "/usr/lib/python2.7/site-packages/ipapython/install/core.py", line 359,
in <lambda>
    step = lambda: next(self.__gen)
  File "/usr/lib/python2.7/site-packages/ipapython/install/util.py", line 81,
in run_generator_with_yield_from
    six.reraise(*exc_info)
  File "/usr/lib/python2.7/site-packages/ipapython/install/util.py", line 59,
in run_generator_with_yield_from
    value = gen.send(prev_value)

  File "/usr/lib/python2.7/site-packages/ipapython/install/common.py", line
63, in _install
    for nothing in self._installer(self.parent):
  File "/usr/lib/python2.7/site-packages/ipaserver/install/server/install.py",
line 1471, in main
    install(self)
  File "/usr/lib/python2.7/site-packages/ipaserver/install/server/install.py",
line 265, in decorated
    func(installer)
  File "/usr/lib/python2.7/site-packages/ipaserver/install/server/install.py",
line 958, in install
    dns.install(False, False, options)
  File "/usr/lib/python2.7/site-packages/ipaserver/install/dns.py", line 322,
in install
    bind.create_instance()
  File "/usr/lib/python2.7/site-packages/ipaserver/install/bindinstance.py",
line 680, in create_instance
    self.start_creation()
  File "/usr/lib/python2.7/site-packages/ipaserver/install/service.py", line
447, in start_creation
    run_step(full_msg, method)
  File "/usr/lib/python2.7/site-packages/ipaserver/install/service.py", line
437, in run_step
    method()
  File "/usr/lib/python2.7/site-packages/ipaserver/install/bindinstance.py",
line 805, in __setup_zone
    ns_hostname=self.api.env.host, force=True, api=self.api)
  File "/usr/lib/python2.7/site-packages/ipaserver/install/bindinstance.py",
line 331, in add_zone
    force=force)
  File "/usr/lib/python2.7/site-packages/ipalib/frontend.py", line 447, in
__call__
    ret = self.run(*args, **options)
  File "/usr/lib/python2.7/site-packages/ipalib/frontend.py", line 764, in run
    return self.execute(*args, **options)
  File "/usr/lib/python2.7/site-packages/ipalib/plugins/dns.py", line 2781, in
execute
    result = super(dnszone_add, self).execute(*keys, **options)
  File "/usr/lib/python2.7/site-packages/ipalib/plugins/baseldap.py", line
1233, in execute
    *keys, **options)
  File "/usr/lib/python2.7/site-packages/ipalib/plugins/dns.py", line 2747, in
pre_callback
    ldap, dn, entry_attrs, attrs_list, *keys, **options)
  File "/usr/lib/python2.7/site-packages/ipalib/plugins/dns.py", line 2153, in
pre_callback
    raise errors.InvocationError(e.message)


2) Please print 'Checking DNS domain <xyz>, please wait ...' when validating
domain parameter in installer. The timeout is 30 seconds and users may be
nervous when the installed blocks for 30 seconds without a visible reason.

Precedent for this is in check_forwarders() in
ipaserver/install/bindinstance.py . Encapsulating check_zone_overlap() in
auxiliary method may be an option.


3) Timeout is a fatal error instead of warning. We have been discussing this
and it should really be just a warning.


4) Nitpicks are attached in .patch file.


5) ipa dnszone-add checks work as expected, good job!


Thank you for patience!

-- 
Petr^2 Spacek
diff --git a/ipalib/plugins/dns.py b/ipalib/plugins/dns.py
index a3dc547..2393bd2 100644
--- a/ipalib/plugins/dns.py
+++ b/ipalib/plugins/dns.py
@@ -2740,6 +2740,7 @@ class dnszone_add(DNSZoneBase_add):
 
         if options.get('force'):
             if options.get('skip_nameserver_check'):
+### what did you mean here?
                 ("'force' is deprecated.")
             options['skip_nameserver_check'] = True
 
diff --git a/ipapython/ipautil.py b/ipapython/ipautil.py
index ca96b20..9f85006 100644
--- a/ipapython/ipautil.py
+++ b/ipapython/ipautil.py
@@ -1006,6 +1006,8 @@ def check_reverse_zone_overlap(zone, raise_on_timeout=True):
         # RFC 7534
         "EMPTY.AS112.ARPA",
     ]
+    # automatic empty zones always exist so checking them is pointless,
+    # do not report them to avoid meaningless error messages
     if zone in automatic_empty_zones:
         return
     check_zone_overlap(zone, raise_on_timeout=raise_on_timeout)
diff --git a/ipaserver/install/bindinstance.py b/ipaserver/install/bindinstance.py
index 84a7023..6b8588f 100644
--- a/ipaserver/install/bindinstance.py
+++ b/ipaserver/install/bindinstance.py
@@ -852,7 +852,7 @@ class BindInstance(service.Service):
             # check if master hostname is resolvable
             try:
                 verify_host_resolvable(fqdn, root_logger)
-            except errors.DNSNotARecordError():
+            except errors.DNSNotARecordError:
                 root_logger.warning("Master FQDN (%s) is not resolvable.",
                                     fqdn)
 
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to