IPSets: re-create ipsets failed during re-sync (#11464)

mazdakn · web-flow · commit 02005a2bb789 · 2025-11-28T14:59:07.000-08:00
diff --git a/felix/ipsets/ipsets.go b/felix/ipsets/ipsets.go
@@ -1,4 +1,4 @@
-// Copyright (c) 2017-2024 Tigera, Inc. All rights reserved.
+// Copyright (c) 2017-2025 Tigera, Inc. All rights reserved.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -36,6 +36,7 @@ import (
 
 const (
 	MaxIPSetDeletionsPerIteration = 1
+	MaxRetryAttempt               = 10
 )
 
 type dataplaneMetadata struct {
@@ -44,6 +45,7 @@ type dataplaneMetadata struct {
 	RangeMin     int
 	RangeMax     int
 	DeleteFailed bool
+	ListFailed   bool
 }
 
 // IPSets manages a whole "plane" of IP sets, i.e. all the IPv4 sets, or all the IPv6 IP sets.
@@ -353,22 +355,31 @@ func (s *IPSets) ApplyUpdates(listener UpdateListener) {
 		retryDelay *= 2
 	}
 
-	for attempt := 0; attempt < 10; attempt++ {
+	var resyncErr, updateErr error
+	for attempt := 0; attempt < MaxRetryAttempt; attempt++ {
 		if attempt > 0 {
 			s.logCxt.Info("Retrying after an ipsets update failure...")
 		}
+		treatFailureAsTransient := attempt < MaxRetryAttempt/2
 		if s.fullResyncRequired || s.ipSetsRequiringResync.Len() > 0 {
 			// Compare our in-memory state against the dataplane and queue up
 			// modifications to fix any inconsistencies.
 			s.logCxt.Debug("Resyncing ipsets with dataplane.")
 			s.opReporter.RecordOperation(fmt.Sprint("resync-ipsets-v", s.IPVersionConfig.Family.Version()))
 
-			if err := s.tryResync(); err != nil {
-				s.logCxt.WithError(err).Warning("Failed to resync with dataplane")
-				backOff()
-				continue
+			if resyncErr = s.tryResync(); resyncErr != nil {
+				s.logCxt.WithError(resyncErr).Warning("Failed to resync with dataplane")
+
+				// After a few attempts, most likely, we are dealing with a persistent failure.
+				// This could be due to different failures, like userspace and kernel incompatibility.
+				// The incompatibility failure can be fixed by swapping the one Felix understand (created from
+				// desired state) and the one (with higher revision) in dataplane. As such, we should stop re-trying
+				// and instead fall through to tryUpdates() below, which will do the swap.
+				if treatFailureAsTransient {
+					backOff()
+					continue
+				}
 			}
-			s.fullResyncRequired = false
 		}
 
 		// Opportunistically delete some temporary IP sets.  It's possible
@@ -377,22 +388,26 @@ func (s *IPSets) ApplyUpdates(listener UpdateListener) {
 		s.tryTempIPSetDeletions()
 
 		dirtyIPSets := s.dirtyIPSetsForUpdate()
-		if err := s.tryUpdates(dirtyIPSets, listener); err != nil {
-			if attempt >= 5 {
+		if updateErr = s.tryUpdates(dirtyIPSets, listener); updateErr != nil {
+			if treatFailureAsTransient {
+				// Transient failure, resync the IP sets that we failed to update.
+				s.logCxt.WithError(updateErr).WithField("attempt", attempt).Warning(
+					"Failed to update IP sets. Will do partial resync.")
+			} else {
 				// Persistent failures, try a full resync.
-				s.logCxt.WithError(err).WithField("attempt", attempt).Warning(
+				s.logCxt.WithError(updateErr).WithField("attempt", attempt).Warning(
 					"Persistently failed to update IP sets. Will do full resync.")
 				s.QueueResync()
-			} else {
-				// More than one failure, resync the IP sets that we failed to update.
-				s.logCxt.WithError(err).WithField("attempt", attempt).Warning(
-					"Failed to update IP sets. Will do partial resync.")
 			}
 			countNumIPSetErrors.Inc()
+		}
+
+		if resyncErr != nil || updateErr != nil {
 			backOff()
 			continue
 		}
 
+		s.fullResyncRequired = false
 		success = true
 		break
 	}
@@ -440,20 +455,18 @@ func (s *IPSets) tryResync() (err error) {
 	// are known to exist.
 	ipSets, err := s.CalicoIPSets()
 	if err != nil {
-		s.logCxt.WithError(err).Error("Failed to get the list of ipsets")
+		s.logCxt.WithError(err).Error("Failed to get the list of Calico ipsets")
 		return
 	}
 	if debug {
-		s.logCxt.Debugf("List of ipsets: %v", ipSets)
+		s.logCxt.Debugf("List of calico ipsets: %v", ipSets)
 	}
 
 	ipSetPartOfSync := func(name string) bool {
-		if s.fullResyncRequired {
-			return true
-		}
-		return s.ipSetsRequiringResync.Contains(name)
+		return s.fullResyncRequired || s.ipSetsRequiringResync.Contains(name)
 	}
 
+	var failedIPSets []string
 	for _, name := range ipSets {
 		if !ipSetPartOfSync(name) {
 			// Skipping this IP set on this pass.
@@ -462,16 +475,29 @@ func (s *IPSets) tryResync() (err error) {
 		if debug {
 			s.logCxt.Debugf("Parsing IP set %v.", name)
 		}
-		err = s.resyncIPSet(name)
-		if err != nil {
-			s.logCxt.WithError(err).Errorf("Failed to parse ipset %v", name)
-			return
+		if err = s.resyncIPSet(name); err != nil {
+			// Ignore failures of IP sets not in desired state, as those will be cleaned up later.
+			_, desired := s.setNameToProgrammedMetadata.Desired().Get(name)
+			if desired {
+				failedIPSets = append(failedIPSets, name)
+			}
+			if desired {
+				s.logCxt.WithError(err).WithField("name", name).
+					Warn("Failed to parse required Calico-owned ipset that is needed, will try recreating it.")
+			} else {
+				s.logCxt.WithError(err).WithField("name", name).
+					Warn("Failed to parse Calico-owned ipset that is no longer needed, will queue it for deletion.")
+			}
 		} else {
 			// Successful resync of this IP set, clear any pending partial resync.
 			s.ipSetsRequiringResync.Discard(name)
 		}
 	}
 
+	if len(failedIPSets) > 0 {
+		return fmt.Errorf("failed to parse IPSets %v", strings.Join(failedIPSets, ","))
+	}
+
 	// Mark any IP sets that we didn't see as empty.
 	for name, members := range s.mainSetNameToMembers {
 		if !ipSetPartOfSync(name) {
@@ -551,8 +577,9 @@ func (s *IPSets) resyncIPSet(ipSetName string) error {
 	//
 	// As we stream through the data, we extract the name of the IP set and its members. We
 	// use the IP set's metadata to convert each member to its canonical form for comparison.
+	meta := dataplaneMetadata{}
+	debug := log.GetLevel() >= log.DebugLevel
 	err := s.runIPSetList(ipSetName, func(scanner *bufio.Scanner) error {
-		debug := log.GetLevel() >= log.DebugLevel
 		ipSetName := ""
 		var ipSetType IPSetType
 		for scanner.Scan() {
@@ -576,9 +603,7 @@ func (s *IPSets) resyncIPSet(ipSetName string) error {
 				// When we hit the Header line we should know the name, and type of the IP set, which lets
 				// us update the tracker.
 				parts := strings.Split(line, " ")
-				meta := dataplaneMetadata{
-					Type: ipSetType,
-				}
+				meta.Type = ipSetType
 				for idx, p := range parts {
 					if p == "maxelem" {
 						if idx+1 >= len(parts) {
@@ -609,7 +634,6 @@ func (s *IPSets) resyncIPSet(ipSetName string) error {
 						break
 					}
 				}
-				s.setNameToProgrammedMetadata.Dataplane().Set(ipSetName, meta)
 			}
 			if strings.HasPrefix(line, "Members:") {
 				// Start of a Members entry, following this, there'll be one member per
@@ -681,9 +705,16 @@ func (s *IPSets) resyncIPSet(ipSetName string) error {
 		return scanner.Err()
 	})
 	if err != nil {
-		return err
+		// This can occur if we have version skew with the version of IP set
+		// used to create the IP set. Mark the metadata as invalid in order
+		// to trigger the IP set to be recreated.
+		meta.ListFailed = true
 	}
-	return nil
+	if debug {
+		s.logCxt.WithField("setName", ipSetName).Debugf("Parsed metadata from dataplane %+v", meta)
+	}
+	s.setNameToProgrammedMetadata.Dataplane().Set(ipSetName, meta)
+	return err
 }
 
 func (s *IPSets) runIPSetList(arg string, parsingFunc func(*bufio.Scanner) error) error {
@@ -1014,7 +1045,8 @@ func (s *IPSets) ApplyDeletions() bool {
 		if numDeletions >= MaxIPSetDeletionsPerIteration {
 			// Deleting IP sets is slow (40ms) and serialised in the kernel.  Avoid holding up the main loop
 			// for too long.  We'll leave the remaining sets pending deletion and mop them up next time.
-			log.Debugf("Deleted batch of %d IP sets, rate limiting further IP set deletions.", MaxIPSetDeletionsPerIteration)
+			log.Debugf("Deleted batch of %d IP sets, rate limiting further IP set deletions.",
+				MaxIPSetDeletionsPerIteration)
 			// Leave the item in the set, so we'll do another batch of deletions next time around the loop.
 			return deltatracker.IterActionNoOpStopIteration
 		}
@@ -1070,7 +1102,8 @@ func (s *IPSets) tryTempIPSetDeletions() {
 		if numDeletions >= MaxIPSetDeletionsPerIteration {
 			// Deleting IP sets is slow (40ms) and serialised in the kernel.  Avoid holding up the main loop
 			// for too long.  We'll leave the remaining sets pending deletion and mop them up next time.
-			log.Debugf("Deleted batch of 20 temp IP sets, rate limiting further IP set deletions.")
+			log.Debugf("Deleted batch of %d IP sets, rate limiting further IP set deletions.",
+				MaxIPSetDeletionsPerIteration)
 			// Leave the item in the set, so we'll do another batch of deletions next time around the loop.
 			return deltatracker.IterActionNoOpStopIteration
 		}
diff --git a/felix/ipsets/ipsets_test.go b/felix/ipsets/ipsets_test.go
@@ -1,4 +1,4 @@
-// Copyright (c) 2016-2024 Tigera, Inc. All rights reserved.
+// Copyright (c) 2016-2025 Tigera, Inc. All rights reserved.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -464,6 +464,45 @@ var _ = Describe("IP sets dataplane", func() {
 		})
 	})
 
+	It("Calico IP sets with unsupported revision and in desired sets should be re-created", func() {
+		dataplane.IPSetMetadata = map[string]setMetadata{
+			v4MainIPSetName: {
+				Name:     v4MainIPSetName,
+				Family:   "inet",
+				Type:     IPSetTypeHashIP,
+				MaxSize:  1234,
+				Revision: supportedMockRevision + 1,
+			},
+			v4MainIPSetName2: {
+				Name:     v4MainIPSetName2,
+				Family:   "inet",
+				Type:     IPSetTypeHashIP,
+				MaxSize:  1234,
+				Revision: supportedMockRevision + 1,
+			},
+			v4MainIPSetName3: {
+				Name:     v4MainIPSetName3,
+				Family:   "inet",
+				Type:     IPSetTypeHashIP,
+				MaxSize:  1234,
+				Revision: supportedMockRevision,
+			},
+		}
+		dataplane.IPSetMembers[v4MainIPSetName] = set.From("10.0.0.1", "10.0.0.3")
+		dataplane.IPSetMembers[v4MainIPSetName2] = set.From("10.0.0.1", "10.0.0.4")
+		dataplane.IPSetMembers[v4MainIPSetName3] = set.From("10.0.0.5", "10.0.0.6")
+
+		ipsets.AddOrReplaceIPSet(meta, []string{"10.0.0.1", "10.0.0.2"})
+		ipsets.AddOrReplaceIPSet(meta3, []string{"10.0.0.5", "10.0.0.6"})
+
+		apply()
+		dataplane.ExpectMembers(map[string][]string{
+			v4MainIPSetName: []string{"10.0.0.1", "10.0.0.2"},
+			// v4MainIPSetName2 should be destroyed since it's not in the desired state.
+			v4MainIPSetName3: []string{"10.0.0.5", "10.0.0.6"}, // This IPSet should not be touched.
+		})
+	})
+
 	Describe("with many left-over IP sets in place", func() {
 		BeforeEach(func() {
 			for i := 0; i < MaxIPSetDeletionsPerIteration*3; i++ {
@@ -795,7 +834,7 @@ var _ = Describe("IP sets dataplane", func() {
 
 				It("should be detected after many transient errors", func() {
 					// Simulate lots of transient failures in a row, followed by success.
-					dataplane.RestoreOpFailures = slices.Repeat([]string{"write-ip"}, 6)
+					dataplane.RestoreOpFailures = slices.Repeat([]string{"write-ip"}, (MaxRetryAttempt/2)+1)
 					// Trigger an update to only one IP set.
 					ipsets.AddMembers(ipSetID2, []string{"10.0.0.4"})
 					apply()
diff --git a/felix/ipsets/utils_for_test.go b/felix/ipsets/utils_for_test.go
@@ -1,4 +1,4 @@
-// Copyright (c) 2016-2024 Tigera, Inc. All rights reserved.
+// Copyright (c) 2016-2025 Tigera, Inc. All rights reserved.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -36,6 +36,10 @@ import (
 
 // This file contains shared test infrastructure for testing the ipsets package.
 
+const (
+	supportedMockRevision = 5
+)
+
 var (
 	transientFailure = errors.New("Simulated transient failure")
 	permanentFailure = errors.New("Simulated permanent failure")
@@ -454,6 +458,7 @@ type setMetadata struct {
 	Name     string
 	Family   IPFamily
 	Type     IPSetType
+	Revision int
 	MaxSize  int
 	RangeMin int
 	RangeMax int
@@ -726,18 +731,26 @@ func (c *listCmd) main() {
 		result = fmt.Errorf("ipset %v does not exists", c.SetName)
 		return
 	}
-	writef("Name: %s\n", c.SetName)
 	meta, ok := c.Dataplane.IPSetMetadata[c.SetName]
 	if !ok {
 		// Default metadata for IP sets created by tests.
 		meta = setMetadata{
-			Name:    v4MainIPSetName,
-			Family:  IPFamilyV4,
-			Type:    IPSetTypeHashIP,
-			MaxSize: 1234,
+			Name:     v4MainIPSetName,
+			Family:   IPFamilyV4,
+			Type:     IPSetTypeHashIP,
+			Revision: supportedMockRevision,
+			MaxSize:  1234,
 		}
 	}
+
+	if meta.Revision > supportedMockRevision {
+		result = fmt.Errorf("revision %v not supported", meta.Revision)
+		return
+	}
+
+	writef("Name: %s\n", c.SetName)
 	writef("Type: %s\n", meta.Type)
+	writef("Revision: %d\n", meta.Revision)
 	if meta.Type == IPSetTypeBitmapPort {
 		writef("Header: family %s range %d-%d\n", meta.Family, meta.RangeMin, meta.RangeMax)
 	} else if meta.Type == "unknown:type" {
