Skip to content

Commit f948149

Browse files
committed
fix: a claim's recorded subnet follows the class
A claim keeps its slot for the life of the node pair, and the link addresses are derived from the class subnet and that slot every pass, never read back from the claim. Changing a class's subnet therefore left the recorded text naming a /31 nothing carried, so the status described a link that did not exist while routing was correct. gcClaims already rewrites existing claims to set and clear UnusedSince, so the correction goes there: when the /31 the class and slot now yield differs from the recorded one, the claim is rewritten with it. The slot is untouched, since only the text was ever wrong. A claim that already reads correctly produces no update, so a pass over a settled class still writes nothing.
1 parent 3d7792d commit f948149

2 files changed

Lines changed: 76 additions & 10 deletions

File tree

internal/reconcile/links.go

Lines changed: 31 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -285,23 +285,44 @@ func sortedPair(a, b string) [2]string {
285285
// sets it once to now; a pair unused for more than 24h is dropped. Any selecting
286286
// agent may run this; the write conflicts sort themselves out.
287287
func gcClaims(class *v1alpha1.PortMapClass, needed map[string]neededPair, now metav1.Time) (updates []v1alpha1.LinkAllocation, drops []string) {
288+
subnet, subnetErr := parseClassSubnet(class)
289+
288290
for i := range class.Status.Links {
289291
la := class.Status.Links[i]
290292
_, isNeeded := needed[la.Key]
293+
294+
if !isNeeded && la.UnusedSince != nil && now.Sub(la.UnusedSince.Time) > 24*time.Hour {
295+
drops = append(drops, la.Key)
296+
continue
297+
}
298+
299+
cp := la
300+
changed := false
301+
291302
switch {
292-
case isNeeded:
293-
if la.UnusedSince != nil {
294-
cp := la
295-
cp.UnusedSince = nil
296-
updates = append(updates, cp)
297-
}
298-
case la.UnusedSince == nil:
299-
cp := la
303+
case isNeeded && la.UnusedSince != nil:
304+
cp.UnusedSince = nil
305+
changed = true
306+
case !isNeeded && la.UnusedSince == nil:
300307
t := now
301308
cp.UnusedSince = &t
309+
changed = true
310+
}
311+
312+
// A link's addresses are derived from the class subnet and the slot
313+
// every pass, never read back from here, so changing the subnet leaves
314+
// this field naming a /31 nothing carries. A claim keeps its slot, so
315+
// correcting the text is the whole repair.
316+
if subnetErr == nil && int(la.Slot) >= 0 && int(la.Slot) < slotCount(subnet) {
317+
lo, _ := nthSlash31(subnet, int(la.Slot))
318+
if want := netip.PrefixFrom(lo, 31).String(); want != cp.Subnet {
319+
cp.Subnet = want
320+
changed = true
321+
}
322+
}
323+
324+
if changed {
302325
updates = append(updates, cp)
303-
case now.Sub(la.UnusedSince.Time) > 24*time.Hour:
304-
drops = append(drops, la.Key)
305326
}
306327
}
307328
sort.Slice(updates, func(i, j int) bool { return updates[i].Key < updates[j].Key })

internal/reconcile/links_test.go

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -193,4 +193,49 @@ func TestLinkNameIsPerClass(t *testing.T) {
193193
}
194194
}
195195

196+
// TestGCClaimsCorrectsStaleSubnet: a claim keeps its slot for the life of the
197+
// pair, and the link addresses are derived from the class subnet and that slot
198+
// every pass. Changing the subnet therefore leaves the recorded text naming a
199+
// /31 nothing carries, which reads as a link that does not exist.
200+
func TestGCClaimsCorrectsStaleSubnet(t *testing.T) {
201+
cls := class("public", []string{"node-a"},
202+
withSubnet("169.254.79.0/24"),
203+
withLinks(claim("node-a", "node-b", 1)), // claim() records 169.254.77.0/31
204+
)
205+
needed := map[string]neededPair{
206+
pairKey("node-a", "node-b"): {key: pairKey("node-a", "node-b")},
207+
}
208+
209+
updates, drops := gcClaims(cls, needed, metav1.NewTime(baseTime))
210+
if len(drops) != 0 {
211+
t.Fatalf("drops = %v, want none for a needed pair", drops)
212+
}
213+
if len(updates) != 1 {
214+
t.Fatalf("updates = %+v, want the claim rewritten", updates)
215+
}
216+
// Slot 1 of 169.254.79.0/24 is the second /31.
217+
if got, want := updates[0].Subnet, "169.254.79.2/31"; got != want {
218+
t.Errorf("subnet = %s, want %s", got, want)
219+
}
220+
if updates[0].Slot != 1 {
221+
t.Errorf("slot = %d, want 1 kept; only the text was wrong", updates[0].Slot)
222+
}
223+
}
224+
225+
// TestGCClaimsLeavesACorrectSubnetAlone keeps the pass idempotent: rewriting a
226+
// claim that already reads correctly would write to the API every reconcile.
227+
func TestGCClaimsLeavesACorrectSubnetAlone(t *testing.T) {
228+
cls := class("public", []string{"node-a"},
229+
withLinks(claim("node-a", "node-b", 0)), // 169.254.77.0/31, the default subnet's slot 0
230+
)
231+
needed := map[string]neededPair{
232+
pairKey("node-a", "node-b"): {key: pairKey("node-a", "node-b")},
233+
}
234+
235+
updates, _ := gcClaims(cls, needed, metav1.NewTime(baseTime))
236+
if len(updates) != 0 {
237+
t.Errorf("updates = %+v, want none when nothing changed", updates)
238+
}
239+
}
240+
196241
var in0now = metav1.NewTime(baseTime)

0 commit comments

Comments
 (0)