Skip to content

Commit 667743d

Browse files
huntergregoryhuntergregory
authored
fix: [NPM] fix incorrect ipset create, fix 1-off prometheus logic, and add cache checking for UTs (#1202)
* wip * dont touch v1 metrics code and fix lints * add comment and comment code to resolve lint * optimize looping and dirty cache updates * address comments and change param type of modifyCacheForKernelMemberUpdate to reduce map lookups * add exec time metrics * UTs * fix lints * initialize metrics in policymanager tests * fix bug in publishing npm logs
1 parent 24c41bd commit 667743d

File tree

9 files changed

+1036
-232
lines changed

9 files changed

+1036
-232
lines changed

‎.pipelines/npm/npm-conformance-tests.yaml‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -187,9 +187,10 @@ jobs:
187187
curl -LO https://dl.k8s.io/release/v1.20.0/bin/linux/amd64/kubectl
188188
chmod +x kubectl
189189
npmPodList=`kubectl get pods -n kube-system | grep npm | awk '{print $1}'`
190-
mkdir -p $(System.DefaultWorkingDirectory)/npmLogs_$(PROFILE)
191-
for npm in $npmPodList; do ./kubectl logs -n kube-system $npm --kubeconfig=./kubeconfig > $(System.DefaultWorkingDirectory)/npmLogs/$npm ;done
192-
mv ./kubeconfig $(System.DefaultWorkingDirectory)/npmLogs_$(PROFILE)/kubeconfig
190+
npmLogsFolder=$(System.DefaultWorkingDirectory)/npmLogs_$(PROFILE)
191+
mkdir -p $npmLogsFolder
192+
for npm in $npmPodList; do ./kubectl logs -n kube-system $npm --kubeconfig=./kubeconfig > $npmLogsFolder/$npm ;done
193+
mv ./kubeconfig $npmLogsFolder/kubeconfig
193194
displayName: "Gather NPM Logs"
194195
condition: always()
195196

‎npm/metrics/prometheus-metrics.go‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,14 @@ func InitializeAll() {
7777
}
7878
}
7979

80+
// ReinitializeAll creates/replaces Prometheus metrics. This function is intended for UTs.
81+
// Be sure to reset helper variables e.g. ipsetInventoryMap.
82+
func ReinitializeAll() {
83+
haveInitialized = false
84+
InitializeAll()
85+
ipsetInventoryMap = make(map[string]int)
86+
}
87+
8088
// GetHandler returns the HTTP handler for the metrics endpoint
8189
func GetHandler(isNodeLevel bool) http.Handler {
8290
return promhttp.HandlerFor(getRegistry(isNodeLevel), promhttp.HandlerOpts{})

‎npm/pkg/dataplane/dataplane_test.go‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@ import (
88
"github.com/Azure/azure-container-networking/npm/metrics"
99
"github.com/Azure/azure-container-networking/npm/pkg/dataplane/ipsets"
1010
"github.com/Azure/azure-container-networking/npm/pkg/dataplane/policies"
11-
"github.com/Azure/azure-container-networking/npm/util"
1211
testutils "github.com/Azure/azure-container-networking/test/utils"
1312
"github.com/stretchr/testify/assert"
1413
"github.com/stretchr/testify/require"
@@ -27,11 +26,6 @@ var (
2726
},
2827
}
2928

30-
fakeIPSetRestoreSuccess = testutils.TestCmd{
31-
Cmd: []string{util.Ipset, util.IpsetRestoreFlag},
32-
ExitCode: 0,
33-
}
34-
3529
setPodKey1 = &ipsets.TranslatedIPSet{
3630
Metadata: ipsets.NewIPSetMetadata("setpodkey1", ipsets.KeyLabelOfPod),
3731
}

‎npm/pkg/dataplane/ipsets/ipset_test.go‎

Lines changed: 134 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,134 @@
1+
package ipsets
2+
3+
import (
4+
"testing"
5+
6+
"github.com/stretchr/testify/require"
7+
)
8+
9+
func TestShouldBeInKernelAndCanDelete(t *testing.T) {
10+
s := &IPSetMetadata{"test-set", Namespace}
11+
l := &IPSetMetadata{"test-list", KeyLabelOfNamespace}
12+
tests := []struct {
13+
name string
14+
set *IPSet
15+
wantInKernel bool
16+
wantDeletable bool
17+
}{
18+
{
19+
name: "only has selector reference",
20+
set: &IPSet{
21+
Name: s.GetPrefixName(),
22+
SetProperties: SetProperties{
23+
Type: s.Type,
24+
Kind: s.GetSetKind(),
25+
},
26+
SelectorReference: map[string]struct{}{
27+
"ref-1": {},
28+
},
29+
},
30+
wantInKernel: true,
31+
wantDeletable: false,
32+
},
33+
{
34+
name: "only has netpol reference",
35+
set: &IPSet{
36+
Name: s.GetPrefixName(),
37+
SetProperties: SetProperties{
38+
Type: s.Type,
39+
Kind: s.GetSetKind(),
40+
},
41+
NetPolReference: map[string]struct{}{
42+
"ref-1": {},
43+
},
44+
},
45+
wantInKernel: true,
46+
wantDeletable: false,
47+
},
48+
{
49+
name: "only referenced in list (in kernel)",
50+
set: &IPSet{
51+
Name: s.GetPrefixName(),
52+
SetProperties: SetProperties{
53+
Type: s.Type,
54+
Kind: s.GetSetKind(),
55+
},
56+
ipsetReferCount: 1,
57+
kernelReferCount: 1,
58+
},
59+
wantInKernel: true,
60+
wantDeletable: false,
61+
},
62+
{
63+
name: "only referenced in list (not in kernel)",
64+
set: &IPSet{
65+
Name: s.GetPrefixName(),
66+
SetProperties: SetProperties{
67+
Type: s.Type,
68+
Kind: s.GetSetKind(),
69+
},
70+
ipsetReferCount: 1,
71+
},
72+
wantInKernel: false,
73+
wantDeletable: false,
74+
},
75+
{
76+
name: "only has set members",
77+
set: &IPSet{
78+
Name: l.GetPrefixName(),
79+
SetProperties: SetProperties{
80+
Type: l.Type,
81+
Kind: l.GetSetKind(),
82+
},
83+
MemberIPSets: map[string]*IPSet{
84+
s.GetPrefixName(): NewIPSet(s),
85+
},
86+
},
87+
wantInKernel: false,
88+
wantDeletable: false,
89+
},
90+
{
91+
name: "only has ip members",
92+
set: &IPSet{
93+
Name: s.GetPrefixName(),
94+
SetProperties: SetProperties{
95+
Type: s.Type,
96+
Kind: s.GetSetKind(),
97+
},
98+
IPPodKey: map[string]string{
99+
"1.2.3.4": "pod-a",
100+
},
101+
},
102+
wantInKernel: false,
103+
wantDeletable: false,
104+
},
105+
{
106+
name: "deletable",
107+
set: &IPSet{
108+
Name: s.GetPrefixName(),
109+
SetProperties: SetProperties{
110+
Type: Namespace,
111+
Kind: s.GetSetKind(),
112+
},
113+
},
114+
wantInKernel: false,
115+
wantDeletable: true,
116+
},
117+
}
118+
for _, tt := range tests {
119+
tt := tt
120+
t.Run(tt.name, func(t *testing.T) {
121+
if tt.wantInKernel {
122+
require.True(t, tt.set.shouldBeInKernel())
123+
} else {
124+
require.False(t, tt.set.shouldBeInKernel())
125+
}
126+
127+
if tt.wantDeletable {
128+
require.True(t, tt.set.canBeDeleted())
129+
} else {
130+
require.False(t, tt.set.canBeDeleted())
131+
}
132+
})
133+
}
134+
}

0 commit comments

Comments
 (0)