markap14 commented on PR #11813: URL: https://github.com/apache/nifi/pull/11813#issuecomment-6371731334
[Claude Opus 5.5] Thanks for digging into this, @awelless. The predictable versioned ID is the right core idea. It's what lets the service a flow author saves match the service each consumer already has. A few things need to change before this can be merged, though. ### 1. Don't use the comment to decide whether a service is removed [`removeMissingControllerServices`](https://github.com/apache/nifi/blob/27ca6a7f63dd76b6dc25b27fcbae59f773b20d3c/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/flow/synchronization/StandardVersionedComponentSynchronizer.java#L1202) now skips any service whose comment equals `MIGRATION_CREATED_COMMENT`. A comment is free text: users can edit it, it's shown in the UI, and it's copied into registry snapshots. We already rely on it in `FlowDifferenceFilters`, and we don't want to give it more meaning. It also causes real problems here: - The flow author has to clear the comment by hand before saving a version, as the system test does. If they don't, every consumer inherits the marker, and no later version can ever remove that service. - The rule applies to every synchronization, including a node reconnecting to the cluster. Suppose that while a node is disconnected, the cluster moves from v1 to v2 (which adopts the service and replaces its comment) and then to v3 (which drops it). The disconnected node still has the marker from v1, so it keeps the service when it reconnects, and its flow no longer matches the cluster's. Please base the decision on references instead: **a service that the proposed flow doesn't contain should be removed only if nothing in the flow still references it once synchronization and property migration have finished.** This isn't a special case for migration. [`StandardControllerServiceNode.verifyCanDelete()`](https://github.com/apache/nifi/blob/27ca6a7f63dd76b6dc25b27fcbae59f773b20d3c/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/controller/service/StandardControllerServiceNode.java#L396-L400) only checks that the service is disabled. So today the synchronizer will delete a service that a processor still points at, and that is exactly the bug in the Jira: the service is deleted, then migration recreates it with the same ID and empty state. Rough outline: - In the per-group `synchronize`, record the services that are missing from the proposed group instead of removing them right away, similar to how `createdAndModifiedExtensions` is collected. - In the public `synchronize`, after the property-migration loop and before `getComponentScheduler().resume()`, remove each recorded service that has no remaining references. References from other recorded services shouldn't count, and the check should repeat until nothing changes, so that a chain of services referencing each other is removed together. - Update the order-of-operations comment in `synchronize`, since step 1 moves to the end. With that rule: - **The new version declares the service:** it's matched by versioned ID and updated in place, as it is now. - **The new version doesn't declare it, but the processor still references it:** it's kept, with its state. - **The new version removes the referencing processor:** it's removed, so unused services don't pile up. - **A node reconnects to the cluster:** it ends up with exactly the services the cluster flow references. - **The author and consumer computed different IDs** (for example, a flow promoted from one registry to another): after the update nothing references the consumer's old service, so it's removed instead of being left beside the new one. One behavior change is worth noting in the Jira. If a nested versioned child group isn't updated (`updateDescendantVersionedFlows` is false), and its processor references a parent-group service that the new version drops, that service is now kept instead of being deleted out from under the processor. The system test's step that clears the migration comment before saving should go away. ### 2. Leave sensitive values out of the versioned ID [`generateVersionedId`](https://github.com/apache/nifi/blob/27ca6a7f63dd76b6dc25b27fcbae59f773b20d3c/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/migration/StandardControllerServiceFactory.java#L139) hashes every property value passed to the new service. When a migration moves a sensitive literal into a service (for example, credentials moved into a credentials provider), the author and consumer will hold different values, because sensitive values aren't stored in the registry. Their IDs won't match. It also puts a hash of the secret into the registry snapshot. Please build the ID from non-sensitive values only. If that would give two services in the same group the same versioned ID, leave the new service unversioned. ### 3. System tests `MigrationCreatedControllerServiceVersioningIT` adds 342 seconds to every system test run, because each of its 11 tests builds a new NiFi instance and restarts it at least once. That's too much for this change. Please reduce it to a single test method with one upgrade restart: set up every flow first, upgrade once, then run the version changes and checks. Only a few things need a real NiFi instance: migration at startup, migration at import, the two producing the same ID, and state surviving a version change. Suggested shape: 1. **Setup (old extensions):** - Register the registry client and copy in legacy v1. - The author imports v1 and starts it, then adds the unrelated processor and saves v2. - Consumer 1 imports v1 and starts it. 2. **Upgrade:** swap the NARs and restart once. 3. **Author:** - The service exists, is enabled, is referenced by the processor, and has a versioned ID. - The processor is valid and the flow shows as up to date. - Consumer 1's service has the same versioned ID as the author's, and its flow is stale but has no local changes. 4. **Consumer 2** imports v2 and starts it. Its service has the same versioned ID as the author's, which shows that the startup and import paths agree. 5. **Consumer 1** changes from v1 to v2. The same service remains and its state is kept. 6. **The author** makes a small change, such as renaming the processor, and saves v3. Consumers 1 and 2 both change to v3. Each keeps the same service instance and its state, the versioned ID matches, and the flow is up to date. The rest can be removed: - **The three restart tests:** the versioned ID is saved and reloaded like any other field. - **The three paste tests:** the unit tests already cover which identity a paste uses. - **`testFlowUpdateMigrationUsesSameServiceIdentityAsStartup`:** migration during a version change uses the same code path as import. `migrationInSynchronizedGroupUsesIncomingFlowIdAfterVersionControlIsCleared` covers it. - **The nested parent-update test:** `migrationInIndependentlyVersionedChildUsesChildFlowId` covers which identity a nested child uses. - **`testRuntimeUpgradeCreatesEnabledReferencedService`:** its checks move into step 3. There's also an unnecessary restart in `AbstractNarSwapMigrationIT`'s `@AfterEach`. That's being handled separately in [NIFI-16500](https://issues.apache.org/jira/browse/NIFI-16500), so no change is needed here. ### 4. Unit tests - [`testUserAddedControllerServiceRemovedWhenAbsentFromProposedFlow`](https://github.com/apache/nifi/blob/27ca6a7f63dd76b6dc25b27fcbae59f773b20d3c/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/test/java/org/apache/nifi/flow/synchronization/StandardVersionedComponentSynchronizerTest.java#L575) duplicates the existing [`testControllerServiceRemoved`](https://github.com/apache/nifi/blob/27ca6a7f63dd76b6dc25b27fcbae59f773b20d3c/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/test/java/org/apache/nifi/flow/synchronization/StandardVersionedComponentSynchronizerTest.java#L1766). Please strengthen that test instead of adding a new one. - The two `synchronizationRetains...` tests should become a single reference-based test: one case where a remaining processor still references the service (it's kept), and one where nothing does (it's removed). - [`expectedMigrationServiceId`](https://github.com/apache/nifi/blob/27ca6a7f63dd76b6dc25b27fcbae59f773b20d3c/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/test/java/org/apache/nifi/flow/synchronization/StandardVersionedComponentSynchronizerTest.java#L836) copies the production hash formula, so these tests break whenever the formula changes, even if the behavior is still correct. Compare against the ID produced by a `StandardControllerServiceFactory` built with the expected identity instead. ### 5. Smaller items - In [`getOwningProcessGroupIdentity`](https://github.com/apache/nifi/blob/27ca6a7f63dd76b6dc25b27fcbae59f773b20d3c/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/flow/synchronization/StandardVersionedComponentSynchronizer.java#L451), the `topLevelGroup` parameter is really the group being synchronized. Please rename it to something like `synchronizedGroup`. Inside `synchronize`, `topLevelGroup` already means the group from `syncOptions.getTopLevelGroupId()`. - In the [comment on line 458](https://github.com/apache/nifi/blob/27ca6a7f63dd76b6dc25b27fcbae59f773b20d3c/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/flow/synchronization/StandardVersionedComponentSynchronizer.java#L458), please spell out "VCI". The line above talks about the versioned component ID, so it's unclear whether "VCI" means that or version control information. - When the deferred removal keeps a service because something still references it, please log that at `info`, naming the service and what references it. It explains why a service the new version doesn't declare is still there. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
