Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 3 additions & 4 deletions calicoctl/calicoctl/commands/datastore/migrate/export_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -87,14 +87,13 @@ var _ = Describe("Etcd to KDD Migration Export handling", func() {
allPlurals.Discard("ipamconfigs")
allPlurals.Discard("blockaffinities")

allPlurals.Iter(func(resource string) error {
for resource := range allPlurals.All() {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧪 Maintainability & Tests — Tests have been updated mechanically, but no new tests cover the semantics of range-over-func vs prior Iter (e.g., Discard while iterating, early break). Please add focused unit tests in the set package validating:

  • Iteration over All() yields each element exactly once.
  • Discard during iteration does not skip or double-visit items.
  • Early break stops iteration cleanly.
    These tests will serve as the contract for all the new loops spread across the codebase.

if strings.HasPrefix(resource, "kubernetes") {
// "kubernetes"-prefixed resources are backed by Kubernetes API
// objects, not Calico objects.
return set.RemoveItem
allPlurals.Discard(resource)
}
return nil
})
}

Expect(allV3Resources).To(ConsistOf(allPlurals.Slice()))
})
Expand Down
7 changes: 3 additions & 4 deletions calicoctl/calicoctl/commands/ipam/check.go
Original file line number Diff line number Diff line change
Expand Up @@ -453,16 +453,15 @@ func (c *IPAMChecker) checkIPAM(ctx context.Context) error {
var missingHandles []string
{
fmt.Printf("Scanning for IPs with missing handle...\n")
c.inUseHandles.Iter(func(handleID string) error {
for handleID := range c.inUseHandles.All() {
if _, ok := handles[handleID]; ok {
return nil
continue
}
if c.showProblemIPs {
fmt.Printf(" %s is in use in a block but doesn't exist.\n", handleID)
}
missingHandles = append(missingHandles, handleID)
return nil
})
}
fmt.Printf("Found %d handles mentioned in blocks with no matching handle resource.\n", len(missingHandles))
}

Expand Down
5 changes: 2 additions & 3 deletions felix/bpf/failsafes/failsafes.go
Original file line number Diff line number Diff line change
Expand Up @@ -149,16 +149,15 @@ func (m *Manager) ResyncFailsafes() error {
addPort(p, true)
}

unknownKeys.Iter(func(k KeyInterface) error {
for k := range unknownKeys.All() {
err := m.failsafesMap.Delete(k.ToSlice())
if err != nil {
log.WithError(err).WithField("key", k).Warn("Failed to remove failsafe port from map.")
syncFailed = true
} else {
log.WithField("key", k).Debug("Deleted failsafe port.")
}
return nil
})
}

m.failsafesInSync = !syncFailed
if syncFailed {
Expand Down
32 changes: 16 additions & 16 deletions felix/bpf/ipsets/ipsets.go
Original file line number Diff line number Diff line change
Expand Up @@ -316,47 +316,48 @@ func (m *bpfIPSets) ApplyUpdates(_ ipsets.UpdateListener) {
}
}

m.dirtyIPSetIDs.Iter(func(setID uint64) error {
for setID := range m.dirtyIPSetIDs.All() {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧪 Maintainability & Tests — This (and many siblings) implement a drain-like pattern manually:
for setID := range m.dirtyIPSetIDs.All() { ... m.dirtyIPSetIDs.Discard(setID) }
Given how frequently this appears now, a helper on the Set type would reduce boilerplate and errors, e.g.:

  • s.Drain(func(item T) { ... }) // iterates over a stable snapshot and clears items as it yields them
    Or at least a shared utility:
    func ForEachAndDiscard[T comparable](s set.Set[T], fn func(T)) { for v := range s.All() { fn(v); s.Discard(v) } }
    Using such a helper would make intent clear and prevent future regressions where Discard is accidentally omitted.

leaveDirty := false
ipSet := m.getExistingIPSet(setID)
if ipSet == nil {
m.lg.WithField("id", setID).Warn("Couldn't find IP set that was marked as dirty.")
m.resyncScheduled = true
return set.RemoveItem
m.dirtyIPSetIDs.Discard(setID)
continue
}

ipSet.PendingRemoves.Iter(func(entry IPSetEntryInterface) error {
for entry := range ipSet.PendingRemoves.All() {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Casey Casey — Same mutation‑while‑iterating question here. We iterate ipSet.PendingRemoves.All() and Discard(entry) inside the loop. If All() is a snapshot, this is OK and mirrors the old RemoveItem behavior; otherwise we may get unpredictable iteration. Can you confirm All()’s semantics?

if debug {
m.lg.WithFields(log.Fields{"setID": setID, "entry": entry}).Debug("Removing entry from IP set")
}
err := m.bpfMap.Delete(entry.AsBytes())
if err != nil {
m.lg.WithFields(log.Fields{"setID": setID, "entry": entry}).WithError(err).Error("Failed to remove IP set entry")
leaveDirty = true
return nil
continue
}
numDels++
return set.RemoveItem
})
ipSet.PendingRemoves.Discard(entry)
}

ipSet.PendingAdds.Iter(func(entry IPSetEntryInterface) error {
for entry := range ipSet.PendingAdds.All() {
if debug {
m.lg.WithFields(log.Fields{"setID": setID, "entry": entry}).Debug("Adding entry to IP set")
}
err := m.bpfMap.Update(entry.AsBytes(), DummyValue)
if err != nil {
m.lg.WithFields(log.Fields{"setID": setID, "entry": entry}).WithError(err).Error("Failed to add IP set entry")
leaveDirty = true
return nil
continue
}
numAdds++
return set.RemoveItem
})
ipSet.PendingAdds.Discard(entry)
}

if leaveDirty {
m.lg.WithField("setID", setID).Debug("IP set still dirty, queueing resync")
m.resyncScheduled = true
return nil
continue
}

if ipSet.Deleted {
Expand All @@ -365,8 +366,8 @@ func (m *bpfIPSets) ApplyUpdates(_ ipsets.UpdateListener) {
}

m.lg.WithField("setID", setID).Debug("IP set is now clean")
return set.RemoveItem
})
m.dirtyIPSetIDs.Discard(setID)
}

duration := time.Since(startTime)
if numDels > 0 || numAdds > 0 {
Expand Down Expand Up @@ -448,10 +449,9 @@ func (m *bpfIPSet) ReplaceMembers(members []string, protoIPSetMemberToBPFEntry f
}

func (m *bpfIPSet) RemoveAll() {
m.DesiredEntries.Iter(func(entry IPSetEntryInterface) error {
for entry := range m.DesiredEntries.All() {
m.RemoveMember(entry)
return nil
})
}
}

func (m *bpfIPSet) AddMembers(members []string, protoIPSetMemberToBPFEntry func(uint64, string) IPSetEntryInterface) {
Expand Down
5 changes: 2 additions & 3 deletions felix/bpf/legacy/cleanup.go
Original file line number Diff line number Diff line change
Expand Up @@ -132,12 +132,11 @@ func CleanUpMaps() {
log.WithError(err).Error("Error while looking for maps.")
}

emptyAutoDirs.Iter(func(p string) error {
for p := range emptyAutoDirs.All() {
log.WithField("path", p).Debug("Removing empty dir.")
err := os.Remove(p)
if err != nil {
log.WithError(err).Error("Error while removing empty dir.")
}
return nil
})
}
}
6 changes: 3 additions & 3 deletions felix/calc/active_rules_calculator.go
Original file line number Diff line number Diff line change
Expand Up @@ -308,13 +308,13 @@ func (arc *ActiveRulesCalculator) OnStatusUpdate(status api.SyncStatus) {
if arc.missingProfiles.Len() > 0 {
// Log out any profiles that were missing during the resync. We defer
// this until now because we may hear about profiles or endpoints first.
arc.missingProfiles.Iter(func(profileID string) error {
for profileID := range arc.missingProfiles.All() {
log.WithField("profileID", profileID).Warning(
"End of resync: local endpoints refer to missing " +
"or invalid profile, profile's rules replaced " +
"with drop rules.")
return set.RemoveItem
})
arc.missingProfiles.Discard(profileID)
}
}
}
}
Expand Down
5 changes: 2 additions & 3 deletions felix/calc/calc_graph_fv_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -818,10 +818,9 @@ func expectCorrectDataplaneState(mockDataplane *mock.MockDataplane, state State)

func stringifyRoutes(routes set.Set[types.RouteUpdate]) []string {
out := make([]string, 0, routes.Len())
routes.Iter(func(item types.RouteUpdate) error {
for item := range routes.All() {
out = append(out, fmt.Sprintf("%+v", item))
return nil
})
}
sort.Strings(out)
return out
}
Expand Down
24 changes: 10 additions & 14 deletions felix/calc/endpoint_lookup_cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -426,16 +426,14 @@ func (ec *EndpointLookupsCache) addOrUpdateEndpoint(key model.EndpointKey, incom
ec.storeEndpoint(key, incomingEndpointData)

// update endpoint data lookup by ips
ipsToUpdate.Iter(func(newIP [16]byte) error {
for newIP := range ipsToUpdate.All() {
ec.updateIPToEndpointMapping(newIP, incomingEndpointData)
return nil
})
}

ipsToRemove.Iter(
func(ip [16]byte) error {
ec.removeEndpointDataIpMapping(key, ip)
return set.RemoveItem
})
for ip := range ipsToRemove.All() {
ec.removeEndpointDataIpMapping(key, ip)
ipsToRemove.Discard(ip)
}

ec.reportEndpointCacheMetrics()
}
Expand Down Expand Up @@ -574,10 +572,9 @@ func (ec *EndpointLookupsCache) removeEndpoint(key model.EndpointKey) {

// Collect the IPs of this endpoint as we will need to remove it from the IP mapping.
ipsMarkedAsDeleted := set.From(currentEndpointData.allIPs()...)
ipsMarkedAsDeleted.Iter(func(ip [16]byte) error {
for ip := range ipsMarkedAsDeleted.All() {
ec.removeEndpointDataIpMapping(key, ip)
return nil
})
}

delete(ec.localEndpointData, key)
delete(ec.remoteEndpointData, key)
Expand Down Expand Up @@ -760,10 +757,9 @@ func (ec *EndpointLookupsCache) DumpEndpoints() string {
deleted = ""
}

ips.Iter(func(ip [16]byte) error {
for ip := range ips.All() {
ipStr = append(ipStr, net.IP(ip[:16]).String())
return nil
})
}
lines = append(lines, endpointName(key)+": "+strings.Join(ipStr, ",")+deleted)
}

Expand Down
Loading
Loading