From 502a2d0d4aa5db6b8d61402730d6c4f9ee39d6cb Mon Sep 17 00:00:00 2001 From: Michael Kelly Date: Fri, 20 May 2022 09:10:36 -0700 Subject: [PATCH 1/4] Limit the amount of objects we attempt to GC on each cycle If something causes the number k8s resources to increase beyond a certain threshold, garbage collection can fail because the query to retrieve those resources will time out, resulting in a perpetual cycle of being unable to garbage collect resources. In lieu of trying to get *every* object each cycle, we can limit the number of resources retrieved per GC cycle to some reasonable number. Signed-off-by: Michael Kelly --- storage/kubernetes/client.go | 34 +++++++++++++++++++++++++++++----- storage/kubernetes/storage.go | 12 ++++++++---- 2 files changed, 37 insertions(+), 9 deletions(-) diff --git a/storage/kubernetes/client.go b/storage/kubernetes/client.go index a5a72afa..6676b0eb 100644 --- a/storage/kubernetes/client.go +++ b/storage/kubernetes/client.go @@ -15,6 +15,7 @@ import ( "io" "net" "net/http" + "net/url" "os" "path" "strconv" @@ -82,7 +83,8 @@ func offlineTokenName(userID string, connID string, h func() hash.Hash) string { return strings.TrimRight(encoding.EncodeToString(hash.Sum(nil)), "=") } -func (cli *client) urlFor(apiVersion, namespace, resource, name string) string { +func (cli *client) urlForWithParams( + apiVersion, namespace, resource, name string, params url.Values) string { basePath := "apis/" if apiVersion == "v1" { basePath = "api/" @@ -97,7 +99,19 @@ func (cli *client) urlFor(apiVersion, namespace, resource, name string) string { if strings.HasSuffix(cli.baseURL, "/") { return cli.baseURL + p } - return cli.baseURL + "/" + p + + r := cli.baseURL + "/" + p + + encodedParams := params.Encode() + if len(encodedParams) > 0 { + return r + "?" + encodedParams + } + + return r +} + +func (cli *client) urlFor(apiVersion, namespace, resource, name string) string { + return cli.urlForWithParams(apiVersion, namespace, resource, name, url.Values{}) } // Define an error interface so we can get at the underlying status code if it's @@ -163,8 +177,7 @@ func (cli *client) get(resource, name string, v interface{}) error { return cli.getResource(cli.apiVersion, cli.namespace, resource, name, v) } -func (cli *client) getResource(apiVersion, namespace, resource, name string, v interface{}) error { - url := cli.urlFor(apiVersion, namespace, resource, name) +func (cli *client) getUrl(url string, v interface{}) error { resp, err := cli.client.Get(url) if err != nil { return err @@ -176,8 +189,19 @@ func (cli *client) getResource(apiVersion, namespace, resource, name string, v i return json.NewDecoder(resp.Body).Decode(v) } +func (cli *client) getResource(apiVersion, namespace, resource, name string, v interface{}) error { + return cli.getUrl(cli.urlFor(apiVersion, namespace, resource, name), v) +} + +func (cli *client) listN(resource string, v interface{}, n int) error { + params := url.Values{} + params.Add("limit", fmt.Sprintf("%d", n)) + u := cli.urlForWithParams(cli.apiVersion, cli.namespace, resource, "", params) + return cli.getUrl(u, v) +} + func (cli *client) list(resource string, v interface{}) error { - return cli.get(resource, "", v) + return cli.listN(resource, v, -1) } func (cli *client) post(resource string, v interface{}) error { diff --git a/storage/kubernetes/storage.go b/storage/kubernetes/storage.go index ca505859..c25d7550 100644 --- a/storage/kubernetes/storage.go +++ b/storage/kubernetes/storage.go @@ -40,6 +40,10 @@ const ( resourceDeviceToken = "devicetokens" ) +const ( + gcResultLimit = 10000 +) + // Config values for the Kubernetes storage type. type Config struct { InCluster bool `json:"inCluster"` @@ -599,7 +603,7 @@ func (cli *client) UpdateConnector(id string, updater func(a storage.Connector) func (cli *client) GarbageCollect(now time.Time) (result storage.GCResult, err error) { var authRequests AuthRequestList - if err := cli.list(resourceAuthRequest, &authRequests); err != nil { + if err := cli.listN(resourceAuthRequest, &authRequests, gcResultLimit); err != nil { return result, fmt.Errorf("failed to list auth requests: %v", err) } @@ -618,7 +622,7 @@ func (cli *client) GarbageCollect(now time.Time) (result storage.GCResult, err e } var authCodes AuthCodeList - if err := cli.list(resourceAuthCode, &authCodes); err != nil { + if err := cli.listN(resourceAuthCode, &authCodes, gcResultLimit); err != nil { return result, fmt.Errorf("failed to list auth codes: %v", err) } @@ -633,7 +637,7 @@ func (cli *client) GarbageCollect(now time.Time) (result storage.GCResult, err e } var deviceRequests DeviceRequestList - if err := cli.list(resourceDeviceRequest, &deviceRequests); err != nil { + if err := cli.listN(resourceDeviceRequest, &deviceRequests, gcResultLimit); err != nil { return result, fmt.Errorf("failed to list device requests: %v", err) } @@ -648,7 +652,7 @@ func (cli *client) GarbageCollect(now time.Time) (result storage.GCResult, err e } var deviceTokens DeviceTokenList - if err := cli.list(resourceDeviceToken, &deviceTokens); err != nil { + if err := cli.listN(resourceDeviceToken, &deviceTokens, gcResultLimit); err != nil { return result, fmt.Errorf("failed to list device tokens: %v", err) } From 6c99a9b99de4587d389e34bfe4e2d0ceedc66cf3 Mon Sep 17 00:00:00 2001 From: Michael Kelly Date: Tue, 24 May 2022 14:13:39 -0700 Subject: [PATCH 2/4] s/getUrl/getURL golang prefers URL not Url Signed-off-by: Michael Kelly --- storage/kubernetes/client.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/storage/kubernetes/client.go b/storage/kubernetes/client.go index 6676b0eb..f46a639c 100644 --- a/storage/kubernetes/client.go +++ b/storage/kubernetes/client.go @@ -177,7 +177,7 @@ func (cli *client) get(resource, name string, v interface{}) error { return cli.getResource(cli.apiVersion, cli.namespace, resource, name, v) } -func (cli *client) getUrl(url string, v interface{}) error { +func (cli *client) getURL(url string, v interface{}) error { resp, err := cli.client.Get(url) if err != nil { return err @@ -190,14 +190,14 @@ func (cli *client) getUrl(url string, v interface{}) error { } func (cli *client) getResource(apiVersion, namespace, resource, name string, v interface{}) error { - return cli.getUrl(cli.urlFor(apiVersion, namespace, resource, name), v) + return cli.getURL(cli.urlFor(apiVersion, namespace, resource, name), v) } func (cli *client) listN(resource string, v interface{}, n int) error { params := url.Values{} params.Add("limit", fmt.Sprintf("%d", n)) u := cli.urlForWithParams(cli.apiVersion, cli.namespace, resource, "", params) - return cli.getUrl(u, v) + return cli.getURL(u, v) } func (cli *client) list(resource string, v interface{}) error { From a51d12056fd7bd17c0c3e2ddd4ef32fa5070fdba Mon Sep 17 00:00:00 2001 From: Michael Kelly Date: Tue, 7 Jun 2022 20:41:35 -0700 Subject: [PATCH 3/4] Tweaks based on review comments Signed-off-by: Michael Kelly --- storage/kubernetes/client.go | 16 ++++++++-------- storage/kubernetes/storage.go | 2 +- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/storage/kubernetes/client.go b/storage/kubernetes/client.go index f46a639c..15a391a4 100644 --- a/storage/kubernetes/client.go +++ b/storage/kubernetes/client.go @@ -96,18 +96,18 @@ func (cli *client) urlForWithParams( } else { p = path.Join(basePath, apiVersion, resource, name) } - if strings.HasSuffix(cli.baseURL, "/") { - return cli.baseURL + p - } - - r := cli.baseURL + "/" + p encodedParams := params.Encode() + paramsSuffix := "" if len(encodedParams) > 0 { - return r + "?" + encodedParams + paramsSuffix = "?" + encodedParams } - return r + if strings.HasSuffix(cli.baseURL, "/") { + return cli.baseURL + p + paramsSuffix + } + + return cli.baseURL + "/" + p + paramsSuffix } func (cli *client) urlFor(apiVersion, namespace, resource, name string) string { @@ -201,7 +201,7 @@ func (cli *client) listN(resource string, v interface{}, n int) error { } func (cli *client) list(resource string, v interface{}) error { - return cli.listN(resource, v, -1) + return cli.get(resource, "", v) } func (cli *client) post(resource string, v interface{}) error { diff --git a/storage/kubernetes/storage.go b/storage/kubernetes/storage.go index c25d7550..033d3e23 100644 --- a/storage/kubernetes/storage.go +++ b/storage/kubernetes/storage.go @@ -41,7 +41,7 @@ const ( ) const ( - gcResultLimit = 10000 + gcResultLimit = 500 ) // Config values for the Kubernetes storage type. From 9079c31637091888c524b6220bff805ff9c38d1b Mon Sep 17 00:00:00 2001 From: Michael Kelly Date: Wed, 8 Jun 2022 09:24:26 -0700 Subject: [PATCH 4/4] Fix formatting Signed-off-by: Michael Kelly --- storage/kubernetes/client.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/storage/kubernetes/client.go b/storage/kubernetes/client.go index 15a391a4..57f21e00 100644 --- a/storage/kubernetes/client.go +++ b/storage/kubernetes/client.go @@ -84,7 +84,8 @@ func offlineTokenName(userID string, connID string, h func() hash.Hash) string { } func (cli *client) urlForWithParams( - apiVersion, namespace, resource, name string, params url.Values) string { + apiVersion, namespace, resource, name string, params url.Values, +) string { basePath := "apis/" if apiVersion == "v1" { basePath = "api/"