Skip to content

Commit 9aad0c0

Browse files
waltherleewaltherlee
committed
fix resource duplication in store builder
Signed-off-by: Walther Lee <[email protected]>
1 parent 012121e commit 9aad0c0

File tree

2 files changed

+176
-33
lines changed

2 files changed

+176
-33
lines changed

‎internal/store/builder.go

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -107,18 +107,26 @@ func (b *Builder) WithMetrics(r prometheus.Registerer) {
107107

108108
// WithEnabledResources sets the enabledResources property of a Builder.
109109
func (b *Builder) WithEnabledResources(r []string) error {
110+
enabledResources := map[string]bool{}
111+
for _, er := range b.enabledResources {
112+
enabledResources[er] = true
113+
}
114+
var newResources []string
110115
for _, resource := range r {
116+
// validate that the resource exists and skip those that are already enabled
111117
if !resourceExists(resource) {
112118
return fmt.Errorf("resource %s does not exist. Available resources: %s", resource, strings.Join(availableResources(), ","))
113119
}
120+
if _, ok := enabledResources[resource]; !ok {
121+
newResources = append(newResources, resource)
122+
}
123+
}
124+
if len(newResources) == 0 {
125+
return nil
114126
}
115127

116-
var sortedResources []string
117-
sortedResources = append(sortedResources, r...)
118-
119-
sort.Strings(sortedResources)
120-
121-
b.enabledResources = append(b.enabledResources, sortedResources...)
128+
b.enabledResources = append(b.enabledResources, newResources...)
129+
sort.Strings(b.enabledResources)
122130
return nil
123131
}
124132

‎tests/e2e/discovery_test.go

Lines changed: 162 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -49,31 +49,47 @@ func TestVariableVKsDiscoveryAndResolution(t *testing.T) {
4949
if err != nil {
5050
t.Fatal(err)
5151
}
52-
crdFile, err := os.CreateTemp("", "crd.yaml")
52+
initCrdFile, err := os.CreateTemp("", "crd.yaml")
5353
if err != nil {
5454
t.Fatal(err)
5555
}
56-
crFile, err := os.CreateTemp("", "cr.yaml")
56+
initCrFile, err := os.CreateTemp("", "cr.yaml")
5757
if err != nil {
5858
t.Fatal(err)
5959
}
60-
klog.InfoS("testdata", "crConfigFile", crConfigFile.Name(), "crdFile", crdFile.Name(), "crFile", crFile.Name())
60+
newCrdFile, err := os.CreateTemp("", "new-crd.yaml")
61+
if err != nil {
62+
t.Fatal(err)
63+
}
64+
newCrFile, err := os.CreateTemp("", "new-cr.yaml")
65+
if err != nil {
66+
t.Fatal(err)
67+
}
68+
klog.InfoS("testdata", "crConfigFile", crConfigFile.Name(), "initCrdFile", initCrdFile.Name(), "initCrFile", initCrFile.Name(), "newCrdFile", newCrdFile.Name(), "newCrFile", newCrFile.Name())
6169

6270
// Delete artefacts.
6371
defer func() {
6472
err := os.Remove(crConfigFile.Name())
6573
if err != nil {
6674
t.Fatalf("failed to remove CR config: %v", err)
6775
}
68-
err = os.Remove(crdFile.Name())
76+
err = os.Remove(initCrdFile.Name())
77+
if err != nil {
78+
t.Fatalf("failed to remove initial CRD manifest: %v", err)
79+
}
80+
err = os.Remove(initCrFile.Name())
81+
if err != nil {
82+
t.Fatalf("failed to remove initial CR manifest: %v", err)
83+
}
84+
err = os.Remove(newCrdFile.Name())
6985
if err != nil {
70-
t.Fatalf("failed to remove CRD manifest: %v", err)
86+
t.Fatalf("failed to remove new CRD manifest: %v", err)
7187
}
72-
err = os.Remove(crFile.Name())
88+
err = os.Remove(newCrFile.Name())
7389
if err != nil {
74-
t.Fatalf("failed to remove CR manifest: %v", err)
90+
t.Fatalf("failed to remove new CR manifest: %v", err)
7591
}
76-
klog.InfoS("deleted artefacts", "crConfigFile", crConfigFile.Name(), "crdFile", crdFile.Name(), "crFile", crFile.Name())
92+
klog.InfoS("deleted artefacts", "crConfigFile", crConfigFile.Name(), "initCrdFile", initCrdFile.Name(), "initCrFile", initCrFile.Name(), "newCrdFile", newCrdFile.Name(), "newCrFile", newCrFile.Name())
7793
}()
7894

7995
// Populate options, and parse them.
@@ -114,32 +130,92 @@ func TestVariableVKsDiscoveryAndResolution(t *testing.T) {
114130
klog.InfoS("port 8080 up")
115131

116132
// Create CRD and CR files.
117-
crd := getCRD()
118-
cr := getCR()
119-
err = os.WriteFile(crdFile.Name(), []byte(crd), 0600 /* rw------- */)
133+
initCr := getCR()
134+
initCrd := getCRD()
135+
136+
newCr := getNewCR()
137+
newCrd := getNewCRD()
138+
err = os.WriteFile(initCrdFile.Name(), []byte(initCrd), 0600 /* rw------- */)
120139
if err != nil {
121-
t.Fatalf("cannot write to crd file: %v", err)
140+
t.Fatalf("cannot write to initial crd file: %v", err)
122141
}
123-
err = os.WriteFile(crFile.Name(), []byte(cr), 0600 /* rw------- */)
142+
err = os.WriteFile(initCrFile.Name(), []byte(initCr), 0600 /* rw------- */)
124143
if err != nil {
125-
t.Fatalf("cannot write to cr file: %v", err)
144+
t.Fatalf("cannot write to initial cr file: %v", err)
126145
}
127-
klog.InfoS("created CR and CRD manifests")
146+
err = os.WriteFile(newCrdFile.Name(), []byte(newCrd), 0600 /* rw------- */)
147+
if err != nil {
148+
t.Fatalf("cannot write to new crd file: %v", err)
149+
}
150+
err = os.WriteFile(newCrFile.Name(), []byte(newCr), 0600 /* rw------- */)
151+
if err != nil {
152+
t.Fatalf("cannot write to new cr file: %v", err)
153+
}
154+
klog.InfoS("created initial and new CR and CRD manifests")
128155

129-
// Apply CRD and CR to the cluster.
130-
err = exec.Command("kubectl", "apply", "-f", crdFile.Name()).Run() //nolint:gosec
156+
// Apply initial CRD and CR to the cluster.
157+
err = exec.Command("kubectl", "apply", "-f", initCrdFile.Name()).Run() //nolint:gosec
131158
if err != nil {
132-
t.Fatalf("failed to apply crd: %v", err)
159+
t.Fatalf("failed to apply initial crd: %v", err)
133160
}
134-
err = exec.Command("kubectl", "apply", "-f", crFile.Name()).Run() //nolint:gosec
161+
err = exec.Command("kubectl", "apply", "-f", initCrFile.Name()).Run() //nolint:gosec
135162
if err != nil {
136-
t.Fatalf("failed to apply cr: %v", err)
163+
t.Fatalf("failed to apply initial cr: %v", err)
137164
}
138-
klog.InfoS("applied CR and CRD manifests")
165+
klog.InfoS("applied initial CR and CRD manifests")
139166

140167
// Wait for the metric to be available.
141168
ch := make(chan bool, 1)
142-
klog.InfoS("waiting for metrics to become available")
169+
klog.InfoS("waiting for first metrics to become available")
170+
testMetric := `kube_customresource_test_metric{customresource_group="contoso.com",customresource_kind="MyPlatform",customresource_version="v1alpha1",name="test-dotnet-app"}`
171+
err = wait.PollUntilContextTimeout(context.TODO(), discovery.Interval, PopulateTimeout, true, func(_ context.Context) (bool, error) {
172+
out, err := exec.Command("curl", "localhost:8080/metrics").Output()
173+
if err != nil {
174+
return false, err
175+
}
176+
if string(out) == "" {
177+
return false, nil
178+
}
179+
// Note: we use count to make sure that only one metrics handler is running
180+
if strings.Count(string(out), testMetric) == 1 {
181+
// klog.InfoS("metrics available", "metric", string(out))
182+
// Signal the process to exit, since we know the metrics are being generated as expected.
183+
ch <- true
184+
return true, nil
185+
}
186+
return false, nil
187+
})
188+
if err != nil {
189+
t.Fatalf("failed while waiting for initial metrics to be available: %v", err)
190+
}
191+
192+
// Wait for process to exit.
193+
select {
194+
case <-ch:
195+
t.Log("initial metrics are available")
196+
case <-time.After(PopulateTimeout * 2):
197+
t.Fatal("timed out waiting for test to pass, check the logs for more info")
198+
}
199+
200+
// Apply new CRD and CR to the cluster.
201+
err = exec.Command("kubectl", "apply", "-f", newCrdFile.Name()).Run() //nolint:gosec
202+
if err != nil {
203+
t.Fatalf("failed to apply new crd: %v", err)
204+
}
205+
err = exec.Command("kubectl", "apply", "-f", newCrFile.Name()).Run() //nolint:gosec
206+
if err != nil {
207+
t.Fatalf("failed to apply new cr: %v", err)
208+
}
209+
err = exec.Command("kubectl", "delete", "myplatform", "test-dotnet-app").Run() //nolint:gosec
210+
if err != nil {
211+
t.Fatalf("failed to delete myplatform resource: %v", err)
212+
}
213+
klog.InfoS("applied new CR and CRD manifests")
214+
215+
// Wait for the the new metric to be available
216+
ch = make(chan bool, 1)
217+
klog.InfoS("waiting for new metrics to become available")
218+
testUpdateCRDMetric := `kube_customresource_test_update_crd_metric{customresource_group="contoso.com",customresource_kind="Update",customresource_version="v1",name="test-dotnet-app-update"}`
143219
err = wait.PollUntilContextTimeout(context.TODO(), discovery.Interval, PopulateTimeout, true, func(_ context.Context) (bool, error) {
144220
out, err := exec.Command("curl", "localhost:8080/metrics").Output()
145221
if err != nil {
@@ -148,17 +224,20 @@ func TestVariableVKsDiscoveryAndResolution(t *testing.T) {
148224
if string(out) == "" {
149225
return false, nil
150226
}
151-
// Note the "{" below. This is to ensure that the metric is not in a comment.
152-
if strings.Contains(string(out), "kube_customresource_test_metric{") {
227+
// Note: we use count to make sure that only one metrics handler is running, and we also want to validate that the
228+
// new metric is available and the old one was removed, otherwise, the response could come from the
229+
// previous handler before its context was cancelled, or maybe because it failed to be cancelled.
230+
if strings.Count(string(out), testUpdateCRDMetric) == 1 && !strings.Contains(string(out), testMetric) {
153231
klog.InfoS("metrics available", "metric", string(out))
154232
// Signal the process to exit, since we know the metrics are being generated as expected.
155233
ch <- true
156234
return true, nil
157235
}
236+
klog.InfoS("metrics available", "metric", string(out))
158237
return false, nil
159238
})
160239
if err != nil {
161-
t.Fatalf("failed while waiting for metrics to be available: %v", err)
240+
t.Fatalf("failed while waiting for new metrics to be available: %v", err)
162241
}
163242

164243
// Wait for process to exit.
@@ -252,8 +331,8 @@ spec:
252331
resources:
253332
- groupVersionKind:
254333
group: "contoso.com"
255-
version: "*"
256-
kind: "*"
334+
version: "v1alpha1"
335+
kind: "MyPlatform"
257336
metrics:
258337
- name: "test_metric"
259338
help: "foo baz"
@@ -263,5 +342,61 @@ spec:
263342
path: [metadata]
264343
labelsFromPath:
265344
name: [name]
345+
- groupVersionKind:
346+
group: "contoso.com"
347+
version: "v1"
348+
kind: "Update"
349+
metrics:
350+
- name: "test_update_crd_metric"
351+
help: "foo baz"
352+
each:
353+
type: Info
354+
info:
355+
path: [metadata]
356+
labelsFromPath:
357+
name: [name]
358+
`
359+
}
360+
361+
func getNewCR() string {
362+
return `
363+
apiVersion: contoso.com/v1
364+
kind: Update
365+
metadata:
366+
name: test-dotnet-app-update
367+
spec:
368+
new: just-added
369+
`
370+
}
371+
372+
func getNewCRD() string {
373+
return `
374+
apiVersion: apiextensions.k8s.io/v1
375+
kind: CustomResourceDefinition
376+
metadata:
377+
name: updates.contoso.com
378+
spec:
379+
group: contoso.com
380+
names:
381+
plural: updates
382+
singular: update
383+
kind: Update
384+
shortNames:
385+
- updt
386+
scope: Namespaced
387+
versions:
388+
- name: v1
389+
served: true
390+
storage: true
391+
schema:
392+
openAPIV3Schema:
393+
type: object
394+
properties:
395+
spec:
396+
type: object
397+
properties:
398+
new:
399+
type: string
400+
required: ["spec"]
266401
`
267402
}

0 commit comments

Comments
 (0)