debezium/dbz#2090 Add SSH config file watcher with host reconciliation#460
debezium/dbz#2090 Add SSH config file watcher with host reconciliation#460d1vyanshu-kumar wants to merge 2 commits into
Conversation
Signed-off-by: divyanshu_Kumar <154233802+d1vyanshu-kumar@users.noreply.github.com>
| * (see {@code AbstractService}, {@code PipelineService}, etc.). | ||
| */ | ||
| @ApplicationScoped | ||
| public class HostReconciliationRepository { |
There was a problem hiding this comment.
Though I like the repository pattern, for historic reason, we use the BlazeBit persistence layer.
Could you please so align with it? This means your service will extend the AbstractService, so same basic method like list, create, update, delete, findBy are already there.
| * @param dbHosts active host entities from the database (status ≠ REMOVED) | ||
| * @return the reconciliation plan describing what needs to change | ||
| */ | ||
| ReconciliationPlan buildReconciliationPlan(List<SshHostEntry> fileHosts, |
There was a problem hiding this comment.
WDYT to put this on a separate class?
| private void scheduleReconciliation() { | ||
| ScheduledFuture<?> existing = pendingReconciliation; | ||
| if (existing != null && !existing.isDone()) { | ||
| existing.cancel(false); |
There was a problem hiding this comment.
Could interrupting the provisioning lead to a bad state?
There was a problem hiding this comment.
@mfvitale no, it can't. The cancel(false) only cancels tasks that are still waiting in the 2-second debounce window (i.e., the reconciliation hasn't started yet). The false parameter means "do not interrupt if already running" - so if a reconciliation is already in progress and executing provisioning calls, it will run to completion uninterrupted.
Essentially, the cancel only prevents duplicate reconciliation runs from being queued during rapid file events - it never interrupts an active one.
| break; | ||
| } | ||
|
|
||
| if (key == null) { |
There was a problem hiding this comment.
Could you just flip this so you can remove the continue?
| } | ||
|
|
||
| for (String alias : plan.toRemove()) { | ||
| repository.markHostRemoved(alias); |
There was a problem hiding this comment.
@mfvitale Right now, when a host is removed from the SSH config, we mark its status as REMOVED in the database. This ensures the scheduler won't deploy any new pipelines to that host going forward.
The actual pipeline undeployment (stopping and migrating active pipelines from the removed host) is planned for a future implementation - the HostPipelineController will handle that lifecycle. For this iteration, marking as REMOVED is the safe first step that prevents further deployments while the undeployment logic is built out.
| * <p>These are plain JUnit 5 tests with no {@code @QuarkusTest} — they test only | ||
| * the stateless comparison function, which has zero database access and zero side effects. | ||
| */ | ||
| class SshConfigWatcherServiceTest { |
There was a problem hiding this comment.
Well effectively this tests the reconciliation plan not the watcher itself. If you move the buildReconciliationPlan in a separate class as suggested this will be the tests for that class, then I could see another test testing the watcher itself.
… extract plan builder Signed-off-by: divyanshu_Kumar <154233802+d1vyanshu-kumar@users.noreply.github.com>
|
Hi @mfvitale, thanks again for the thorough review and the guidance! I just pushed a new commit. I've completely refactored the persistence layer to align with the Blaze-Persistence architecture. We now have a proper HostStatusService extending AbstractService, along with the HostStatus and HostStatusReference entity views. I also took your advice on separating concerns—I pulled all the pure reconciliation math out of the Watcher and put it into a dedicated HostReconciliationPlanBuilder class. The tests are much cleaner now too, since they only test the pure logic without needing to mock the entire Watcher. The minor points (flipping the if guard, etc.) are also sorted, and I noted your point about the REMOVED status for the future pipeline undeployment implementation. Let me know if everything looks good to go now! |
| * injected dependencies, no mutable state, and no side effects. Every method | ||
| * is a deterministic transformation of its inputs. | ||
| */ | ||
| class HostReconciliationPlanBuilder { |
There was a problem hiding this comment.
Why this is not a bean? I think it should be.
There was a problem hiding this comment.
@mfvitale I kept it as a plain Java object instantiated with new because it's completely stateless and acts as a pure function (no dependencies or side effects).
There was a problem hiding this comment.
I think you make a good point about consistency — I can see that making it a bean keeps the dependency graph explicit and follows the same pattern as the rest of the codebase. I'll convert it to an @ApplicationScoped bean and inject it into the watcher in the next commit.
| logger.debugv("SSH config file event: {0} — {1}", | ||
| event.kind().name(), changedFile); | ||
| configFileAffected = true; | ||
| if (configFileAffected) { |
There was a problem hiding this comment.
Should this be inside the if above?
There was a problem hiding this comment.
@mfvitale, I can see how this looks odd at first glance. The reason it's outside the loop is to handle the OVERFLOW case cleanly. When WatchService overflows, we set the flag and break out of the loop. The if (configFileAffected) check after the loop catches both the normal case (our config file was modified) and the overflow case (we missed events and need a full resync) in a single place.
That said, I could also just call scheduleReconciliation() directly from both places and drop the flag entirely — let me know if you'd prefer that, happy to refactor it either way.
There was a problem hiding this comment.
That said, I could also just call scheduleReconciliation() directly from both places and drop the flag entirely — let me know if you'd prefer that, happy to refactor it either way.
Yes, this is exactly where I was leading you.
|
@d1vyanshu-kumar Could you also add an IT for the Watcher? |
Sure, I'll add one! I'm thinking of a |
Fixes debezium/dbz#2090
Description
Adds the SSH config file watcher that monitors
~/.ssh/configand keeps thehost_statusdatabase table in sync automatically.How it works:
SshConfigWatcherServiceregisters a Java NIOWatchServiceon the SSH config parent directory and filters events to theconfigfile specifically. A 2-second debounce coalesces rapid editor saves into a single reconciliation run. A@Scheduled(every = "5m", delayed = "5m")fallback handles environments whereWatchServicemisses events — such as NFS mounts and Kubernetes ConfigMap volumes, which use atomic symlink-swap instead ofENTRY_MODIFYevents.Reconciliation logic:
HostReconciliationPlanBuilderis a dedicated, package-private class that compares SSH config state against database state and produces aReconciliationPlan— three lists describing what to add, remove, or update. It is intentionally not a CDI bean: no injected dependencies, no mutable state, no side effects. This makes it directlyunit-testable without a running Quarkus application.
Persistence layer:
HostStatusServiceextendsAbstractService<HostStatusEntity, HostStatus, HostStatusReference>following the same Blaze-Persistence pattern used byPipelineService,VaultService, andConnectionService. TheHostStatusentity view is annotated@CreatableEntityViewand@UpdatableEntityViewto support all three operations the reconciler needs.CDI transaction boundary:
@Transactionalon self-invocations is silently ignored. All database write methods live in the separateHostStatusServicebean so every call goes through the CDI proxy and transactions are properly committed and rolled back.Runtime mode guard:
The bean is always instantiated. Each
@Observesand@Scheduledmethod is guarded byif (!isHostMode()) return;so no file system access happens in operator mode.HostProvisioningServicestub:The watcher calls
provisioningService.provision(alias)for new and updated hosts. The class is included here with empty method bodies so this sub-issue compiles independently. Ansible execution will be implemented in a future implementation.Tests:
HostReconciliationPlanBuilderTestcontains 7 plain JUnit 5 tests — no@QuarkusTest, no database, no mocks of the watcher itself. It directly instantiates the builder and verifies every reconciliation scenario: add, remove, update, mixed, empty, unchanged,and null-hostname fallback.
PR Checklist
and the governance document on PR expectations.
main