NIFI-15901 Disable controller services before updating#11201
NIFI-15901 Disable controller services before updating#11201adwk67 wants to merge 3 commits intoapache:mainfrom
Conversation
| // disable them (e.g. COMPONENT_ADDED diffs are skipped by AffectedComponentSet). | ||
| // We must disable before calling updateControllerService, which calls setProperties | ||
| // which calls verifyModifiable and throws IllegalStateException on ENABLED services. | ||
| final long stopTimeout = System.currentTimeMillis() + syncOptions.getComponentStopTimeout().toMillis(); |
There was a problem hiding this comment.
Should this deadline be recomputed per service inside the loop, like processorStopDeadline in synchronizeProcessors, so later iterations don't inherit an exhausted budget?
| // restoring the controller to its pre-update running state. | ||
| if (proposedService.getScheduledState() != org.apache.nifi.flow.ScheduledState.DISABLED) { | ||
| context.getControllerServiceProvider().enableControllerServicesAsync(servicesToRestart); | ||
| context.getControllerServiceProvider().scheduleReferencingComponents( |
There was a problem hiding this comment.
Should we also call notifyScheduledStateChange for servicesToRestart (ENABLED) and referencesToRestart (RUNNING) here, to match the per-service synchronize(ControllerServiceNode, …) overload?
| // Re-enable services and restart components that were stopped for the update, | ||
| // restoring the controller to its pre-update running state. | ||
| if (proposedService.getScheduledState() != org.apache.nifi.flow.ScheduledState.DISABLED) { | ||
| context.getControllerServiceProvider().enableControllerServicesAsync(servicesToRestart); |
There was a problem hiding this comment.
Does enabling via controllerServiceProvider here race with the subsequent componentScheduler.enableControllerServicesAsync loop, and should both paths use the same scheduler?
There was a problem hiding this comment.
I've gone with using the componentScheduler for both as it has already been paused by the caller: d4666a4
| // Update all of the Controller Services to match the VersionedControllerService | ||
| // Update all Controller Services to match the VersionedControllerService. | ||
| // Services may be ENABLED here if the outer "affected components" pass did not | ||
| // disable them (e.g. COMPONENT_ADDED diffs are skipped by AffectedComponentSet). |
There was a problem hiding this comment.
Should this reference FlowDifferenceFilters.isComponentUpdateRequired instead of AffectedComponentSet, since that's the filter actually populating updatedVersionedComponentIds?
There was a problem hiding this comment.
I've simplified the comment to keep the salient bit: d4666a4. If I am reading the code correctly then AffectedComponentSet and isComponentUpdateRequired are doing different things (the former marking what gets disabled, the latter marking things for update).
exceptionfactory
left a comment
exceptionfactory
left a comment
There was a problem hiding this comment.
Tagging @markap14 for additional review. Although there are some similarities to the recent change for Processors, it looks like this needs to be considered in the larger context of calls to the Synchronizer.
3 participants
Summary
NIFI-15901
This is similar to #11111 (https://issues.apache.org/jira/browse/NIFI-15801) but applies to controller services.
If a controller service in a child process group shares an identifier with a CS in the root process group, and a node is disconnected, and the child-group CS is deleted on that node, then when the node reconnects a FlowSynchronizationException is thrown because the root CS is not disabled before being updated.
Tracking
Please complete the following tracking steps prior to pull request creation.
Issue Tracking
Pull Request Tracking
NIFI-00000NIFI-00000VerifiedstatusPull Request Formatting
mainbranchVerification
Please indicate the verification steps performed prior to pull request creation.
Build
./mvnw clean install -P contrib-checkLicensing
LICENSEandNOTICEfilesDocumentation