On 13/03/15 12:50, Jan Cholasta wrote:
Dne 13.3.2015 v 12:08 Petr Spacek napsal(a):
On 13.3.2015 12:01, Martin Basti wrote:
On 13/03/15 11:55, Petr Spacek wrote:
On 13.3.2015 11:34, Jan Cholasta wrote:
Dne 13.3.2015 v 11:17 Martin Kosek napsal(a):
On 03/13/2015 11:00 AM, Petr Spacek wrote:
On 13.3.2015 10:42, Alexander Bokovoy wrote:
On Fri, 13 Mar 2015, Petr Spacek wrote:
On 13.3.2015 10:18, Martin Kosek wrote:
On 03/12/2015 05:10 PM, Rob Crittenden wrote:
Petr Spacek wrote:
On 12.3.2015 16:23, Rob Crittenden wrote:
David Kupka wrote:
On 03/06/2015 06:00 PM, Martin Basti wrote:
Upgrade plugins which modify LDAP data directly should
not be
executed
in --test mode.
This patch is a workaround, to ensure update with --test
option will not
modify any LDAP data.
https://fedorahosted.org/freeipa/ticket/3448
Patch attached.
Ideally we want to fix all plugins to dry-run the upgrade
not
just skip
when there is '--test' option but it is a good first step.
Works for me, ACK.
I agree that this breaks the spirit of --test and think it
should be
fixed before committing.
Considering how often is the option is used ... I do not think
that this
requires 'proper' fix now. It was broken for *years* so this
patch is a huge
improvement and IMHO should be commited in current form. We
can
re-visit it
later on, open a ticket :-)
No. There is no rush for this, at least not for the promise
of a
future
fix that will never come.
I checked the code and to me, the proper fix looks like
instrumenting
ldap.update_entry calls in upgrade plugins with
if options.test:
log message
else
do the update
right? I see just couple places that would need to be updated:
$ git grep -E "(ldap|conn).update_entry"
ipaserver/install/plugins
ipaserver/install/plugins/dns.py:
ldap.update_entry(container_entry)
ipaserver/install/plugins/fix_replica_agreements.py:
repl.conn.update_entry(replica)
ipaserver/install/plugins/fix_replica_agreements.py:
repl.conn.update_entry(replica)
ipaserver/install/plugins/update_idranges.py:
ldap.update_entry(entry)
ipaserver/install/plugins/update_idranges.py:
ldap.update_entry(entry)
ipaserver/install/plugins/update_managed_permissions.py:
ldap.update_entry(base_entry)
ipaserver/install/plugins/update_managed_permissions.py:
ldap.update_entry(entry)
ipaserver/install/plugins/update_pacs.py:
ldap.update_entry(entry)
ipaserver/install/plugins/update_referint.py:
ldap.update_entry(entry)
ipaserver/install/plugins/update_services.py:
ldap.update_entry(entry)
ipaserver/install/plugins/update_uniqueness.py:
ldap.update_entry(uid_uniqueness_plugin)
So from my POV, very quick fix. In that case, I would also
prefer a
fix now
than a ticket that would never be done.
I really dislike this approach because I consider it flawed by
design. Plugin
author has to think about it all the time and do not forget to
add if
otherwise ... too bad.
I can see two 'safer' ways to do that:
- LDAP transactions :-)
- 'mock_writes=True' option in LDAP backend which would print
modlists instead
of applying them (and return success to the caller).
Both cases eliminate the need to scatter 'ifs' all over update
plugins and do
not add risk of forgetting about one of them when adding/changing
plugin code.
I like idea about mock_writes=True. However, I think we still
need to
make
sure plugin writers rely on options.test value to see that we
aren't
going to write the data. The reason for it is that we might get
into
configurations where plugins would be doing updates based on
earlier
performed tasks. If task configuration is not going to be
written, its
status will never be correct and plugin would get an error.
That is exactly why I mentioned LDAP transactions. There is no
other
way how
to test complex plugins which actually read own writes (except
mocking
the
whole LDAP interface somewhere).
While this may be a good idea long term, I do not think any of us is
considering implementing the LDAP transaction support within work on
this refactoring.
So in this thread, let us focus on how to fix options.test
mid-term. I
currently see 2 proposed ways:
- Making the plugins aware of options.test
- Make ldap2 write operations only print the update and not do it.
Although thinking of this approach, I think it may make some plugins
like DNS loop forever. IIRC, at least DNS upgrade plugin have loop
- search for all unfixed DNS zones
- fix them with ldap update
- was the search truncated, i.e. are there more zones to
update?
if yes, go back to start
- Make the plugins not call {add,update,delete}_entry
themselves but rather
return the updates like they should. This is what the ticket
(<https://fedorahosted.org/freeipa/ticket/3448>) requests and what
should be
done to make --test work for them.
How do you propose to handle iterative updates like the DNS upgrade
mentioned
by Martin^1? Return set of updates along with boolean 'call me again'?
Something else?
So instead of DNS commands logic, which can be used in plugin, we
should
reimplement the dnzone commands in upgrade plugin, to get modlist?
And keep
watching it and do same modifications for upgrade plugin as are done
in DNS
plugin.
Yes, this is how it currently works and how it is done in other update
plugins. For proper command support some serious changes will have to
be done in the framework.
Again, I think we are investing to much effort into an option which
is almost
never used. Let it die in Git history. After all, it can be revived
at any
time when needed or when we have proper support in DS.
+1
Updated patch attached.
--
Martin Basti
From 9a1a4aa06cdd4e72c99b45f9f2aa5c9fa2b4028a Mon Sep 17 00:00:00 2001
From: Martin Basti <mba...@redhat.com>
Date: Fri, 6 Mar 2015 17:44:47 +0100
Subject: [PATCH] Server Upgrade: respect --test option in plugins
Several plugins do the LDAP data modification directly.
In test mode these plugis should not be executed.
https://fedorahosted.org/freeipa/ticket/3448
---
ipaserver/install/plugins/dns.py | 22 +++++++++-----
.../install/plugins/fix_replica_agreements.py | 20 ++++++++-----
ipaserver/install/plugins/update_idranges.py | 6 ++--
.../install/plugins/update_managed_permissions.py | 34 +++++++++++++---------
ipaserver/install/plugins/update_pacs.py | 3 +-
ipaserver/install/plugins/update_referint.py | 4 +--
ipaserver/install/plugins/update_services.py | 3 +-
7 files changed, 59 insertions(+), 33 deletions(-)
diff --git a/ipaserver/install/plugins/dns.py b/ipaserver/install/plugins/dns.py
index f562978bcbcc02321c0e9a668af88b4f596f8556..8de457dc18f2615262951922a80e1c47843d6e4b 100644
--- a/ipaserver/install/plugins/dns.py
+++ b/ipaserver/install/plugins/dns.py
@@ -85,7 +85,7 @@ class update_dnszones(PostUpdate):
update['idnsupdatepolicy'] = util.get_dns_forward_zone_update_policy(\
api.env.realm)
- if update:
+ if update and options.get('live_run'):
# FIXME: https://fedorahosted.org/freeipa/ticket/4722
api.Command.dnszone_mod(zone[u'idnsname'][0].make_absolute(),
**update)
@@ -157,6 +157,7 @@ class update_master_to_dnsforwardzones(PostUpdate):
backup_dir = u'/var/lib/ipa/backup/'
backup_filename = u'dns-forward-zones-backup-%Y-%m-%d-%H-%M-%S.ldif'
backup_path = u'%s%s' % (backup_dir, backup_filename)
+ backup_path_test = u'%s%s.test_run' % (backup_dir, backup_filename)
def execute(self, **options):
ldap = self.obj.backend
@@ -221,7 +222,10 @@ class update_master_to_dnsforwardzones(PostUpdate):
if zones_to_transform:
# add time to filename
- self.backup_path = time.strftime(self.backup_path)
+ if options.get('live_run'):
+ backup_file_path = time.strftime(self.backup_path)
+ else:
+ backup_file_path = time.strftime(self.backup_path_test)
# DNs of privileges which contain dns managed permissions
privileges_to_ldif = set() # store priviledges only once
@@ -233,7 +237,7 @@ class update_master_to_dnsforwardzones(PostUpdate):
'%s file' % self.backup_path)
try:
- with open(self.backup_path, 'w') as f:
+ with open(backup_file_path, 'w') as f:
writer = LDIFWriter(f)
for zone in zones_to_transform:
# save backup to ldif
@@ -293,7 +297,8 @@ class update_master_to_dnsforwardzones(PostUpdate):
for zone in zones_to_transform:
# delete master zone
try:
- api.Command['dnszone_del'](zone['idnsname'])
+ if options.get('live_run'):
+ api.Command['dnszone_del'](zone['idnsname'])
except Exception, e:
self.log.error('Transform to forwardzone terminated: '
'removing zone %s failed (%s)' % (
@@ -308,7 +313,9 @@ class update_master_to_dnsforwardzones(PostUpdate):
'idnsforwarders': zone.get('idnsforwarders', []),
'idnsforwardpolicy': zone.get('idnsforwardpolicy', [u'first'])[0]
}
- api.Command['dnsforwardzone_add'](zone['idnsname'][0], **kw)
+ if options.get('live_run'):
+ api.Command['dnsforwardzone_add'](zone['idnsname'][0],
+ **kw)
except Exception, e:
self.log.error('Transform to forwardzone terminated: creating '
'forwardzone %s failed' %
@@ -337,8 +344,9 @@ class update_master_to_dnsforwardzones(PostUpdate):
dn[0].value for dn in zone_to_privileges[zone['idnsname'][0]]
]
try:
- api.Command['permission_add_member'](perm_name,
- privilege=privileges)
+ if options.get('live_run'):
+ api.Command['permission_add_member'](
+ perm_name, privilege=privileges)
except Exception, e:
self.log.error('Unable to restore privileges for '
'permission %s, for zone %s'
diff --git a/ipaserver/install/plugins/fix_replica_agreements.py b/ipaserver/install/plugins/fix_replica_agreements.py
index a5ff4819fa4c432b378a9f1c0f6952bc312a6792..b287480fbd0996ad7ed57159556da18f3f57416a 100644
--- a/ipaserver/install/plugins/fix_replica_agreements.py
+++ b/ipaserver/install/plugins/fix_replica_agreements.py
@@ -53,21 +53,25 @@ class update_replica_attribute_lists(PreUpdate):
for replica in ipa_replicas:
self.log.debug(replica.single_value.get('description'))
-
+ live_run = options.get('live_run', False)
self._update_attr(repl, replica,
'nsDS5ReplicatedAttributeList',
- replication.EXCLUDES, template=EXCLUDE_TEMPLATE)
+ replication.EXCLUDES, template=EXCLUDE_TEMPLATE,
+ live_run=live_run)
self._update_attr(repl, replica,
'nsDS5ReplicatedAttributeListTotal',
- replication.TOTAL_EXCLUDES, template=EXCLUDE_TEMPLATE)
+ replication.TOTAL_EXCLUDES, template=EXCLUDE_TEMPLATE,
+ live_run=live_run)
self._update_attr(repl, replica,
- 'nsds5ReplicaStripAttrs', replication.STRIP_ATTRS)
+ 'nsds5ReplicaStripAttrs', replication.STRIP_ATTRS,
+ live_run=live_run)
self.log.debug("Done updating agreements")
return (False, False, []) # No restart, no apply now, no updates
- def _update_attr(self, repl, replica, attribute, values, template='%s'):
+ def _update_attr(self, repl, replica, attribute, values, template='%s',
+ live_run=False):
"""Add or update an attribute of a replication agreement
If the attribute doesn't already exist, it is added and set to
@@ -89,7 +93,8 @@ class update_replica_attribute_lists(PreUpdate):
replica[attribute] = [template % " ".join(values)]
try:
- repl.conn.update_entry(replica)
+ if live_run:
+ repl.conn.update_entry(replica)
self.log.debug("Updated")
except Exception, e:
self.log.error("Error caught updating replica: %s", str(e))
@@ -107,7 +112,8 @@ class update_replica_attribute_lists(PreUpdate):
'%s %s' % (attrlist, ' '.join(missing))]
try:
- repl.conn.update_entry(replica)
+ if live_run:
+ repl.conn.update_entry(replica)
self.log.debug("Updated %s", attribute)
except Exception, e:
self.log.error("Error caught updating %s: %s",
diff --git a/ipaserver/install/plugins/update_idranges.py b/ipaserver/install/plugins/update_idranges.py
index 1aa5fa7631fd35a7aaf4a23a5eee44e4e0a2e904..ea7ce5005b7b50e2972a1488ff0c8177941a97f1 100644
--- a/ipaserver/install/plugins/update_idranges.py
+++ b/ipaserver/install/plugins/update_idranges.py
@@ -89,7 +89,8 @@ class update_idrange_type(PostUpdate):
"to 'unknown' for entry: %s" % str(entry.dn))
try:
- ldap.update_entry(entry)
+ if options.get('live_run'):
+ ldap.update_entry(entry)
except (errors.EmptyModlist, errors.NotFound):
pass
except errors.ExecutionError, e:
@@ -158,7 +159,8 @@ class update_idrange_baserid(PostUpdate):
entry['ipabaserid'] = 0
try:
root_logger.info("Updating existing idrange: %s" % (entry.dn))
- ldap.update_entry(entry)
+ if options.get('live_run'):
+ ldap.update_entry(entry)
root_logger.info("Done")
except (errors.EmptyModlist, errors.NotFound):
pass
diff --git a/ipaserver/install/plugins/update_managed_permissions.py b/ipaserver/install/plugins/update_managed_permissions.py
index 430a2919a315bfd8d8e6174a915890d44b782c5c..7d0839b96e3ee3f7eb525d08198835480a6e3d73 100644
--- a/ipaserver/install/plugins/update_managed_permissions.py
+++ b/ipaserver/install/plugins/update_managed_permissions.py
@@ -422,10 +422,11 @@ class update_managed_permissions(PostUpdate):
current_obj = obj
self.update_permission(ldap,
- obj,
- unicode(name),
- template,
- anonymous_read_aci)
+ obj,
+ unicode(name),
+ template,
+ anonymous_read_aci,
+ live_run=options.get('live_run', False))
if anonymous_read_aci:
self.remove_anonymous_read_aci(ldap, anonymous_read_aci)
@@ -433,9 +434,10 @@ class update_managed_permissions(PostUpdate):
for obsolete_name in OBSOLETE_PERMISSIONS:
self.log.debug('Deleting obsolete permission %s', obsolete_name)
try:
- self.api.Command[permission_del](unicode(obsolete_name),
- force=True,
- version=u'2.101')
+ if options.get('live_run'):
+ self.api.Command[permission_del](unicode(obsolete_name),
+ force=True,
+ version=u'2.101')
except errors.NotFound:
self.log.debug('Obsolete permission not found')
else:
@@ -443,7 +445,8 @@ class update_managed_permissions(PostUpdate):
return False, False, ()
- def update_permission(self, ldap, obj, name, template, anonymous_read_aci):
+ def update_permission(self, ldap, obj, name, template, anonymous_read_aci,
+ live_run=False):
"""Update the given permission and the corresponding ACI"""
assert name.startswith('System:')
@@ -518,21 +521,25 @@ class update_managed_permissions(PostUpdate):
update_aci = True
self.log.debug('Updating managed permission: %s', name)
if is_new:
- ldap.add_entry(entry)
+ if live_run:
+ ldap.add_entry(entry)
else:
try:
- ldap.update_entry(entry)
+ if live_run:
+ ldap.update_entry(entry)
except errors.EmptyModlist:
self.log.debug('No changes to permission: %s', name)
update_aci = False
if update_aci:
self.log.debug('Updating ACI for managed permission: %s', name)
- permission_plugin.update_aci(entry)
+ if live_run:
+ permission_plugin.update_aci(entry)
if remove_legacy:
self.log.info("Removing legacy permission '%s'", legacy_name)
- self.api.Command[permission_del](unicode(legacy_name))
+ if live_run:
+ self.api.Command[permission_del](unicode(legacy_name))
for name in template.get('replaces_system', ()):
name = unicode(name)
@@ -545,7 +552,8 @@ class update_managed_permissions(PostUpdate):
flags = entry.get('ipapermissiontype', [])
if list(flags) == ['SYSTEM']:
self.log.info("Removing legacy permission '%s'", name)
- self.api.Command[permission_del](name, force=True)
+ if live_run:
+ self.api.Command[permission_del](name, force=True)
else:
self.log.info("Ignoring V2 permission '%s'", name)
diff --git a/ipaserver/install/plugins/update_pacs.py b/ipaserver/install/plugins/update_pacs.py
index 653456bb84d5464022024f5baaf4a7543f01f96f..c1d5f3e939d743e4bf5189d132ea168829225edb 100644
--- a/ipaserver/install/plugins/update_pacs.py
+++ b/ipaserver/install/plugins/update_pacs.py
@@ -48,7 +48,8 @@ class update_pacs(PostUpdate):
updated_pacs = pacs + [u'nfs:NONE']
entry['ipakrbauthzdata'] = updated_pacs
- ldap.update_entry(entry)
+ if options.get('live_run'):
+ ldap.update_entry(entry)
else:
self.log.debug('PAC for nfs is already set, not adding nfs:NONE.')
diff --git a/ipaserver/install/plugins/update_referint.py b/ipaserver/install/plugins/update_referint.py
index 1b7411035b27ebba04246a7ee6f220d470b46688..73411dc7173718c462beb66c84c44b2c7dab739a 100644
--- a/ipaserver/install/plugins/update_referint.py
+++ b/ipaserver/install/plugins/update_referint.py
@@ -28,7 +28,6 @@ class update_referint(PreUpdate):
('cn', 'plugins'), ('cn', 'config'))
def execute(self, **options):
-
root_logger.debug("Upgrading referential integrity plugin configuration")
ldap = self.obj.backend
try:
@@ -80,7 +79,8 @@ class update_referint(PreUpdate):
root_logger.debug("Final value: %s", repr(entry))
try:
- ldap.update_entry(entry)
+ if options.get('live_run'):
+ ldap.update_entry(entry)
except errors.EmptyModlist:
root_logger.debug("No modifications required")
return False, False, []
diff --git a/ipaserver/install/plugins/update_services.py b/ipaserver/install/plugins/update_services.py
index 2122abb10a14824ea752123cb59bea8ce9a7d665..cc5855f8d9c496ccd2d4d508797e11b97061e581 100644
--- a/ipaserver/install/plugins/update_services.py
+++ b/ipaserver/install/plugins/update_services.py
@@ -71,7 +71,8 @@ class update_service_principalalias(PostUpdate):
['ipakrbprincipal'])
entry['ipakrbprincipalalias'] = entry['krbprincipalname']
try:
- ldap.update_entry(entry)
+ if options.get('live_run'):
+ ldap.update_entry(entry)
except (errors.EmptyModlist, errors.NotFound):
pass
except errors.ExecutionError, e:
--
2.1.0
--
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
