Skip to content

Commit 0d12031

Browse files
clobranoclobrano
committed
Fix: Handle missing PodIP in Node assignment
The logic to assign PodIP to Node is extracted into a dedicated function, to allow unit testing. Respect the original logic, the function now ensures only valid PodIP are assigned to all Nodes, and logs and returns an error either if the matching PodIP is missing/empty, or if there is no matching Pod available. NOTE: Even when an error is returned, all valid IPs are also returned, so that SNR can request a peer update quicker than the default interval, and also use as much as valid Nodes it can find. fix: RHWA-13
1 parent ee8985c commit 0d12031

File tree

2 files changed

+271
-10
lines changed

2 files changed

+271
-10
lines changed

‎pkg/peers/peers.go‎

Lines changed: 25 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package peers
22

33
import (
44
"context"
5+
"errors"
56
"fmt"
67
"sync"
78
"time"
@@ -11,7 +12,7 @@ import (
1112
pkgerrors "github.com/pkg/errors"
1213

1314
v1 "k8s.io/api/core/v1"
14-
"k8s.io/apimachinery/pkg/api/errors"
15+
apierrors "k8s.io/apimachinery/pkg/api/errors"
1516
"k8s.io/apimachinery/pkg/labels"
1617
"k8s.io/apimachinery/pkg/selection"
1718
"k8s.io/apimachinery/pkg/util/wait"
@@ -122,7 +123,7 @@ func (p *Peers) updatePeers(ctx context.Context, getSelector func() labels.Selec
122123
nodes := v1.NodeList{}
123124
// get some nodes, but not ourself
124125
if err := p.List(readerCtx, &nodes, client.MatchingLabelsSelector{Selector: getSelector()}); err != nil {
125-
if errors.IsNotFound(err) {
126+
if apierrors.IsNotFound(err) {
126127
// we are the only node at the moment... reset peerList
127128
p.workerPeersAddresses = []v1.PodIP{}
128129
}
@@ -142,20 +143,34 @@ func (p *Peers) updatePeers(ctx context.Context, getSelector func() labels.Selec
142143
return pkgerrors.Wrap(err, "could not get pods")
143144
}
144145

145-
nodesCount := len(nodes.Items)
146-
addresses := make([]v1.PodIP, nodesCount)
147-
for i, node := range nodes.Items {
146+
addresses, err := p.mapNodesToPrimaryPodIPs(nodes, pods)
147+
setAddresses(addresses)
148+
return err
149+
}
150+
151+
func (p *Peers) mapNodesToPrimaryPodIPs(nodes v1.NodeList, pods v1.PodList) ([]v1.PodIP, error) {
152+
var err error
153+
addresses := []v1.PodIP{}
154+
155+
for _, node := range nodes.Items {
156+
found := false
148157
for _, pod := range pods.Items {
149158
if pod.Spec.NodeName == node.Name {
150-
if pod.Status.PodIPs == nil || len(pod.Status.PodIPs) == 0 {
151-
return pkgerrors.New(fmt.Sprintf("empty Pod IP for Pod %s on Node %s", pod.Name, node.Name))
159+
if len(pod.Status.PodIPs) == 0 || pod.Status.PodIPs[0].IP == "" {
160+
err = errors.Join(err, fmt.Errorf("empty IP for Pod %s on Node %s", pod.Name, node.Name))
161+
} else {
162+
found = true
163+
addresses = append(addresses, pod.Status.PodIPs[0])
152164
}
153-
addresses[i] = pod.Status.PodIPs[0]
165+
break
154166
}
155167
}
168+
if !found {
169+
err = errors.Join(err, fmt.Errorf("Node %s has no matching Pod", node.Name))
170+
}
156171
}
157-
setAddresses(addresses)
158-
return nil
172+
173+
return addresses, err
159174
}
160175

161176
func (p *Peers) GetPeersAddresses(role Role) []v1.PodIP {

‎pkg/peers/peers_test.go‎

Lines changed: 246 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,246 @@
1+
package peers
2+
3+
import (
4+
"reflect"
5+
"testing"
6+
7+
"github.com/go-logr/logr"
8+
9+
v1 "k8s.io/api/core/v1"
10+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
11+
)
12+
13+
func TestMapNodesToPrimaryPodIPs(t *testing.T) {
14+
testCases := []struct {
15+
name string
16+
nodes v1.NodeList
17+
pods v1.PodList
18+
expectedIPs []v1.PodIP // Expected list of *valid* IPs
19+
expectError bool
20+
}{
21+
{
22+
name: "Empty lists",
23+
nodes: v1.NodeList{},
24+
pods: v1.PodList{},
25+
expectedIPs: []v1.PodIP{},
26+
expectError: false,
27+
},
28+
{
29+
name: "Nodes exist, no pods (should return error)",
30+
nodes: v1.NodeList{
31+
Items: []v1.Node{
32+
{ObjectMeta: metav1.ObjectMeta{Name: "node1"}},
33+
{ObjectMeta: metav1.ObjectMeta{Name: "node2"}},
34+
},
35+
},
36+
pods: v1.PodList{}, // Empty pods list
37+
expectedIPs: []v1.PodIP{}, // No matching pods means no IPs can be found
38+
expectError: true,
39+
},
40+
{
41+
name: "Single node, single matching pod, one IP",
42+
nodes: v1.NodeList{
43+
Items: []v1.Node{
44+
{ObjectMeta: metav1.ObjectMeta{Name: "node1"}},
45+
},
46+
},
47+
pods: v1.PodList{
48+
Items: []v1.Pod{
49+
{
50+
ObjectMeta: metav1.ObjectMeta{Name: "pod1"},
51+
Spec: v1.PodSpec{NodeName: "node1"},
52+
Status: v1.PodStatus{PodIPs: []v1.PodIP{{IP: "10.0.0.1"}}},
53+
},
54+
},
55+
},
56+
expectedIPs: []v1.PodIP{{IP: "10.0.0.1"}},
57+
expectError: false,
58+
},
59+
{
60+
name: "Multiple nodes, all with a valid matching pod",
61+
nodes: v1.NodeList{
62+
Items: []v1.Node{
63+
{ObjectMeta: metav1.ObjectMeta{Name: "node1"}}, // Has matching pod
64+
{ObjectMeta: metav1.ObjectMeta{Name: "node2"}}, // Has matching pod
65+
},
66+
},
67+
pods: v1.PodList{
68+
Items: []v1.Pod{
69+
{ObjectMeta: metav1.ObjectMeta{Name: "pod1"}, Spec: v1.PodSpec{NodeName: "node1"}, Status: v1.PodStatus{PodIPs: []v1.PodIP{{IP: "10.0.0.1"}}}},
70+
{ObjectMeta: metav1.ObjectMeta{Name: "pod2"}, Spec: v1.PodSpec{NodeName: "node2"}, Status: v1.PodStatus{PodIPs: []v1.PodIP{{IP: "10.0.0.2"}}}},
71+
},
72+
},
73+
expectedIPs: []v1.PodIP{{IP: "10.0.0.1"}, {IP: "10.0.0.2"}},
74+
expectError: false,
75+
},
76+
{
77+
name: "Error: Multiple nodes, one has no matching pod (should return error and skip node)",
78+
nodes: v1.NodeList{
79+
Items: []v1.Node{
80+
{ObjectMeta: metav1.ObjectMeta{Name: "node1"}}, // Has matching pod
81+
{ObjectMeta: metav1.ObjectMeta{Name: "node2"}}, // No matching pod in pods list
82+
},
83+
},
84+
pods: v1.PodList{
85+
Items: []v1.Pod{
86+
{ObjectMeta: metav1.ObjectMeta{Name: "pod1"}, Spec: v1.PodSpec{NodeName: "node1"}, Status: v1.PodStatus{PodIPs: []v1.PodIP{{IP: "10.0.0.1"}}}},
87+
},
88+
},
89+
expectedIPs: []v1.PodIP{{IP: "10.0.0.1"}}, // Only the IP for node1 is returned
90+
expectError: true,
91+
},
92+
{
93+
name: "Error: Matching pod found, PodIPs is nil (should return error and skip node)",
94+
nodes: v1.NodeList{
95+
Items: []v1.Node{
96+
{ObjectMeta: metav1.ObjectMeta{Name: "node1"}},
97+
},
98+
},
99+
pods: v1.PodList{
100+
Items: []v1.Pod{
101+
{
102+
ObjectMeta: metav1.ObjectMeta{Name: "pod1-nil"},
103+
Spec: v1.PodSpec{NodeName: "node1"},
104+
Status: v1.PodStatus{PodIPs: nil}, // PodIPs is nil - should set error and skip
105+
},
106+
},
107+
},
108+
expectedIPs: []v1.PodIP{}, // No valid IPs found
109+
expectError: true,
110+
},
111+
{
112+
name: "Error: Matching pod found, PodIPs is empty slice (should return error and skip node)",
113+
nodes: v1.NodeList{
114+
Items: []v1.Node{
115+
{ObjectMeta: metav1.ObjectMeta{Name: "node1"}},
116+
},
117+
},
118+
pods: v1.PodList{
119+
Items: []v1.Pod{
120+
{
121+
ObjectMeta: metav1.ObjectMeta{Name: "pod1-empty"},
122+
Spec: v1.PodSpec{NodeName: "node1"},
123+
Status: v1.PodStatus{PodIPs: []v1.PodIP{}}, // PodIPs is empty slice - should set error and skip
124+
},
125+
},
126+
},
127+
expectedIPs: []v1.PodIP{}, // No valid IPs found
128+
expectError: true,
129+
},
130+
{
131+
name: "Error: Matching pod found, Primary IP is empty string (should return error and skip node)",
132+
nodes: v1.NodeList{
133+
Items: []v1.Node{
134+
{ObjectMeta: metav1.ObjectMeta{Name: "node1"}},
135+
},
136+
},
137+
pods: v1.PodList{
138+
Items: []v1.Pod{
139+
{
140+
ObjectMeta: metav1.ObjectMeta{Name: "pod1-emptyip"},
141+
Spec: v1.PodSpec{NodeName: "node1"},
142+
Status: v1.PodStatus{PodIPs: []v1.PodIP{{IP: ""}, {IP: "2.2.2.2"}}}, // Primary IP is empty string - should set error and skip
143+
},
144+
},
145+
},
146+
expectedIPs: []v1.PodIP{}, // No valid IPs found
147+
expectError: true,
148+
},
149+
{
150+
name: "Error: Multiple nodes, one has error (nil IPs), others are fine (should return error and valid IPs)",
151+
nodes: v1.NodeList{
152+
Items: []v1.Node{
153+
{ObjectMeta: metav1.ObjectMeta{Name: "nodeA"}}, // Has error pod
154+
{ObjectMeta: metav1.ObjectMeta{Name: "nodeB"}}, // Fine pod
155+
{ObjectMeta: metav1.ObjectMeta{Name: "nodeC"}}, // Fine pod
156+
},
157+
},
158+
pods: v1.PodList{
159+
Items: []v1.Pod{
160+
{ObjectMeta: metav1.ObjectMeta{Name: "podA-err"}, Spec: v1.PodSpec{NodeName: "nodeA"}, Status: v1.PodStatus{PodIPs: nil}}, // Error
161+
{ObjectMeta: metav1.ObjectMeta{Name: "podB-ok"}, Spec: v1.PodSpec{NodeName: "nodeB"}, Status: v1.PodStatus{PodIPs: []v1.PodIP{{IP: "10.1.0.2"}}}}, // OK
162+
{ObjectMeta: metav1.ObjectMeta{Name: "podC-ok"}, Spec: v1.PodSpec{NodeName: "nodeC"}, Status: v1.PodStatus{PodIPs: []v1.PodIP{{IP: "10.1.0.3"}}}}, // OK
163+
},
164+
},
165+
expectedIPs: []v1.PodIP{{IP: "10.1.0.2"}, {IP: "10.1.0.3"}}, // Only valid IPs returned
166+
expectError: true,
167+
},
168+
{
169+
name: "Error: Multiple nodes, one has error (empty Primary IP), others are fine (should return error and valid IPs)",
170+
nodes: v1.NodeList{
171+
Items: []v1.Node{
172+
{ObjectMeta: metav1.ObjectMeta{Name: "nodeA"}}, // Fine pod
173+
{ObjectMeta: metav1.ObjectMeta{Name: "nodeB"}}, // Has error pod
174+
{ObjectMeta: metav1.ObjectMeta{Name: "nodeC"}}, // Fine pod
175+
},
176+
},
177+
pods: v1.PodList{
178+
Items: []v1.Pod{
179+
{ObjectMeta: metav1.ObjectMeta{Name: "podA-ok"}, Spec: v1.PodSpec{NodeName: "nodeA"}, Status: v1.PodStatus{PodIPs: []v1.PodIP{{IP: "10.1.0.1"}}}}, // OK
180+
{ObjectMeta: metav1.ObjectMeta{Name: "podB-err"}, Spec: v1.PodSpec{NodeName: "nodeB"}, Status: v1.PodStatus{PodIPs: []v1.PodIP{{IP: ""}}}}, // Error
181+
{ObjectMeta: metav1.ObjectMeta{Name: "podC-ok"}, Spec: v1.PodSpec{NodeName: "nodeC"}, Status: v1.PodStatus{PodIPs: []v1.PodIP{{IP: "10.1.0.3"}}}}, // OK
182+
},
183+
},
184+
expectedIPs: []v1.PodIP{{IP: "10.1.0.1"}, {IP: "10.1.0.3"}}, // Only valid IPs returned
185+
expectError: true,
186+
},
187+
{
188+
name: "Error: Multiple nodes, multiple errors (should return error and valid IPs)",
189+
nodes: v1.NodeList{
190+
Items: []v1.Node{
191+
{ObjectMeta: metav1.ObjectMeta{Name: "nodeA"}}, // Error 1 (nil IPs)
192+
{ObjectMeta: metav1.ObjectMeta{Name: "nodeB"}}, // Fine
193+
{ObjectMeta: metav1.ObjectMeta{Name: "nodeC"}}, // Error 2 (empty Primary IP)
194+
},
195+
},
196+
pods: v1.PodList{
197+
Items: []v1.Pod{
198+
{ObjectMeta: metav1.ObjectMeta{Name: "podA-err"}, Spec: v1.PodSpec{NodeName: "nodeA"}, Status: v1.PodStatus{PodIPs: nil}}, // Error 1
199+
{ObjectMeta: metav1.ObjectMeta{Name: "podB-ok"}, Spec: v1.PodSpec{NodeName: "nodeB"}, Status: v1.PodStatus{PodIPs: []v1.PodIP{{IP: "10.1.0.2"}}}}, // OK
200+
{ObjectMeta: metav1.ObjectMeta{Name: "podC-err"}, Spec: v1.PodSpec{NodeName: "nodeC"}, Status: v1.PodStatus{PodIPs: []v1.PodIP{{IP: ""}}}}, // Error 2
201+
},
202+
},
203+
expectedIPs: []v1.PodIP{{IP: "10.1.0.2"}}, // Only valid IPs returned
204+
expectError: true,
205+
},
206+
}
207+
208+
for _, tc := range testCases {
209+
t.Run(tc.name, func(t *testing.T) {
210+
p := Peers{
211+
log: logr.Logger{},
212+
}
213+
actualIPs, actualErr := p.mapNodesToPrimaryPodIPs(tc.nodes, tc.pods)
214+
215+
// Check if error expectation matches
216+
if tc.expectError {
217+
if actualErr == nil {
218+
t.Errorf("Expected an error but got none")
219+
}
220+
} else { // Not expecting an error
221+
if actualErr != nil {
222+
t.Errorf("Expected no error but got: %v", actualErr)
223+
}
224+
}
225+
226+
// Check if the returned slice contains only valid IPs and matches the expected list
227+
// This assertion applies to BOTH success and error cases based on the intended behavior.
228+
if !reflect.DeepEqual(actualIPs, tc.expectedIPs) {
229+
t.Errorf("Result slice mismatch. Expected: %v, Got: %v", tc.expectedIPs, actualIPs)
230+
}
231+
232+
// Additional check for the length requirement (length <= input node list length)
233+
if len(actualIPs) > len(tc.nodes.Items) {
234+
t.Errorf("Returned slice length (%d) is greater than input node list length (%d)", len(actualIPs), len(tc.nodes.Items))
235+
}
236+
237+
// Additional check: Ensure no empty IPs are present in the returned slice
238+
for i, ip := range actualIPs {
239+
if ip.IP == "" {
240+
t.Errorf("Returned slice contains an empty IP at index %d: %v", i, actualIPs)
241+
break // No need to check further if one is found
242+
}
243+
}
244+
})
245+
}
246+
}

0 commit comments

Comments
 (0)