Skip to content

Commit 6ca8ab5

Browse files
openshift-merge-bot[bot]openshift-merge-bot[bot]
authored
Merge pull request #259 from clobrano/rhwa-13-skip-nodes-without-ip
Fix: Handle missing PodIP in Node assignment
2 parents ee8985c + 0d12031 commit 6ca8ab5

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)