From b532014c56ffc44bd31cdbbe2d422488f29d4ac4 Mon Sep 17 00:00:00 2001 From: Danny Thomson Date: Fri, 31 Jul 2020 13:23:51 -0700 Subject: [PATCH 1/2] Fix panic in rollout restart --- rollout/restart.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/rollout/restart.go b/rollout/restart.go index 20d040cb4c..8f0d7e1b55 100644 --- a/rollout/restart.go +++ b/rollout/restart.go @@ -4,6 +4,8 @@ import ( "sort" "time" + "github.com/argoproj/argo-rollouts/utils/defaults" + appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -57,7 +59,8 @@ func (p *RolloutPodRestarter) Reconcile(roCtx rolloutContext) error { logCtx.Info("Reconcile pod restarts") s := NewSortReplicaSetsByPriority(roCtx) for _, rs := range s.allRSs { - if rs.Status.AvailableReplicas != *rs.Spec.Replicas { + rsReplicas := defaults.GetReplicasOrDefault(rs.Spec.Replicas) + if rs.Status.AvailableReplicas != rsReplicas { logCtx.WithField("ReplicaSet", rs.Name).Info("cannot restart pods as not all ReplicasSets are fully available") return nil } From f05223e21ced6da3b62e40b9564428db766398d0 Mon Sep 17 00:00:00 2001 From: Danny Thomson Date: Fri, 31 Jul 2020 16:04:07 -0700 Subject: [PATCH 2/2] Fix panic in metrics package --- controller/metrics/client.go | 3 +-- controller/metrics/client_test.go | 8 ++------ ingress/ingress.go | 2 +- utils/kubeclientmetrics/metric.go | 11 +++++++++-- utils/kubeclientmetrics/metric_test.go | 24 ++++++++---------------- 5 files changed, 21 insertions(+), 27 deletions(-) diff --git a/controller/metrics/client.go b/controller/metrics/client.go index 495df5a11e..f6aef585a0 100644 --- a/controller/metrics/client.go +++ b/controller/metrics/client.go @@ -34,7 +34,7 @@ var ( ) // IncKubernetesRequest increments the kubernetes client counter -func (m *K8sRequestsCountProvider) IncKubernetesRequest(resourceInfo kubeclientmetrics.ResourceInfo) error { +func (m *K8sRequestsCountProvider) IncKubernetesRequest(resourceInfo kubeclientmetrics.ResourceInfo) { name := resourceInfo.Name namespace := resourceInfo.Namespace kind := resourceInfo.Kind @@ -49,5 +49,4 @@ func (m *K8sRequestsCountProvider) IncKubernetesRequest(resourceInfo kubeclientm } m.k8sRequestsCount.WithLabelValues(kind, namespace, name, string(resourceInfo.Verb), statusCode).Inc() - return nil } diff --git a/controller/metrics/client_test.go b/controller/metrics/client_test.go index f0ba02da31..ae8770b074 100644 --- a/controller/metrics/client_test.go +++ b/controller/metrics/client_test.go @@ -3,8 +3,6 @@ package metrics import ( "testing" - "github.com/stretchr/testify/assert" - "github.com/argoproj/argo-rollouts/utils/kubeclientmetrics" ) @@ -21,18 +19,16 @@ func TestIncKubernetesRequest(t *testing.T) { AnalysisRunLister: fakeAnalysisRunLister{}, K8SRequestProvider: provider, }) - err := provider.IncKubernetesRequest(kubeclientmetrics.ResourceInfo{ + provider.IncKubernetesRequest(kubeclientmetrics.ResourceInfo{ Kind: "replicasets", Namespace: "default", Name: "test", Verb: kubeclientmetrics.List, StatusCode: 200, }) - assert.Nil(t, err) - err = provider.IncKubernetesRequest(kubeclientmetrics.ResourceInfo{ + provider.IncKubernetesRequest(kubeclientmetrics.ResourceInfo{ Verb: kubeclientmetrics.Unknown, StatusCode: 200, }) - assert.Nil(t, err) testHttpResponse(t, metricsServ.Handler, expectedKubernetesRequest) } diff --git a/ingress/ingress.go b/ingress/ingress.go index 5f8bb353f2..c2b562ac98 100644 --- a/ingress/ingress.go +++ b/ingress/ingress.go @@ -128,8 +128,8 @@ func (c *Controller) syncIngress(key string) error { if !strings.HasSuffix(name, ingressutil.CanaryIngressSuffix) { // a primary ingress was deleted, simply ignore the event log.WithField(logutil.IngressKey, key).Warn("primary ingress has been deleted") - return nil } + return nil } rollouts, err := c.getRolloutsByIngress(ingress.Namespace, ingress.Name) if err != nil { diff --git a/utils/kubeclientmetrics/metric.go b/utils/kubeclientmetrics/metric.go index c76e7cbd75..8429bd0e73 100644 --- a/utils/kubeclientmetrics/metric.go +++ b/utils/kubeclientmetrics/metric.go @@ -1,6 +1,7 @@ package kubeclientmetrics import ( + "fmt" "io/ioutil" "net/http" "path" @@ -41,7 +42,7 @@ func (ri ResourceInfo) HasAllFields() bool { type metricsRoundTripper struct { roundTripper http.RoundTripper - inc func(ResourceInfo) error + inc func(ResourceInfo) processPath *regexp.Regexp } @@ -184,6 +185,12 @@ func handleUpdate(r *http.Request, statusCode int) ResourceInfo { func (mrt *metricsRoundTripper) RoundTrip(r *http.Request) (*http.Response, error) { resp, roundTimeErr := mrt.roundTripper.RoundTrip(r) + if roundTimeErr != nil { + return resp, roundTimeErr + } + if resp == nil { + return nil, fmt.Errorf("round tripper has no response when there is no error. please file an issue at https://github.com/argoproj/argo-rollouts") + } var info ResourceInfo switch verb := mrt.resolveK8sRequestVerb(r); verb { case List: @@ -210,7 +217,7 @@ func (mrt *metricsRoundTripper) RoundTrip(r *http.Request) (*http.Response, erro } // AddMetricsTransportWrapper adds a transport wrapper which wraps a function call around each kubernetes request -func AddMetricsTransportWrapper(config *rest.Config, incFunc func(ResourceInfo) error) *rest.Config { +func AddMetricsTransportWrapper(config *rest.Config, incFunc func(ResourceInfo)) *rest.Config { regex := regexp.MustCompile(findPathRegex) wrap := config.WrapTransport config.WrapTransport = func(rt http.RoundTripper) http.RoundTripper { diff --git a/utils/kubeclientmetrics/metric_test.go b/utils/kubeclientmetrics/metric_test.go index 9304bae567..34033d76d5 100644 --- a/utils/kubeclientmetrics/metric_test.go +++ b/utils/kubeclientmetrics/metric_test.go @@ -43,9 +43,8 @@ func TestAddMetricsTransportWrapperWrapTwice(t *testing.T) { } } - newConfig := AddMetricsTransportWrapper(config, func(info ResourceInfo) error { + newConfig := AddMetricsTransportWrapper(config, func(info ResourceInfo) { currentCount++ - return nil }) client := kubernetes.NewForConfigOrDie(newConfig) @@ -133,14 +132,13 @@ func TestGetRequest(t *testing.T) { config := &rest.Config{ Host: ts.URL, } - newConfig := AddMetricsTransportWrapper(config, func(info ResourceInfo) error { + newConfig := AddMetricsTransportWrapper(config, func(info ResourceInfo) { assert.Equal(t, expectedStatusCode, info.StatusCode) assert.Equal(t, "replicasets", info.Kind) assert.Equal(t, metav1.NamespaceDefault, info.Namespace) assert.Equal(t, "test", info.Name) assert.Equal(t, Get, info.Verb) executed = true - return nil }) client := kubernetes.NewForConfigOrDie(newConfig) client.AppsV1().ReplicaSets(metav1.NamespaceDefault).Get("test", metav1.GetOptions{}) @@ -157,14 +155,13 @@ func TestListRequest(t *testing.T) { config := &rest.Config{ Host: ts.URL, } - newConfig := AddMetricsTransportWrapper(config, func(info ResourceInfo) error { + newConfig := AddMetricsTransportWrapper(config, func(info ResourceInfo) { assert.Equal(t, expectedStatusCode, info.StatusCode) assert.Equal(t, "replicasets", info.Kind) assert.Equal(t, metav1.NamespaceDefault, info.Namespace) assert.Equal(t, "", info.Name) assert.Equal(t, List, info.Verb) executed = true - return nil }) client := kubernetes.NewForConfigOrDie(newConfig) client.AppsV1().ReplicaSets(metav1.NamespaceDefault).List(metav1.ListOptions{}) @@ -181,14 +178,13 @@ func TestCreateRequest(t *testing.T) { config := &rest.Config{ Host: ts.URL, } - newConfig := AddMetricsTransportWrapper(config, func(info ResourceInfo) error { + newConfig := AddMetricsTransportWrapper(config, func(info ResourceInfo) { assert.Equal(t, expectedStatusCode, info.StatusCode) assert.Equal(t, "replicasets", info.Kind) assert.Equal(t, metav1.NamespaceDefault, info.Namespace) assert.Equal(t, "test", info.Name) assert.Equal(t, Create, info.Verb) executed = true - return nil }) client := kubernetes.NewForConfigOrDie(newConfig) rs := &appsv1.ReplicaSet{ @@ -211,14 +207,13 @@ func TestDeleteRequest(t *testing.T) { config := &rest.Config{ Host: ts.URL, } - newConfig := AddMetricsTransportWrapper(config, func(info ResourceInfo) error { + newConfig := AddMetricsTransportWrapper(config, func(info ResourceInfo) { assert.Equal(t, expectedStatusCode, info.StatusCode) assert.Equal(t, "replicasets", info.Kind) assert.Equal(t, metav1.NamespaceDefault, info.Namespace) assert.Equal(t, "test", info.Name) assert.Equal(t, Delete, info.Verb) executed = true - return nil }) client := kubernetes.NewForConfigOrDie(newConfig) client.AppsV1().ReplicaSets(metav1.NamespaceDefault).Delete("test", &metav1.DeleteOptions{}) @@ -235,14 +230,13 @@ func TestPatchRequest(t *testing.T) { config := &rest.Config{ Host: ts.URL, } - newConfig := AddMetricsTransportWrapper(config, func(info ResourceInfo) error { + newConfig := AddMetricsTransportWrapper(config, func(info ResourceInfo) { assert.Equal(t, expectedStatusCode, info.StatusCode) assert.Equal(t, "replicasets", info.Kind) assert.Equal(t, metav1.NamespaceDefault, info.Namespace) assert.Equal(t, "test", info.Name) assert.Equal(t, Patch, info.Verb) executed = true - return nil }) client := kubernetes.NewForConfigOrDie(newConfig) client.AppsV1().ReplicaSets(metav1.NamespaceDefault).Patch("test", types.MergePatchType, []byte("{}")) @@ -259,14 +253,13 @@ func TestUpdateRequest(t *testing.T) { config := &rest.Config{ Host: ts.URL, } - newConfig := AddMetricsTransportWrapper(config, func(info ResourceInfo) error { + newConfig := AddMetricsTransportWrapper(config, func(info ResourceInfo) { assert.Equal(t, expectedStatusCode, info.StatusCode) assert.Equal(t, "replicasets", info.Kind) assert.Equal(t, metav1.NamespaceDefault, info.Namespace) assert.Equal(t, "test", info.Name) assert.Equal(t, Update, info.Verb) executed = true - return nil }) client := kubernetes.NewForConfigOrDie(newConfig) rs := &appsv1.ReplicaSet{ @@ -288,11 +281,10 @@ func TestUnknownRequest(t *testing.T) { config := &rest.Config{ Host: ts.URL, } - newConfig := AddMetricsTransportWrapper(config, func(info ResourceInfo) error { + newConfig := AddMetricsTransportWrapper(config, func(info ResourceInfo) { assert.Equal(t, expectedStatusCode, info.StatusCode) assert.Equal(t, Unknown, info.Verb) executed = true - return nil }) client := kubernetes.NewForConfigOrDie(newConfig) client.Discovery().RESTClient().Verb("invalid-verb").Do()