[Freeipa-devel] [TEST][Patch 0021] Fixed recent replica installation issues in the lab
Martin Basti
mbasti at redhat.com
Thu Feb 4 13:07:59 UTC 2016
On 04.02.2016 13:49, Oleg Fayans wrote:
> Hi Petr,
>
> An updated patch is attached. Please see my comment inline.
>
> On 02/01/2016 12:47 PM, Petr Spacek wrote:
>> On 1.2.2016 11:52, Oleg Fayans wrote:
>>> Hi Petr,
>>>
>>> Please find the new version of the patch attached. Comments are inline
>>>
>>> On 01/29/2016 11:58 AM, Petr Spacek wrote:
>>>> On 27.1.2016 11:16, Oleg Fayans wrote:
>>>>> Sorry, trailing whitespace detected. This version passes lint
>>>>>
>>>>> On 01/27/2016 09:23 AM, Oleg Fayans wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 01/21/2016 04:41 PM, Petr Spacek wrote:
>>>>>>>>> Hello,
>>>>>>>>>
>>>>>>>>> On 21.1.2016 13:42, Oleg Fayans wrote:
>>>>>>>>>>> freeipa-ofayans-0021-Removed-ip-address-option-from-replica-installation.patch
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> From d7ab06a4dcddb919fda351b983d478f1b6968578 Mon Sep 17 00:00:00 2001
>>>>>>>>>>> From: Oleg Fayans <ofayans at redhat.com>
>>>>>>>>>>> Date: Thu, 21 Jan 2016 13:30:02 +0100
>>>>>>>>>>> Subject: [PATCH] Removed --ip-address option from replica installation
>>>>>>>>>>>
>>>>>>>>>>> Explicitly specifying ip-address of the replica messes up with the current
>>>>>>>>>>> bind-dyndb-ldap logic, causing reverse zone not to be created.
>>>>>>>>>>>
>>>>>>>>>>> Enabled reverse-zone creation for the clients residing in different subnet from
>>>>>>>>>>> master
>>>>>>>>>>> ---
>>>>>>>>>>> ipatests/test_integration/tasks.py | 19 ++++++++++++-------
>>>>>>>>>>> 1 file changed, 12 insertions(+), 7 deletions(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/ipatests/test_integration/tasks.py b/ipatests/test_integration/tasks.py
>>>>>>>>>>> index 6eb55501389c72b4c7aaa599fd4852d7e8f1f3c2..43ef78b0c55deed24a0444f0ac6c38ddb2517481 100644
>>>>>>>>>>> --- a/ipatests/test_integration/tasks.py
>>>>>>>>>>> +++ b/ipatests/test_integration/tasks.py
>>>>>>>>>>> @@ -69,6 +69,8 @@ def prepare_reverse_zone(host, ip):
>>>>>>>>>>> host.run_command(["ipa",
>>>>>>>>>>> "dnszone-add",
>>>>>>>>>>> zone], raiseonerr=False)
>>>>>>>>>>> + return zone
>>>>>>>>>>> +
>>>>>>>>>>>
>>>>>>>>>>> def prepare_host(host):
>>>>>>>>>>> if isinstance(host, Host):
>>>>>>>>>>> @@ -319,11 +321,8 @@ def domainlevel(host):
>>>>>>>>>>> def replica_prepare(master, replica):
>>>>>>>>>>> apply_common_fixes(replica)
>>>>>>>>>>> fix_apache_semaphores(replica)
>>>>>>>>>>> - prepare_reverse_zone(master, replica.ip)
>>>>>>>>>>> - master.run_command(['ipa-replica-prepare',
>>>>>>>>>>> - '-p', replica.config.dirman_password,
>>>>>>>>>>> - '--ip-address', replica.ip,
>>>>>>>>>>> - replica.hostname])
>>>>>>>>>>> + master.run_command(['ipa-replica-prepare', '-p', replica.config.dirman_password,
>>>>>>>>>>> + '--auto-reverse', replica.hostname])
>>>>>>>>> I guess that you will need --ip-address option in cases where master's reverse
>>>>>>>>> record does not exist (yet).
>>>>>>> And yo were right. Fixed
>>>>>>>
>>>>>>>>> I would recommend you to test this in libvirt or somewhere without revere
>>>>>>>>> records, I suspect that it might blow up.
>>>>>>>>>
>>>>>>>>>>> replica_bundle = master.get_file_contents(
>>>>>>>>>>> paths.REPLICA_INFO_GPG_TEMPLATE % replica.hostname)
>>>>>>>>>>> replica_filename = get_replica_filename(replica)
>>>>>>>>>>> @@ -339,8 +338,7 @@ def install_replica(master, replica, setup_ca=True, setup_dns=False,
>>>>>>>>>>> # and replica installation would fail
>>>>>>>>>>> args = ['ipa-replica-install', '-U',
>>>>>>>>>>> '-p', replica.config.dirman_password,
>>>>>>>>>>> - '-w', replica.config.admin_password,
>>>>>>>>>>> - '--ip-address', replica.ip]
>>>>>>>>>>> + '-w', replica.config.admin_password]
>>>>>>>>>>> if setup_ca:
>>>>>>>>>>> args.append('--setup-ca')
>>>>>>>>>>> if setup_dns:
>>>>>>>>>>> @@ -380,6 +378,13 @@ def install_client(master, client, extra_args=()):
>>>>>>>>>>> client.collect_log(paths.IPACLIENT_INSTALL_LOG)
>>>>>>>>>>>
>>>>>>>>>>> apply_common_fixes(client)
>>>>>>>>>>> + # Now, for the situations where a client resides in a different subnet from
>>>>>>>>>>> + # master, we need to explicitly tell master to create a reverse zone for
>>>>>>>>>>> + # the client and enable dynamic updates for this zone.
>>>>>>>>>>> + allow_sync_ptr(master)
>>>>>>>>>>> + zone = prepare_reverse_zone(master, client.ip)
>>>>>>>>>>> + master.run_command(["ipa", "dnszone-mod", zone,
>>>>>>>>>>> + "--dynamic-update=TRUE"], raiseonerr=False)
>>>>>>>>> I'm not a big fan of ignoring exceptions here, it might be better to
>>>>>>>>> encapsulate the first command with try: except: and run the zone-mod only if
>>>>>>>>> the add worked as expected.
>>>>>>>>>
>>>>>>>>> Also, logging an message that reverse zone was not added might be a good idea.
>>>>>>> Agreed. Done.
>>>>>>>
>>>>>>>>> HTH
>>>>>>>>>
>>>>>>>>> Petr^2 Spacek
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> client.run_command(['ipa-client-install', '-U',
>>>>>>>>>>> '--domain', client.domain.name,
>>>>>>>
>>>>>>>
>>>>> -- Oleg Fayans Quality Engineer FreeIPA team RedHat.
>>>>>
>>>>>
>>>>> freeipa-ofayans-0021.2-Removed-ip-address-option-from-replica-installation.patch
>>>>>
>>>>>
>>>>> From e70daf4ed9dfbac7e8ea75cb8e9ab0f2af12ad48 Mon Sep 17 00:00:00 2001
>>>>> From: Oleg Fayans <ofayans at redhat.com>
>>>>> Date: Wed, 27 Jan 2016 11:09:03 +0100
>>>>> Subject: [PATCH] Removed --ip-address option from replica installation
>>>>>
>>>>> Explicitly specifying ip-address of the replica messes up with the current
>>>>> bind-dyndb-ldap logic, causing reverse zone not to be created.
>>>> NACK
>>>>
>>>> The commit message and logic is incorrect. In fact it has nothing to do with
>>>> bind-dyndb-ldap. Either:
>>>> a] The zone already exists somewhere so you should not create it in IPA, and
>>>> this --ip-address option is invalid.
>>>> b] The zone does not exist yet and then --ip-address option is required.
>>>>
>>>> I do not see any logic around
>>>>> '--ip-address', replica.ip,
>>> The explicit ip-address passing was removed (that's the main point of
>>> this patch)
>> Sure, but it cannot be done unconditionally (without additional logic).
> Fair point. Implemented a check whether ipa master is authoritative for
> the client's domain.
>
>>>> thus the NACK.
>>>>
>>>>
>>>> In other words, network setup is job for a provisioning system. IPA needs to
>>>> respect whatever the provisioning system did or did not do.
>> Let me rephrase it:
>> If IPA DNS manages the zone with replica FQDN or associated reverse zone, we
>> need the --ip-address option.
>>
>> If IPA does not manage any of these, we do not need --ip-address option.
>>
>>>> Feel free to ask if something is unclear.
>> Petr^2 Spacek
>>
>>>> (other comments inline)
>>>>
>>>>
>>>>> Enabled reverse-zone creation for the clients residing in different subnet from
>>>>> master
>>>>> ---
>>>>> ipatests/test_integration/tasks.py | 20 +++++++++++++++-----
>>>>> 1 file changed, 15 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/ipatests/test_integration/tasks.py b/ipatests/test_integration/tasks.py
>>>>> index 6eb55501389c72b4c7aaa599fd4852d7e8f1f3c2..b8b7fcfcbbb1dc672349119bbbb4cdbbd68c54ec 100644
>>>>> --- a/ipatests/test_integration/tasks.py
>>>>> +++ b/ipatests/test_integration/tasks.py
>>>>> @@ -66,9 +66,13 @@ def check_arguments_are(slice, instanceof):
>>>>>
>>>>> def prepare_reverse_zone(host, ip):
>>>>> zone = get_reverse_zone_default(ip)
>>>>> - host.run_command(["ipa",
>>>>> + result = host.run_command(["ipa",
>>>>> "dnszone-add",
>>>>> zone], raiseonerr=False)
>>>>> + if result.returncode > 0:
>>>>> + log.warn(result.stderr_text)
>>>>> + return zone, result.returncode
>>>>> +
>>>>>
>>>>> def prepare_host(host):
>>>>> if isinstance(host, Host):
>>>>> @@ -319,11 +323,10 @@ def domainlevel(host):
>>>>> def replica_prepare(master, replica):
>>>>> apply_common_fixes(replica)
>>>>> fix_apache_semaphores(replica)
>>>>> - prepare_reverse_zone(master, replica.ip)
>>>>> master.run_command(['ipa-replica-prepare',
>>>>> '-p', replica.config.dirman_password,
>>>>> '--ip-address', replica.ip,
>>>>> - replica.hostname])
>>>>> + '--auto-reverse', replica.hostname])
>>>>> replica_bundle = master.get_file_contents(
>>>>> paths.REPLICA_INFO_GPG_TEMPLATE % replica.hostname)
>>>>> replica_filename = get_replica_filename(replica)
>>>>> @@ -339,8 +342,7 @@ def install_replica(master, replica, setup_ca=True, setup_dns=False,
>>>>> # and replica installation would fail
>>>>> args = ['ipa-replica-install', '-U',
>>>>> '-p', replica.config.dirman_password,
>>>>> - '-w', replica.config.admin_password,
>>>>> - '--ip-address', replica.ip]
>>>>> + '-w', replica.config.admin_password]
>>>>> if setup_ca:
>>>>> args.append('--setup-ca')
>>>>> if setup_dns:
>>>>> @@ -380,6 +382,14 @@ def install_client(master, client, extra_args=()):
>>>>> client.collect_log(paths.IPACLIENT_INSTALL_LOG)
>>>>>
>>>>> apply_common_fixes(client)
>>>>> + # Now, for the situations where a client resides in a different subnet from
>>>>> + # master, we need to explicitly tell master to create a reverse zone for
>>>>> + # the client and enable dynamic updates for this zone.
>>>>> + allow_sync_ptr(master)
>>>>> + zone, error = prepare_reverse_zone(master, client.ip)
>>>>> + if not error:
>>>>> + master.run_command(["ipa", "dnszone-mod", zone,
>>>>> + "--dynamic-update=TRUE"], raiseonerr=False)
>>>> I believe that this call to dnszone-mod should use raiseonerr=True so you know
>>>> that environment was not configured to accept dynamic updates.
>>> Agreed. Fixed.
>>>
>>>>>
>>>>> client.run_command(['ipa-client-install', '-U',
>>>>> '--domain', client.domain.name,
>>>>> -- 2.4.3
>>>>
>>
>
>
> + log.warn(result.stderr_text)
warn is deprecated, please use log.warning instead
Martin^2
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160204/73fe9544/attachment.htm>
More information about the Freeipa-devel
mailing list