Clean up imported nodes/services/checks as needed #13367
Conversation
freddygv
added
the
pr/no-changelog
| // deletedNodeChecks tracks node checks that were not present in the latest response. | ||
| // A single node check will be attached to all service instances of a node, so this | ||
| // deduplication prevents issuing multiple deregistrations for a single check. | ||
| deletedNodeChecks = make(map[nodeCheckTuple]struct{}) |
There was a problem hiding this comment.
Conveniently after i re-activate mesh-gateway-only-mode there will be no node checks replicated to worry about since the checks on a service will be flattened into a single summary check per service instance.
There was a problem hiding this comment.
Wouldn't this still apply to non-mesh exports?
There was a problem hiding this comment.
Right, unless we wanted to squash those checks as well.
agent/rpc/peering/replication.go
Outdated
| PeerName: peerName, | ||
| }) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to deregister service: %w", err) |
There was a problem hiding this comment.
Do we want to fail fast here vs using merr and grabbing all the raft apply errors? Wondering if it is meaningful to output node/service/check IDs in the error
There was a problem hiding this comment.
Hmmm I'm a bit hesitant to use merr for raft apply errors because the error could get really unwieldy. For example, if leadership is lost and all raft applies fail because of it there could be thousands of errors
Thoughts @rboyer ?
There was a problem hiding this comment.
Almost all times a raft apply will work. If it doesn't work it almost certainly is going to be due to leadership loss so we'd want fast failure at the first raft error, not collection of errors and display at the end.
|
The comments really helped me review and understand what's going on in this PR! 👍 |
agent/rpc/peering/replication.go
Outdated
|
|
||
| // All services on the node were deleted, so the node is also cleaned up. | ||
| err = s.Backend.Apply().CatalogDeregister(&structs.DeregisterRequest{ | ||
| Node: string(node), |
There was a problem hiding this comment.
You need to specify the partition for the node using a empty-namespace, populated-partition entmeta here.
There was a problem hiding this comment.
I'll add some ent tests in a separate PR
agent/rpc/peering/replication.go
Outdated
| PeerName: peerName, | ||
| }) | ||
| if err != nil { | ||
| ident := fmt.Sprintf("partition:%s/peer:%s/node:%s/service_id:%s", csn.Service.PartitionOrDefault(), peerName, csn.Node.Node, csn.Service.ID) |
There was a problem hiding this comment.
You are missing the namespace here.
Previously, imported data would never be deleted. As nodes/services/checks were registered and deregistered, resources deleted from the exporting cluster would accumulate in the imported cluster. This commit makes updates to replication so that whenever an update is received for a service name we reconcile what was present in the catalog against what was received. This handleUpdateService method can handle both updates and deletions.
freddygv
force-pushed
the
peering/service-cleanup
branch
from
db7d8a8 to
c96847c
Compare
Previously, imported data would never be deleted. As nodes/services/checks were registered and deregistered, resources deleted from the exporting cluster would accumulate in the imported cluster. This commit makes updates to replication so that whenever an update is received for a service name we reconcile what was present in the catalog against what was received. This handleUpdateService method can handle both updates and deletions.
kisunji
added
the
theme/cluster-peering
Description
Previously, imported data would never be deleted. As
nodes/services/checks were registered and deregistered, resources
deleted from the exporting cluster would accumulate in the imported
cluster.
This commit makes updates to replication so that whenever an update is
received for a service name we reconcile what was present in the catalog
against what was received.
This handleUpdateService method can handle both updates and deletions.
Testing & Reproduction steps
PR Checklist
external facing docs updated