[Freeipa-devel] [TEST][Patch 0021] Fixed recent replica installation issues in the lab
Martin Basti
mbasti at redhat.com
Thu Feb 4 14:54:10 UTC 2016
On 04.02.2016 15:43, Petr Spacek wrote:
> On 4.2.2016 15:38, Oleg Fayans wrote:
>> Hi,
>>
>> On 02/04/2016 02:07 PM, Martin Basti wrote:
>>>
>>> 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
>> Fixed
> I'm still not big fan of this, but never mind. ACK
>
>
Pushed to:
master: 42d364427606e39486645e4064ca16940b2f8837
ipa-4-3: 91bd73455c244932486889d32bcf9352f1c038a4
More information about the Freeipa-devel
mailing list