[Freeipa-devel] [PATCHES 0066-0067] Upgrade procedure for forwardzones

[Martin Basti]

On Tue, 2014-06-24 at 16:36 +0200, Martin Kosek wrote:
> On 06/18/2014 01:46 PM, Martin Basti wrote:
> > On Wed, 2014-06-18 at 13:44 +0200, Martin Basti wrote:
> >> On Fri, 2014-06-13 at 10:28 +0200, Martin Basti wrote:
> >>> Patches attached, require patches mbasti 0052-0055.
> >>> _______________________________________________
> >>> Freeipa-devel mailing list
> >>> Freeipa-devel at redhat.com
> >>> https://www.redhat.com/mailman/listinfo/freeipa-devel
> >> Rebased patches attached.
> >> PEP8 fixes.
> >>
> > Sorry, patches are here
> 
> 66.2:
> 
> 1) Is it OK to just change constants in the update plugins?
> 
> -PRE_UPDATE = 1
> -POST_UPDATE = 2
> +PRE_SCHEMA_UPDATE = 1
> +PRE_UPDATE = 2
> +POST_UPDATE = 3
> 
> When people refer to the types via names, it should be OK. It just seemed
> unnecessary to me.
> 
I checked where the constants are used, and it shouldn't broke anything.
It looks weird to me to have something which happens first with last
number.
Should I set PRE_SCHEMA_UPDATE = 3 and leave other unchanged then?

> 67.2:
> 
> 1) update_check_forwardzones:
> 
> I think we should set update_to_forward_zones to False when the objectclass is
> there and add a check at the beginning of the execute to simply bail out, if
> update_to_forward_zones is present in the sysupgrade file.
> 
> This will prevent the objectclass check (which takes some time) to run again
> and again.
Good point thanks.

> 2) I would use different backup name:
> 
> +    backup_filename = u'master-zones-transform-backup.ldif'
> 
> Probably something based on time so that different installations' backup do not
> step on each other. You can inspire yourself in other backup files we create:
> 
> # ll /var/lib/ipa/backup/
> total 16
> drwx------. 2 root root 4096 May 30 08:10 ipa-full-2014-05-30-08-10-13
> drwx------. 2 root root 4096 May 30 08:11 ipa-full-2014-05-30-08-11-09
> drwx------. 2 root root 4096 May 30 08:13 ipa-full-2014-05-30-08-13-21
> -rw-r--r--. 1 root root 3441 Jun 24 16:25 master-zones-transform-backup.ldif
Is this better: forward-zones-transform-%datetime%.ldif ?

> 
> 3) This exception does not provide much information, we should at least log the
> error message itself:
> 
> +            except Exception:
> +                self.log.error('Unable to create backup file')
> +                return (False, False, [])
Sorry my bad, I missed that

> 
> 4) The actual upgrade failed in my case, when I had set forward policy:
> 
> # ipa-ldap-upgrade --upgrade
> ...
> Updating managed permissions for user
> Updating non-object managed permissions
> Zones with specified forwarders with policy different than none will be
> transformed to forward zones.
> Original zones will be saved in LDIF format in
> /var/lib/ipa/backup/master-zones-transform-backup.ldif file
> No changes to ACI
> Zone forward1.test. was sucessfully transformed to forward zone
> Transform to forwardzone terminated: creating forwardzone forward2.test. failed
> Traceback (most recent call last):
>   File "/usr/lib/python2.7/site-packages/ipaserver/install/plugins/dns.py",
> line 265, in execute
>     api.Command['dnsforwardzone_add'](zone['idnsname'][0], **kw)
>   File "/usr/lib/python2.7/site-packages/ipalib/frontend.py", line 433, in __call__
>     params = self.convert(**params)
>   File "/usr/lib/python2.7/site-packages/ipalib/frontend.py", line 631, in convert
>     (k, self.params[k].convert(v)) for (k, v) in kw.iteritems()
>   File "/usr/lib/python2.7/site-packages/ipalib/frontend.py", line 631, in
> <genexpr>
>     (k, self.params[k].convert(v)) for (k, v) in kw.iteritems()
>   File "/usr/lib/python2.7/site-packages/ipalib/parameters.py", line 797, in
> convert
>     return self._convert_scalar(value)
>   File "/usr/lib/python2.7/site-packages/ipalib/parameters.py", line 806, in
> _convert_scalar
>     error=ugettext(self.type_error),
> ConversionError: invalid 'idnsforwardpolicy': Gettext('incorrect type',
> domain='ipa', localedir=None)
> ...
> 
> Attaching my DNS zone setting.
I will inspect it.
> 
> Martin
Thank you for review

-- 
Martin^2 Basti