Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

state: ensure that identical manual virtual IP updates result in not bumping the modify indexes #21909

Merged
merged 5 commits into from
Nov 22, 2024
+440 −91
Merged

state: ensure that identical manual virtual IP updates result in not bumping the modify indexes #21909

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
36d5891
state: ensure that identical manual virtual IP updates result in not …
rboyer Oct 31, 2024
0754411
changelog
rboyer Oct 31, 2024
7e55ff3
move out of package
rboyer Oct 31, 2024
90f8e8e
also prevent taking a trip through raft if we know the list of ips ha…
rboyer Oct 31, 2024
e86a5d9
discard duplicates silently for back compat
rboyer Oct 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 55 additions & 10 deletions agent/consul/state/catalog.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import (
"fmt"
"net"
"reflect"
"slices"
"sort"
"strings"

"github.com/hashicorp/go-memdb"
Expand Down Expand Up @@ -1106,6 +1108,9 @@ func (s *Store) AssignManualServiceVIPs(idx uint64, psn structs.PeeredServiceNam
for _, ip := range ips {
assignedIPs[ip] = struct{}{}
}

txnNeedsCommit := false
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is practically an issue, but I did notice that the logic was:

begin txn
maybe write
maybe early return
write
commit

and with this change i fixed it to

begin txn
maybe write
maybe write
maybe commit


modifiedEntries := make(map[structs.PeeredServiceName]struct{})
for ip := range assignedIPs {
entry, err := tx.First(tableServiceVirtualIPs, indexManualVIPs, psn.ServiceName.PartitionOrDefault(), ip)
Expand All @@ -1118,7 +1123,13 @@ func (s *Store) AssignManualServiceVIPs(idx uint64, psn structs.PeeredServiceNam
}

newEntry := entry.(ServiceVirtualIP)
if newEntry.Service.ServiceName.Matches(psn.ServiceName) {

var (
thisServiceName = newEntry.Service.ServiceName
thisPeer = newEntry.Service.Peer
)

if thisServiceName.Matches(psn.ServiceName) && thisPeer == psn.Peer {
continue
}

Expand All @@ -1130,13 +1141,20 @@ func (s *Store) AssignManualServiceVIPs(idx uint64, psn structs.PeeredServiceNam
filteredIPs = append(filteredIPs, existingIP)
}
}
sort.Strings(filteredIPs)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously we were storing VIPs in whatever order they happened to be in. It seemed silly to not be sorting them.


newEntry.ManualIPs = filteredIPs
newEntry.ModifyIndex = idx
if err := tx.Insert(tableServiceVirtualIPs, newEntry); err != nil {
return false, nil, fmt.Errorf("failed inserting service virtual IP entry: %s", err)
}
modifiedEntries[newEntry.Service] = struct{}{}

if err := updateVirtualIPMaxIndexes(tx, idx, thisServiceName.PartitionOrDefault(), thisPeer); err != nil {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously we were not updating the max index table for the entries that had VIPs stolen from them.

return false, nil, err
}

txnNeedsCommit = true
}

entry, err := tx.First(tableServiceVirtualIPs, indexID, psn)
Expand All @@ -1149,23 +1167,49 @@ func (s *Store) AssignManualServiceVIPs(idx uint64, psn structs.PeeredServiceNam
}

newEntry := entry.(ServiceVirtualIP)
newEntry.ManualIPs = ips
newEntry.ModifyIndex = idx

if err := tx.Insert(tableServiceVirtualIPs, newEntry); err != nil {
return false, nil, fmt.Errorf("failed inserting service virtual IP entry: %s", err)
}
if err := updateVirtualIPMaxIndexes(tx, idx, psn.ServiceName.PartitionOrDefault(), psn.Peer); err != nil {
return false, nil, err
// Check to see if the slice already contains the same ips.
if !vipSliceEqualsMapKeys(newEntry.ManualIPs, assignedIPs) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the key part of the fix.

newEntry.ManualIPs = slices.Clone(ips)
newEntry.ModifyIndex = idx

sort.Strings(newEntry.ManualIPs)

if err := tx.Insert(tableServiceVirtualIPs, newEntry); err != nil {
return false, nil, fmt.Errorf("failed inserting service virtual IP entry: %s", err)
}
if err := updateVirtualIPMaxIndexes(tx, idx, psn.ServiceName.PartitionOrDefault(), psn.Peer); err != nil {
return false, nil, err
}
txnNeedsCommit = true
}
if err = tx.Commit(); err != nil {
return false, nil, err

if txnNeedsCommit {
if err = tx.Commit(); err != nil {
return false, nil, err
}
}

return true, maps.SliceOfKeys(modifiedEntries), nil
}

func vipSliceEqualsMapKeys(a []string, b map[string]struct{}) bool {
if len(a) != len(b) {
return false
}
for _, ip := range a {
if _, ok := b[ip]; !ok {
return false
}
}
return true
}

func updateVirtualIPMaxIndexes(txn WriteTxn, idx uint64, partition, peerName string) error {
// update global max index (for snapshots)
if err := indexUpdateMaxTxn(txn, idx, tableServiceVirtualIPs); err != nil {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The snapshot logic grabs the max index from this table without peering/partition prefixes, so in order for that to be more correct we update the un-prefixed index here too.

return fmt.Errorf("failed while updating index: %w", err)
}
// update per-partition max index
if err := indexUpdateMaxTxn(txn, idx, partitionedIndexEntryName(tableServiceVirtualIPs, partition)); err != nil {
return fmt.Errorf("failed while updating partitioned index: %w", err)
Expand Down Expand Up @@ -3086,6 +3130,7 @@ func servicesVirtualIPsTxn(tx ReadTxn, ws memdb.WatchSet) (uint64, []ServiceVirt
vips = append(vips, vip)
}

// Pull from the global one
idx := maxIndexWatchTxn(tx, nil, tableServiceVirtualIPs)

return idx, vips, nil
Expand Down
Loading
Loading