qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Rudyy (JIRA)" <j...@apache.org>
Subject [jira] [Comment Edited] (QPID-8066) [Broker-J] Virtual host logger rules are left over in configuration store after deletion of virtual host logger on provided virtual host causing virtualhost restart failure
Date Fri, 26 Jan 2018 11:02:00 GMT

    [ https://issues.apache.org/jira/browse/QPID-8066?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16340908#comment-16340908
] 

Alex Rudyy edited comment on QPID-8066 at 1/26/18 11:01 AM:
------------------------------------------------------------

Keith,
I reviewed the changes in the attached patch.
In general I believe that proposed changes is a move in right direction. It looks like the
changes prepare the ground for QPID-7197. Also, they address partially QPID-7547 which might
be closed after committing the patch.
Here are my review comments
* BDBHAVirtualHostNodeImpl
** {{_helpers}} field itself and setting of {{_helper}} field in {{beforeDelete}} does not
look necessary to me. It seems that local variable in {{onDelete}} can be introduced for this
purpose.
** Flag {{_isClosed}} is set on both deletion and close. Would it be better to rename the
flag into {{_isClosingOrDeleting}} to reflect that it can be set from both operations?
** {{#removeRemoteReplicationNode}} invokes {{deleteAsync}}.  I think that {{deleteNoChecks}}
should be called instead off {{deleteAsync}} in order to avoid unnecessary authorization checks.
** In {{onDelete}} of BDB HA VHN its VH is get closed. We discussed that before and you implemented
exactly what we discussed. Though, after reviewing the changes, it looks more logical to me
to close all direct children of CO with {{managesChildStorage=true}} in {{AbstractConfiguredObject#deleteChildren}}.
Thus, no call to close the children would be required in the implementation of {{onDelete}}.
* AbstractExchange
** {{#onDelete}}; why performDelete is called only on condition {{getState() != State.UNINITIALIZED}}?
It seems to me that this condition need to be removed otherwise a reference to _alternateBindingDestination
will not removed and binding count statistics on queues will not be updated.
* AmqpPortImpl
** {{_closing}}; is set on deletion and close. Would it be better to rename the flag into
{{_isClosingOrDeleting}} to reflect that it can be set from both operations?
* AbstractConfiguredObject
** In {{deleteChildren}} when {{managesChildStorage=true}}, the immediate future is returned.
Would it be better to close the children and return close future? Though, it is not required
now, but taking that it is a generic code, potentially, on close some system resources can
be cleaned up. At the moment, COs managing children close the children by themselves in {{doDelete}}
which looks to me like a duplication of code.
** {{CreateExceptionHandler#handleException}} invokes {{deleteAsync}}. IMHO, it should call
{{deleteNoChecks}} in order to avoid making unnecessary authorization checks.
** when child is not instance AbstractConfiguredObject or AbstractConfiguredObjectProxy, it
seems it will not be deleted. It does not look like a problem at the moment, but, it points
to the potential gap in a public API of {{ConfiguredObject}} interface. We might need a public
method to perform the delete checks.
* AbstractQueue
** {{_deleteQueueDepthFuture}} is set twice: once prematurely at the beigining of {{#performDelete}}
and second time at the end  of {{#performDelete}}
** Method {{deleteAndReturnCountAsync}} calls {{deleteAsync}}. It seems it should call {{deleteNoCheck}}
as authorization and checks are done in {{deleteAndReturnCountAsync}}.
* QueueConsumerImpl
** {{deleteAsync}} is invoked from {{onClose}}. As result authorization check would be performed
which is not really necessary. Would it be better to call {{deleteNoChecks}} instead of {{deleteAsync}}
* AbstractAuthenticationManager
** Method {{performDelete}} can be inlined.
* Immediate future is created explicitly in {{doDelete}}. Would it be better to call {{super.doDelete()}}
for consistency reasons?
* AbstractKeyStore
** Method {{deleteIfNotInUse}} is not used and can be safely removed.
** Method {{doDelete}} can call and return {{super.doDelete}} instead of creation of intermediate
future
* AbstractTrustStore
** Immediate future is created explicitly in {{doDelete}}. Would it be better to call {{super.doDelete()}}
for consistency?
* AbstractAMQPSession
** Immediate future is created explicitly in {{doDelete}}. Would it be better to call {{super.doDelete()}}
for consistency?
** Task {{_deleteModelTask}} calls {{deleteAsync}}. IMHO, it should call {{deleteNoChecks}}
to avoid unnecessary authorization and checks.
* PrincipalDatabaseAuthenticationManager
** In method {{addChildAsync}} on {{RuntimeException}} the user is deleted asynchronously
by calling {{principalAdapter.deleteAsync();}}. IMHO, {{deleteNoChecks}} should be called
instead to avoid no necessary auth checks.




was (Author: alex.rufous):
Keith,
I reviewed the changes in the attached patch.
In general I believe that proposed changes is a move in right direction. It looks like the
changes prepare the ground for QPID-7197. Also, they address partially QPID-7547 which might
be closed after committing the patch.
Here are my review comments
* BDBHAVirtualHostNodeImpl
** {{_helpers} field itself and setting of {{_helper}} field in {{beforeDelete}} does not
look necessary to me. It seems that local variable in {{onDelete}} can be introduced for this
purpose.
** Flag {{_isClosed}} is set on both deletion and close. Would it be better to rename the
flag into {{_isClosingOrDeleting}} to reflect that it can be set from both operations?
** {{#removeRemoteReplicationNode}} invokes {{deleteAsync}}.  I think that {{deleteNoChecks}}
should be called instead off {{deleteAsync}} in order to avoid unnecessary authorization checks.
** In {{onDelete}} of BDB HA VHN its VH is get closed. We discussed that before and you implemented
exactly what we discussed. Though, after reviewing the changes, it looks more logical to me
to close all direct children of CO with {{managesChildStorage=true}} in {{AbstractConfiguredObject#deleteChildren}}.
Thus, no call to close the children would be required in the implementation of {{onDelete}}.
* AbstractExchange
** {{#onDelete}}; why performDelete is called only on condition {{getState() != State.UNINITIALIZED}}?
It seems to me that this condition need to be removed otherwise a reference to _alternateBindingDestination
will not removed and binding count statistics on queues will not be updated.
* AmqpPortImpl
** {{_closing}}; is set on deletion and close. Would it be better to rename the flag into
{{_isClosingOrDeleting}} to reflect that it can be set from both operations?
* AbstractConfiguredObject
** In {{deleteChildren}} when {{managesChildStorage=true}}, the immediate future is returned.
Would it be better to close the children and return close future? Though, it is not required
now, but taking that it is a generic code, potentially, on close some system resources can
be cleaned up. At the moment, COs managing children close the children by themselves in {{doDelete}}
which looks to me like a duplication of code.
** {{CreateExceptionHandler#handleException}} invokes {{deleteAsync}}. IMHO, it should call
{{deleteNoChecks}} in order to avoid making unnecessary authorization checks.
** when child is not instance AbstractConfiguredObject or AbstractConfiguredObjectProxy, it
seems it will not be deleted. It does not look like a problem at the moment, but, it points
to the potential gap in a public API of {{ConfiguredObject}} interface. We might need a public
method to perform the delete checks.
* AbstractQueue
** {{_deleteQueueDepthFuture}} is set twice: once prematurely at the beigining of {{#performDelete}}
and second time at the end  of {{#performDelete}}
** Method {{deleteAndReturnCountAsync}} calls {{deleteAsync}}. It seems it should call {{deleteNoCheck}}
as authorization and checks are done in {{deleteAndReturnCountAsync}}.
* QueueConsumerImpl
** {{deleteAsync}} is invoked from {{onClose}}. As result authorization check would be performed
which is not really necessary. Would it be better to call {{deleteNoChecks}} instead of {{deleteAsync}}
* AbstractAuthenticationManager
** Method {{performDelete}} can be inlined.
* Immediate future is created explicitly in {{doDelete}}. Would it be better to call {{super.doDelete()}}
for consistency reasons?
* AbstractKeyStore
** Method {{deleteIfNotInUse}} is not used and can be safely removed.
** Method {{doDelete}} can call and return {{super.doDelete}} instead of creation of intermediate
future
* AbstractTrustStore
** Immediate future is created explicitly in {{doDelete}}. Would it be better to call {{super.doDelete()}}
for consistency?
* AbstractAMQPSession
** Immediate future is created explicitly in {{doDelete}}. Would it be better to call {{super.doDelete()}}
for consistency?
** Task {{_deleteModelTask}} calls {{deleteAsync}}. IMHO, it should call {{deleteNoChecks}}
to avoid unnecessary authorization and checks.
* PrincipalDatabaseAuthenticationManager
** In method {{addChildAsync}} on {{RuntimeException}} the user is deleted asynchronously
by calling {{principalAdapter.deleteAsync();}}. IMHO, {{deleteNoChecks}} should be called
instead to avoid no necessary auth checks.



> [Broker-J] Virtual host logger rules are left over in configuration store after deletion
of virtual host logger on provided virtual host causing virtualhost restart failure
> ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------
>
>                 Key: QPID-8066
>                 URL: https://issues.apache.org/jira/browse/QPID-8066
>             Project: Qpid
>          Issue Type: Bug
>          Components: Broker-J
>    Affects Versions: qpid-java-6.0, qpid-java-6.0.1, qpid-java-6.0.2, qpid-java-6.0.3,
qpid-java-6.0.4, qpid-java-6.0.5, qpid-java-6.1, qpid-java-6.0.6, qpid-java-6.1.1, qpid-java-6.1.2,
qpid-java-6.0.7, qpid-java-6.1.3, qpid-java-6.0.8, qpid-java-6.1.4, qpid-java-broker-7.0.0,
qpid-java-6.1.5
>            Reporter: Alex Rudyy
>            Assignee: Keith Wall
>            Priority: Critical
>             Fix For: qpid-java-broker-7.0.1, qpid-java-broker-7.1.0
>
>         Attachments: 0001-QPID-8066-Broker-J-Remove-all-children-records-recur.patch,
QPID-8066.tar.bz2
>
>
> Virtual host logger rules are left over in configuration store after deletion of virtual
host logger on provided virtual host. On restart of VHN or Broker, virtual host recovery fails
with IllegalArgumentException as the one below:
> {noformat}
> java.lang.IllegalArgumentException: Recovered configured object record BDBConfiguredObjectRecord
[id=8e9c5547-c41b-4443-9333-48dac61f3b40, type=VirtualHostLogInclusionRule, name=test, parents={}]
has no recorded parents and is not a valid child type [[interface org.apache.qpid.server.model.VirtualHost,
interface org.apache.qpid.server.model.RemoteReplicationNode]] for the root BDBVirtualHostNodeImplWithAccessChecking
[id=591aa6d9-c2e0-474c-a0a9-86eb14bc3c6a, name=bdb, storePath=/home/alex/.qpid-7.1.0-SNAPSHOT/bdb/config]
>         at org.apache.qpid.server.store.GenericRecoverer.resolveDiscontinuity(GenericRecoverer.java:119)
>         at org.apache.qpid.server.store.GenericRecoverer.performRecover(GenericRecoverer.java:90)
>         at org.apache.qpid.server.store.GenericRecoverer.access$000(GenericRecoverer.java:41)
>         at org.apache.qpid.server.store.GenericRecoverer$1.execute(GenericRecoverer.java:59)
>         at org.apache.qpid.server.store.GenericRecoverer$1.execute(GenericRecoverer.java:55)
>         at org.apache.qpid.server.configuration.updater.TaskExecutorImpl$TaskLoggingWrapper.execute(TaskExecutorImpl.java:248)
>         at org.apache.qpid.server.configuration.updater.TaskExecutorImpl.submitWrappedTask(TaskExecutorImpl.java:165)
>         at org.apache.qpid.server.configuration.updater.TaskExecutorImpl.run(TaskExecutorImpl.java:190)
>         at org.apache.qpid.server.store.GenericRecoverer.recover(GenericRecoverer.java:54)
>         at org.apache.qpid.server.store.VirtualHostStoreUpgraderAndRecoverer.recover(VirtualHostStoreUpgraderAndRecoverer.java:1085)
>         at org.apache.qpid.server.store.VirtualHostStoreUpgraderAndRecoverer.upgradeAndRecover(VirtualHostStoreUpgraderAndRecoverer.java:1058)
>         at org.apache.qpid.server.virtualhostnode.AbstractStandardVirtualHostNode.activate(AbstractStandardVirtualHostNode.java:91)
>         at org.apache.qpid.server.virtualhostnode.AbstractVirtualHostNode.doActivate(AbstractVirtualHostNode.java:162)
>         at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
>         at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
>         at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
>         at java.lang.reflect.Method.invoke(Method.java:498)
>         at org.apache.qpid.server.model.AbstractConfiguredObject.attainState(AbstractConfiguredObject.java:1524)
>         at org.apache.qpid.server.model.AbstractConfiguredObject.attainState(AbstractConfiguredObject.java:1503)
>         at org.apache.qpid.server.model.AbstractConfiguredObject$8.onSuccess(AbstractConfiguredObject.java:1070)
>         at org.apache.qpid.server.model.AbstractConfiguredObject$8.onSuccess(AbstractConfiguredObject.java:1064)
>         at org.apache.qpid.server.model.AbstractConfiguredObject$22$1.run(AbstractConfiguredObject.java:2639)
>         at org.apache.qpid.server.model.AbstractConfiguredObject$22$1.run(AbstractConfiguredObject.java:2635)
>         at java.security.AccessController.doPrivileged(Native Method)
>         at javax.security.auth.Subject.doAs(Subject.java:360)
>         at org.apache.qpid.server.model.AbstractConfiguredObject$22.onSuccess(AbstractConfiguredObject.java:2634)
>         at com.google.common.util.concurrent.Futures$CallbackListener.run(Futures.java:1237)
>         at org.apache.qpid.server.configuration.updater.TaskExecutorImpl$ImmediateIfSameThreadExecutor.execute(TaskExecutorImpl.java:400)
>         at org.apache.qpid.server.configuration.updater.TaskExecutorImpl.execute(TaskExecutorImpl.java:183)
>         at com.google.common.util.concurrent.AbstractFuture.executeListener(AbstractFuture.java:911)
>         at com.google.common.util.concurrent.AbstractFuture.addListener(AbstractFuture.java:645)
>         at com.google.common.util.concurrent.AbstractFuture$TrustedFuture.addListener(AbstractFuture.java:101)
>         at com.google.common.util.concurrent.Futures.addCallback(Futures.java:1209)
>         at org.apache.qpid.server.model.AbstractConfiguredObject.addFutureCallback(AbstractConfiguredObject.java:2629)
>         at org.apache.qpid.server.model.AbstractConfiguredObject.doAttainState(AbstractConfiguredObject.java:1063)
>         at org.apache.qpid.server.model.AbstractConfiguredObject.access$600(AbstractConfiguredObject.java:95)
>         at org.apache.qpid.server.model.AbstractConfiguredObject$7.performAction(AbstractConfiguredObject.java:1048)
>         at org.apache.qpid.server.model.AbstractConfiguredObject$7.performAction(AbstractConfiguredObject.java:1038)
>         at org.apache.qpid.server.model.AbstractConfiguredObject.applyToChildren(AbstractConfiguredObject.java:1311)
>         at org.apache.qpid.server.model.AbstractConfiguredObject.doAttainState(AbstractConfiguredObject.java:1037)
>         at org.apache.qpid.server.model.AbstractConfiguredObject.access$600(AbstractConfiguredObject.java:95)
>         at org.apache.qpid.server.model.AbstractConfiguredObject$1.execute(AbstractConfiguredObject.java:589)
>         at org.apache.qpid.server.model.AbstractConfiguredObject$1.execute(AbstractConfiguredObject.java:576)
>         at org.apache.qpid.server.model.AbstractConfiguredObject$2.execute(AbstractConfiguredObject.java:637)
>         at org.apache.qpid.server.model.AbstractConfiguredObject$2.execute(AbstractConfiguredObject.java:630)
>         at org.apache.qpid.server.configuration.updater.TaskExecutorImpl$TaskLoggingWrapper.execute(TaskExecutorImpl.java:248)
>         at org.apache.qpid.server.configuration.updater.TaskExecutorImpl.submitWrappedTask(TaskExecutorImpl.java:165)
>         at org.apache.qpid.server.configuration.updater.TaskExecutorImpl.submit(TaskExecutorImpl.java:153)
>         at org.apache.qpid.server.model.AbstractConfiguredObject.doOnConfigThread(AbstractConfiguredObject.java:629)
>        at org.apache.qpid.server.model.AbstractConfiguredObject.openAsync(AbstractConfiguredObject.java:575)
>         at org.apache.qpid.server.model.AbstractSystemConfig.makeActive(AbstractSystemConfig.java:304)
>         at org.apache.qpid.server.model.AbstractSystemConfig.activate(AbstractSystemConfig.java:280)
>         at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
>         at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
>         at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
>         at java.lang.reflect.Method.invoke(Method.java:498)
>         at org.apache.qpid.server.model.AbstractConfiguredObject.attainState(AbstractConfiguredObject.java:1524)
>         at org.apache.qpid.server.model.AbstractConfiguredObject.attainState(AbstractConfiguredObject.java:1503)
>         at org.apache.qpid.server.model.AbstractConfiguredObject$8.onSuccess(AbstractConfiguredObject.java:1070)
>         at org.apache.qpid.server.model.AbstractConfiguredObject$8.onSuccess(AbstractConfiguredObject.java:1064)
>         at org.apache.qpid.server.model.AbstractConfiguredObject$22$1.run(AbstractConfiguredObject.java:2639)
>         at org.apache.qpid.server.model.AbstractConfiguredObject$22$1.run(AbstractConfiguredObject.java:2635)
>         at java.security.AccessController.doPrivileged(Native Method)
>         at javax.security.auth.Subject.doAs(Subject.java:360)
>         at org.apache.qpid.server.model.AbstractConfiguredObject$22.onSuccess(AbstractConfiguredObject.java:2634)
>         at com.google.common.util.concurrent.Futures$CallbackListener.run(Futures.java:1237)
>         at org.apache.qpid.server.configuration.updater.TaskExecutorImpl$ImmediateIfSameThreadExecutor.execute(TaskExecutorImpl.java:400)
>         at org.apache.qpid.server.configuration.updater.TaskExecutorImpl.execute(TaskExecutorImpl.java:183)
>         at com.google.common.util.concurrent.AbstractFuture.executeListener(AbstractFuture.java:911)
>         at com.google.common.util.concurrent.AbstractFuture.addListener(AbstractFuture.java:645)
>         at com.google.common.util.concurrent.AbstractFuture$TrustedFuture.addListener(AbstractFuture.java:101)
>         at com.google.common.util.concurrent.Futures.addCallback(Futures.java:1209)
>         at org.apache.qpid.server.model.AbstractConfiguredObject.addFutureCallback(AbstractConfiguredObject.java:2629)
>         at org.apache.qpid.server.model.AbstractConfiguredObject.doAttainState(AbstractConfiguredObject.java:1063)
>         at org.apache.qpid.server.model.AbstractConfiguredObject.access$600(AbstractConfiguredObject.java:95)
>         at org.apache.qpid.server.model.AbstractConfiguredObject$1.execute(AbstractConfiguredObject.java:589)
>         at org.apache.qpid.server.model.AbstractConfiguredObject$1.execute(AbstractConfiguredObject.java:576)
>         at org.apache.qpid.server.model.AbstractConfiguredObject$2.execute(AbstractConfiguredObject.java:637)
>         at org.apache.qpid.server.model.AbstractConfiguredObject$2.execute(AbstractConfiguredObject.java:630)
>         at org.apache.qpid.server.configuration.updater.TaskExecutorImpl$TaskLoggingWrapper.execute(TaskExecutorImpl.java:248)
>         at org.apache.qpid.server.configuration.updater.TaskExecutorImpl$CallableWrapper$1.run(TaskExecutorImpl.java:320)
>         at java.security.AccessController.doPrivileged(Native Method)
>         at javax.security.auth.Subject.doAs(Subject.java:360)
>         at org.apache.qpid.server.configuration.updater.TaskExecutorImpl$CallableWrapper.call(TaskExecutorImpl.java:313)
>         at com.google.common.util.concurrent.TrustedListenableFutureTask$TrustedFutureInterruptibleTask.runInterruptibly(TrustedListenableFutureTask.java:111)
>         at com.google.common.util.concurrent.InterruptibleTask.run(InterruptibleTask.java:58)
>         at com.google.common.util.concurrent.TrustedListenableFutureTask.run(TrustedListenableFutureTask.java:75)
>         at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
>         at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
>         at org.apache.qpid.server.bytebuffer.QpidByteBufferFactory.lambda$null$0(QpidByteBufferFactory.java:464)
>         at java.lang.Thread.run(Thread.java:745)
> {noformat}
> The rule entry can only be deleted from outside of the Broker which would be difficult
to achieve with BDB store. If entry cannot be deleted, the Virtual host node removal can only
resolve the issue which means the loss of the configuration and messages. Though, configuration
and messages can be exported from impacted VHN and re-imported into re-created  VHN.
> Alternatively, all VH logging rules need to be deleted before the deletion of VH logger
> I would vote for fixing this issue in 7.0.1 and porting the changes into 6.x



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org


Mime
View raw message