[Freeipa-devel] [TEST][Patch 0021] Fixed recent replica installation issues in the lab
Petr Spacek
pspacek at redhat.com
Mon Feb 1 11:47:16 UTC 2016
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).
>> 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
>>
>>
>
--
Petr^2 Spacek
More information about the Freeipa-devel
mailing list