From 8e9b9a358f7787e710276499a5ff10cce3ee4c61 Mon Sep 17 00:00:00 2001 From: mathetake Date: Sat, 7 Mar 2020 23:10:07 +0900 Subject: [PATCH 01/11] pkg/canary: refator error handlings and enhance messages --- pkg/canary/config_tracker.go | 60 ++++------ pkg/canary/controller.go | 4 +- pkg/canary/daemonset_controller.go | 135 ++++++++------------- pkg/canary/daemonset_controller_test.go | 4 +- pkg/canary/daemonset_ready.go | 24 ++-- pkg/canary/daemonset_ready_test.go | 2 +- pkg/canary/daemonset_status.go | 9 +- pkg/canary/deployment_controller.go | 144 ++++++++++------------- pkg/canary/deployment_controller_test.go | 8 +- pkg/canary/deployment_ready.go | 36 ++---- pkg/canary/deployment_status.go | 9 +- pkg/canary/factory.go | 8 +- pkg/canary/nop_tracker.go | 5 +- pkg/canary/service_controller.go | 86 +++++--------- pkg/canary/status.go | 79 +++++++------ pkg/canary/util.go | 6 +- pkg/controller/scheduler.go | 8 +- 17 files changed, 264 insertions(+), 363 deletions(-) diff --git a/pkg/canary/config_tracker.go b/pkg/canary/config_tracker.go index e3922fcb7..16ad93332 100644 --- a/pkg/canary/config_tracker.go +++ b/pkg/canary/config_tracker.go @@ -54,7 +54,7 @@ func checksum(data interface{}) string { func (ct *ConfigTracker) getRefFromConfigMap(name string, namespace string) (*ConfigRef, error) { config, err := ct.KubeClient.CoreV1().ConfigMaps(namespace).Get(name, metav1.GetOptions{}) if err != nil { - return nil, err + return nil, fmt.Errorf("configmap %s.%s get query error: %w", name, namespace, err) } return &ConfigRef{ @@ -69,7 +69,7 @@ func (ct *ConfigTracker) getRefFromConfigMap(name string, namespace string) (*Co func (ct *ConfigTracker) getRefFromSecret(name string, namespace string) (*ConfigRef, error) { secret, err := ct.KubeClient.CoreV1().Secrets(namespace).Get(name, metav1.GetOptions{}) if err != nil { - return nil, err + return nil, fmt.Errorf("secret %s.%s get query error: %w", name, namespace, err) } // ignore registry secrets (those should be set via service account) @@ -100,20 +100,14 @@ func (ct *ConfigTracker) GetTargetConfigs(cd *flaggerv1.Canary) (map[string]Conf case "Deployment": targetDep, err := ct.KubeClient.AppsV1().Deployments(cd.Namespace).Get(targetName, metav1.GetOptions{}) if err != nil { - if errors.IsNotFound(err) { - return res, fmt.Errorf("deployment %s.%s not found", targetName, cd.Namespace) - } - return res, fmt.Errorf("deployment %s.%s query error %v", targetName, cd.Namespace, err) + return res, fmt.Errorf("deployment %s.%s get query error: %w", targetName, cd.Namespace, err) } vs = targetDep.Spec.Template.Spec.Volumes cs = targetDep.Spec.Template.Spec.Containers case "DaemonSet": targetDae, err := ct.KubeClient.AppsV1().DaemonSets(cd.Namespace).Get(targetName, metav1.GetOptions{}) if err != nil { - if errors.IsNotFound(err) { - return res, fmt.Errorf("daemonset %s.%s not found", targetName, cd.Namespace) - } - return res, fmt.Errorf("daemonset %s.%s query error %v", targetName, cd.Namespace, err) + return res, fmt.Errorf("daemonset %s.%s get query error: %w", targetName, cd.Namespace, err) } vs = targetDae.Spec.Template.Spec.Volumes cs = targetDae.Spec.Template.Spec.Containers @@ -126,18 +120,16 @@ func (ct *ConfigTracker) GetTargetConfigs(cd *flaggerv1.Canary) (map[string]Conf if cmv := volume.ConfigMap; cmv != nil { config, err := ct.getRefFromConfigMap(cmv.Name, cd.Namespace) if err != nil { - ct.Logger.Errorf("configMap %s.%s query error %v", cmv.Name, cd.Namespace, err) + ct.Logger.Errorf("getRefFromConfigMap failed: %v", err) continue } - if config != nil { - res[config.GetName()] = *config - } + res[config.GetName()] = *config } if sv := volume.Secret; sv != nil { secret, err := ct.getRefFromSecret(sv.SecretName, cd.Namespace) if err != nil { - ct.Logger.Errorf("secret %s.%s query error %v", sv.SecretName, cd.Namespace, err) + ct.Logger.Errorf("getRefFromSecret failed: %v", err) continue } if secret != nil { @@ -150,18 +142,16 @@ func (ct *ConfigTracker) GetTargetConfigs(cd *flaggerv1.Canary) (map[string]Conf if cmv := source.ConfigMap; cmv != nil { config, err := ct.getRefFromConfigMap(cmv.Name, cd.Namespace) if err != nil { - ct.Logger.Errorf("configMap %s.%s query error %v", cmv.Name, cd.Namespace, err) + ct.Logger.Errorf("getRefFromConfigMap failed: %v", err) continue } - if config != nil { - res[config.GetName()] = *config - } + res[config.GetName()] = *config } if sv := source.Secret; sv != nil { secret, err := ct.getRefFromSecret(sv.Name, cd.Namespace) if err != nil { - ct.Logger.Errorf("secret %s.%s query error %v", sv.Name, cd.Namespace, err) + ct.Logger.Errorf("getRefFromSecret failed: %v", err) continue } if secret != nil { @@ -181,17 +171,15 @@ func (ct *ConfigTracker) GetTargetConfigs(cd *flaggerv1.Canary) (map[string]Conf name := env.ValueFrom.ConfigMapKeyRef.LocalObjectReference.Name config, err := ct.getRefFromConfigMap(name, cd.Namespace) if err != nil { - ct.Logger.Errorf("configMap %s.%s query error %v", name, cd.Namespace, err) + ct.Logger.Errorf("getRefFromConfigMap failed: %v", err) continue } - if config != nil { - res[config.GetName()] = *config - } + res[config.GetName()] = *config case env.ValueFrom.SecretKeyRef != nil: name := env.ValueFrom.SecretKeyRef.LocalObjectReference.Name secret, err := ct.getRefFromSecret(name, cd.Namespace) if err != nil { - ct.Logger.Errorf("secret %s.%s query error %v", name, cd.Namespace, err) + ct.Logger.Errorf("getRefFromSecret failed: %v", err) continue } if secret != nil { @@ -207,17 +195,15 @@ func (ct *ConfigTracker) GetTargetConfigs(cd *flaggerv1.Canary) (map[string]Conf name := envFrom.ConfigMapRef.LocalObjectReference.Name config, err := ct.getRefFromConfigMap(name, cd.Namespace) if err != nil { - ct.Logger.Errorf("configMap %s.%s query error %v", name, cd.Namespace, err) + ct.Logger.Errorf("getRefFromConfigMap failed %v", err) continue } - if config != nil { - res[config.GetName()] = *config - } + res[config.GetName()] = *config case envFrom.SecretRef != nil: name := envFrom.SecretRef.LocalObjectReference.Name secret, err := ct.getRefFromSecret(name, cd.Namespace) if err != nil { - ct.Logger.Errorf("secret %s.%s query error %v", name, cd.Namespace, err) + ct.Logger.Errorf("getRefFromSecret failed %v", err) continue } if secret != nil { @@ -235,7 +221,7 @@ func (ct *ConfigTracker) GetConfigRefs(cd *flaggerv1.Canary) (*map[string]string res := make(map[string]string) configs, err := ct.GetTargetConfigs(cd) if err != nil { - return nil, err + return nil, fmt.Errorf("GetTargetConfigs failed: %w", err) } for _, cfg := range configs { @@ -250,7 +236,7 @@ func (ct *ConfigTracker) GetConfigRefs(cd *flaggerv1.Canary) (*map[string]string func (ct *ConfigTracker) HasConfigChanged(cd *flaggerv1.Canary) (bool, error) { configs, err := ct.GetTargetConfigs(cd) if err != nil { - return false, err + return false, fmt.Errorf("GetTargetConfigs failed: %w", err) } if len(configs) == 0 && cd.Status.TrackedConfigs == nil { @@ -286,7 +272,7 @@ func (ct *ConfigTracker) CreatePrimaryConfigs(cd *flaggerv1.Canary, refs map[str case ConfigRefMap: config, err := ct.KubeClient.CoreV1().ConfigMaps(cd.Namespace).Get(ref.Name, metav1.GetOptions{}) if err != nil { - return err + return fmt.Errorf("configmap %s.%s query failed : %w", ref.Name, cd.Name, err) } primaryName := fmt.Sprintf("%s-primary", config.GetName()) primaryConfigMap := &corev1.ConfigMap{ @@ -311,10 +297,10 @@ func (ct *ConfigTracker) CreatePrimaryConfigs(cd *flaggerv1.Canary, refs map[str if errors.IsNotFound(err) { _, err = ct.KubeClient.CoreV1().ConfigMaps(cd.Namespace).Create(primaryConfigMap) if err != nil { - return err + return fmt.Errorf("creating configmap %s.%s failed: %w", primaryConfigMap.Name, cd.Namespace, err) } } else { - return err + return fmt.Errorf("updating configmap %s.%s failed: %w", primaryConfigMap.Name, cd.Namespace, err) } } @@ -349,10 +335,10 @@ func (ct *ConfigTracker) CreatePrimaryConfigs(cd *flaggerv1.Canary, refs map[str if errors.IsNotFound(err) { _, err = ct.KubeClient.CoreV1().Secrets(cd.Namespace).Create(primarySecret) if err != nil { - return err + return fmt.Errorf("creating secret %s.%s failed: %w", primarySecret.Name, cd.Namespace, err) } } else { - return err + return fmt.Errorf("updating secret %s.%s failed: %w", primarySecret.Name, cd.Namespace, err) } } diff --git a/pkg/canary/controller.go b/pkg/canary/controller.go index c615cc5e7..d54c1a202 100644 --- a/pkg/canary/controller.go +++ b/pkg/canary/controller.go @@ -5,7 +5,7 @@ import ( ) type Controller interface { - IsPrimaryReady(canary *flaggerv1.Canary) (bool, error) + IsPrimaryReady(canary *flaggerv1.Canary) error IsCanaryReady(canary *flaggerv1.Canary) (bool, error) GetMetadata(canary *flaggerv1.Canary) (string, map[string]int32, error) SyncStatus(canary *flaggerv1.Canary, status flaggerv1.CanaryStatus) error @@ -17,6 +17,6 @@ type Controller interface { Promote(canary *flaggerv1.Canary) error HasTargetChanged(canary *flaggerv1.Canary) (bool, error) HaveDependenciesChanged(canary *flaggerv1.Canary) (bool, error) - Scale(canary *flaggerv1.Canary, replicas int32) error + ScaleToZero(canary *flaggerv1.Canary) error ScaleFromZero(canary *flaggerv1.Canary) error } diff --git a/pkg/canary/daemonset_controller.go b/pkg/canary/daemonset_controller.go index 0041247ba..074d343aa 100644 --- a/pkg/canary/daemonset_controller.go +++ b/pkg/canary/daemonset_controller.go @@ -28,33 +28,27 @@ type DaemonSetController struct { labels []string } -func (c *DaemonSetController) Scale(cd *flaggerv1.Canary, v int32) error { - // there's no concept `replicas` for DaemonSet - if v == 0 { - targetName := cd.Spec.TargetRef.Name - dae, err := c.kubeClient.AppsV1().DaemonSets(cd.Namespace).Get(targetName, metav1.GetOptions{}) - if err != nil { - if errors.IsNotFound(err) { - return fmt.Errorf("daemonset %s.%s not found", targetName, cd.Namespace) - } - return fmt.Errorf("daemonset %s.%s query error %v", targetName, cd.Namespace, err) - } +func (c *DaemonSetController) ScaleToZero(cd *flaggerv1.Canary) error { + targetName := cd.Spec.TargetRef.Name + dae, err := c.kubeClient.AppsV1().DaemonSets(cd.Namespace).Get(targetName, metav1.GetOptions{}) + if err != nil { + return fmt.Errorf("daemonset %s.%s query error: %w", targetName, cd.Namespace, err) + } - daeCopy := dae.DeepCopy() - daeCopy.Spec.Template.Spec.NodeSelector = make(map[string]string, - len(dae.Spec.Template.Spec.NodeSelector)+len(daemonSetScaleDownNodeSelector)) - for k, v := range dae.Spec.Template.Spec.NodeSelector { - daeCopy.Spec.Template.Spec.NodeSelector[k] = v - } + daeCopy := dae.DeepCopy() + daeCopy.Spec.Template.Spec.NodeSelector = make(map[string]string, + len(dae.Spec.Template.Spec.NodeSelector)+len(daemonSetScaleDownNodeSelector)) + for k, v := range dae.Spec.Template.Spec.NodeSelector { + daeCopy.Spec.Template.Spec.NodeSelector[k] = v + } - for k, v := range daemonSetScaleDownNodeSelector { - daeCopy.Spec.Template.Spec.NodeSelector[k] = v - } + for k, v := range daemonSetScaleDownNodeSelector { + daeCopy.Spec.Template.Spec.NodeSelector[k] = v + } - _, err = c.kubeClient.AppsV1().DaemonSets(dae.Namespace).Update(daeCopy) - if err != nil { - return fmt.Errorf("scaling down daemonset %s.%s failed: %v", daeCopy.GetName(), daeCopy.Namespace, err) - } + _, err = c.kubeClient.AppsV1().DaemonSets(dae.Namespace).Update(daeCopy) + if err != nil { + return fmt.Errorf("updating daemonset %s.%s failed: %w", daeCopy.GetName(), daeCopy.Namespace, err) } return nil } @@ -63,10 +57,7 @@ func (c *DaemonSetController) ScaleFromZero(cd *flaggerv1.Canary) error { targetName := cd.Spec.TargetRef.Name dep, err := c.kubeClient.AppsV1().DaemonSets(cd.Namespace).Get(targetName, metav1.GetOptions{}) if err != nil { - if errors.IsNotFound(err) { - return fmt.Errorf("daemonset %s.%s not found", targetName, cd.Namespace) - } - return fmt.Errorf("daemonset %s.%s query error %v", targetName, cd.Namespace, err) + return fmt.Errorf("daemonset %s.%s query error: %w", targetName, cd.Namespace, err) } depCopy := dep.DeepCopy() @@ -76,7 +67,7 @@ func (c *DaemonSetController) ScaleFromZero(cd *flaggerv1.Canary) error { _, err = c.kubeClient.AppsV1().DaemonSets(dep.Namespace).Update(depCopy) if err != nil { - return fmt.Errorf("scaling up daemonset %s.%s failed: %v", depCopy.GetName(), depCopy.Namespace, err) + return fmt.Errorf("scaling up daemonset %s.%s failed: %w", depCopy.GetName(), depCopy.Namespace, err) } return nil } @@ -84,23 +75,21 @@ func (c *DaemonSetController) ScaleFromZero(cd *flaggerv1.Canary) error { // Initialize creates the primary DaemonSet and // delete the canary DaemonSet and returns the pod selector label and container ports func (c *DaemonSetController) Initialize(cd *flaggerv1.Canary, skipLivenessChecks bool) (err error) { - primaryName := fmt.Sprintf("%s-primary", cd.Spec.TargetRef.Name) err = c.createPrimaryDaemonSet(cd) if err != nil { - return fmt.Errorf("creating daemonset %s.%s failed: %v", primaryName, cd.Namespace, err) + return fmt.Errorf("createPrimaryDaemonSet failed: %w", err) } if cd.Status.Phase == "" || cd.Status.Phase == flaggerv1.CanaryPhaseInitializing { if !skipLivenessChecks && !cd.SkipAnalysis() { - _, readyErr := c.IsPrimaryReady(cd) - if readyErr != nil { - return readyErr + if err := c.IsPrimaryReady(cd); err != nil { + return fmt.Errorf("IsPrimaryReady failed: %w", err) } } c.logger.With("canary", fmt.Sprintf("%s.%s", cd.Name, cd.Namespace)).Infof("Scaling down %s.%s", cd.Spec.TargetRef.Name, cd.Namespace) - if err := c.Scale(cd, 0); err != nil { - return err + if err := c.ScaleToZero(cd); err != nil { + return fmt.Errorf("ScaleToZero failed: %w", err) } } return nil @@ -113,33 +102,26 @@ func (c *DaemonSetController) Promote(cd *flaggerv1.Canary) error { canary, err := c.kubeClient.AppsV1().DaemonSets(cd.Namespace).Get(targetName, metav1.GetOptions{}) if err != nil { - if errors.IsNotFound(err) { - return fmt.Errorf("damonset %s.%s not found", targetName, cd.Namespace) - } - return fmt.Errorf("damonset %s.%s query error %v", targetName, cd.Namespace, err) + return fmt.Errorf("damonset %s.%s get query error %v", targetName, cd.Namespace, err) } label, err := c.getSelectorLabel(canary) if err != nil { - return fmt.Errorf("invalid label selector! DaemonSet %s.%s spec.selector.matchLabels must contain selector 'app: %s'", - targetName, cd.Namespace, targetName) + return fmt.Errorf("getSelectorLabel failed: %w", err) } primary, err := c.kubeClient.AppsV1().DaemonSets(cd.Namespace).Get(primaryName, metav1.GetOptions{}) if err != nil { - if errors.IsNotFound(err) { - return fmt.Errorf("daemonset %s.%s not found", primaryName, cd.Namespace) - } - return fmt.Errorf("daemonset %s.%s query error %v", primaryName, cd.Namespace, err) + return fmt.Errorf("daemonset %s.%s get query error %w", primaryName, cd.Namespace, err) } // promote secrets and config maps configRefs, err := c.configTracker.GetTargetConfigs(cd) if err != nil { - return err + return fmt.Errorf("GetTargetConfigs failed: %w", err) } if err := c.configTracker.CreatePrimaryConfigs(cd, configRefs); err != nil { - return err + return fmt.Errorf("CreatePrimaryConfigs failed: %w", err) } primaryCopy := primary.DeepCopy() @@ -158,16 +140,16 @@ func (c *DaemonSetController) Promote(cd *flaggerv1.Canary) error { // update pod annotations to ensure a rolling update annotations, err := makeAnnotations(canary.Spec.Template.Annotations) if err != nil { - return err + return fmt.Errorf("makeAnnotations failed: %w", err) } - primaryCopy.Spec.Template.Annotations = annotations + primaryCopy.Spec.Template.Annotations = annotations primaryCopy.Spec.Template.Labels = makePrimaryLabels(canary.Spec.Template.Labels, primaryName, label) // apply update _, err = c.kubeClient.AppsV1().DaemonSets(cd.Namespace).Update(primaryCopy) if err != nil { - return fmt.Errorf("updating deployment %s.%s template spec failed: %v", + return fmt.Errorf("updating daemonset %s.%s template spec failed: %w", primaryCopy.GetName(), primaryCopy.Namespace, err) } return nil @@ -178,10 +160,7 @@ func (c *DaemonSetController) HasTargetChanged(cd *flaggerv1.Canary) (bool, erro targetName := cd.Spec.TargetRef.Name canary, err := c.kubeClient.AppsV1().DaemonSets(cd.Namespace).Get(targetName, metav1.GetOptions{}) if err != nil { - if errors.IsNotFound(err) { - return false, fmt.Errorf("daemonset %s.%s not found", targetName, cd.Namespace) - } - return false, fmt.Errorf("daemonset %s.%s query error %v", targetName, cd.Namespace, err) + return false, fmt.Errorf("daemonset %s.%s get query error %w", targetName, cd.Namespace, err) } // ignore `daemonSetScaleDownNodeSelector` node selector @@ -203,27 +182,18 @@ func (c *DaemonSetController) GetMetadata(cd *flaggerv1.Canary) (string, map[str canaryDae, err := c.kubeClient.AppsV1().DaemonSets(cd.Namespace).Get(targetName, metav1.GetOptions{}) if err != nil { - if errors.IsNotFound(err) { - return "", nil, fmt.Errorf("daemonset %s.%s not found, retrying", targetName, cd.Namespace) - } - return "", nil, err + return "", nil, fmt.Errorf("daemonset %s.%s get query error %w", targetName, cd.Namespace, err) } label, err := c.getSelectorLabel(canaryDae) if err != nil { - return "", nil, fmt.Errorf("invalid label selector! DaemonSet %s.%s spec.selector.matchLabels must contain selector 'app: %s'", - targetName, cd.Namespace, targetName) + return "", nil, fmt.Errorf("getSelectorLabel failed: %w", err) } var ports map[string]int32 if cd.Spec.Service.PortDiscovery { - p, err := getPorts(cd, canaryDae.Spec.Template.Spec.Containers) - if err != nil { - return "", nil, fmt.Errorf("port discovery failed with error: %v", err) - } - ports = p + ports = getPorts(cd, canaryDae.Spec.Template.Spec.Containers) } - return label, ports, nil } @@ -233,10 +203,7 @@ func (c *DaemonSetController) createPrimaryDaemonSet(cd *flaggerv1.Canary) error canaryDae, err := c.kubeClient.AppsV1().DaemonSets(cd.Namespace).Get(targetName, metav1.GetOptions{}) if err != nil { - if errors.IsNotFound(err) { - return fmt.Errorf("daemonset %s.%s not found, retrying", targetName, cd.Namespace) - } - return err + return fmt.Errorf("daemonset %s.%s get query error %w", targetName, cd.Namespace, err) } if canaryDae.Spec.UpdateStrategy.Type != "" && @@ -247,27 +214,26 @@ func (c *DaemonSetController) createPrimaryDaemonSet(cd *flaggerv1.Canary) error label, err := c.getSelectorLabel(canaryDae) if err != nil { - return fmt.Errorf("invalid label selector! DaemonSet %s.%s spec.selector.matchLabels must contain selector 'app: %s'", - targetName, cd.Namespace, targetName) + return fmt.Errorf("getSelectorLabel failed: %w", err) } - primaryDep, err := c.kubeClient.AppsV1().DaemonSets(cd.Namespace).Get(primaryName, metav1.GetOptions{}) + primaryDae, err := c.kubeClient.AppsV1().DaemonSets(cd.Namespace).Get(primaryName, metav1.GetOptions{}) if errors.IsNotFound(err) { // create primary secrets and config maps configRefs, err := c.configTracker.GetTargetConfigs(cd) if err != nil { - return err + return fmt.Errorf("GetTargetConfigs failed: %w", err) } if err := c.configTracker.CreatePrimaryConfigs(cd, configRefs); err != nil { - return err + return fmt.Errorf("CreatePrimaryConfigs failed: %w", err) } annotations, err := makeAnnotations(canaryDae.Spec.Template.Annotations) if err != nil { - return err + return fmt.Errorf("makeAnnotations failed: %w", err) } - // create primary deployment - primaryDep = &appsv1.DaemonSet{ + // create primary daemonset + primaryDae = &appsv1.DaemonSet{ ObjectMeta: metav1.ObjectMeta{ Name: primaryName, Namespace: cd.Namespace, @@ -302,12 +268,12 @@ func (c *DaemonSetController) createPrimaryDaemonSet(cd *flaggerv1.Canary) error }, } - _, err = c.kubeClient.AppsV1().DaemonSets(cd.Namespace).Create(primaryDep) + _, err = c.kubeClient.AppsV1().DaemonSets(cd.Namespace).Create(primaryDae) if err != nil { - return err + return fmt.Errorf("creating daemonset %s.%s failed: %w", primaryDae.Name, cd.Namespace, err) } - c.logger.With("canary", fmt.Sprintf("%s.%s", cd.Name, cd.Namespace)).Infof("DaemonSet %s.%s created", primaryDep.GetName(), cd.Namespace) + c.logger.With("canary", fmt.Sprintf("%s.%s", cd.Name, cd.Namespace)).Infof("DaemonSet %s.%s created", primaryDae.GetName(), cd.Namespace) } return nil } @@ -320,7 +286,10 @@ func (c *DaemonSetController) getSelectorLabel(daemonSet *appsv1.DaemonSet) (str } } - return "", fmt.Errorf("selector not found") + return "", fmt.Errorf( + "daemonset %s.%s spec.selector.matchLabels must contain one of %v'", + c.labels, daemonSet.Name, daemonSet.Namespace, + ) } func (c *DaemonSetController) HaveDependenciesChanged(cd *flaggerv1.Canary) (bool, error) { diff --git a/pkg/canary/daemonset_controller_test.go b/pkg/canary/daemonset_controller_test.go index 9bd5bc531..0222a7103 100644 --- a/pkg/canary/daemonset_controller_test.go +++ b/pkg/canary/daemonset_controller_test.go @@ -161,12 +161,12 @@ func TestDaemonSetController_HasTargetChanged(t *testing.T) { } func TestDaemonSetController_Scale(t *testing.T) { - t.Run("Scale", func(t *testing.T) { + t.Run("ScaleToZero", func(t *testing.T) { mocks := newDaemonSetFixture() err := mocks.controller.Initialize(mocks.canary, true) require.NoError(t, err) - err = mocks.controller.Scale(mocks.canary, 0) + err = mocks.controller.ScaleToZero(mocks.canary) require.NoError(t, err) c, err := mocks.kubeClient.AppsV1().DaemonSets("default").Get("podinfo", metav1.GetOptions{}) diff --git a/pkg/canary/daemonset_ready.go b/pkg/canary/daemonset_ready.go index 4120082a9..81b892b1f 100644 --- a/pkg/canary/daemonset_ready.go +++ b/pkg/canary/daemonset_ready.go @@ -5,7 +5,6 @@ import ( "time" appsv1 "k8s.io/api/apps/v1" - "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" flaggerv1 "github.com/weaveworks/flagger/pkg/apis/flagger/v1beta1" @@ -13,21 +12,18 @@ import ( // IsPrimaryReady checks the primary daemonset status and returns an error if // the daemonset is in the middle of a rolling update -func (c *DaemonSetController) IsPrimaryReady(cd *flaggerv1.Canary) (bool, error) { +func (c *DaemonSetController) IsPrimaryReady(cd *flaggerv1.Canary) error { primaryName := fmt.Sprintf("%s-primary", cd.Spec.TargetRef.Name) primary, err := c.kubeClient.AppsV1().DaemonSets(cd.Namespace).Get(primaryName, metav1.GetOptions{}) if err != nil { - if errors.IsNotFound(err) { - return true, fmt.Errorf("deployment %s.%s not found", primaryName, cd.Namespace) - } - return true, fmt.Errorf("deployment %s.%s query error %v", primaryName, cd.Namespace, err) + return fmt.Errorf("daemonset %s.%s get query error %w", primaryName, cd.Namespace, err) } - retriable, err := c.isDaemonSetReady(cd, primary) + _, err = c.isDaemonSetReady(cd, primary) if err != nil { - return retriable, fmt.Errorf("halt advancement %s.%s %s", primaryName, cd.Namespace, err.Error()) + return fmt.Errorf("primary daemonset %s.%s not ready: %w", primaryName, cd.Namespace, err) } - return true, nil + return nil } // IsCanaryReady checks the primary daemonset and returns an error if @@ -36,15 +32,13 @@ func (c *DaemonSetController) IsCanaryReady(cd *flaggerv1.Canary) (bool, error) targetName := cd.Spec.TargetRef.Name canary, err := c.kubeClient.AppsV1().DaemonSets(cd.Namespace).Get(targetName, metav1.GetOptions{}) if err != nil { - if errors.IsNotFound(err) { - return true, fmt.Errorf("daemonset %s.%s not found", targetName, cd.Namespace) - } - return true, fmt.Errorf("daemonset %s.%s query error %v", targetName, cd.Namespace, err) + return true, fmt.Errorf("daemonset %s.%s get query error %w", targetName, cd.Namespace, err) } retriable, err := c.isDaemonSetReady(cd, canary) if err != nil { - return retriable, fmt.Errorf("halt advancement %s.%s %s", targetName, cd.Namespace, err.Error()) + return retriable, fmt.Errorf("canary damonset %s.%s not ready with retryablility: %v: %w", + targetName, cd.Namespace, retriable, err) } return true, nil } @@ -56,7 +50,7 @@ func (c *DaemonSetController) isDaemonSetReady(cd *flaggerv1.Canary, daemonSet * delta := time.Duration(cd.GetProgressDeadlineSeconds()) * time.Second dl := from.Add(delta) if dl.Before(time.Now()) { - return false, fmt.Errorf("daemonset %s exceeded its progress deadline", cd.GetName()) + return false, fmt.Errorf("exceeded its progressDeadlineSeconds: %d", cd.GetProgressDeadlineSeconds()) } else { return true, fmt.Errorf( "waiting for rollout to finish: desiredNumberScheduled=%d, updatedNumberScheduled=%d, numberUnavailable=%d", diff --git a/pkg/canary/daemonset_ready_test.go b/pkg/canary/daemonset_ready_test.go index 9754b8663..76a810cf8 100644 --- a/pkg/canary/daemonset_ready_test.go +++ b/pkg/canary/daemonset_ready_test.go @@ -16,7 +16,7 @@ func TestDaemonSetController_IsReady(t *testing.T) { err := mocks.controller.Initialize(mocks.canary, true) assert.NoError(t, err, "Expected primary readiness check to fail") - _, err = mocks.controller.IsPrimaryReady(mocks.canary) + err = mocks.controller.IsPrimaryReady(mocks.canary) require.NoError(t, err) _, err = mocks.controller.IsCanaryReady(mocks.canary) diff --git a/pkg/canary/daemonset_status.go b/pkg/canary/daemonset_status.go index 49329866b..dd558f966 100644 --- a/pkg/canary/daemonset_status.go +++ b/pkg/canary/daemonset_status.go @@ -3,8 +3,6 @@ package canary import ( "fmt" - ex "github.com/pkg/errors" - "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" flaggerv1 "github.com/weaveworks/flagger/pkg/apis/flagger/v1beta1" @@ -14,10 +12,7 @@ import ( func (c *DaemonSetController) SyncStatus(cd *flaggerv1.Canary, status flaggerv1.CanaryStatus) error { dae, err := c.kubeClient.AppsV1().DaemonSets(cd.Namespace).Get(cd.Spec.TargetRef.Name, metav1.GetOptions{}) if err != nil { - if errors.IsNotFound(err) { - return fmt.Errorf("daemonset %s.%s not found", cd.Spec.TargetRef.Name, cd.Namespace) - } - return ex.Wrap(err, "SyncStatus daemonset query error") + return fmt.Errorf("daemonset %s.%s get query error: %w", cd.Spec.TargetRef.Name, cd.Namespace, err) } // ignore `daemonSetScaleDownNodeSelector` node selector @@ -32,7 +27,7 @@ func (c *DaemonSetController) SyncStatus(cd *flaggerv1.Canary, status flaggerv1. configs, err := c.configTracker.GetConfigRefs(cd) if err != nil { - return ex.Wrap(err, "SyncStatus configs query error") + return fmt.Errorf("GetConfigRefs failed: %w", err) } return syncCanaryStatus(c.flaggerClient, cd, status, dae.Spec.Template, func(cdCopy *flaggerv1.Canary) { diff --git a/pkg/canary/deployment_controller.go b/pkg/canary/deployment_controller.go index 96b6b77f8..8364845d2 100644 --- a/pkg/canary/deployment_controller.go +++ b/pkg/canary/deployment_controller.go @@ -33,26 +33,31 @@ func (c *DeploymentController) Initialize(cd *flaggerv1.Canary, skipLivenessChec err = c.createPrimaryDeployment(cd) if err != nil { - return fmt.Errorf("creating deployment %s.%s failed: %v", primaryName, cd.Namespace, err) + return fmt.Errorf("createPrimaryDeployment %s.%s failed: %w", primaryName, cd.Namespace, err) } if cd.Status.Phase == "" || cd.Status.Phase == flaggerv1.CanaryPhaseInitializing { if !skipLivenessChecks && !cd.SkipAnalysis() { - _, readyErr := c.IsPrimaryReady(cd) - if readyErr != nil { - return readyErr + if err := c.IsPrimaryReady(cd); err != nil { + return fmt.Errorf("primary deployment %s.%s not ready: %w", primaryName, cd.Namespace, err) } } c.logger.With("canary", fmt.Sprintf("%s.%s", cd.Name, cd.Namespace)).Infof("Scaling down %s.%s", cd.Spec.TargetRef.Name, cd.Namespace) - if err := c.Scale(cd, 0); err != nil { - return err + if err := c.ScaleToZero(cd); err != nil { + return fmt.Errorf("scaling down canary daemon set %s.%s failed: %w", cd.Spec.TargetRef.Name, cd.Namespace, err) } } - if cd.Spec.AutoscalerRef != nil && cd.Spec.AutoscalerRef.Kind == "HorizontalPodAutoscaler" { - if err := c.reconcilePrimaryHpa(cd, true); err != nil { - return fmt.Errorf("creating HorizontalPodAutoscaler %s.%s failed: %v", primaryName, cd.Namespace, err) + if cd.Spec.AutoscalerRef != nil { + switch cd.Spec.AutoscalerRef.Kind { + case "HorizontalPodAutoscaler": + if err := c.reconcilePrimaryHpa(cd, true); err != nil { + return fmt.Errorf( + "initial reconcilePrimaryHpa for %s.%s failed: %w", primaryName, cd.Namespace, err) + } + default: + return fmt.Errorf("cd.Spec.AutoscalerRef.Kind is invalid: %s", cd.Spec.AutoscalerRef.Kind) } } return nil @@ -65,33 +70,26 @@ func (c *DeploymentController) Promote(cd *flaggerv1.Canary) error { canary, err := c.kubeClient.AppsV1().Deployments(cd.Namespace).Get(targetName, metav1.GetOptions{}) if err != nil { - if errors.IsNotFound(err) { - return fmt.Errorf("deployment %s.%s not found", targetName, cd.Namespace) - } return fmt.Errorf("deployment %s.%s query error %v", targetName, cd.Namespace, err) } label, err := c.getSelectorLabel(canary) if err != nil { - return fmt.Errorf("invalid label selector! Deployment %s.%s spec.selector.matchLabels must contain selector 'app: %s'", - targetName, cd.Namespace, targetName) + return fmt.Errorf("getSelectorLabel failed: %w", err) } primary, err := c.kubeClient.AppsV1().Deployments(cd.Namespace).Get(primaryName, metav1.GetOptions{}) if err != nil { - if errors.IsNotFound(err) { - return fmt.Errorf("deployment %s.%s not found", primaryName, cd.Namespace) - } return fmt.Errorf("deployment %s.%s query error %v", primaryName, cd.Namespace, err) } // promote secrets and config maps configRefs, err := c.configTracker.GetTargetConfigs(cd) if err != nil { - return err + return fmt.Errorf("GetTargetConfigs failed: %w", err) } if err := c.configTracker.CreatePrimaryConfigs(cd, configRefs); err != nil { - return err + return fmt.Errorf("CreatePrimaryConfigs failed: %w", err) } primaryCopy := primary.DeepCopy() @@ -106,26 +104,31 @@ func (c *DeploymentController) Promote(cd *flaggerv1.Canary) error { // update pod annotations to ensure a rolling update annotations, err := makeAnnotations(canary.Spec.Template.Annotations) if err != nil { - return err + return fmt.Errorf("makeAnnotations failed: %w", err) } - primaryCopy.Spec.Template.Annotations = annotations + primaryCopy.Spec.Template.Annotations = annotations primaryCopy.Spec.Template.Labels = makePrimaryLabels(canary.Spec.Template.Labels, primaryName, label) // apply update _, err = c.kubeClient.AppsV1().Deployments(cd.Namespace).Update(primaryCopy) if err != nil { - return fmt.Errorf("updating deployment %s.%s template spec failed: %v", + return fmt.Errorf("updating deployment %s.%s template spec failed: %w", primaryCopy.GetName(), primaryCopy.Namespace, err) } // update HPA - if cd.Spec.AutoscalerRef != nil && cd.Spec.AutoscalerRef.Kind == "HorizontalPodAutoscaler" { - if err := c.reconcilePrimaryHpa(cd, false); err != nil { - return fmt.Errorf("updating HorizontalPodAutoscaler %s.%s failed: %v", primaryName, cd.Namespace, err) + if cd.Spec.AutoscalerRef != nil { + switch cd.Spec.AutoscalerRef.Kind { + case "HorizontalPodAutoscaler": + if err := c.reconcilePrimaryHpa(cd, false); err != nil { + return fmt.Errorf( + "reconcilePrimaryHpa for %s.%s failed: %w", primaryName, cd.Namespace, err) + } + default: + return fmt.Errorf("cd.Spec.AutoscalerRef.Kind is invalid: %s", cd.Spec.AutoscalerRef.Kind) } } - return nil } @@ -134,32 +137,26 @@ func (c *DeploymentController) HasTargetChanged(cd *flaggerv1.Canary) (bool, err targetName := cd.Spec.TargetRef.Name canary, err := c.kubeClient.AppsV1().Deployments(cd.Namespace).Get(targetName, metav1.GetOptions{}) if err != nil { - if errors.IsNotFound(err) { - return false, fmt.Errorf("deployment %s.%s not found", targetName, cd.Namespace) - } - return false, fmt.Errorf("deployment %s.%s query error %v", targetName, cd.Namespace, err) + return false, fmt.Errorf("deployment %s.%s query error: %w", targetName, cd.Namespace, err) } return hasSpecChanged(cd, canary.Spec.Template) } // Scale sets the canary deployment replicas -func (c *DeploymentController) Scale(cd *flaggerv1.Canary, replicas int32) error { +func (c *DeploymentController) ScaleToZero(cd *flaggerv1.Canary) error { targetName := cd.Spec.TargetRef.Name dep, err := c.kubeClient.AppsV1().Deployments(cd.Namespace).Get(targetName, metav1.GetOptions{}) if err != nil { - if errors.IsNotFound(err) { - return fmt.Errorf("deployment %s.%s not found", targetName, cd.Namespace) - } - return fmt.Errorf("deployment %s.%s query error %v", targetName, cd.Namespace, err) + return fmt.Errorf("deployment %s.%s get query error: %w", targetName, cd.Namespace, err) } depCopy := dep.DeepCopy() - depCopy.Spec.Replicas = int32p(replicas) + depCopy.Spec.Replicas = int32p(0) _, err = c.kubeClient.AppsV1().Deployments(dep.Namespace).Update(depCopy) if err != nil { - return fmt.Errorf("scaling %s.%s to %v failed: %v", depCopy.GetName(), depCopy.Namespace, replicas, err) + return fmt.Errorf("deployment %s.%s update query error: %w", targetName, cd.Namespace, err) } return nil } @@ -168,10 +165,7 @@ func (c *DeploymentController) ScaleFromZero(cd *flaggerv1.Canary) error { targetName := cd.Spec.TargetRef.Name dep, err := c.kubeClient.AppsV1().Deployments(cd.Namespace).Get(targetName, metav1.GetOptions{}) if err != nil { - if errors.IsNotFound(err) { - return fmt.Errorf("deployment %s.%s not found", targetName, cd.Namespace) - } - return fmt.Errorf("deployment %s.%s query error %v", targetName, cd.Namespace, err) + return fmt.Errorf("deployment %s.%s get query error: %w", targetName, cd.Namespace, err) } replicas := int32p(1) @@ -183,7 +177,7 @@ func (c *DeploymentController) ScaleFromZero(cd *flaggerv1.Canary) error { _, err = c.kubeClient.AppsV1().Deployments(dep.Namespace).Update(depCopy) if err != nil { - return fmt.Errorf("scaling %s.%s to %v failed: %v", depCopy.GetName(), depCopy.Namespace, replicas, err) + return fmt.Errorf("scaling up %s.%s to %v failed: %v", depCopy.GetName(), depCopy.Namespace, replicas, err) } return nil } @@ -194,25 +188,17 @@ func (c *DeploymentController) GetMetadata(cd *flaggerv1.Canary) (string, map[st canaryDep, err := c.kubeClient.AppsV1().Deployments(cd.Namespace).Get(targetName, metav1.GetOptions{}) if err != nil { - if errors.IsNotFound(err) { - return "", nil, fmt.Errorf("deployment %s.%s not found, retrying", targetName, cd.Namespace) - } - return "", nil, err + return "", nil, fmt.Errorf("deployment %s.%s get query error %w", targetName, cd.Namespace, err) } label, err := c.getSelectorLabel(canaryDep) if err != nil { - return "", nil, fmt.Errorf("invalid label selector! Deployment %s.%s spec.selector.matchLabels must contain selector 'app: %s'", - targetName, cd.Namespace, targetName) + return "", nil, fmt.Errorf("getSelectorLabel failed: %w", err) } var ports map[string]int32 if cd.Spec.Service.PortDiscovery { - p, err := getPorts(cd, canaryDep.Spec.Template.Spec.Containers) - if err != nil { - return "", nil, fmt.Errorf("port discovery failed with error: %v", err) - } - ports = p + ports = getPorts(cd, canaryDep.Spec.Template.Spec.Containers) } return label, ports, nil @@ -223,16 +209,12 @@ func (c *DeploymentController) createPrimaryDeployment(cd *flaggerv1.Canary) err canaryDep, err := c.kubeClient.AppsV1().Deployments(cd.Namespace).Get(targetName, metav1.GetOptions{}) if err != nil { - if errors.IsNotFound(err) { - return fmt.Errorf("deployment %s.%s not found, retrying", targetName, cd.Namespace) - } - return err + return fmt.Errorf("deplyoment %s.%s get query error %w", targetName, cd.Namespace, err) } label, err := c.getSelectorLabel(canaryDep) if err != nil { - return fmt.Errorf("invalid label selector! Deployment %s.%s spec.selector.matchLabels must contain selector 'app: %s'", - targetName, cd.Namespace, targetName) + return fmt.Errorf("getSelectorLabel failed: %w", err) } primaryDep, err := c.kubeClient.AppsV1().Deployments(cd.Namespace).Get(primaryName, metav1.GetOptions{}) @@ -240,14 +222,14 @@ func (c *DeploymentController) createPrimaryDeployment(cd *flaggerv1.Canary) err // create primary secrets and config maps configRefs, err := c.configTracker.GetTargetConfigs(cd) if err != nil { - return err + return fmt.Errorf("GetTargetConfigs failed: %w", err) } if err := c.configTracker.CreatePrimaryConfigs(cd, configRefs); err != nil { - return err + return fmt.Errorf("CreatePrimaryConfigs failed: %w", err) } annotations, err := makeAnnotations(canaryDep.Spec.Template.Annotations) if err != nil { - return err + return fmt.Errorf("makeAnnotations failed: %w", err) } replicas := int32(1) @@ -295,7 +277,7 @@ func (c *DeploymentController) createPrimaryDeployment(cd *flaggerv1.Canary) err _, err = c.kubeClient.AppsV1().Deployments(cd.Namespace).Create(primaryDep) if err != nil { - return err + return fmt.Errorf("creating deployment %s.%s failed: %w", primaryDep.Name, cd.Namespace, err) } c.logger.With("canary", fmt.Sprintf("%s.%s", cd.Name, cd.Namespace)).Infof("Deployment %s.%s created", primaryDep.GetName(), cd.Namespace) @@ -308,11 +290,8 @@ func (c *DeploymentController) reconcilePrimaryHpa(cd *flaggerv1.Canary, init bo primaryName := fmt.Sprintf("%s-primary", cd.Spec.TargetRef.Name) hpa, err := c.kubeClient.AutoscalingV2beta1().HorizontalPodAutoscalers(cd.Namespace).Get(cd.Spec.AutoscalerRef.Name, metav1.GetOptions{}) if err != nil { - if errors.IsNotFound(err) { - return fmt.Errorf("HorizontalPodAutoscaler %s.%s not found, retrying", - cd.Spec.AutoscalerRef.Name, cd.Namespace) - } - return err + return fmt.Errorf("HorizontalPodAutoscaler %s.%s get query error: %w", + cd.Spec.AutoscalerRef.Name, cd.Namespace, err) } hpaSpec := hpav1.HorizontalPodAutoscalerSpec{ @@ -349,14 +328,15 @@ func (c *DeploymentController) reconcilePrimaryHpa(cd *flaggerv1.Canary, init bo _, err = c.kubeClient.AutoscalingV2beta1().HorizontalPodAutoscalers(cd.Namespace).Create(primaryHpa) if err != nil { - return err + return fmt.Errorf("creating HorizontalPodAutoscaler %s.%s failed: %w", + primaryHpa.Name, primaryHpa.Namespace, err) } - c.logger.With("canary", fmt.Sprintf("%s.%s", cd.Name, cd.Namespace)).Infof("HorizontalPodAutoscaler %s.%s created", primaryHpa.GetName(), cd.Namespace) + c.logger.With("canary", fmt.Sprintf("%s.%s", cd.Name, cd.Namespace)).Infof( + "HorizontalPodAutoscaler %s.%s created", primaryHpa.GetName(), cd.Namespace) return nil - } - - if err != nil { - return err + } else if err != nil { + return fmt.Errorf("HorizontalPodAutoscaler %s.%s exists but get query failed: %w", + primaryHpa.Name, primaryHpa.Namespace, err) } // update HPA @@ -369,14 +349,15 @@ func (c *DeploymentController) reconcilePrimaryHpa(cd *flaggerv1.Canary, init bo hpaClone.Spec.MinReplicas = hpaSpec.MinReplicas hpaClone.Spec.Metrics = hpaSpec.Metrics - _, upErr := c.kubeClient.AutoscalingV2beta1().HorizontalPodAutoscalers(cd.Namespace).Update(hpaClone) - if upErr != nil { - return upErr + _, err := c.kubeClient.AutoscalingV2beta1().HorizontalPodAutoscalers(cd.Namespace).Update(hpaClone) + if err != nil { + return fmt.Errorf("updating HorizontalPodAutoscaler %s.%s failed: %w", + hpaClone.Name, hpaClone.Namespace, err) } - c.logger.With("canary", fmt.Sprintf("%s.%s", cd.Name, cd.Namespace)).Infof("HorizontalPodAutoscaler %s.%s updated", primaryHpa.GetName(), cd.Namespace) + c.logger.With("canary", fmt.Sprintf("%s.%s", cd.Name, cd.Namespace)). + Infof("HorizontalPodAutoscaler %s.%s updated", primaryHpa.GetName(), cd.Namespace) } } - return nil } @@ -388,7 +369,10 @@ func (c *DeploymentController) getSelectorLabel(deployment *appsv1.Deployment) ( } } - return "", fmt.Errorf("selector not found") + return "", fmt.Errorf( + "deployment %s.%s spec.selector.matchLabels must contain one of %v", + c.labels, deployment.Name, deployment.Namespace, + ) } func (c *DeploymentController) HaveDependenciesChanged(cd *flaggerv1.Canary) (bool, error) { diff --git a/pkg/canary/deployment_controller_test.go b/pkg/canary/deployment_controller_test.go index 06958e3f8..fb8d574c7 100644 --- a/pkg/canary/deployment_controller_test.go +++ b/pkg/canary/deployment_controller_test.go @@ -77,7 +77,7 @@ func TestDeploymentController_IsReady(t *testing.T) { err := mocks.controller.Initialize(mocks.canary, true) require.NoError(t, err, "Expected primary readiness check to fail") - _, err = mocks.controller.IsPrimaryReady(mocks.canary) + err = mocks.controller.IsPrimaryReady(mocks.canary) require.Error(t, err) _, err = mocks.controller.IsCanaryReady(mocks.canary) @@ -134,17 +134,17 @@ func TestDeploymentController_SyncStatus(t *testing.T) { assert.True(t, exists, "Secret %s not found in status", secret.GetName()) } -func TestDeploymentController_Scale(t *testing.T) { +func TestDeploymentController_ScaleToZero(t *testing.T) { mocks := newDeploymentFixture() err := mocks.controller.Initialize(mocks.canary, true) require.NoError(t, err) - err = mocks.controller.Scale(mocks.canary, 2) + err = mocks.controller.ScaleToZero(mocks.canary) require.NoError(t, err) c, err := mocks.kubeClient.AppsV1().Deployments("default").Get("podinfo", metav1.GetOptions{}) require.NoError(t, err) - assert.Equal(t, int32(2), *c.Spec.Replicas) + assert.Equal(t, int32(0), *c.Spec.Replicas) } func TestDeploymentController_NoConfigTracking(t *testing.T) { diff --git a/pkg/canary/deployment_ready.go b/pkg/canary/deployment_ready.go index 083633f96..6fdd9d9a9 100644 --- a/pkg/canary/deployment_ready.go +++ b/pkg/canary/deployment_ready.go @@ -5,7 +5,6 @@ import ( "time" appsv1 "k8s.io/api/apps/v1" - "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" flaggerv1 "github.com/weaveworks/flagger/pkg/apis/flagger/v1beta1" @@ -14,26 +13,23 @@ import ( // IsPrimaryReady checks the primary deployment status and returns an error if // the deployment is in the middle of a rolling update or if the pods are unhealthy // it will return a non retriable error if the rolling update is stuck -func (c *DeploymentController) IsPrimaryReady(cd *flaggerv1.Canary) (bool, error) { +func (c *DeploymentController) IsPrimaryReady(cd *flaggerv1.Canary) error { primaryName := fmt.Sprintf("%s-primary", cd.Spec.TargetRef.Name) primary, err := c.kubeClient.AppsV1().Deployments(cd.Namespace).Get(primaryName, metav1.GetOptions{}) if err != nil { - if errors.IsNotFound(err) { - return true, fmt.Errorf("deployment %s.%s not found", primaryName, cd.Namespace) - } - return true, fmt.Errorf("deployment %s.%s query error %v", primaryName, cd.Namespace, err) + return fmt.Errorf("deployment %s.%s get query error %w", primaryName, cd.Namespace, err) } - retriable, err := c.isDeploymentReady(primary, cd.GetProgressDeadlineSeconds()) + _, err = c.isDeploymentReady(primary, cd.GetProgressDeadlineSeconds()) if err != nil { - return retriable, fmt.Errorf("Halt advancement %s.%s %s", primaryName, cd.Namespace, err.Error()) + return fmt.Errorf("primary daemonset %s.%s not ready: %w", primaryName, cd.Namespace, err) } if primary.Spec.Replicas == int32p(0) { - return true, fmt.Errorf("Halt %s.%s advancement primary deployment is scaled to zero", + return fmt.Errorf("halt %s.%s advancement: primary deployment is scaled to zero", cd.Name, cd.Namespace) } - return true, nil + return nil } // IsCanaryReady checks the canary deployment status and returns an error if @@ -43,22 +39,16 @@ func (c *DeploymentController) IsCanaryReady(cd *flaggerv1.Canary) (bool, error) targetName := cd.Spec.TargetRef.Name canary, err := c.kubeClient.AppsV1().Deployments(cd.Namespace).Get(targetName, metav1.GetOptions{}) if err != nil { - if errors.IsNotFound(err) { - return true, fmt.Errorf("deployment %s.%s not found", targetName, cd.Namespace) - } - return true, fmt.Errorf("deployment %s.%s query error %v", targetName, cd.Namespace, err) + return true, fmt.Errorf("deployment %s.%s get query error: %w", targetName, cd.Namespace, err) } retriable, err := c.isDeploymentReady(canary, cd.GetProgressDeadlineSeconds()) if err != nil { - if retriable { - return retriable, fmt.Errorf("Halt advancement %s.%s %s", targetName, cd.Namespace, err.Error()) - } else { - return retriable, fmt.Errorf("deployment does not have minimum availability for more than %vs", - cd.GetProgressDeadlineSeconds()) - } + return retriable, fmt.Errorf( + "canary deployment %s.%s not ready with retriablility %v: %w", + targetName, cd.Namespace, retriable, err, + ) } - return true, nil } @@ -93,9 +83,9 @@ func (c *DeploymentController) isDeploymentReady(deployment *appsv1.Deployment, } } else { - return true, fmt.Errorf("waiting for rollout to finish: observed deployment generation less then desired generation") + return true, fmt.Errorf( + "waiting for rollout to finish: observed deployment generation less then desired generation") } - return true, nil } diff --git a/pkg/canary/deployment_status.go b/pkg/canary/deployment_status.go index e5577b511..6c2babe60 100644 --- a/pkg/canary/deployment_status.go +++ b/pkg/canary/deployment_status.go @@ -3,8 +3,6 @@ package canary import ( "fmt" - ex "github.com/pkg/errors" - "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" flaggerv1 "github.com/weaveworks/flagger/pkg/apis/flagger/v1beta1" @@ -14,15 +12,12 @@ import ( func (c *DeploymentController) SyncStatus(cd *flaggerv1.Canary, status flaggerv1.CanaryStatus) error { dep, err := c.kubeClient.AppsV1().Deployments(cd.Namespace).Get(cd.Spec.TargetRef.Name, metav1.GetOptions{}) if err != nil { - if errors.IsNotFound(err) { - return fmt.Errorf("deployment %s.%s not found", cd.Spec.TargetRef.Name, cd.Namespace) - } - return ex.Wrap(err, "SyncStatus deployment query error") + return fmt.Errorf("deployment %s.%s get query error: %w", cd.Spec.TargetRef.Name, cd.Namespace, err) } configs, err := c.configTracker.GetConfigRefs(cd) if err != nil { - return ex.Wrap(err, "SyncStatus configs query error") + return fmt.Errorf("GetConfigRefs failed: %w", err) } return syncCanaryStatus(c.flaggerClient, cd, status, dep.Spec.Template, func(cdCopy *flaggerv1.Canary) { diff --git a/pkg/canary/factory.go b/pkg/canary/factory.go index 416a0c91e..95f243b8b 100644 --- a/pkg/canary/factory.go +++ b/pkg/canary/factory.go @@ -50,12 +50,12 @@ func (factory *Factory) Controller(kind string) Controller { flaggerClient: factory.flaggerClient, } - switch { - case kind == "DaemonSet": + switch kind { + case "DaemonSet": return daemonSetCtrl - case kind == "Deployment": + case "Deployment": return deploymentCtrl - case kind == "Service": + case "Service": return serviceCtrl default: return deploymentCtrl diff --git a/pkg/canary/nop_tracker.go b/pkg/canary/nop_tracker.go index d2234a0b0..866c08c48 100644 --- a/pkg/canary/nop_tracker.go +++ b/pkg/canary/nop_tracker.go @@ -6,8 +6,7 @@ import ( ) // NopTracker no-operation tracker -type NopTracker struct { -} +type NopTracker struct{} func (nt *NopTracker) GetTargetConfigs(*flaggerv1.Canary) (map[string]ConfigRef, error) { res := make(map[string]ConfigRef) @@ -26,6 +25,6 @@ func (nt *NopTracker) CreatePrimaryConfigs(*flaggerv1.Canary, map[string]ConfigR return nil } -func (nt *NopTracker) ApplyPrimaryConfigs(spec corev1.PodSpec, refs map[string]ConfigRef) corev1.PodSpec { +func (nt *NopTracker) ApplyPrimaryConfigs(spec corev1.PodSpec, _ map[string]ConfigRef) corev1.PodSpec { return spec } diff --git a/pkg/canary/service_controller.go b/pkg/canary/service_controller.go index 3e3724d2c..cfd1703fb 100644 --- a/pkg/canary/service_controller.go +++ b/pkg/canary/service_controller.go @@ -3,7 +3,6 @@ package canary import ( "fmt" - ex "github.com/pkg/errors" "go.uber.org/zap" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" @@ -43,31 +42,27 @@ func (c *ServiceController) SetStatusPhase(cd *flaggerv1.Canary, phase flaggerv1 } // GetMetadata returns the pod label selector and svc ports -func (c *ServiceController) GetMetadata(cd *flaggerv1.Canary) (string, map[string]int32, error) { +func (c *ServiceController) GetMetadata(_ *flaggerv1.Canary) (string, map[string]int32, error) { return "", nil, nil } // Initialize creates or updates the primary and canary services to prepare for the canary release process targeted on the K8s service -func (c *ServiceController) Initialize(cd *flaggerv1.Canary, skipLivenessChecks bool) (err error) { +func (c *ServiceController) Initialize(cd *flaggerv1.Canary, _ bool) (err error) { targetName := cd.Spec.TargetRef.Name primaryName := fmt.Sprintf("%s-primary", targetName) canaryName := fmt.Sprintf("%s-canary", targetName) svc, err := c.kubeClient.CoreV1().Services(cd.Namespace).Get(targetName, metav1.GetOptions{}) if err != nil { - return err + return fmt.Errorf("service %s.%s get query error: %w", primaryName, cd.Namespace, err) } - // canary svc - err = c.reconcileCanaryService(cd, canaryName, svc) - if err != nil { - return err + if err = c.reconcileCanaryService(cd, canaryName, svc); err != nil { + return fmt.Errorf("reconcileCanaryService failed: %w", err) } - // primary svc - err = c.reconcilePrimaryService(cd, primaryName, svc) - if err != nil { - return err + if err = c.reconcilePrimaryService(cd, primaryName, svc); err != nil { + return fmt.Errorf("reconcilePrimaryService failed: %w", err) } return nil @@ -77,31 +72,29 @@ func (c *ServiceController) reconcileCanaryService(canary *flaggerv1.Canary, nam current, err := c.kubeClient.CoreV1().Services(canary.Namespace).Get(name, metav1.GetOptions{}) if errors.IsNotFound(err) { return c.createService(canary, name, src) + } else if err != nil { + return fmt.Errorf("service %s.%s get query error: %w", name, canary.Namespace, err) } - if err != nil { - return fmt.Errorf("service %s query error %v", name, err) - } + ns := buildService(canary, name, src) - new := buildService(canary, name, src) - - if new.Spec.Type == "ClusterIP" { + if ns.Spec.Type == "ClusterIP" { // We can't change this immutable field - new.Spec.ClusterIP = current.Spec.ClusterIP + ns.Spec.ClusterIP = current.Spec.ClusterIP } // We can't change this immutable field - new.ObjectMeta.UID = current.ObjectMeta.UID + ns.ObjectMeta.UID = current.ObjectMeta.UID - new.ObjectMeta.ResourceVersion = current.ObjectMeta.ResourceVersion + ns.ObjectMeta.ResourceVersion = current.ObjectMeta.ResourceVersion - _, err = c.kubeClient.CoreV1().Services(canary.Namespace).Update(new) + _, err = c.kubeClient.CoreV1().Services(canary.Namespace).Update(ns) if err != nil { - return err + return fmt.Errorf("updating service %s.%s failed: %w", name, canary.Namespace, err) } c.logger.With("canary", fmt.Sprintf("%s.%s", canary.Name, canary.Namespace)). - Infof("Service %s.%s updated", new.GetName(), canary.Namespace) + Infof("Service %s.%s updated", ns.GetName(), canary.Namespace) return nil } @@ -109,12 +102,9 @@ func (c *ServiceController) reconcilePrimaryService(canary *flaggerv1.Canary, na _, err := c.kubeClient.CoreV1().Services(canary.Namespace).Get(name, metav1.GetOptions{}) if errors.IsNotFound(err) { return c.createService(canary, name, src) + } else if err != nil { + return fmt.Errorf("service %s.%s get query error: %w", name, canary.Namespace, err) } - - if err != nil { - return fmt.Errorf("service %s query error %v", name, err) - } - return nil } @@ -131,7 +121,7 @@ func (c *ServiceController) createService(canary *flaggerv1.Canary, name string, _, err := c.kubeClient.CoreV1().Services(canary.Namespace).Create(svc) if err != nil { - return err + return fmt.Errorf("creating service %s.%s query error: %w", canary.Name, canary.Namespace, err) } c.logger.With("canary", fmt.Sprintf("%s.%s", canary.Name, canary.Namespace)). @@ -156,7 +146,6 @@ func buildService(canary *flaggerv1.Canary, name string, src *corev1.Service) *c // Operation cannot be fulfilled on services "mysvc-canary": the object has been modified; please apply your changes to the latest version and try again delete(svc.ObjectMeta.Annotations, "kubectl.kubernetes.io/last-applied-configuration") } - return svc } @@ -167,18 +156,12 @@ func (c *ServiceController) Promote(cd *flaggerv1.Canary) error { canary, err := c.kubeClient.CoreV1().Services(cd.Namespace).Get(targetName, metav1.GetOptions{}) if err != nil { - if errors.IsNotFound(err) { - return fmt.Errorf("service %s.%s not found", targetName, cd.Namespace) - } - return fmt.Errorf("service %s.%s query error %v", targetName, cd.Namespace, err) + return fmt.Errorf("service %s.%s get query error %w", targetName, cd.Namespace, err) } primary, err := c.kubeClient.CoreV1().Services(cd.Namespace).Get(primaryName, metav1.GetOptions{}) if err != nil { - if errors.IsNotFound(err) { - return fmt.Errorf("service %s.%s not found", primaryName, cd.Namespace) - } - return fmt.Errorf("service %s.%s query error %v", primaryName, cd.Namespace, err) + return fmt.Errorf("service %s.%s get query error %w", primaryName, cd.Namespace, err) } primaryCopy := canary.DeepCopy() @@ -192,7 +175,7 @@ func (c *ServiceController) Promote(cd *flaggerv1.Canary) error { // apply update _, err = c.kubeClient.CoreV1().Services(cd.Namespace).Update(primaryCopy) if err != nil { - return fmt.Errorf("updating service %s.%s spec failed: %v", + return fmt.Errorf("updating service %s.%s spec failed: %w", primaryCopy.GetName(), primaryCopy.Namespace, err) } @@ -204,44 +187,37 @@ func (c *ServiceController) HasTargetChanged(cd *flaggerv1.Canary) (bool, error) targetName := cd.Spec.TargetRef.Name canary, err := c.kubeClient.CoreV1().Services(cd.Namespace).Get(targetName, metav1.GetOptions{}) if err != nil { - if errors.IsNotFound(err) { - return false, fmt.Errorf("service %s.%s not found", targetName, cd.Namespace) - } - return false, fmt.Errorf("service %s.%s query error %v", targetName, cd.Namespace, err) + return false, fmt.Errorf("service %s.%s get query error %w", targetName, cd.Namespace, err) } - return hasSpecChanged(cd, canary.Spec) } // Scale sets the canary deployment replicas -func (c *ServiceController) Scale(cd *flaggerv1.Canary, replicas int32) error { +func (c *ServiceController) ScaleToZero(_ *flaggerv1.Canary) error { return nil } -func (c *ServiceController) ScaleFromZero(cd *flaggerv1.Canary) error { +func (c *ServiceController) ScaleFromZero(_ *flaggerv1.Canary) error { return nil } func (c *ServiceController) SyncStatus(cd *flaggerv1.Canary, status flaggerv1.CanaryStatus) error { dep, err := c.kubeClient.CoreV1().Services(cd.Namespace).Get(cd.Spec.TargetRef.Name, metav1.GetOptions{}) if err != nil { - if errors.IsNotFound(err) { - return fmt.Errorf("service %s.%s not found", cd.Spec.TargetRef.Name, cd.Namespace) - } - return ex.Wrap(err, "SyncStatus service query error") + return fmt.Errorf("service %s.%s get query error %w", cd.Spec.TargetRef.Name, cd.Namespace, err) } return syncCanaryStatus(c.flaggerClient, cd, status, dep.Spec, func(cdCopy *flaggerv1.Canary) {}) } -func (c *ServiceController) HaveDependenciesChanged(cd *flaggerv1.Canary) (bool, error) { +func (c *ServiceController) HaveDependenciesChanged(_ *flaggerv1.Canary) (bool, error) { return false, nil } -func (c *ServiceController) IsPrimaryReady(cd *flaggerv1.Canary) (bool, error) { - return true, nil +func (c *ServiceController) IsPrimaryReady(_ *flaggerv1.Canary) error { + return nil } -func (c *ServiceController) IsCanaryReady(cd *flaggerv1.Canary) (bool, error) { +func (c *ServiceController) IsCanaryReady(_ *flaggerv1.Canary) (bool, error) { return true, nil } diff --git a/pkg/canary/status.go b/pkg/canary/status.go index d65fd979d..6d3f2e347 100644 --- a/pkg/canary/status.go +++ b/pkg/canary/status.go @@ -4,7 +4,6 @@ import ( "fmt" "strings" - ex "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/util/retry" @@ -17,12 +16,12 @@ func syncCanaryStatus(flaggerClient clientset.Interface, cd *flaggerv1.Canary, s hash := computeHash(canaryResource) firstTry := true + name, ns := cd.GetName(), cd.GetNamespace() err := retry.RetryOnConflict(retry.DefaultBackoff, func() (err error) { - var selErr error if !firstTry { - cd, selErr = flaggerClient.FlaggerV1beta1().Canaries(cd.Namespace).Get(cd.GetName(), metav1.GetOptions{}) - if selErr != nil { - return selErr + cd, err = flaggerClient.FlaggerV1beta1().Canaries(ns).Get(name, metav1.GetOptions{}) + if err != nil { + return fmt.Errorf("canary %s.%s get query failed: %w", name, ns, err) } } @@ -39,72 +38,79 @@ func syncCanaryStatus(flaggerClient clientset.Interface, cd *flaggerv1.Canary, s cdCopy.Status.Conditions = conditions } - err = updateStatusWithUpgrade(flaggerClient, cdCopy) + if err = updateStatusWithUpgrade(flaggerClient, cdCopy); err != nil { + return fmt.Errorf("updateStatusWithUpgrade failed: %w", err) + } firstTry = false return }) + if err != nil { - return ex.Wrap(err, "SyncStatus") + return fmt.Errorf("failed after retries: %w", err) } return nil } func setStatusFailedChecks(flaggerClient clientset.Interface, cd *flaggerv1.Canary, val int) error { firstTry := true + name, ns := cd.GetName(), cd.GetNamespace() err := retry.RetryOnConflict(retry.DefaultBackoff, func() (err error) { - var selErr error if !firstTry { - cd, selErr = flaggerClient.FlaggerV1beta1().Canaries(cd.Namespace).Get(cd.GetName(), metav1.GetOptions{}) - if selErr != nil { - return selErr + cd, err = flaggerClient.FlaggerV1beta1().Canaries(name).Get(ns, metav1.GetOptions{}) + if err != nil { + return fmt.Errorf("canary %s.%s get query failed: %w", name, ns, err) } } cdCopy := cd.DeepCopy() cdCopy.Status.FailedChecks = val cdCopy.Status.LastTransitionTime = metav1.Now() - err = updateStatusWithUpgrade(flaggerClient, cdCopy) + if err = updateStatusWithUpgrade(flaggerClient, cdCopy); err != nil { + return fmt.Errorf("updateStatusWithUpgrade failed: %w", err) + } firstTry = false return }) if err != nil { - return ex.Wrap(err, "SetStatusFailedChecks") + return fmt.Errorf("failed after retries: %w", err) } return nil } func setStatusWeight(flaggerClient clientset.Interface, cd *flaggerv1.Canary, val int) error { firstTry := true + name, ns := cd.GetName(), cd.GetNamespace() err := retry.RetryOnConflict(retry.DefaultBackoff, func() (err error) { - var selErr error if !firstTry { - cd, selErr = flaggerClient.FlaggerV1beta1().Canaries(cd.Namespace).Get(cd.GetName(), metav1.GetOptions{}) - if selErr != nil { - return selErr + cd, err = flaggerClient.FlaggerV1beta1().Canaries(cd.Namespace).Get(cd.GetName(), metav1.GetOptions{}) + if err != nil { + return fmt.Errorf("canary %s.%s get query failed: %w", name, ns, err) } } cdCopy := cd.DeepCopy() cdCopy.Status.CanaryWeight = val cdCopy.Status.LastTransitionTime = metav1.Now() - err = updateStatusWithUpgrade(flaggerClient, cdCopy) + if err = updateStatusWithUpgrade(flaggerClient, cdCopy); err != nil { + return fmt.Errorf("updateStatusWithUpgrade failed: %w", err) + } firstTry = false return }) if err != nil { - return ex.Wrap(err, "SetStatusWeight") + return fmt.Errorf("failed after retries: %w", err) } return nil } func setStatusIterations(flaggerClient clientset.Interface, cd *flaggerv1.Canary, val int) error { firstTry := true + name, ns := cd.GetName(), cd.GetNamespace() err := retry.RetryOnConflict(retry.DefaultBackoff, func() (err error) { - var selErr error if !firstTry { - cd, selErr = flaggerClient.FlaggerV1beta1().Canaries(cd.Namespace).Get(cd.GetName(), metav1.GetOptions{}) - if selErr != nil { - return selErr + cd, err = flaggerClient.FlaggerV1beta1().Canaries(cd.Namespace).Get(cd.GetName(), metav1.GetOptions{}) + if err != nil { + return fmt.Errorf("canary %s.%s get query failed: %w", name, ns, err) } } @@ -112,25 +118,27 @@ func setStatusIterations(flaggerClient clientset.Interface, cd *flaggerv1.Canary cdCopy.Status.Iterations = val cdCopy.Status.LastTransitionTime = metav1.Now() - err = updateStatusWithUpgrade(flaggerClient, cdCopy) + if err = updateStatusWithUpgrade(flaggerClient, cdCopy); err != nil { + return fmt.Errorf("updateStatusWithUpgrade failed: %w", err) + } firstTry = false return }) if err != nil { - return ex.Wrap(err, "SetStatusIterations") + return fmt.Errorf("failed after retries: %w", err) } return nil } func setStatusPhase(flaggerClient clientset.Interface, cd *flaggerv1.Canary, phase flaggerv1.CanaryPhase) error { firstTry := true + name, ns := cd.GetName(), cd.GetNamespace() err := retry.RetryOnConflict(retry.DefaultBackoff, func() (err error) { - var selErr error if !firstTry { - cd, selErr = flaggerClient.FlaggerV1beta1().Canaries(cd.Namespace).Get(cd.GetName(), metav1.GetOptions{}) - if selErr != nil { - return selErr + cd, err = flaggerClient.FlaggerV1beta1().Canaries(cd.Namespace).Get(cd.GetName(), metav1.GetOptions{}) + if err != nil { + return fmt.Errorf("canary %s.%s get query failed: %w", name, ns, err) } } @@ -152,12 +160,14 @@ func setStatusPhase(flaggerClient clientset.Interface, cd *flaggerv1.Canary, pha cdCopy.Status.Conditions = conditions } - err = updateStatusWithUpgrade(flaggerClient, cdCopy) + if err = updateStatusWithUpgrade(flaggerClient, cdCopy); err != nil { + return fmt.Errorf("updateStatusWithUpgrade failed: %w", err) + } firstTry = false return }) if err != nil { - return ex.Wrap(err, "SetStatusPhase") + return fmt.Errorf("failed after retries: %w", err) } return nil } @@ -239,11 +249,14 @@ func updateStatusWithUpgrade(flaggerClient clientset.Interface, cd *flaggerv1.Ca // upgrade alpha resource _, updateErr := flaggerClient.FlaggerV1beta1().Canaries(cd.Namespace).Update(cd) if updateErr != nil { - return updateErr + return fmt.Errorf("updating canary %s.%s from v1alpha to v1beta failed: %w", cd.Name, cd.Namespace, updateErr) } - // retry status update _, err = flaggerClient.FlaggerV1beta1().Canaries(cd.Namespace).UpdateStatus(cd) } + + if err != nil { + return fmt.Errorf("updating canary %s.%s status failed: %w", cd.Name, cd.Namespace, err) + } return err } diff --git a/pkg/canary/util.go b/pkg/canary/util.go index cbe9c0e84..f4f583045 100644 --- a/pkg/canary/util.go +++ b/pkg/canary/util.go @@ -16,7 +16,7 @@ var sidecars = map[string]bool{ "envoy": true, } -func getPorts(cd *flaggerv1.Canary, cs []corev1.Container) (map[string]int32, error) { +func getPorts(cd *flaggerv1.Canary, cs []corev1.Container) map[string]int32 { ports := make(map[string]int32, len(cs)) for _, container := range cs { // exclude service mesh proxies based on container name @@ -49,7 +49,7 @@ func getPorts(cd *flaggerv1.Canary, cs []corev1.Container) (map[string]int32, er ports[name] = p.ContainerPort } } - return ports, nil + return ports } // makeAnnotations appends an unique ID to annotations map @@ -59,7 +59,7 @@ func makeAnnotations(annotations map[string]string) (map[string]string, error) { uuid := make([]byte, 16) n, err := io.ReadFull(rand.Reader, uuid) if n != len(uuid) || err != nil { - return res, err + return res, fmt.Errorf("%w", err) } uuid[8] = uuid[8]&^0xc0 | 0x80 uuid[6] = uuid[6]&^0xf0 | 0x40 diff --git a/pkg/controller/scheduler.go b/pkg/controller/scheduler.go index 6d393b064..050fbdd1f 100644 --- a/pkg/controller/scheduler.go +++ b/pkg/controller/scheduler.go @@ -163,7 +163,7 @@ func (c *Controller) advanceCanary(name string, namespace string, skipLivenessCh // check primary status if !skipLivenessChecks && !cd.SkipAnalysis() { - if _, err := canaryController.IsPrimaryReady(cd); err != nil { + if err := canaryController.IsPrimaryReady(cd); err != nil { c.recordEventWarningf(cd, "%v", err) return } @@ -258,7 +258,7 @@ func (c *Controller) advanceCanary(name string, namespace string, skipLivenessCh // scale canary to zero if promotion has finished if cd.Status.Phase == flaggerv1.CanaryPhaseFinalising { - if err := canaryController.Scale(cd, 0); err != nil { + if err := canaryController.ScaleToZero(cd); err != nil { c.recordEventWarningf(cd, "%v", err) return } @@ -554,7 +554,7 @@ func (c *Controller) shouldSkipAnalysis(canary *flaggerv1.Canary, canaryControll } // shutdown canary - if err := canaryController.Scale(canary, 0); err != nil { + if err := canaryController.ScaleToZero(canary); err != nil { c.recordEventWarningf(canary, "%v", err) return false } @@ -1030,7 +1030,7 @@ func (c *Controller) rollback(canary *flaggerv1.Canary, canaryController canary. c.recorder.SetWeight(canary, primaryWeight, canaryWeight) // shutdown canary - if err := canaryController.Scale(canary, 0); err != nil { + if err := canaryController.ScaleToZero(canary); err != nil { c.recordEventWarningf(canary, "%v", err) return } From 7fb675e8aa152dbda04744b057f754743f033ed9 Mon Sep 17 00:00:00 2001 From: mathetake Date: Sat, 7 Mar 2020 23:28:50 +0900 Subject: [PATCH 02/11] make deployment tests aligned with daemonset --- pkg/canary/daemonset_controller.go | 10 ++-- pkg/canary/daemonset_ready.go | 4 +- pkg/canary/deployment_controller.go | 8 +-- pkg/canary/deployment_controller_test.go | 62 ------------------------ pkg/canary/deployment_ready.go | 2 +- pkg/canary/deployment_ready_test.go | 19 ++++++++ pkg/canary/deployment_status_test.go | 61 +++++++++++++++++++++++ pkg/canary/service_controller.go | 8 +-- 8 files changed, 96 insertions(+), 78 deletions(-) create mode 100644 pkg/canary/deployment_ready_test.go create mode 100644 pkg/canary/deployment_status_test.go diff --git a/pkg/canary/daemonset_controller.go b/pkg/canary/daemonset_controller.go index 074d343aa..1eded3eb4 100644 --- a/pkg/canary/daemonset_controller.go +++ b/pkg/canary/daemonset_controller.go @@ -102,7 +102,7 @@ func (c *DaemonSetController) Promote(cd *flaggerv1.Canary) error { canary, err := c.kubeClient.AppsV1().DaemonSets(cd.Namespace).Get(targetName, metav1.GetOptions{}) if err != nil { - return fmt.Errorf("damonset %s.%s get query error %v", targetName, cd.Namespace, err) + return fmt.Errorf("damonset %s.%s get query error: %v", targetName, cd.Namespace, err) } label, err := c.getSelectorLabel(canary) @@ -112,7 +112,7 @@ func (c *DaemonSetController) Promote(cd *flaggerv1.Canary) error { primary, err := c.kubeClient.AppsV1().DaemonSets(cd.Namespace).Get(primaryName, metav1.GetOptions{}) if err != nil { - return fmt.Errorf("daemonset %s.%s get query error %w", primaryName, cd.Namespace, err) + return fmt.Errorf("daemonset %s.%s get query error: %w", primaryName, cd.Namespace, err) } // promote secrets and config maps @@ -160,7 +160,7 @@ func (c *DaemonSetController) HasTargetChanged(cd *flaggerv1.Canary) (bool, erro targetName := cd.Spec.TargetRef.Name canary, err := c.kubeClient.AppsV1().DaemonSets(cd.Namespace).Get(targetName, metav1.GetOptions{}) if err != nil { - return false, fmt.Errorf("daemonset %s.%s get query error %w", targetName, cd.Namespace, err) + return false, fmt.Errorf("daemonset %s.%s get query error: %w", targetName, cd.Namespace, err) } // ignore `daemonSetScaleDownNodeSelector` node selector @@ -182,7 +182,7 @@ func (c *DaemonSetController) GetMetadata(cd *flaggerv1.Canary) (string, map[str canaryDae, err := c.kubeClient.AppsV1().DaemonSets(cd.Namespace).Get(targetName, metav1.GetOptions{}) if err != nil { - return "", nil, fmt.Errorf("daemonset %s.%s get query error %w", targetName, cd.Namespace, err) + return "", nil, fmt.Errorf("daemonset %s.%s get query error: %w", targetName, cd.Namespace, err) } label, err := c.getSelectorLabel(canaryDae) @@ -203,7 +203,7 @@ func (c *DaemonSetController) createPrimaryDaemonSet(cd *flaggerv1.Canary) error canaryDae, err := c.kubeClient.AppsV1().DaemonSets(cd.Namespace).Get(targetName, metav1.GetOptions{}) if err != nil { - return fmt.Errorf("daemonset %s.%s get query error %w", targetName, cd.Namespace, err) + return fmt.Errorf("daemonset %s.%s get query error: %w", targetName, cd.Namespace, err) } if canaryDae.Spec.UpdateStrategy.Type != "" && diff --git a/pkg/canary/daemonset_ready.go b/pkg/canary/daemonset_ready.go index 81b892b1f..7e68b6dba 100644 --- a/pkg/canary/daemonset_ready.go +++ b/pkg/canary/daemonset_ready.go @@ -16,7 +16,7 @@ func (c *DaemonSetController) IsPrimaryReady(cd *flaggerv1.Canary) error { primaryName := fmt.Sprintf("%s-primary", cd.Spec.TargetRef.Name) primary, err := c.kubeClient.AppsV1().DaemonSets(cd.Namespace).Get(primaryName, metav1.GetOptions{}) if err != nil { - return fmt.Errorf("daemonset %s.%s get query error %w", primaryName, cd.Namespace, err) + return fmt.Errorf("daemonset %s.%s get query error: %w", primaryName, cd.Namespace, err) } _, err = c.isDaemonSetReady(cd, primary) @@ -32,7 +32,7 @@ func (c *DaemonSetController) IsCanaryReady(cd *flaggerv1.Canary) (bool, error) targetName := cd.Spec.TargetRef.Name canary, err := c.kubeClient.AppsV1().DaemonSets(cd.Namespace).Get(targetName, metav1.GetOptions{}) if err != nil { - return true, fmt.Errorf("daemonset %s.%s get query error %w", targetName, cd.Namespace, err) + return true, fmt.Errorf("daemonset %s.%s get query error: %w", targetName, cd.Namespace, err) } retriable, err := c.isDaemonSetReady(cd, canary) diff --git a/pkg/canary/deployment_controller.go b/pkg/canary/deployment_controller.go index 8364845d2..c1dc5cc8f 100644 --- a/pkg/canary/deployment_controller.go +++ b/pkg/canary/deployment_controller.go @@ -70,7 +70,7 @@ func (c *DeploymentController) Promote(cd *flaggerv1.Canary) error { canary, err := c.kubeClient.AppsV1().Deployments(cd.Namespace).Get(targetName, metav1.GetOptions{}) if err != nil { - return fmt.Errorf("deployment %s.%s query error %v", targetName, cd.Namespace, err) + return fmt.Errorf("deployment %s.%s query error: %v", targetName, cd.Namespace, err) } label, err := c.getSelectorLabel(canary) @@ -80,7 +80,7 @@ func (c *DeploymentController) Promote(cd *flaggerv1.Canary) error { primary, err := c.kubeClient.AppsV1().Deployments(cd.Namespace).Get(primaryName, metav1.GetOptions{}) if err != nil { - return fmt.Errorf("deployment %s.%s query error %v", primaryName, cd.Namespace, err) + return fmt.Errorf("deployment %s.%s query error: %v", primaryName, cd.Namespace, err) } // promote secrets and config maps @@ -188,7 +188,7 @@ func (c *DeploymentController) GetMetadata(cd *flaggerv1.Canary) (string, map[st canaryDep, err := c.kubeClient.AppsV1().Deployments(cd.Namespace).Get(targetName, metav1.GetOptions{}) if err != nil { - return "", nil, fmt.Errorf("deployment %s.%s get query error %w", targetName, cd.Namespace, err) + return "", nil, fmt.Errorf("deployment %s.%s get query error: %w", targetName, cd.Namespace, err) } label, err := c.getSelectorLabel(canaryDep) @@ -209,7 +209,7 @@ func (c *DeploymentController) createPrimaryDeployment(cd *flaggerv1.Canary) err canaryDep, err := c.kubeClient.AppsV1().Deployments(cd.Namespace).Get(targetName, metav1.GetOptions{}) if err != nil { - return fmt.Errorf("deplyoment %s.%s get query error %w", targetName, cd.Namespace, err) + return fmt.Errorf("deplyoment %s.%s get query error: %w", targetName, cd.Namespace, err) } label, err := c.getSelectorLabel(canaryDep) diff --git a/pkg/canary/deployment_controller_test.go b/pkg/canary/deployment_controller_test.go index fb8d574c7..0c24d8e79 100644 --- a/pkg/canary/deployment_controller_test.go +++ b/pkg/canary/deployment_controller_test.go @@ -72,68 +72,6 @@ func TestDeploymentController_Promote(t *testing.T) { assert.Equal(t, int32(2), hpaPrimary.Spec.MaxReplicas) } -func TestDeploymentController_IsReady(t *testing.T) { - mocks := newDeploymentFixture() - err := mocks.controller.Initialize(mocks.canary, true) - require.NoError(t, err, "Expected primary readiness check to fail") - - err = mocks.controller.IsPrimaryReady(mocks.canary) - require.Error(t, err) - - _, err = mocks.controller.IsCanaryReady(mocks.canary) - require.NoError(t, err) -} - -func TestDeploymentController_SetFailedChecks(t *testing.T) { - mocks := newDeploymentFixture() - err := mocks.controller.Initialize(mocks.canary, true) - require.NoError(t, err) - - err = mocks.controller.SetStatusFailedChecks(mocks.canary, 1) - require.NoError(t, err) - - res, err := mocks.flaggerClient.FlaggerV1beta1().Canaries("default").Get("podinfo", metav1.GetOptions{}) - require.NoError(t, err) - assert.Equal(t, 1, res.Status.FailedChecks) -} - -func TestDeploymentController_SetState(t *testing.T) { - mocks := newDeploymentFixture() - err := mocks.controller.Initialize(mocks.canary, true) - require.NoError(t, err) - - err = mocks.controller.SetStatusPhase(mocks.canary, flaggerv1.CanaryPhaseProgressing) - require.NoError(t, err) - - res, err := mocks.flaggerClient.FlaggerV1beta1().Canaries("default").Get("podinfo", metav1.GetOptions{}) - require.NoError(t, err) - assert.Equal(t, flaggerv1.CanaryPhaseProgressing, res.Status.Phase) -} - -func TestDeploymentController_SyncStatus(t *testing.T) { - mocks := newDeploymentFixture() - err := mocks.controller.Initialize(mocks.canary, true) - require.NoError(t, err) - - status := flaggerv1.CanaryStatus{ - Phase: flaggerv1.CanaryPhaseProgressing, - FailedChecks: 2, - } - err = mocks.controller.SyncStatus(mocks.canary, status) - require.NoError(t, err) - - res, err := mocks.flaggerClient.FlaggerV1beta1().Canaries("default").Get("podinfo", metav1.GetOptions{}) - require.NoError(t, err) - assert.Equal(t, status.Phase, res.Status.Phase) - assert.Equal(t, status.FailedChecks, res.Status.FailedChecks) - - require.NotNil(t, res.Status.TrackedConfigs) - configs := *res.Status.TrackedConfigs - secret := newDeploymentControllerTestSecret() - _, exists := configs["secret/"+secret.GetName()] - assert.True(t, exists, "Secret %s not found in status", secret.GetName()) -} - func TestDeploymentController_ScaleToZero(t *testing.T) { mocks := newDeploymentFixture() err := mocks.controller.Initialize(mocks.canary, true) diff --git a/pkg/canary/deployment_ready.go b/pkg/canary/deployment_ready.go index 6fdd9d9a9..78d7e5ae6 100644 --- a/pkg/canary/deployment_ready.go +++ b/pkg/canary/deployment_ready.go @@ -17,7 +17,7 @@ func (c *DeploymentController) IsPrimaryReady(cd *flaggerv1.Canary) error { primaryName := fmt.Sprintf("%s-primary", cd.Spec.TargetRef.Name) primary, err := c.kubeClient.AppsV1().Deployments(cd.Namespace).Get(primaryName, metav1.GetOptions{}) if err != nil { - return fmt.Errorf("deployment %s.%s get query error %w", primaryName, cd.Namespace, err) + return fmt.Errorf("deployment %s.%s get query error: %w", primaryName, cd.Namespace, err) } _, err = c.isDeploymentReady(primary, cd.GetProgressDeadlineSeconds()) diff --git a/pkg/canary/deployment_ready_test.go b/pkg/canary/deployment_ready_test.go new file mode 100644 index 000000000..bb6e8d9fa --- /dev/null +++ b/pkg/canary/deployment_ready_test.go @@ -0,0 +1,19 @@ +package canary + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestDeploymentController_IsReady(t *testing.T) { + mocks := newDeploymentFixture() + err := mocks.controller.Initialize(mocks.canary, true) + require.NoError(t, err, "Expected primary readiness check to fail") + + err = mocks.controller.IsPrimaryReady(mocks.canary) + require.Error(t, err) + + _, err = mocks.controller.IsCanaryReady(mocks.canary) + require.NoError(t, err) +} diff --git a/pkg/canary/deployment_status_test.go b/pkg/canary/deployment_status_test.go new file mode 100644 index 000000000..95b2b5586 --- /dev/null +++ b/pkg/canary/deployment_status_test.go @@ -0,0 +1,61 @@ +package canary + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + flaggerv1 "github.com/weaveworks/flagger/pkg/apis/flagger/v1beta1" +) + +func TestDeploymentController_SyncStatus(t *testing.T) { + mocks := newDeploymentFixture() + err := mocks.controller.Initialize(mocks.canary, true) + require.NoError(t, err) + + status := flaggerv1.CanaryStatus{ + Phase: flaggerv1.CanaryPhaseProgressing, + FailedChecks: 2, + } + err = mocks.controller.SyncStatus(mocks.canary, status) + require.NoError(t, err) + + res, err := mocks.flaggerClient.FlaggerV1beta1().Canaries("default").Get("podinfo", metav1.GetOptions{}) + require.NoError(t, err) + assert.Equal(t, status.Phase, res.Status.Phase) + assert.Equal(t, status.FailedChecks, res.Status.FailedChecks) + + require.NotNil(t, res.Status.TrackedConfigs) + configs := *res.Status.TrackedConfigs + secret := newDeploymentControllerTestSecret() + _, exists := configs["secret/"+secret.GetName()] + assert.True(t, exists, "Secret %s not found in status", secret.GetName()) +} + +func TestDeploymentController_SetFailedChecks(t *testing.T) { + mocks := newDeploymentFixture() + err := mocks.controller.Initialize(mocks.canary, true) + require.NoError(t, err) + + err = mocks.controller.SetStatusFailedChecks(mocks.canary, 1) + require.NoError(t, err) + + res, err := mocks.flaggerClient.FlaggerV1beta1().Canaries("default").Get("podinfo", metav1.GetOptions{}) + require.NoError(t, err) + assert.Equal(t, 1, res.Status.FailedChecks) +} + +func TestDeploymentController_SetState(t *testing.T) { + mocks := newDeploymentFixture() + err := mocks.controller.Initialize(mocks.canary, true) + require.NoError(t, err) + + err = mocks.controller.SetStatusPhase(mocks.canary, flaggerv1.CanaryPhaseProgressing) + require.NoError(t, err) + + res, err := mocks.flaggerClient.FlaggerV1beta1().Canaries("default").Get("podinfo", metav1.GetOptions{}) + require.NoError(t, err) + assert.Equal(t, flaggerv1.CanaryPhaseProgressing, res.Status.Phase) +} diff --git a/pkg/canary/service_controller.go b/pkg/canary/service_controller.go index cfd1703fb..c62949a25 100644 --- a/pkg/canary/service_controller.go +++ b/pkg/canary/service_controller.go @@ -156,12 +156,12 @@ func (c *ServiceController) Promote(cd *flaggerv1.Canary) error { canary, err := c.kubeClient.CoreV1().Services(cd.Namespace).Get(targetName, metav1.GetOptions{}) if err != nil { - return fmt.Errorf("service %s.%s get query error %w", targetName, cd.Namespace, err) + return fmt.Errorf("service %s.%s get query error: %w", targetName, cd.Namespace, err) } primary, err := c.kubeClient.CoreV1().Services(cd.Namespace).Get(primaryName, metav1.GetOptions{}) if err != nil { - return fmt.Errorf("service %s.%s get query error %w", primaryName, cd.Namespace, err) + return fmt.Errorf("service %s.%s get query error: %w", primaryName, cd.Namespace, err) } primaryCopy := canary.DeepCopy() @@ -187,7 +187,7 @@ func (c *ServiceController) HasTargetChanged(cd *flaggerv1.Canary) (bool, error) targetName := cd.Spec.TargetRef.Name canary, err := c.kubeClient.CoreV1().Services(cd.Namespace).Get(targetName, metav1.GetOptions{}) if err != nil { - return false, fmt.Errorf("service %s.%s get query error %w", targetName, cd.Namespace, err) + return false, fmt.Errorf("service %s.%s get query error: %w", targetName, cd.Namespace, err) } return hasSpecChanged(cd, canary.Spec) } @@ -204,7 +204,7 @@ func (c *ServiceController) ScaleFromZero(_ *flaggerv1.Canary) error { func (c *ServiceController) SyncStatus(cd *flaggerv1.Canary, status flaggerv1.CanaryStatus) error { dep, err := c.kubeClient.CoreV1().Services(cd.Namespace).Get(cd.Spec.TargetRef.Name, metav1.GetOptions{}) if err != nil { - return fmt.Errorf("service %s.%s get query error %w", cd.Spec.TargetRef.Name, cd.Namespace, err) + return fmt.Errorf("service %s.%s get query error: %w", cd.Spec.TargetRef.Name, cd.Namespace, err) } return syncCanaryStatus(c.flaggerClient, cd, status, dep.Spec, func(cdCopy *flaggerv1.Canary) {}) From 2ec24bb17d649a950ea4229799557f28d0f56cfd Mon Sep 17 00:00:00 2001 From: mathetake Date: Sun, 8 Mar 2020 00:17:52 +0900 Subject: [PATCH 03/11] pkg/metrics/providers: wrap ErrNoValuesFound and modify controller accordingly --- pkg/controller/controller.go | 4 +- pkg/controller/events.go | 14 ++-- pkg/controller/scheduler.go | 31 ++++---- pkg/metrics/observers/errors.go | 7 ++ pkg/metrics/providers/cloudwatch.go | 6 +- pkg/metrics/providers/cloudwatch_test.go | 6 +- pkg/metrics/providers/datadog.go | 26 ++++--- pkg/metrics/providers/datadog_test.go | 93 +++++++++++++++--------- pkg/metrics/providers/errors.go | 7 ++ pkg/metrics/providers/prometheus.go | 20 ++--- pkg/metrics/providers/prometheus_test.go | 77 +++++++++++++------- 11 files changed, 180 insertions(+), 111 deletions(-) create mode 100644 pkg/metrics/observers/errors.go create mode 100644 pkg/metrics/providers/errors.go diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index 5c0d1caa4..673dc5d15 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -192,7 +192,7 @@ func (c *Controller) processNextWorkItem() bool { // Run the syncHandler, passing it the namespace/name string of the // Foo resource to be synced. if err := c.syncHandler(key); err != nil { - return fmt.Errorf("error syncing '%s': %s", key, err.Error()) + return fmt.Errorf("error syncing '%s': %w", key, err) } // Finally, if no error occurs we Forget this item so it does not // get queued again until another change happens. @@ -230,7 +230,7 @@ func (c *Controller) syncHandler(key string) error { _, err := c.flaggerClient.FlaggerV1beta1().Canaries(cd.Namespace).UpdateStatus(cdCopy) if err != nil { c.logger.Errorf("%s status condition update error: %v", key, err) - return fmt.Errorf("%s status condition update error: %v", key, err) + return fmt.Errorf("%s status condition update error: %w", key, err) } } } diff --git a/pkg/controller/events.go b/pkg/controller/events.go index fb29915a6..6f9740c2f 100644 --- a/pkg/controller/events.go +++ b/pkg/controller/events.go @@ -31,14 +31,12 @@ func (c *Controller) recordEventWarningf(r *flaggerv1.Canary, template string, a func (c *Controller) sendEventToWebhook(r *flaggerv1.Canary, eventType, template string, args []interface{}) { webhookOverride := false - if len(r.GetAnalysis().Webhooks) > 0 { - for _, canaryWebhook := range r.GetAnalysis().Webhooks { - if canaryWebhook.Type == flaggerv1.EventHook { - webhookOverride = true - err := CallEventWebhook(r, canaryWebhook.URL, fmt.Sprintf(template, args...), eventType) - if err != nil { - c.logger.With("canary", fmt.Sprintf("%s.%s", r.Name, r.Namespace)).Errorf("error sending event to webhook: %s", err) - } + for _, canaryWebhook := range r.GetAnalysis().Webhooks { + if canaryWebhook.Type == flaggerv1.EventHook { + webhookOverride = true + err := CallEventWebhook(r, canaryWebhook.URL, fmt.Sprintf(template, args...), eventType) + if err != nil { + c.logger.With("canary", fmt.Sprintf("%s.%s", r.Name, r.Namespace)).Errorf("error sending event to webhook: %s", err) } } } diff --git a/pkg/controller/scheduler.go b/pkg/controller/scheduler.go index 050fbdd1f..65d2dcb20 100644 --- a/pkg/controller/scheduler.go +++ b/pkg/controller/scheduler.go @@ -1,6 +1,7 @@ package controller import ( + "errors" "fmt" "strings" "time" @@ -333,7 +334,7 @@ func (c *Controller) advanceCanary(name string, namespace string, skipLivenessCh // strategy: A/B testing if len(cd.GetAnalysis().Match) > 0 && cd.GetAnalysis().Iterations > 0 { - c.runAB(cd, canaryController, meshRouter, provider) + c.runAB(cd, canaryController, meshRouter) return } @@ -345,12 +346,13 @@ func (c *Controller) advanceCanary(name string, namespace string, skipLivenessCh // strategy: Canary progressive traffic increase if cd.GetAnalysis().StepWeight > 0 { - c.runCanary(cd, canaryController, meshRouter, provider, mirrored, canaryWeight, primaryWeight, maxWeight) + c.runCanary(cd, canaryController, meshRouter, mirrored, canaryWeight, primaryWeight, maxWeight) } } -func (c *Controller) runCanary(canary *flaggerv1.Canary, canaryController canary.Controller, meshRouter router.Interface, provider string, mirrored bool, canaryWeight int, primaryWeight int, maxWeight int) { +func (c *Controller) runCanary(canary *flaggerv1.Canary, canaryController canary.Controller, + meshRouter router.Interface, mirrored bool, canaryWeight int, primaryWeight int, maxWeight int) { primaryName := fmt.Sprintf("%s-primary", canary.Spec.TargetRef.Name) // increase traffic weight @@ -420,7 +422,8 @@ func (c *Controller) runCanary(canary *flaggerv1.Canary, canaryController canary } } -func (c *Controller) runAB(canary *flaggerv1.Canary, canaryController canary.Controller, meshRouter router.Interface, provider string) { +func (c *Controller) runAB(canary *flaggerv1.Canary, canaryController canary.Controller, + meshRouter router.Interface) { primaryName := fmt.Sprintf("%s-primary", canary.Spec.TargetRef.Name) // route traffic to canary and increment iterations @@ -462,7 +465,8 @@ func (c *Controller) runAB(canary *flaggerv1.Canary, canaryController canary.Con } } -func (c *Controller) runBlueGreen(canary *flaggerv1.Canary, canaryController canary.Controller, meshRouter router.Interface, provider string, mirrored bool) { +func (c *Controller) runBlueGreen(canary *flaggerv1.Canary, canaryController canary.Controller, + meshRouter router.Interface, provider string, mirrored bool) { primaryName := fmt.Sprintf("%s-primary", canary.Spec.TargetRef.Name) // increment iterations @@ -820,9 +824,10 @@ func (c *Controller) runBuiltinMetricChecks(canary *flaggerv1.Canary) bool { if metric.Name == "request-success-rate" { val, err := observer.GetRequestSuccessRate(toMetricModel(canary, metric.Interval)) if err != nil { - if strings.Contains(err.Error(), "no values found") { - c.recordEventWarningf(canary, "Halt advancement no values found for %s metric %s probably %s.%s is not receiving traffic", - metricsProvider, metric.Name, canary.Spec.TargetRef.Name, canary.Namespace) + if errors.Is(err, observers.ErrNoValuesFound) { + c.recordEventWarningf(canary, + "Halt advancement no values found for %s metric %s probably %s.%s is not receiving traffic: %v", + metricsProvider, metric.Name, canary.Spec.TargetRef.Name, canary.Namespace, err) } else { c.recordEventErrorf(canary, "Prometheus query failed: %v", err) } @@ -851,7 +856,7 @@ func (c *Controller) runBuiltinMetricChecks(canary *flaggerv1.Canary) bool { if metric.Name == "request-duration" { val, err := observer.GetRequestDuration(toMetricModel(canary, metric.Interval)) if err != nil { - if strings.Contains(err.Error(), "no values found") { + if errors.Is(err, observers.ErrNoValuesFound) { c.recordEventWarningf(canary, "Halt advancement no values found for %s metric %s probably %s.%s is not receiving traffic", metricsProvider, metric.Name, canary.Spec.TargetRef.Name, canary.Namespace) } else { @@ -882,7 +887,7 @@ func (c *Controller) runBuiltinMetricChecks(canary *flaggerv1.Canary) bool { if metric.Query != "" { val, err := observerFactory.Client.RunQuery(metric.Query) if err != nil { - if strings.Contains(err.Error(), "no values found") { + if errors.Is(err, observers.ErrNoValuesFound) { c.recordEventWarningf(canary, "Halt advancement no values found for metric: %s", metric.Name) } else { @@ -955,9 +960,9 @@ func (c *Controller) runMetricChecks(canary *flaggerv1.Canary) bool { val, err := provider.RunQuery(query) if err != nil { - if strings.Contains(err.Error(), "no values found") { - c.recordEventWarningf(canary, "Halt advancement no values found for custom metric: %s", - metric.Name) + if errors.Is(err, providers.ErrNoValuesFound) { + c.recordEventWarningf(canary, "Halt advancement no values found for custom metric: %s: %v", + metric.Name, err) } else { c.recordEventErrorf(canary, "Metric query failed for %s: %v", metric.Name, err) } diff --git a/pkg/metrics/observers/errors.go b/pkg/metrics/observers/errors.go new file mode 100644 index 000000000..1e3993a6c --- /dev/null +++ b/pkg/metrics/observers/errors.go @@ -0,0 +1,7 @@ +package observers + +import "errors" + +var ( + ErrNoValuesFound = errors.New("no values found") +) diff --git a/pkg/metrics/providers/cloudwatch.go b/pkg/metrics/providers/cloudwatch.go index 700bef3f1..dc709641e 100644 --- a/pkg/metrics/providers/cloudwatch.go +++ b/pkg/metrics/providers/cloudwatch.go @@ -77,12 +77,12 @@ func (p *CloudWatchProvider) RunQuery(query string) (float64, error) { mr := res.MetricDataResults if len(mr) < 1 { - return 0, fmt.Errorf("no values found in response: %s", res.String()) + return 0, fmt.Errorf("invalid response: %s: %w", res.String(), ErrNoValuesFound) } - vs := res.MetricDataResults[0].Values + vs := mr[0].Values if len(vs) < 1 { - return 0, fmt.Errorf("no values found in response: %s", res.String()) + return 0, fmt.Errorf("invalid reponse %s: %w", res.String(), ErrNoValuesFound) } return aws.Float64Value(vs[0]), nil diff --git a/pkg/metrics/providers/cloudwatch_test.go b/pkg/metrics/providers/cloudwatch_test.go index ed51b70d6..dd5b3ae3b 100644 --- a/pkg/metrics/providers/cloudwatch_test.go +++ b/pkg/metrics/providers/cloudwatch_test.go @@ -1,8 +1,8 @@ package providers import ( + "errors" "net/http" - "strings" "testing" "time" @@ -146,13 +146,13 @@ func TestCloudWatchProvider_RunQuery(t *testing.T) { _, err := p.RunQuery(query) require.Error(t, err) - require.True(t, strings.Contains(err.Error(), "no values")) + require.True(t, errors.Is(err, ErrNoValuesFound)) p = CloudWatchProvider{client: cloudWatchClientMock{ o: &cloudwatch.GetMetricDataOutput{}}} _, err = p.RunQuery(query) require.Error(t, err) - require.True(t, strings.Contains(err.Error(), "no values")) + require.True(t, errors.Is(err, ErrNoValuesFound)) }) } diff --git a/pkg/metrics/providers/datadog.go b/pkg/metrics/providers/datadog.go index a27763fdb..6ef898b43 100644 --- a/pkg/metrics/providers/datadog.go +++ b/pkg/metrics/providers/datadog.go @@ -76,7 +76,7 @@ func NewDatadogProvider(metricInterval string, md, err := time.ParseDuration(metricInterval) if err != nil { - return nil, fmt.Errorf("error parsing metric interval: %s", err.Error()) + return nil, fmt.Errorf("error parsing metric interval: %w", err) } dd.fromDelta = int64(datadogFromDeltaMultiplierOnMetricInterval * md.Seconds()) @@ -89,7 +89,7 @@ func (p *DatadogProvider) RunQuery(query string) (float64, error) { req, err := http.NewRequest("GET", p.metricsQueryEndpoint, nil) if err != nil { - return 0, fmt.Errorf("error http.NewRequest: %s", err.Error()) + return 0, fmt.Errorf("error http.NewRequest: %w", err) } req.Header.Set(datadogAPIKeyHeaderKey, p.apiKey) @@ -111,26 +111,30 @@ func (p *DatadogProvider) RunQuery(query string) (float64, error) { defer r.Body.Close() b, err := ioutil.ReadAll(r.Body) if err != nil { - return 0, fmt.Errorf("error reading body: %s", err.Error()) + return 0, fmt.Errorf("error reading body: %w", err) } if r.StatusCode != http.StatusOK { - return 0, fmt.Errorf("error response: %s", string(b)) + return 0, fmt.Errorf("error response: %s: %w", string(b), err) } var res datadogResponse if err := json.Unmarshal(b, &res); err != nil { - return 0, fmt.Errorf("error unmarshaling result: %s, '%s'", err.Error(), string(b)) + return 0, fmt.Errorf("error unmarshaling result: %w, '%s'", err, string(b)) } if len(res.Series) < 1 { - return 0, fmt.Errorf("no values found in response: %s", string(b)) + return 0, fmt.Errorf("invalid response: %s: %w", string(b), ErrNoValuesFound) } - s := res.Series[0] - vs := s.Pointlist[len(s.Pointlist)-1] + pl := res.Series[0].Pointlist + if len(pl) < 1 { + return 0, fmt.Errorf("invalid response: %s: %w", string(b), ErrNoValuesFound) + } + + vs := pl[len(pl)-1] if len(vs) < 1 { - return 0, fmt.Errorf("no values found in response: %s", string(b)) + return 0, fmt.Errorf("invalid response: %s: %w", string(b), ErrNoValuesFound) } return vs[1], nil @@ -141,7 +145,7 @@ func (p *DatadogProvider) RunQuery(query string) (float64, error) { func (p *DatadogProvider) IsOnline() (bool, error) { req, err := http.NewRequest("GET", p.apiKeyValidationEndpoint, nil) if err != nil { - return false, fmt.Errorf("error http.NewRequest: %s", err.Error()) + return false, fmt.Errorf("error http.NewRequest: %w", err) } req.Header.Add(datadogAPIKeyHeaderKey, p.apiKey) @@ -157,7 +161,7 @@ func (p *DatadogProvider) IsOnline() (bool, error) { b, err := ioutil.ReadAll(r.Body) if err != nil { - return false, fmt.Errorf("error reading body: %s", err.Error()) + return false, fmt.Errorf("error reading body: %w", err) } if r.StatusCode != http.StatusOK { diff --git a/pkg/metrics/providers/datadog_test.go b/pkg/metrics/providers/datadog_test.go index d863a2a3b..8dd4c120a 100644 --- a/pkg/metrics/providers/datadog_test.go +++ b/pkg/metrics/providers/datadog_test.go @@ -1,6 +1,7 @@ package providers import ( + "errors" "fmt" "net/http" "net/http/httptest" @@ -36,45 +37,65 @@ func TestNewDatadogProvider(t *testing.T) { } func TestDatadogProvider_RunQuery(t *testing.T) { - eq := `avg:system.cpu.user\{*}by{host}` appKey := "app-key" apiKey := "api-key" - expected := 1.11111 - - now := time.Now().Unix() - ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - aq := r.URL.Query().Get("query") - assert.Equal(t, eq, aq) - assert.Equal(t, appKey, r.Header.Get(datadogApplicationKeyHeaderKey)) - assert.Equal(t, apiKey, r.Header.Get(datadogAPIKeyHeaderKey)) - - from, err := strconv.ParseInt(r.URL.Query().Get("from"), 10, 64) - if assert.NoError(t, err) { - assert.Less(t, from, now) - } - - to, err := strconv.ParseInt(r.URL.Query().Get("to"), 10, 64) - if assert.NoError(t, err) { - assert.GreaterOrEqual(t, to, now) - } - - json := fmt.Sprintf(`{"series": [{"pointlist": [[1577232000000,29325.102158814265],[1577318400000,56294.46758591842],[1577404800000,%f]]}]}`, expected) - w.Write([]byte(json)) - })) - defer ts.Close() - - dp, err := NewDatadogProvider("1m", - flaggerv1.MetricTemplateProvider{Address: ts.URL}, - map[string][]byte{ - datadogApplicationKeySecretKey: []byte(appKey), - datadogAPIKeySecretKey: []byte(apiKey), - }, - ) - require.NoError(t, err) + t.Run("ok", func(t *testing.T) { + expected := 1.11111 + eq := `avg:system.cpu.user{*}by{host}` + now := time.Now().Unix() + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + aq := r.URL.Query().Get("query") + assert.Equal(t, eq, aq) + assert.Equal(t, appKey, r.Header.Get(datadogApplicationKeyHeaderKey)) + assert.Equal(t, apiKey, r.Header.Get(datadogAPIKeyHeaderKey)) + + from, err := strconv.ParseInt(r.URL.Query().Get("from"), 10, 64) + if assert.NoError(t, err) { + assert.Less(t, from, now) + } - f, err := dp.RunQuery(eq) - require.NoError(t, err) - assert.Equal(t, expected, f) + to, err := strconv.ParseInt(r.URL.Query().Get("to"), 10, 64) + if assert.NoError(t, err) { + assert.GreaterOrEqual(t, to, now) + } + + json := fmt.Sprintf(`{"series": [{"pointlist": [[1577232000000,29325.102158814265],[1577318400000,56294.46758591842],[1577404800000,%f]]}]}`, expected) + w.Write([]byte(json)) + })) + defer ts.Close() + + dp, err := NewDatadogProvider("1m", + flaggerv1.MetricTemplateProvider{Address: ts.URL}, + map[string][]byte{ + datadogApplicationKeySecretKey: []byte(appKey), + datadogAPIKeySecretKey: []byte(apiKey), + }, + ) + require.NoError(t, err) + + f, err := dp.RunQuery(eq) + require.NoError(t, err) + assert.Equal(t, expected, f) + }) + + t.Run("no values", func(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + json := fmt.Sprintf(`{"series": [{"pointlist": []}]}`) + w.Write([]byte(json)) + })) + defer ts.Close() + + dp, err := NewDatadogProvider("1m", + flaggerv1.MetricTemplateProvider{Address: ts.URL}, + map[string][]byte{ + datadogApplicationKeySecretKey: []byte(appKey), + datadogAPIKeySecretKey: []byte(apiKey), + }, + ) + require.NoError(t, err) + _, err = dp.RunQuery("") + require.True(t, errors.Is(err, ErrNoValuesFound)) + }) } func TestDatadogProvider_IsOnline(t *testing.T) { diff --git a/pkg/metrics/providers/errors.go b/pkg/metrics/providers/errors.go new file mode 100644 index 000000000..90a6f01fd --- /dev/null +++ b/pkg/metrics/providers/errors.go @@ -0,0 +1,7 @@ +package providers + +import "errors" + +var ( + ErrNoValuesFound = errors.New("no values found") +) diff --git a/pkg/metrics/providers/prometheus.go b/pkg/metrics/providers/prometheus.go index bd7815293..146022bb5 100644 --- a/pkg/metrics/providers/prometheus.go +++ b/pkg/metrics/providers/prometheus.go @@ -74,7 +74,7 @@ func (p *PrometheusProvider) RunQuery(query string) (float64, error) { query = url.QueryEscape(p.trimQuery(query)) u, err := url.Parse(fmt.Sprintf("./api/v1/query?query=%s", query)) if err != nil { - return 0, err + return 0, fmt.Errorf("url.Parase failed: %w", err) } u.Path = path.Join(p.url.Path, u.Path) @@ -82,7 +82,7 @@ func (p *PrometheusProvider) RunQuery(query string) (float64, error) { req, err := http.NewRequest("GET", u.String(), nil) if err != nil { - return 0, err + return 0, fmt.Errorf("http.NewRequest failed: %w", err) } if p.username != "" && p.password != "" { @@ -94,13 +94,13 @@ func (p *PrometheusProvider) RunQuery(query string) (float64, error) { r, err := http.DefaultClient.Do(req.WithContext(ctx)) if err != nil { - return 0, err + return 0, fmt.Errorf("request failed: %w", err) } defer r.Body.Close() b, err := ioutil.ReadAll(r.Body) if err != nil { - return 0, fmt.Errorf("error reading body: %s", err.Error()) + return 0, fmt.Errorf("error reading body: %w", err) } if 400 <= r.StatusCode { @@ -110,7 +110,7 @@ func (p *PrometheusProvider) RunQuery(query string) (float64, error) { var result prometheusResponse err = json.Unmarshal(b, &result) if err != nil { - return 0, fmt.Errorf("error unmarshaling result: %s, '%s'", err.Error(), string(b)) + return 0, fmt.Errorf("error unmarshaling result: %w, '%s'", err, string(b)) } var value *float64 @@ -126,7 +126,7 @@ func (p *PrometheusProvider) RunQuery(query string) (float64, error) { } } if value == nil { - return 0, fmt.Errorf("no values found") + return 0, fmt.Errorf("%w", ErrNoValuesFound) } return *value, nil @@ -136,7 +136,7 @@ func (p *PrometheusProvider) RunQuery(query string) (float64, error) { func (p *PrometheusProvider) IsOnline() (bool, error) { u, err := url.Parse("./api/v1/status/flags") if err != nil { - return false, err + return false, fmt.Errorf("url.Parse failed: %w", err) } u.Path = path.Join(p.url.Path, u.Path) @@ -144,7 +144,7 @@ func (p *PrometheusProvider) IsOnline() (bool, error) { req, err := http.NewRequest("GET", u.String(), nil) if err != nil { - return false, err + return false, fmt.Errorf("http.NewRequest failed: %w", err) } if p.username != "" && p.password != "" { @@ -156,13 +156,13 @@ func (p *PrometheusProvider) IsOnline() (bool, error) { r, err := http.DefaultClient.Do(req.WithContext(ctx)) if err != nil { - return false, err + return false, fmt.Errorf("request failed: %w", err) } defer r.Body.Close() b, err := ioutil.ReadAll(r.Body) if err != nil { - return false, fmt.Errorf("error reading body: %s", err.Error()) + return false, fmt.Errorf("error reading body: %w", err) } if 400 <= r.StatusCode { diff --git a/pkg/metrics/providers/prometheus_test.go b/pkg/metrics/providers/prometheus_test.go index effe82f54..4aae8c9c3 100644 --- a/pkg/metrics/providers/prometheus_test.go +++ b/pkg/metrics/providers/prometheus_test.go @@ -1,6 +1,7 @@ package providers import ( + "errors" "net/http" "net/http/httptest" "strings" @@ -82,40 +83,66 @@ func TestNewPrometheusProvider(t *testing.T) { } func TestPrometheusProvider_RunQueryWithBasicAuth(t *testing.T) { - expected := `sum(envoy_cluster_upstream_rq)` - ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - promql := r.URL.Query()["query"][0] - assert.Equal(t, expected, promql) + t.Run("ok", func(t *testing.T) { + expected := `sum(envoy_cluster_upstream_rq)` + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + promql := r.URL.Query()["query"][0] + assert.Equal(t, expected, promql) - if assert.Contains(t, r.Header, "Authorization") { + if assert.Contains(t, r.Header, "Authorization") { - } - header, ok := r.Header["Authorization"] - if assert.True(t, ok, "Authorization header not found") { - assert.True(t, strings.Contains(header[0], "Basic"), "Basic authorization header not found") - } + } + header, ok := r.Header["Authorization"] + if assert.True(t, ok, "Authorization header not found") { + assert.True(t, strings.Contains(header[0], "Basic"), "Basic authorization header not found") + } - json := `{"status":"success","data":{"resultType":"vector","result":[{"metric":{},"value":[1545905245.458,"100"]}]}}` - w.Write([]byte(json)) - })) - defer ts.Close() + json := `{"status":"success","data":{"resultType":"vector","result":[{"metric":{},"value":[1545905245.458,"100"]}]}}` + w.Write([]byte(json)) + })) + defer ts.Close() - clients := prometheusFake() + clients := prometheusFake() - template, err := clients.flaggerClient.FlaggerV1beta1().MetricTemplates("default").Get("prometheus", metav1.GetOptions{}) - require.NoError(t, err) - template.Spec.Provider.Address = ts.URL + template, err := clients.flaggerClient.FlaggerV1beta1().MetricTemplates("default").Get("prometheus", metav1.GetOptions{}) + require.NoError(t, err) + template.Spec.Provider.Address = ts.URL - secret, err := clients.kubeClient.CoreV1().Secrets("default").Get("prometheus", metav1.GetOptions{}) - require.NoError(t, err) + secret, err := clients.kubeClient.CoreV1().Secrets("default").Get("prometheus", metav1.GetOptions{}) + require.NoError(t, err) - prom, err := NewPrometheusProvider(template.Spec.Provider, secret.Data) - require.NoError(t, err) + prom, err := NewPrometheusProvider(template.Spec.Provider, secret.Data) + require.NoError(t, err) - val, err := prom.RunQuery(template.Spec.Query) - require.NoError(t, err) + val, err := prom.RunQuery(template.Spec.Query) + require.NoError(t, err) + + assert.Equal(t, float64(100), val) + }) + + t.Run("no values", func(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + json := `{"status":"success","data":{"resultType":"vector","result":[]}}` + w.Write([]byte(json)) + })) + defer ts.Close() + + clients := prometheusFake() + + template, err := clients.flaggerClient.FlaggerV1beta1(). + MetricTemplates("default").Get("prometheus", metav1.GetOptions{}) + require.NoError(t, err) + template.Spec.Provider.Address = ts.URL + + secret, err := clients.kubeClient.CoreV1().Secrets("default").Get("prometheus", metav1.GetOptions{}) + require.NoError(t, err) + + prom, err := NewPrometheusProvider(template.Spec.Provider, secret.Data) + require.NoError(t, err) - assert.Equal(t, float64(100), val) + _, err = prom.RunQuery(template.Spec.Query) + require.True(t, errors.Is(err, ErrNoValuesFound)) + }) } func TestPrometheusProvider_IsOnline(t *testing.T) { From 5843b02931fada09844eb034954f2d3a8d828f2b Mon Sep 17 00:00:00 2001 From: mathetake Date: Sun, 8 Mar 2020 10:42:51 +0900 Subject: [PATCH 04/11] pkg/canary: refator error messages --- pkg/canary/config_tracker.go | 4 ++-- pkg/canary/daemonset_controller.go | 9 +++++---- pkg/canary/daemonset_ready.go | 6 +++--- pkg/canary/deployment_controller.go | 27 +++++++++++++-------------- pkg/canary/deployment_ready.go | 8 ++++---- pkg/canary/status.go | 3 +-- 6 files changed, 28 insertions(+), 29 deletions(-) diff --git a/pkg/canary/config_tracker.go b/pkg/canary/config_tracker.go index 16ad93332..75c6102b4 100644 --- a/pkg/canary/config_tracker.go +++ b/pkg/canary/config_tracker.go @@ -272,7 +272,7 @@ func (ct *ConfigTracker) CreatePrimaryConfigs(cd *flaggerv1.Canary, refs map[str case ConfigRefMap: config, err := ct.KubeClient.CoreV1().ConfigMaps(cd.Namespace).Get(ref.Name, metav1.GetOptions{}) if err != nil { - return fmt.Errorf("configmap %s.%s query failed : %w", ref.Name, cd.Name, err) + return fmt.Errorf("configmap %s.%s get query failed : %w", ref.Name, cd.Name, err) } primaryName := fmt.Sprintf("%s-primary", config.GetName()) primaryConfigMap := &corev1.ConfigMap{ @@ -309,7 +309,7 @@ func (ct *ConfigTracker) CreatePrimaryConfigs(cd *flaggerv1.Canary, refs map[str case ConfigRefSecret: secret, err := ct.KubeClient.CoreV1().Secrets(cd.Namespace).Get(ref.Name, metav1.GetOptions{}) if err != nil { - return err + return fmt.Errorf("secret %s.%s get query failed : %w", ref.Name, cd.Name, err) } primaryName := fmt.Sprintf("%s-primary", secret.GetName()) primarySecret := &corev1.Secret{ diff --git a/pkg/canary/daemonset_controller.go b/pkg/canary/daemonset_controller.go index 1eded3eb4..4beb1b1db 100644 --- a/pkg/canary/daemonset_controller.go +++ b/pkg/canary/daemonset_controller.go @@ -32,7 +32,7 @@ func (c *DaemonSetController) ScaleToZero(cd *flaggerv1.Canary) error { targetName := cd.Spec.TargetRef.Name dae, err := c.kubeClient.AppsV1().DaemonSets(cd.Namespace).Get(targetName, metav1.GetOptions{}) if err != nil { - return fmt.Errorf("daemonset %s.%s query error: %w", targetName, cd.Namespace, err) + return fmt.Errorf("daemonset %s.%s get query error: %w", targetName, cd.Namespace, err) } daeCopy := dae.DeepCopy() @@ -72,8 +72,8 @@ func (c *DaemonSetController) ScaleFromZero(cd *flaggerv1.Canary) error { return nil } -// Initialize creates the primary DaemonSet and -// delete the canary DaemonSet and returns the pod selector label and container ports +// Initialize creates the primary DaemonSet, scales down the canary DaemonSet, +// and returns the pod selector label and container ports func (c *DaemonSetController) Initialize(cd *flaggerv1.Canary, skipLivenessChecks bool) (err error) { err = c.createPrimaryDaemonSet(cd) if err != nil { @@ -87,7 +87,8 @@ func (c *DaemonSetController) Initialize(cd *flaggerv1.Canary, skipLivenessCheck } } - c.logger.With("canary", fmt.Sprintf("%s.%s", cd.Name, cd.Namespace)).Infof("Scaling down %s.%s", cd.Spec.TargetRef.Name, cd.Namespace) + c.logger.With("canary", fmt.Sprintf("%s.%s", cd.Name, cd.Namespace)). + Infof("Scaling down DaemonSet %s.%s", cd.Spec.TargetRef.Name, cd.Namespace) if err := c.ScaleToZero(cd); err != nil { return fmt.Errorf("ScaleToZero failed: %w", err) } diff --git a/pkg/canary/daemonset_ready.go b/pkg/canary/daemonset_ready.go index 7e68b6dba..1c5a392d8 100644 --- a/pkg/canary/daemonset_ready.go +++ b/pkg/canary/daemonset_ready.go @@ -35,10 +35,10 @@ func (c *DaemonSetController) IsCanaryReady(cd *flaggerv1.Canary) (bool, error) return true, fmt.Errorf("daemonset %s.%s get query error: %w", targetName, cd.Namespace, err) } - retriable, err := c.isDaemonSetReady(cd, canary) + retryable, err := c.isDaemonSetReady(cd, canary) if err != nil { - return retriable, fmt.Errorf("canary damonset %s.%s not ready with retryablility: %v: %w", - targetName, cd.Namespace, retriable, err) + return retryable, fmt.Errorf("canary damonset %s.%s not ready with retryable %v: %w", + targetName, cd.Namespace, retryable, err) } return true, nil } diff --git a/pkg/canary/deployment_controller.go b/pkg/canary/deployment_controller.go index c1dc5cc8f..ea5153d3f 100644 --- a/pkg/canary/deployment_controller.go +++ b/pkg/canary/deployment_controller.go @@ -30,33 +30,31 @@ type DeploymentController struct { // scales to zero the canary deployment and returns the pod selector label and container ports func (c *DeploymentController) Initialize(cd *flaggerv1.Canary, skipLivenessChecks bool) (err error) { primaryName := fmt.Sprintf("%s-primary", cd.Spec.TargetRef.Name) - - err = c.createPrimaryDeployment(cd) - if err != nil { - return fmt.Errorf("createPrimaryDeployment %s.%s failed: %w", primaryName, cd.Namespace, err) + if err := c.createPrimaryDeployment(cd); err != nil { + return fmt.Errorf("createPrimaryDeployment failed: %w", err) } if cd.Status.Phase == "" || cd.Status.Phase == flaggerv1.CanaryPhaseInitializing { if !skipLivenessChecks && !cd.SkipAnalysis() { if err := c.IsPrimaryReady(cd); err != nil { - return fmt.Errorf("primary deployment %s.%s not ready: %w", primaryName, cd.Namespace, err) + return fmt.Errorf("IsPrimaryReady failed: %w", err) } } - c.logger.With("canary", fmt.Sprintf("%s.%s", cd.Name, cd.Namespace)).Infof("Scaling down %s.%s", cd.Spec.TargetRef.Name, cd.Namespace) + c.logger.With("canary", fmt.Sprintf("%s.%s", cd.Name, cd.Namespace)). + Infof("Scaling down Deployment %s.%s", cd.Spec.TargetRef.Name, cd.Namespace) if err := c.ScaleToZero(cd); err != nil { return fmt.Errorf("scaling down canary daemon set %s.%s failed: %w", cd.Spec.TargetRef.Name, cd.Namespace, err) } } if cd.Spec.AutoscalerRef != nil { - switch cd.Spec.AutoscalerRef.Kind { - case "HorizontalPodAutoscaler": + if cd.Spec.AutoscalerRef.Kind == "HorizontalPodAutoscaler" { if err := c.reconcilePrimaryHpa(cd, true); err != nil { return fmt.Errorf( "initial reconcilePrimaryHpa for %s.%s failed: %w", primaryName, cd.Namespace, err) } - default: + } else { return fmt.Errorf("cd.Spec.AutoscalerRef.Kind is invalid: %s", cd.Spec.AutoscalerRef.Kind) } } @@ -70,7 +68,7 @@ func (c *DeploymentController) Promote(cd *flaggerv1.Canary) error { canary, err := c.kubeClient.AppsV1().Deployments(cd.Namespace).Get(targetName, metav1.GetOptions{}) if err != nil { - return fmt.Errorf("deployment %s.%s query error: %v", targetName, cd.Namespace, err) + return fmt.Errorf("deployment %s.%s get query error: %w", targetName, cd.Namespace, err) } label, err := c.getSelectorLabel(canary) @@ -80,7 +78,7 @@ func (c *DeploymentController) Promote(cd *flaggerv1.Canary) error { primary, err := c.kubeClient.AppsV1().Deployments(cd.Namespace).Get(primaryName, metav1.GetOptions{}) if err != nil { - return fmt.Errorf("deployment %s.%s query error: %v", primaryName, cd.Namespace, err) + return fmt.Errorf("deployment %s.%s get query error: %w", primaryName, cd.Namespace, err) } // promote secrets and config maps @@ -137,7 +135,7 @@ func (c *DeploymentController) HasTargetChanged(cd *flaggerv1.Canary) (bool, err targetName := cd.Spec.TargetRef.Name canary, err := c.kubeClient.AppsV1().Deployments(cd.Namespace).Get(targetName, metav1.GetOptions{}) if err != nil { - return false, fmt.Errorf("deployment %s.%s query error: %w", targetName, cd.Namespace, err) + return false, fmt.Errorf("deployment %s.%s get query error: %w", targetName, cd.Namespace, err) } return hasSpecChanged(cd, canary.Spec.Template) @@ -280,7 +278,8 @@ func (c *DeploymentController) createPrimaryDeployment(cd *flaggerv1.Canary) err return fmt.Errorf("creating deployment %s.%s failed: %w", primaryDep.Name, cd.Namespace, err) } - c.logger.With("canary", fmt.Sprintf("%s.%s", cd.Name, cd.Namespace)).Infof("Deployment %s.%s created", primaryDep.GetName(), cd.Namespace) + c.logger.With("canary", fmt.Sprintf("%s.%s", cd.Name, cd.Namespace)). + Infof("Deployment %s.%s created", primaryDep.GetName(), cd.Namespace) } return nil @@ -335,7 +334,7 @@ func (c *DeploymentController) reconcilePrimaryHpa(cd *flaggerv1.Canary, init bo "HorizontalPodAutoscaler %s.%s created", primaryHpa.GetName(), cd.Namespace) return nil } else if err != nil { - return fmt.Errorf("HorizontalPodAutoscaler %s.%s exists but get query failed: %w", + return fmt.Errorf("HorizontalPodAutoscaler %s.%s get query failed: %w", primaryHpa.Name, primaryHpa.Namespace, err) } diff --git a/pkg/canary/deployment_ready.go b/pkg/canary/deployment_ready.go index 78d7e5ae6..86b2a776a 100644 --- a/pkg/canary/deployment_ready.go +++ b/pkg/canary/deployment_ready.go @@ -42,11 +42,11 @@ func (c *DeploymentController) IsCanaryReady(cd *flaggerv1.Canary) (bool, error) return true, fmt.Errorf("deployment %s.%s get query error: %w", targetName, cd.Namespace, err) } - retriable, err := c.isDeploymentReady(canary, cd.GetProgressDeadlineSeconds()) + retryable, err := c.isDeploymentReady(canary, cd.GetProgressDeadlineSeconds()) if err != nil { - return retriable, fmt.Errorf( - "canary deployment %s.%s not ready with retriablility %v: %w", - targetName, cd.Namespace, retriable, err, + return retryable, fmt.Errorf( + "canary deployment %s.%s not ready with retryable %v: %w", + targetName, cd.Namespace, retryable, err, ) } return true, nil diff --git a/pkg/canary/status.go b/pkg/canary/status.go index 6d3f2e347..cfe1d657b 100644 --- a/pkg/canary/status.go +++ b/pkg/canary/status.go @@ -247,8 +247,7 @@ func updateStatusWithUpgrade(flaggerClient clientset.Interface, cd *flaggerv1.Ca _, err := flaggerClient.FlaggerV1beta1().Canaries(cd.Namespace).UpdateStatus(cd) if err != nil && strings.Contains(err.Error(), "flagger.app/v1alpha") { // upgrade alpha resource - _, updateErr := flaggerClient.FlaggerV1beta1().Canaries(cd.Namespace).Update(cd) - if updateErr != nil { + if _, updateErr := flaggerClient.FlaggerV1beta1().Canaries(cd.Namespace).Update(cd); updateErr != nil { return fmt.Errorf("updating canary %s.%s from v1alpha to v1beta failed: %w", cd.Name, cd.Namespace, updateErr) } // retry status update From 64efd56ce916c858dc36493e53c8dafbd960bfdc Mon Sep 17 00:00:00 2001 From: mathetake Date: Sun, 8 Mar 2020 10:56:35 +0900 Subject: [PATCH 05/11] pkg/metrics/observers: wrap errors --- pkg/controller/scheduler.go | 6 +- pkg/metrics/observers/appmesh.go | 9 +-- pkg/metrics/observers/contour.go | 9 +-- pkg/metrics/observers/crossover.go | 5 +- pkg/metrics/observers/crossover_service.go | 9 +-- pkg/metrics/observers/errors.go | 7 -- pkg/metrics/observers/gloo.go | 9 +-- pkg/metrics/observers/http.go | 5 +- pkg/metrics/observers/istio.go | 9 +-- pkg/metrics/observers/linkerd.go | 9 +-- pkg/metrics/observers/nginx.go | 9 +-- pkg/metrics/observers/nginx_test.go | 84 ++++++++++++++-------- pkg/metrics/observers/render.go | 8 +-- 13 files changed, 102 insertions(+), 76 deletions(-) delete mode 100644 pkg/metrics/observers/errors.go diff --git a/pkg/controller/scheduler.go b/pkg/controller/scheduler.go index 65d2dcb20..0a2004916 100644 --- a/pkg/controller/scheduler.go +++ b/pkg/controller/scheduler.go @@ -824,7 +824,7 @@ func (c *Controller) runBuiltinMetricChecks(canary *flaggerv1.Canary) bool { if metric.Name == "request-success-rate" { val, err := observer.GetRequestSuccessRate(toMetricModel(canary, metric.Interval)) if err != nil { - if errors.Is(err, observers.ErrNoValuesFound) { + if errors.Is(err, providers.ErrNoValuesFound) { c.recordEventWarningf(canary, "Halt advancement no values found for %s metric %s probably %s.%s is not receiving traffic: %v", metricsProvider, metric.Name, canary.Spec.TargetRef.Name, canary.Namespace, err) @@ -856,7 +856,7 @@ func (c *Controller) runBuiltinMetricChecks(canary *flaggerv1.Canary) bool { if metric.Name == "request-duration" { val, err := observer.GetRequestDuration(toMetricModel(canary, metric.Interval)) if err != nil { - if errors.Is(err, observers.ErrNoValuesFound) { + if errors.Is(err, providers.ErrNoValuesFound) { c.recordEventWarningf(canary, "Halt advancement no values found for %s metric %s probably %s.%s is not receiving traffic", metricsProvider, metric.Name, canary.Spec.TargetRef.Name, canary.Namespace) } else { @@ -887,7 +887,7 @@ func (c *Controller) runBuiltinMetricChecks(canary *flaggerv1.Canary) bool { if metric.Query != "" { val, err := observerFactory.Client.RunQuery(metric.Query) if err != nil { - if errors.Is(err, observers.ErrNoValuesFound) { + if errors.Is(err, providers.ErrNoValuesFound) { c.recordEventWarningf(canary, "Halt advancement no values found for metric: %s", metric.Name) } else { diff --git a/pkg/metrics/observers/appmesh.go b/pkg/metrics/observers/appmesh.go index 4d4a0362f..b8e672f7a 100644 --- a/pkg/metrics/observers/appmesh.go +++ b/pkg/metrics/observers/appmesh.go @@ -1,6 +1,7 @@ package observers import ( + "fmt" "time" flaggerv1 "github.com/weaveworks/flagger/pkg/apis/flagger/v1beta1" @@ -49,12 +50,12 @@ type AppMeshObserver struct { func (ob *AppMeshObserver) GetRequestSuccessRate(model flaggerv1.MetricTemplateModel) (float64, error) { query, err := RenderQuery(appMeshQueries["request-success-rate"], model) if err != nil { - return 0, err + return 0, fmt.Errorf("rendering query failed: %w", err) } value, err := ob.client.RunQuery(query) if err != nil { - return 0, err + return 0, fmt.Errorf("running query failed: %w", err) } return value, nil @@ -63,12 +64,12 @@ func (ob *AppMeshObserver) GetRequestSuccessRate(model flaggerv1.MetricTemplateM func (ob *AppMeshObserver) GetRequestDuration(model flaggerv1.MetricTemplateModel) (time.Duration, error) { query, err := RenderQuery(appMeshQueries["request-duration"], model) if err != nil { - return 0, err + return 0, fmt.Errorf("rendering query failed: %w", err) } value, err := ob.client.RunQuery(query) if err != nil { - return 0, err + return 0, fmt.Errorf("running query failed: %w", err) } ms := time.Duration(int64(value)) * time.Millisecond diff --git a/pkg/metrics/observers/contour.go b/pkg/metrics/observers/contour.go index 919797bf9..819b40cbc 100644 --- a/pkg/metrics/observers/contour.go +++ b/pkg/metrics/observers/contour.go @@ -1,6 +1,7 @@ package observers import ( + "fmt" "time" flaggerv1 "github.com/weaveworks/flagger/pkg/apis/flagger/v1beta1" @@ -48,12 +49,12 @@ type ContourObserver struct { func (ob *ContourObserver) GetRequestSuccessRate(model flaggerv1.MetricTemplateModel) (float64, error) { query, err := RenderQuery(contourQueries["request-success-rate"], model) if err != nil { - return 0, err + return 0, fmt.Errorf("rendering query failed: %w", err) } value, err := ob.client.RunQuery(query) if err != nil { - return 0, err + return 0, fmt.Errorf("running query failed: %w", err) } return value, nil @@ -62,12 +63,12 @@ func (ob *ContourObserver) GetRequestSuccessRate(model flaggerv1.MetricTemplateM func (ob *ContourObserver) GetRequestDuration(model flaggerv1.MetricTemplateModel) (time.Duration, error) { query, err := RenderQuery(contourQueries["request-duration"], model) if err != nil { - return 0, err + return 0, fmt.Errorf("rendering query failed: %w", err) } value, err := ob.client.RunQuery(query) if err != nil { - return 0, err + return 0, fmt.Errorf("running query failed: %w", err) } ms := time.Duration(int64(value)) * time.Millisecond diff --git a/pkg/metrics/observers/crossover.go b/pkg/metrics/observers/crossover.go index ac985201b..0ecf3d150 100644 --- a/pkg/metrics/observers/crossover.go +++ b/pkg/metrics/observers/crossover.go @@ -1,6 +1,7 @@ package observers import ( + "fmt" "time" flaggerv1 "github.com/weaveworks/flagger/pkg/apis/flagger/v1beta1" @@ -63,12 +64,12 @@ func (ob *CrossoverObserver) GetRequestSuccessRate(model flaggerv1.MetricTemplat func (ob *CrossoverObserver) GetRequestDuration(model flaggerv1.MetricTemplateModel) (time.Duration, error) { query, err := RenderQuery(crossoverQueries["request-duration"], model) if err != nil { - return 0, err + return 0, fmt.Errorf("rendering query failed: %w", err) } value, err := ob.client.RunQuery(query) if err != nil { - return 0, err + return 0, fmt.Errorf("running query failed: %w", err) } ms := time.Duration(int64(value)) * time.Millisecond diff --git a/pkg/metrics/observers/crossover_service.go b/pkg/metrics/observers/crossover_service.go index c8c3565fc..0aa27a6a3 100644 --- a/pkg/metrics/observers/crossover_service.go +++ b/pkg/metrics/observers/crossover_service.go @@ -1,6 +1,7 @@ package observers import ( + "fmt" "time" flaggerv1 "github.com/weaveworks/flagger/pkg/apis/flagger/v1beta1" @@ -49,12 +50,12 @@ type CrossoverServiceObserver struct { func (ob *CrossoverServiceObserver) GetRequestSuccessRate(model flaggerv1.MetricTemplateModel) (float64, error) { query, err := RenderQuery(crossoverServiceQueries["request-success-rate"], model) if err != nil { - return 0, err + return 0, fmt.Errorf("rendering query failed: %w", err) } value, err := ob.client.RunQuery(query) if err != nil { - return 0, err + return 0, fmt.Errorf("running query failed: %w", err) } return value, nil @@ -63,12 +64,12 @@ func (ob *CrossoverServiceObserver) GetRequestSuccessRate(model flaggerv1.Metric func (ob *CrossoverServiceObserver) GetRequestDuration(model flaggerv1.MetricTemplateModel) (time.Duration, error) { query, err := RenderQuery(crossoverServiceQueries["request-duration"], model) if err != nil { - return 0, err + return 0, fmt.Errorf("rendering query failed: %w", err) } value, err := ob.client.RunQuery(query) if err != nil { - return 0, err + return 0, fmt.Errorf("running query failed: %w", err) } ms := time.Duration(int64(value)) * time.Millisecond diff --git a/pkg/metrics/observers/errors.go b/pkg/metrics/observers/errors.go deleted file mode 100644 index 1e3993a6c..000000000 --- a/pkg/metrics/observers/errors.go +++ /dev/null @@ -1,7 +0,0 @@ -package observers - -import "errors" - -var ( - ErrNoValuesFound = errors.New("no values found") -) diff --git a/pkg/metrics/observers/gloo.go b/pkg/metrics/observers/gloo.go index 116787a03..90350edb6 100644 --- a/pkg/metrics/observers/gloo.go +++ b/pkg/metrics/observers/gloo.go @@ -1,6 +1,7 @@ package observers import ( + "fmt" "time" flaggerv1 "github.com/weaveworks/flagger/pkg/apis/flagger/v1beta1" @@ -48,12 +49,12 @@ type GlooObserver struct { func (ob *GlooObserver) GetRequestSuccessRate(model flaggerv1.MetricTemplateModel) (float64, error) { query, err := RenderQuery(glooQueries["request-success-rate"], model) if err != nil { - return 0, err + return 0, fmt.Errorf("rendering query failed: %w", err) } value, err := ob.client.RunQuery(query) if err != nil { - return 0, err + return 0, fmt.Errorf("running query failed: %w", err) } return value, nil @@ -62,12 +63,12 @@ func (ob *GlooObserver) GetRequestSuccessRate(model flaggerv1.MetricTemplateMode func (ob *GlooObserver) GetRequestDuration(model flaggerv1.MetricTemplateModel) (time.Duration, error) { query, err := RenderQuery(glooQueries["request-duration"], model) if err != nil { - return 0, err + return 0, fmt.Errorf("rendering query failed: %w", err) } value, err := ob.client.RunQuery(query) if err != nil { - return 0, err + return 0, fmt.Errorf("running query failed: %w", err) } ms := time.Duration(int64(value)) * time.Millisecond diff --git a/pkg/metrics/observers/http.go b/pkg/metrics/observers/http.go index 632fa72cc..24dbc4b7c 100644 --- a/pkg/metrics/observers/http.go +++ b/pkg/metrics/observers/http.go @@ -1,6 +1,7 @@ package observers import ( + "fmt" "time" flaggerv1 "github.com/weaveworks/flagger/pkg/apis/flagger/v1beta1" @@ -63,12 +64,12 @@ func (ob *HttpObserver) GetRequestSuccessRate(model flaggerv1.MetricTemplateMode func (ob *HttpObserver) GetRequestDuration(model flaggerv1.MetricTemplateModel) (time.Duration, error) { query, err := RenderQuery(httpQueries["request-duration"], model) if err != nil { - return 0, err + return 0, fmt.Errorf("rendering query failed: %w", err) } value, err := ob.client.RunQuery(query) if err != nil { - return 0, err + return 0, fmt.Errorf("running query failed: %w", err) } ms := time.Duration(int64(value*1000)) * time.Millisecond diff --git a/pkg/metrics/observers/istio.go b/pkg/metrics/observers/istio.go index 52945ea47..26084451f 100644 --- a/pkg/metrics/observers/istio.go +++ b/pkg/metrics/observers/istio.go @@ -1,6 +1,7 @@ package observers import ( + "fmt" "time" flaggerv1 "github.com/weaveworks/flagger/pkg/apis/flagger/v1beta1" @@ -52,12 +53,12 @@ type IstioObserver struct { func (ob *IstioObserver) GetRequestSuccessRate(model flaggerv1.MetricTemplateModel) (float64, error) { query, err := RenderQuery(istioQueries["request-success-rate"], model) if err != nil { - return 0, err + return 0, fmt.Errorf("rendering query failed: %w", err) } value, err := ob.client.RunQuery(query) if err != nil { - return 0, err + return 0, fmt.Errorf("running query failed: %w", err) } return value, nil @@ -66,12 +67,12 @@ func (ob *IstioObserver) GetRequestSuccessRate(model flaggerv1.MetricTemplateMod func (ob *IstioObserver) GetRequestDuration(model flaggerv1.MetricTemplateModel) (time.Duration, error) { query, err := RenderQuery(istioQueries["request-duration"], model) if err != nil { - return 0, err + return 0, fmt.Errorf("rendering query failed: %w", err) } value, err := ob.client.RunQuery(query) if err != nil { - return 0, err + return 0, fmt.Errorf("running query failed: %w", err) } ms := time.Duration(int64(value*1000)) * time.Millisecond diff --git a/pkg/metrics/observers/linkerd.go b/pkg/metrics/observers/linkerd.go index 5257a7c6f..d27ffa6ce 100644 --- a/pkg/metrics/observers/linkerd.go +++ b/pkg/metrics/observers/linkerd.go @@ -1,6 +1,7 @@ package observers import ( + "fmt" "time" flaggerv1 "github.com/weaveworks/flagger/pkg/apis/flagger/v1beta1" @@ -52,12 +53,12 @@ type LinkerdObserver struct { func (ob *LinkerdObserver) GetRequestSuccessRate(model flaggerv1.MetricTemplateModel) (float64, error) { query, err := RenderQuery(linkerdQueries["request-success-rate"], model) if err != nil { - return 0, err + return 0, fmt.Errorf("rendering query failed: %w", err) } value, err := ob.client.RunQuery(query) if err != nil { - return 0, err + return 0, fmt.Errorf("running query failed: %w", err) } return value, nil @@ -66,12 +67,12 @@ func (ob *LinkerdObserver) GetRequestSuccessRate(model flaggerv1.MetricTemplateM func (ob *LinkerdObserver) GetRequestDuration(model flaggerv1.MetricTemplateModel) (time.Duration, error) { query, err := RenderQuery(linkerdQueries["request-duration"], model) if err != nil { - return 0, err + return 0, fmt.Errorf("rendering query failed: %w", err) } value, err := ob.client.RunQuery(query) if err != nil { - return 0, err + return 0, fmt.Errorf("running query failed: %w", err) } ms := time.Duration(int64(value)) * time.Millisecond diff --git a/pkg/metrics/observers/nginx.go b/pkg/metrics/observers/nginx.go index 442dc5d1d..416670866 100644 --- a/pkg/metrics/observers/nginx.go +++ b/pkg/metrics/observers/nginx.go @@ -1,6 +1,7 @@ package observers import ( + "fmt" "time" flaggerv1 "github.com/weaveworks/flagger/pkg/apis/flagger/v1beta1" @@ -56,12 +57,12 @@ type NginxObserver struct { func (ob *NginxObserver) GetRequestSuccessRate(model flaggerv1.MetricTemplateModel) (float64, error) { query, err := RenderQuery(nginxQueries["request-success-rate"], model) if err != nil { - return 0, err + return 0, fmt.Errorf("rendering query failed: %w", err) } value, err := ob.client.RunQuery(query) if err != nil { - return 0, err + return 0, fmt.Errorf("running query failed: %w", err) } return value, nil @@ -70,12 +71,12 @@ func (ob *NginxObserver) GetRequestSuccessRate(model flaggerv1.MetricTemplateMod func (ob *NginxObserver) GetRequestDuration(model flaggerv1.MetricTemplateModel) (time.Duration, error) { query, err := RenderQuery(nginxQueries["request-duration"], model) if err != nil { - return 0, err + return 0, fmt.Errorf("rendering query failed: %w", err) } value, err := ob.client.RunQuery(query) if err != nil { - return 0, err + return 0, fmt.Errorf("running query failed: %w", err) } ms := time.Duration(int64(value)) * time.Millisecond diff --git a/pkg/metrics/observers/nginx_test.go b/pkg/metrics/observers/nginx_test.go index 10724c0d7..75df49c30 100644 --- a/pkg/metrics/observers/nginx_test.go +++ b/pkg/metrics/observers/nginx_test.go @@ -1,6 +1,7 @@ package observers import ( + "errors" "net/http" "net/http/httptest" "testing" @@ -14,38 +15,61 @@ import ( ) func TestNginxObserver_GetRequestSuccessRate(t *testing.T) { - expected := ` sum( rate( nginx_ingress_controller_requests{ namespace="nginx", ingress="podinfo", status!~"5.*" }[1m] ) ) / sum( rate( nginx_ingress_controller_requests{ namespace="nginx", ingress="podinfo" }[1m] ) ) * 100` - - ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - promql := r.URL.Query()["query"][0] - assert.Equal(t, expected, promql) - - json := `{"status":"success","data":{"resultType":"vector","result":[{"metric":{},"value":[1,"100"]}]}}` - w.Write([]byte(json)) - })) - defer ts.Close() - - client, err := providers.NewPrometheusProvider(flaggerv1.MetricTemplateProvider{ - Type: "prometheus", - Address: ts.URL, - SecretRef: nil, - }, nil) - require.NoError(t, err) - - observer := &NginxObserver{ - client: client, - } - - val, err := observer.GetRequestSuccessRate(flaggerv1.MetricTemplateModel{ - Name: "podinfo", - Namespace: "nginx", - Target: "podinfo", - Ingress: "podinfo", - Interval: "1m", + t.Run("ok", func(t *testing.T) { + expected := ` sum( rate( nginx_ingress_controller_requests{ namespace="nginx", ingress="podinfo", status!~"5.*" }[1m] ) ) / sum( rate( nginx_ingress_controller_requests{ namespace="nginx", ingress="podinfo" }[1m] ) ) * 100` + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + promql := r.URL.Query()["query"][0] + assert.Equal(t, expected, promql) + + json := `{"status":"success","data":{"resultType":"vector","result":[{"metric":{},"value":[1,"100"]}]}}` + w.Write([]byte(json)) + })) + defer ts.Close() + + client, err := providers.NewPrometheusProvider(flaggerv1.MetricTemplateProvider{ + Type: "prometheus", + Address: ts.URL, + SecretRef: nil, + }, nil) + require.NoError(t, err) + + observer := &NginxObserver{ + client: client, + } + + val, err := observer.GetRequestSuccessRate(flaggerv1.MetricTemplateModel{ + Name: "podinfo", + Namespace: "nginx", + Target: "podinfo", + Ingress: "podinfo", + Interval: "1m", + }) + require.NoError(t, err) + + assert.Equal(t, float64(100), val) }) - require.NoError(t, err) - assert.Equal(t, float64(100), val) + t.Run("no values", func(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + json := `{"status":"success","data":{"resultType":"vector","result":[]}}` + w.Write([]byte(json)) + })) + defer ts.Close() + + client, err := providers.NewPrometheusProvider(flaggerv1.MetricTemplateProvider{ + Type: "prometheus", + Address: ts.URL, + SecretRef: nil, + }, nil) + require.NoError(t, err) + + observer := &NginxObserver{ + client: client, + } + + _, err = observer.GetRequestSuccessRate(flaggerv1.MetricTemplateModel{}) + require.True(t, errors.Is(err, providers.ErrNoValuesFound)) + }) } func TestNginxObserver_GetRequestDuration(t *testing.T) { diff --git a/pkg/metrics/observers/render.go b/pkg/metrics/observers/render.go index ca3aadd43..bcf11b57f 100644 --- a/pkg/metrics/observers/render.go +++ b/pkg/metrics/observers/render.go @@ -3,6 +3,7 @@ package observers import ( "bufio" "bytes" + "fmt" "text/template" flaggerv1 "github.com/weaveworks/flagger/pkg/apis/flagger/v1beta1" @@ -11,19 +12,18 @@ import ( func RenderQuery(queryTemplate string, model flaggerv1.MetricTemplateModel) (string, error) { t, err := template.New("tmpl").Funcs(model.TemplateFunctions()).Parse(queryTemplate) if err != nil { - return "", err + return "", fmt.Errorf("template parsing failed: %w", err) } var data bytes.Buffer b := bufio.NewWriter(&data) if err := t.Execute(b, nil); err != nil { - return "", err + return "", fmt.Errorf("template excution failed: %w", err) } err = b.Flush() if err != nil { - return "", err + return "", fmt.Errorf("buffer flush failed: %w", err) } - return data.String(), nil } From 23ab1bdb4b7da664c06be7c4773873c8fc3e1048 Mon Sep 17 00:00:00 2001 From: mathetake Date: Sun, 8 Mar 2020 11:45:09 +0900 Subject: [PATCH 06/11] pkg/router: improve error handling messages --- pkg/router/appmesh.go | 56 ++++++++++++----------------- pkg/router/contour.go | 26 +++++--------- pkg/router/gloo.go | 26 +++++--------- pkg/router/ingress.go | 34 +++++++++--------- pkg/router/istio.go | 51 +++++++++----------------- pkg/router/kubernetes_deployment.go | 22 ++++++------ pkg/router/kubernetes_noop.go | 4 +-- pkg/router/nop.go | 4 +-- pkg/router/smi.go | 32 +++++++---------- 9 files changed, 97 insertions(+), 158 deletions(-) diff --git a/pkg/router/appmesh.go b/pkg/router/appmesh.go index 2f0964177..83a32cf34 100644 --- a/pkg/router/appmesh.go +++ b/pkg/router/appmesh.go @@ -42,35 +42,35 @@ func (ar *AppMeshRouter) Reconcile(canary *flaggerv1.Canary) error { // DNS app.namespace err := ar.reconcileVirtualNode(canary, apexName, primaryHost) if err != nil { - return err + return fmt.Errorf("reconcileVirtualNode failed: %w", err) } // sync virtual node e.g. app-primary-namespace // DNS app-primary.namespace err = ar.reconcileVirtualNode(canary, primaryName, primaryHost) if err != nil { - return err + return fmt.Errorf("reconcileVirtualNode failed: %w", err) } // sync virtual node e.g. app-canary-namespace // DNS app-canary.namespace err = ar.reconcileVirtualNode(canary, canaryName, canaryHost) if err != nil { - return err + return fmt.Errorf("reconcileVirtualNode failed: %w", err) } // sync main virtual service // DNS app.namespace err = ar.reconcileVirtualService(canary, targetHost, 0) if err != nil { - return err + return fmt.Errorf("reconcileVirtualService failed: %w", err) } // sync canary virtual service // DNS app-canary.namespace err = ar.reconcileVirtualService(canary, fmt.Sprintf("%s.%s", canaryName, canary.Namespace), 100) if err != nil { - return err + return fmt.Errorf("reconcileVirtualService failed: %w", err) } return nil @@ -97,15 +97,15 @@ func (ar *AppMeshRouter) reconcileVirtualNode(canary *flaggerv1.Canary, name str }, } - backends := []appmeshv1.Backend{} - for _, b := range canary.Spec.Service.Backends { - backend := appmeshv1.Backend{ + backends := make([]appmeshv1.Backend, len(canary.Spec.Service.Backends)) + for i, b := range canary.Spec.Service.Backends { + backends[i] = appmeshv1.Backend{ VirtualService: appmeshv1.VirtualServiceBackend{ VirtualServiceName: b, }, } - backends = append(backends, backend) } + if len(backends) > 0 { vnSpec.Backends = backends } @@ -130,15 +130,13 @@ func (ar *AppMeshRouter) reconcileVirtualNode(canary *flaggerv1.Canary, name str } _, err = ar.appmeshClient.AppmeshV1beta1().VirtualNodes(canary.Namespace).Create(virtualnode) if err != nil { - return fmt.Errorf("VirtualNode %s.%s create error %v", name, canary.Namespace, err) + return fmt.Errorf("VirtualNode %s.%s create error %w", name, canary.Namespace, err) } ar.logger.With("canary", fmt.Sprintf("%s.%s", canary.Name, canary.Namespace)). Infof("VirtualNode %s.%s created", virtualnode.GetName(), canary.Namespace) return nil - } - - if err != nil { - return fmt.Errorf("VirtualNode %s query error %v", name, err) + } else if err != nil { + return fmt.Errorf("VirtualNode %s get query error %w", name, err) } // update virtual node @@ -148,7 +146,7 @@ func (ar *AppMeshRouter) reconcileVirtualNode(canary *flaggerv1.Canary, name str vnClone.Spec = vnSpec _, err = ar.appmeshClient.AppmeshV1beta1().VirtualNodes(canary.Namespace).Update(vnClone) if err != nil { - return fmt.Errorf("VirtualNode %s update error %v", name, err) + return fmt.Errorf("VirtualNode %s update error %w", name, err) } ar.logger.With("canary", fmt.Sprintf("%s.%s", canary.Name, canary.Namespace)). Infof("VirtualNode %s updated", virtualnode.GetName()) @@ -294,15 +292,13 @@ func (ar *AppMeshRouter) reconcileVirtualService(canary *flaggerv1.Canary, name _, err = ar.appmeshClient.AppmeshV1beta1().VirtualServices(canary.Namespace).Create(virtualService) if err != nil { - return fmt.Errorf("VirtualService %s create error %v", name, err) + return fmt.Errorf("VirtualService %s create error %w", name, err) } ar.logger.With("canary", fmt.Sprintf("%s.%s", canary.Name, canary.Namespace)). Infof("VirtualService %s created", virtualService.GetName()) return nil - } - - if err != nil { - return fmt.Errorf("VirtualService %s query error %v", name, err) + } else if err != nil { + return fmt.Errorf("VirtualService %s get query error: %w", name, err) } // update virtual service but keep the original target weights @@ -322,7 +318,7 @@ func (ar *AppMeshRouter) reconcileVirtualService(canary *flaggerv1.Canary, name _, err = ar.appmeshClient.AppmeshV1beta1().VirtualServices(canary.Namespace).Update(vsClone) if err != nil { - return fmt.Errorf("VirtualService %s update error %v", name, err) + return fmt.Errorf("VirtualService %s update error: %w", name, err) } ar.logger.With("canary", fmt.Sprintf("%s.%s", canary.Name, canary.Namespace)). Infof("VirtualService %s updated", virtualService.GetName()) @@ -343,11 +339,7 @@ func (ar *AppMeshRouter) GetRoutes(canary *flaggerv1.Canary) ( vsName := fmt.Sprintf("%s.%s", apexName, canary.Namespace) vs, err := ar.appmeshClient.AppmeshV1beta1().VirtualServices(canary.Namespace).Get(vsName, metav1.GetOptions{}) if err != nil { - if errors.IsNotFound(err) { - err = fmt.Errorf("VirtualService %s not found", vsName) - return - } - err = fmt.Errorf("VirtualService %s query error %v", vsName, err) + err = fmt.Errorf("VirtualService %s get query error: %w", vsName, err) return } @@ -381,16 +373,13 @@ func (ar *AppMeshRouter) SetRoutes( canary *flaggerv1.Canary, primaryWeight int, canaryWeight int, - mirrored bool, + _ bool, ) error { apexName, _, _ := canary.GetServiceNames() vsName := fmt.Sprintf("%s.%s", apexName, canary.Namespace) vs, err := ar.appmeshClient.AppmeshV1beta1().VirtualServices(canary.Namespace).Get(vsName, metav1.GetOptions{}) if err != nil { - if errors.IsNotFound(err) { - return fmt.Errorf("VirtualService %s not found", vsName) - } - return fmt.Errorf("VirtualService %s query error %v", vsName, err) + return fmt.Errorf("VirtualService %s get query error: %w", vsName, err) } vsClone := vs.DeepCopy() @@ -409,9 +398,8 @@ func (ar *AppMeshRouter) SetRoutes( _, err = ar.appmeshClient.AppmeshV1beta1().VirtualServices(canary.Namespace).Update(vsClone) if err != nil { - return fmt.Errorf("VirtualService %s update error %v", vsName, err) + return fmt.Errorf("VirtualService %s update error: %w", vsName, err) } - return nil } @@ -449,8 +437,8 @@ func makeRetryPolicy(canary *flaggerv1.Canary) *appmeshv1.HttpRetryPolicy { // makeRetryPolicy creates an App Mesh HttpRouteHeader from the Canary.CanaryAnalysis.Match func (ar *AppMeshRouter) makeHeaders(canary *flaggerv1.Canary) []appmeshv1.HttpRouteHeader { - headers := []appmeshv1.HttpRouteHeader{} + var headers []appmeshv1.HttpRouteHeader for _, m := range canary.GetAnalysis().Match { for key, value := range m.Headers { header := appmeshv1.HttpRouteHeader{ diff --git a/pkg/router/contour.go b/pkg/router/contour.go index 8ad846ab1..dfd7dd113 100644 --- a/pkg/router/contour.go +++ b/pkg/router/contour.go @@ -152,15 +152,13 @@ func (cr *ContourRouter) Reconcile(canary *flaggerv1.Canary) error { _, err = cr.contourClient.ProjectcontourV1().HTTPProxies(canary.Namespace).Create(proxy) if err != nil { - return fmt.Errorf("HTTPProxy %s.%s create error %v", apexName, canary.Namespace, err) + return fmt.Errorf("HTTPProxy %s.%s create error: %w", apexName, canary.Namespace, err) } cr.logger.With("canary", fmt.Sprintf("%s.%s", canary.Name, canary.Namespace)). Infof("HTTPProxy %s.%s created", proxy.GetName(), canary.Namespace) return nil - } - - if err != nil { - return fmt.Errorf("HTTPProxy %s.%s query error %v", apexName, canary.Namespace, err) + } else if err != nil { + return fmt.Errorf("HTTPProxy %s.%s get query error: %w", apexName, canary.Namespace, err) } // update HTTPProxy but keep the original destination weights @@ -175,7 +173,7 @@ func (cr *ContourRouter) Reconcile(canary *flaggerv1.Canary) error { _, err = cr.contourClient.ProjectcontourV1().HTTPProxies(canary.Namespace).Update(clone) if err != nil { - return fmt.Errorf("HTTPProxy %s.%s update error %v", apexName, canary.Namespace, err) + return fmt.Errorf("HTTPProxy %s.%s update error: %w", apexName, canary.Namespace, err) } cr.logger.With("canary", fmt.Sprintf("%s.%s", canary.Name, canary.Namespace)). Infof("HTTPProxy %s.%s updated", proxy.GetName(), canary.Namespace) @@ -196,11 +194,7 @@ func (cr *ContourRouter) GetRoutes(canary *flaggerv1.Canary) ( proxy, err := cr.contourClient.ProjectcontourV1().HTTPProxies(canary.Namespace).Get(apexName, metav1.GetOptions{}) if err != nil { - if errors.IsNotFound(err) { - err = fmt.Errorf("HTTPProxy %s.%s not found", apexName, canary.Namespace) - return - } - err = fmt.Errorf("HTTPProxy %s.%s query error %v", apexName, canary.Namespace, err) + err = fmt.Errorf("HTTPProxy %s.%s get query error %w", apexName, canary.Namespace, err) return } @@ -225,7 +219,7 @@ func (cr *ContourRouter) SetRoutes( canary *flaggerv1.Canary, primaryWeight int, canaryWeight int, - mirrored bool, + _ bool, ) error { apexName, primaryName, canaryName := canary.GetServiceNames() @@ -235,11 +229,7 @@ func (cr *ContourRouter) SetRoutes( proxy, err := cr.contourClient.ProjectcontourV1().HTTPProxies(canary.Namespace).Get(apexName, metav1.GetOptions{}) if err != nil { - if errors.IsNotFound(err) { - return fmt.Errorf("HTTPProxy %s.%s not found", apexName, canary.Namespace) - - } - return fmt.Errorf("HTTPProxy %s.%s query error %v", apexName, canary.Namespace, err) + return fmt.Errorf("HTTPProxy %s.%s query error: %w", apexName, canary.Namespace, err) } proxy.Spec = contourv1.HTTPProxySpec{ @@ -344,7 +334,7 @@ func (cr *ContourRouter) SetRoutes( _, err = cr.contourClient.ProjectcontourV1().HTTPProxies(canary.Namespace).Update(proxy) if err != nil { - return fmt.Errorf("HTTPProxy %s.%s update error %v", apexName, canary.Namespace, err) + return fmt.Errorf("HTTPProxy %s.%s update error: %w", apexName, canary.Namespace, err) } return nil } diff --git a/pkg/router/gloo.go b/pkg/router/gloo.go index 8648a7e36..c2401e04e 100644 --- a/pkg/router/gloo.go +++ b/pkg/router/gloo.go @@ -73,15 +73,13 @@ func (gr *GlooRouter) Reconcile(canary *flaggerv1.Canary) error { _, err = gr.glooClient.GlooV1().UpstreamGroups(canary.Namespace).Create(upstreamGroup) if err != nil { - return fmt.Errorf("UpstreamGroup %s.%s create error %v", apexName, canary.Namespace, err) + return fmt.Errorf("UpstreamGroup %s.%s create error: %w", apexName, canary.Namespace, err) } gr.logger.With("canary", fmt.Sprintf("%s.%s", canary.Name, canary.Namespace)). Infof("UpstreamGroup %s.%s created", upstreamGroup.GetName(), canary.Namespace) return nil - } - - if err != nil { - return fmt.Errorf("UpstreamGroup %s.%s query error %v", apexName, canary.Namespace, err) + } else if err != nil { + return fmt.Errorf("UpstreamGroup %s.%s get query error: %w", apexName, canary.Namespace, err) } // update upstreamGroup but keep the original destination weights @@ -96,7 +94,7 @@ func (gr *GlooRouter) Reconcile(canary *flaggerv1.Canary) error { _, err = gr.glooClient.GlooV1().UpstreamGroups(canary.Namespace).Update(clone) if err != nil { - return fmt.Errorf("UpstreamGroup %s.%s update error %v", apexName, canary.Namespace, err) + return fmt.Errorf("UpstreamGroup %s.%s update error: %w", apexName, canary.Namespace, err) } gr.logger.With("canary", fmt.Sprintf("%s.%s", canary.Name, canary.Namespace)). Infof("UpstreamGroup %s.%s updated", upstreamGroup.GetName(), canary.Namespace) @@ -118,11 +116,7 @@ func (gr *GlooRouter) GetRoutes(canary *flaggerv1.Canary) ( upstreamGroup, err := gr.glooClient.GlooV1().UpstreamGroups(canary.Namespace).Get(apexName, metav1.GetOptions{}) if err != nil { - if errors.IsNotFound(err) { - err = fmt.Errorf("UpstreamGroup %s.%s not found", apexName, canary.Namespace) - return - } - err = fmt.Errorf("UpstreamGroup %s.%s query error %v", apexName, canary.Namespace, err) + err = fmt.Errorf("UpstreamGroup %s.%s get query error: %w", apexName, canary.Namespace, err) return } @@ -147,7 +141,7 @@ func (gr *GlooRouter) SetRoutes( canary *flaggerv1.Canary, primaryWeight int, canaryWeight int, - mirrored bool, + _ bool, ) error { apexName, _, _ := canary.GetServiceNames() canaryName := fmt.Sprintf("%s-%s-canary-%v", canary.Namespace, apexName, canary.Spec.Service.Port) @@ -159,11 +153,7 @@ func (gr *GlooRouter) SetRoutes( upstreamGroup, err := gr.glooClient.GlooV1().UpstreamGroups(canary.Namespace).Get(apexName, metav1.GetOptions{}) if err != nil { - if errors.IsNotFound(err) { - return fmt.Errorf("UpstreamGroup %s.%s not found", apexName, canary.Namespace) - - } - return fmt.Errorf("UpstreamGroup %s.%s query error %v", apexName, canary.Namespace, err) + return fmt.Errorf("UpstreamGroup %s.%s query error: %w", apexName, canary.Namespace, err) } upstreamGroup.Spec = gloov1.UpstreamGroupSpec{ @@ -191,7 +181,7 @@ func (gr *GlooRouter) SetRoutes( _, err = gr.glooClient.GlooV1().UpstreamGroups(canary.Namespace).Update(upstreamGroup) if err != nil { - return fmt.Errorf("UpstreamGroup %s.%s update error %v", apexName, canary.Namespace, err) + return fmt.Errorf("UpstreamGroup %s.%s update error: %w", apexName, canary.Namespace, err) } return nil } diff --git a/pkg/router/ingress.go b/pkg/router/ingress.go index e97d68c41..70682aac0 100644 --- a/pkg/router/ingress.go +++ b/pkg/router/ingress.go @@ -33,7 +33,7 @@ func (i *IngressRouter) Reconcile(canary *flaggerv1.Canary) error { ingress, err := i.kubeClient.ExtensionsV1beta1().Ingresses(canary.Namespace).Get(canary.Spec.IngressRef.Name, metav1.GetOptions{}) if err != nil { - return err + return fmt.Errorf("ingress %s.%s get query error: %w", canary.Spec.IngressRef.Name, canary.Namespace, err) } ingressClone := ingress.DeepCopy() @@ -76,16 +76,14 @@ func (i *IngressRouter) Reconcile(canary *flaggerv1.Canary) error { _, err := i.kubeClient.ExtensionsV1beta1().Ingresses(canary.Namespace).Create(ing) if err != nil { - return err + return fmt.Errorf("ingress %s.%s create error: %w", ing.Name, ing.Namespace, err) } i.logger.With("canary", fmt.Sprintf("%s.%s", canary.Name, canary.Namespace)). Infof("Ingress %s.%s created", ing.GetName(), canary.Namespace) return nil - } - - if err != nil { - return fmt.Errorf("ingress %s query error %v", canaryIngressName, err) + } else if err != nil { + return fmt.Errorf("ingress %s.%s query error: %w", canaryIngressName, canary.Namespace, err) } if diff := cmp.Diff(ingressClone.Spec, canaryIngress.Spec); diff != "" { @@ -94,7 +92,7 @@ func (i *IngressRouter) Reconcile(canary *flaggerv1.Canary) error { _, err := i.kubeClient.ExtensionsV1beta1().Ingresses(canary.Namespace).Update(iClone) if err != nil { - return fmt.Errorf("ingress %s update error %v", canaryIngressName, err) + return fmt.Errorf("ingress %s.%s update error: %w", canaryIngressName, iClone.Namespace, err) } i.logger.With("canary", fmt.Sprintf("%s.%s", canary.Name, canary.Namespace)). @@ -113,7 +111,8 @@ func (i *IngressRouter) GetRoutes(canary *flaggerv1.Canary) ( canaryIngressName := fmt.Sprintf("%s-canary", canary.Spec.IngressRef.Name) canaryIngress, err := i.kubeClient.ExtensionsV1beta1().Ingresses(canary.Namespace).Get(canaryIngressName, metav1.GetOptions{}) if err != nil { - return 0, 0, false, err + err = fmt.Errorf("ingress %s.%s get query error: %w", canaryIngressName, canary.Namespace, err) + return } // A/B testing @@ -128,9 +127,10 @@ func (i *IngressRouter) GetRoutes(canary *flaggerv1.Canary) ( // Canary for k, v := range canaryIngress.Annotations { if k == i.GetAnnotationWithPrefix("canary-weight") { - val, err := strconv.Atoi(v) - if err != nil { - return 0, 0, false, err + val, errAtoi := strconv.Atoi(v) + if errAtoi != nil { + err = fmt.Errorf("failed to convert %s to int: %w", v, errAtoi) + return } canaryWeight = val @@ -145,23 +145,21 @@ func (i *IngressRouter) GetRoutes(canary *flaggerv1.Canary) ( func (i *IngressRouter) SetRoutes( canary *flaggerv1.Canary, - primaryWeight int, + _ int, canaryWeight int, - mirrored bool, + _ bool, ) error { canaryIngressName := fmt.Sprintf("%s-canary", canary.Spec.IngressRef.Name) canaryIngress, err := i.kubeClient.ExtensionsV1beta1().Ingresses(canary.Namespace).Get(canaryIngressName, metav1.GetOptions{}) if err != nil { - return err + return fmt.Errorf("ingress %s.%s get query error: %w", canaryIngressName, canary.Namespace, err) } iClone := canaryIngress.DeepCopy() // A/B testing if len(canary.GetAnalysis().Match) > 0 { - cookie := "" - header := "" - headerValue := "" + var cookie, header, headerValue string for _, m := range canary.GetAnalysis().Match { for k, v := range m.Headers { if k == "cookie" { @@ -188,7 +186,7 @@ func (i *IngressRouter) SetRoutes( _, err = i.kubeClient.ExtensionsV1beta1().Ingresses(canary.Namespace).Update(iClone) if err != nil { - return fmt.Errorf("ingress %s update error %v", canaryIngressName, err) + return fmt.Errorf("ingress %s.%s update error %v", iClone.Name, iClone.Namespace, err) } return nil diff --git a/pkg/router/istio.go b/pkg/router/istio.go index df8093131..c6d43b929 100644 --- a/pkg/router/istio.go +++ b/pkg/router/istio.go @@ -28,21 +28,17 @@ type IstioRouter struct { func (ir *IstioRouter) Reconcile(canary *flaggerv1.Canary) error { _, primaryName, canaryName := canary.GetServiceNames() - err := ir.reconcileDestinationRule(canary, canaryName) - if err != nil { - return err + if err := ir.reconcileDestinationRule(canary, canaryName); err != nil { + return fmt.Errorf("reconcileDestinationRule failed: %w", err) } - err = ir.reconcileDestinationRule(canary, primaryName) - if err != nil { - return err + if err := ir.reconcileDestinationRule(canary, primaryName); err != nil { + return fmt.Errorf("reconcileDestinationRule failed: %w", err) } - err = ir.reconcileVirtualService(canary) - if err != nil { - return err + if err := ir.reconcileVirtualService(canary); err != nil { + return fmt.Errorf("reconcileVirtualService failed: %w", err) } - return nil } @@ -71,15 +67,13 @@ func (ir *IstioRouter) reconcileDestinationRule(canary *flaggerv1.Canary, name s } _, err = ir.istioClient.NetworkingV1alpha3().DestinationRules(canary.Namespace).Create(destinationRule) if err != nil { - return fmt.Errorf("DestinationRule %s.%s create error %v", name, canary.Namespace, err) + return fmt.Errorf("DestinationRule %s.%s create error: %w", name, canary.Namespace, err) } ir.logger.With("canary", fmt.Sprintf("%s.%s", canary.Name, canary.Namespace)). Infof("DestinationRule %s.%s created", destinationRule.GetName(), canary.Namespace) return nil - } - - if err != nil { - return fmt.Errorf("DestinationRule %s.%s query error %v", name, canary.Namespace, err) + } else if err != nil { + return fmt.Errorf("DestinationRule %s.%s get query error: %w", name, canary.Namespace, err) } // update @@ -89,7 +83,7 @@ func (ir *IstioRouter) reconcileDestinationRule(canary *flaggerv1.Canary, name s clone.Spec = newSpec _, err = ir.istioClient.NetworkingV1alpha3().DestinationRules(canary.Namespace).Update(clone) if err != nil { - return fmt.Errorf("DestinationRule %s.%s update error %v", name, canary.Namespace, err) + return fmt.Errorf("DestinationRule %s.%s update error: %w", name, canary.Namespace, err) } ir.logger.With("canary", fmt.Sprintf("%s.%s", canary.Name, canary.Namespace)). Infof("DestinationRule %s.%s updated", destinationRule.GetName(), canary.Namespace) @@ -197,15 +191,13 @@ func (ir *IstioRouter) reconcileVirtualService(canary *flaggerv1.Canary) error { } _, err = ir.istioClient.NetworkingV1alpha3().VirtualServices(canary.Namespace).Create(virtualService) if err != nil { - return fmt.Errorf("VirtualService %s.%s create error %v", apexName, canary.Namespace, err) + return fmt.Errorf("VirtualService %s.%s create error: %w", apexName, canary.Namespace, err) } ir.logger.With("canary", fmt.Sprintf("%s.%s", canary.Name, canary.Namespace)). Infof("VirtualService %s.%s created", virtualService.GetName(), canary.Namespace) return nil - } - - if err != nil { - return fmt.Errorf("VirtualService %s.%s query error %v", apexName, canary.Namespace, err) + } else if err != nil { + return fmt.Errorf("VirtualService %s.%s get query error %v", apexName, canary.Namespace, err) } // update service but keep the original destination weights and mirror @@ -221,7 +213,7 @@ func (ir *IstioRouter) reconcileVirtualService(canary *flaggerv1.Canary) error { _, err = ir.istioClient.NetworkingV1alpha3().VirtualServices(canary.Namespace).Update(vtClone) if err != nil { - return fmt.Errorf("VirtualService %s.%s update error %v", apexName, canary.Namespace, err) + return fmt.Errorf("VirtualService %s.%s update error: %w", apexName, canary.Namespace, err) } ir.logger.With("canary", fmt.Sprintf("%s.%s", canary.Name, canary.Namespace)). Infof("VirtualService %s.%s updated", virtualService.GetName(), canary.Namespace) @@ -242,11 +234,7 @@ func (ir *IstioRouter) GetRoutes(canary *flaggerv1.Canary) ( vs := &istiov1alpha3.VirtualService{} vs, err = ir.istioClient.NetworkingV1alpha3().VirtualServices(canary.Namespace).Get(apexName, metav1.GetOptions{}) if err != nil { - if errors.IsNotFound(err) { - err = fmt.Errorf("VirtualService %s.%s not found", apexName, canary.Namespace) - return - } - err = fmt.Errorf("VirtualService %s.%s query error %v", apexName, canary.Namespace, err) + err = fmt.Errorf("VirtualService %s.%s get query error %v", apexName, canary.Namespace, err) return } @@ -291,11 +279,7 @@ func (ir *IstioRouter) SetRoutes( vs, err := ir.istioClient.NetworkingV1alpha3().VirtualServices(canary.Namespace).Get(apexName, metav1.GetOptions{}) if err != nil { - if errors.IsNotFound(err) { - return fmt.Errorf("VirtualService %s.%s not found", apexName, canary.Namespace) - - } - return fmt.Errorf("VirtualService %s.%s query error %v", apexName, canary.Namespace, err) + return fmt.Errorf("VirtualService %s.%s get query error %v", apexName, canary.Namespace, err) } vsCopy := vs.DeepCopy() @@ -355,8 +339,7 @@ func (ir *IstioRouter) SetRoutes( vs, err = ir.istioClient.NetworkingV1alpha3().VirtualServices(canary.Namespace).Update(vsCopy) if err != nil { - return fmt.Errorf("VirtualService %s.%s update failed: %v", apexName, canary.Namespace, err) - + return fmt.Errorf("VirtualService %s.%s update failed: %w", apexName, canary.Namespace, err) } return nil } diff --git a/pkg/router/kubernetes_deployment.go b/pkg/router/kubernetes_deployment.go index 112a2be21..48854ab7f 100644 --- a/pkg/router/kubernetes_deployment.go +++ b/pkg/router/kubernetes_deployment.go @@ -34,13 +34,13 @@ func (c *KubernetesDeploymentRouter) Initialize(canary *flaggerv1.Canary) error // canary svc err := c.reconcileService(canary, canaryName, canary.Spec.TargetRef.Name) if err != nil { - return err + return fmt.Errorf("reconcileService failed: %w", err) } // primary svc err = c.reconcileService(canary, primaryName, fmt.Sprintf("%s-primary", canary.Spec.TargetRef.Name)) if err != nil { - return err + return fmt.Errorf("reconcileService failed: %w", err) } return nil @@ -53,17 +53,17 @@ func (c *KubernetesDeploymentRouter) Reconcile(canary *flaggerv1.Canary) error { // main svc err := c.reconcileService(canary, apexName, fmt.Sprintf("%s-primary", canary.Spec.TargetRef.Name)) if err != nil { - return err + return fmt.Errorf("reconcileService failed: %w", err) } return nil } -func (c *KubernetesDeploymentRouter) SetRoutes(canary *flaggerv1.Canary, primaryRoute int, canaryRoute int) error { +func (c *KubernetesDeploymentRouter) SetRoutes(_ *flaggerv1.Canary, _ int, _ int) error { return nil } -func (c *KubernetesDeploymentRouter) GetRoutes(canary *flaggerv1.Canary) (primaryRoute int, canaryRoute int, err error) { +func (c *KubernetesDeploymentRouter) GetRoutes(_ *flaggerv1.Canary) (primaryRoute int, canaryRoute int, err error) { return 0, 0, nil } @@ -128,18 +128,16 @@ func (c *KubernetesDeploymentRouter) reconcileService(canary *flaggerv1.Canary, Spec: svcSpec, } - _, err = c.kubeClient.CoreV1().Services(canary.Namespace).Create(svc) + _, err := c.kubeClient.CoreV1().Services(canary.Namespace).Create(svc) if err != nil { - return err + return fmt.Errorf("service %s.%s create error: %w", svc.Name, canary.Namespace, err) } c.logger.With("canary", fmt.Sprintf("%s.%s", canary.Name, canary.Namespace)). Infof("Service %s.%s created", svc.GetName(), canary.Namespace) return nil - } - - if err != nil { - return fmt.Errorf("service %s query error %v", name, err) + } else if err != nil { + return fmt.Errorf("service %s get query error: %w", name, err) } if svc != nil { @@ -155,7 +153,7 @@ func (c *KubernetesDeploymentRouter) reconcileService(canary *flaggerv1.Canary, svcClone.Spec.Selector = svcSpec.Selector _, err = c.kubeClient.CoreV1().Services(canary.Namespace).Update(svcClone) if err != nil { - return fmt.Errorf("service %s update error %v", name, err) + return fmt.Errorf("service %s update error: %w", name, err) } c.logger.With("canary", fmt.Sprintf("%s.%s", canary.Name, canary.Namespace)). Infof("Service %s updated", svc.GetName()) diff --git a/pkg/router/kubernetes_noop.go b/pkg/router/kubernetes_noop.go index 3a66249cb..b516cee8c 100644 --- a/pkg/router/kubernetes_noop.go +++ b/pkg/router/kubernetes_noop.go @@ -9,10 +9,10 @@ import ( type KubernetesNoopRouter struct { } -func (c *KubernetesNoopRouter) Initialize(canary *flaggerv1.Canary) error { +func (c *KubernetesNoopRouter) Initialize(_ *flaggerv1.Canary) error { return nil } -func (c *KubernetesNoopRouter) Reconcile(canary *flaggerv1.Canary) error { +func (c *KubernetesNoopRouter) Reconcile(_ *flaggerv1.Canary) error { return nil } diff --git a/pkg/router/nop.go b/pkg/router/nop.go index 665389d3f..4775cfda8 100644 --- a/pkg/router/nop.go +++ b/pkg/router/nop.go @@ -8,11 +8,11 @@ import ( type NopRouter struct { } -func (*NopRouter) Reconcile(canary *flaggerv1.Canary) error { +func (*NopRouter) Reconcile(_ *flaggerv1.Canary) error { return nil } -func (*NopRouter) SetRoutes(canary *flaggerv1.Canary, primaryWeight int, canaryWeight int, mirror bool) error { +func (*NopRouter) SetRoutes(_ *flaggerv1.Canary, _ int, _ int, _ bool) error { return nil } diff --git a/pkg/router/smi.go b/pkg/router/smi.go index 88d678d29..332822073 100644 --- a/pkg/router/smi.go +++ b/pkg/router/smi.go @@ -73,16 +73,14 @@ func (sr *SmiRouter) Reconcile(canary *flaggerv1.Canary) error { _, err := sr.smiClient.SplitV1alpha1().TrafficSplits(canary.Namespace).Create(t) if err != nil { - return err + return fmt.Errorf("TrafficSplit %s.%s create error: %w", apexName, canary.Namespace, err) } sr.logger.With("canary", fmt.Sprintf("%s.%s", canary.Name, canary.Namespace)). Infof("TrafficSplit %s.%s created", t.GetName(), canary.Namespace) return nil - } - - if err != nil { - return fmt.Errorf("traffic split %s query error %v", apexName, err) + } else if err != nil { + return fmt.Errorf("TrafficSplit %s.%s get query error: %w", apexName, canary.Namespace, err) } // update traffic split @@ -92,7 +90,7 @@ func (sr *SmiRouter) Reconcile(canary *flaggerv1.Canary) error { _, err := sr.smiClient.SplitV1alpha1().TrafficSplits(canary.Namespace).Update(tsClone) if err != nil { - return fmt.Errorf("TrafficSplit %s update error %v", apexName, err) + return fmt.Errorf("TrafficSplit %s.%s update error: %w", apexName, canary.Namespace, err) } sr.logger.With("canary", fmt.Sprintf("%s.%s", canary.Name, canary.Namespace)). @@ -113,11 +111,7 @@ func (sr *SmiRouter) GetRoutes(canary *flaggerv1.Canary) ( apexName, primaryName, canaryName := canary.GetServiceNames() ts, err := sr.smiClient.SplitV1alpha1().TrafficSplits(canary.Namespace).Get(apexName, metav1.GetOptions{}) if err != nil { - if errors.IsNotFound(err) { - err = fmt.Errorf("TrafficSplit %s.%s not found", apexName, canary.Namespace) - return - } - err = fmt.Errorf("TrafficSplit %s.%s query error %v", apexName, canary.Namespace, err) + err = fmt.Errorf("TrafficSplit %s.%s get query error %v", apexName, canary.Namespace, err) return } @@ -146,16 +140,12 @@ func (sr *SmiRouter) SetRoutes( canary *flaggerv1.Canary, primaryWeight int, canaryWeight int, - mirrored bool, + _ bool, ) error { apexName, primaryName, canaryName := canary.GetServiceNames() ts, err := sr.smiClient.SplitV1alpha1().TrafficSplits(canary.Namespace).Get(apexName, metav1.GetOptions{}) if err != nil { - if errors.IsNotFound(err) { - return fmt.Errorf("TrafficSplit %s.%s not found", apexName, canary.Namespace) - - } - return fmt.Errorf("TrafficSplit %s.%s query error %v", apexName, canary.Namespace, err) + return fmt.Errorf("TrafficSplit %s.%s get query error %v", apexName, canary.Namespace, err) } backends := []smiv1alpha1.TrafficSplitBackend{ @@ -174,7 +164,7 @@ func (sr *SmiRouter) SetRoutes( _, err = sr.smiClient.SplitV1alpha1().TrafficSplits(canary.Namespace).Update(tsClone) if err != nil { - return fmt.Errorf("TrafficSplit %s update error %v", apexName, err) + return fmt.Errorf("TrafficSplit %s.%s update error %v", apexName, canary.Namespace, err) } return nil @@ -224,11 +214,13 @@ func (sr *SmiRouter) getWithConvert(canary *flaggerv1.Canary, host string) (*smi _, err := sr.smiClient.SplitV1alpha2().TrafficSplits(canary.Namespace).Update(t) if err != nil { - return nil, err + return nil, fmt.Errorf("TrafficSplit %s.%s update error: %w", apexName, canary.Namespace, err) } sr.logger.With("canary", fmt.Sprintf("%s.%s", canary.Name, canary.Namespace)). Infof("TrafficSplit %s.%s converted", t.GetName(), canary.Namespace) + } else if err != nil { + return nil, fmt.Errorf("TrafficSplit %s.%s get query error %v", apexName, canary.Namespace, err) } - return ts, err + return ts, nil } From 0be72ab981fe9fe1105fec9cafdf8f47daf56967 Mon Sep 17 00:00:00 2001 From: mathetake Date: Sun, 8 Mar 2020 11:53:03 +0900 Subject: [PATCH 07/11] pkg/notifier: improve error handling messages --- pkg/notifier/client.go | 8 ++++---- pkg/notifier/discord.go | 2 +- pkg/notifier/factory.go | 10 +++++----- pkg/notifier/rocket.go | 3 +-- pkg/notifier/slack.go | 3 +-- pkg/notifier/teams.go | 2 +- 6 files changed, 13 insertions(+), 15 deletions(-) diff --git a/pkg/notifier/client.go b/pkg/notifier/client.go index 72d0b1bd3..b966368dd 100644 --- a/pkg/notifier/client.go +++ b/pkg/notifier/client.go @@ -13,14 +13,14 @@ import ( func postMessage(address string, payload interface{}) error { data, err := json.Marshal(payload) if err != nil { - return fmt.Errorf("marshalling notification payload failed %v", err) + return fmt.Errorf("marshalling notification payload failed: %w", err) } b := bytes.NewBuffer(data) req, err := http.NewRequest("POST", address, b) if err != nil { - return err + return fmt.Errorf("http.NewRequest failed: %w", err) } req.Header.Set("Content-type", "application/json") @@ -29,14 +29,14 @@ func postMessage(address string, payload interface{}) error { res, err := http.DefaultClient.Do(req.WithContext(ctx)) if err != nil { - return fmt.Errorf("sending notification failed %v", err) + return fmt.Errorf("sending notification failed: %w", err) } defer res.Body.Close() statusCode := res.StatusCode if statusCode != 200 { body, _ := ioutil.ReadAll(res.Body) - return fmt.Errorf("sending notification failed %v", string(body)) + return fmt.Errorf("sending notification failed: %s", string(body)) } return nil diff --git a/pkg/notifier/discord.go b/pkg/notifier/discord.go index 952f45e9c..61e453465 100644 --- a/pkg/notifier/discord.go +++ b/pkg/notifier/discord.go @@ -74,7 +74,7 @@ func (s *Discord) Post(workload string, namespace string, message string, fields err := postMessage(s.URL, payload) if err != nil { - return err + return fmt.Errorf("postMessage failed: %w", err) } return nil diff --git a/pkg/notifier/factory.go b/pkg/notifier/factory.go index deabe4b8f..8bcab4df2 100644 --- a/pkg/notifier/factory.go +++ b/pkg/notifier/factory.go @@ -17,14 +17,14 @@ func NewFactory(URL string, username string, channel string) *Factory { } func (f Factory) Notifier(provider string) (Interface, error) { - switch { - case provider == "slack": + switch provider { + case "slack": return NewSlack(f.URL, f.Username, f.Channel) - case provider == "discord": + case "discord": return NewDiscord(f.URL, f.Username, f.Channel) - case provider == "rocket": + case "rocket": return NewRocket(f.URL, f.Username, f.Channel) - case provider == "msteams": + case "msteams": return NewMSTeams(f.URL) } diff --git a/pkg/notifier/rocket.go b/pkg/notifier/rocket.go index 4bdf549fd..d15fef89d 100644 --- a/pkg/notifier/rocket.go +++ b/pkg/notifier/rocket.go @@ -65,8 +65,7 @@ func (s *Rocket) Post(workload string, namespace string, message string, fields err := postMessage(s.URL, payload) if err != nil { - return err + return fmt.Errorf("postMessage failed: %w", err) } - return nil } diff --git a/pkg/notifier/slack.go b/pkg/notifier/slack.go index 52084e3e7..88e4fb5ed 100644 --- a/pkg/notifier/slack.go +++ b/pkg/notifier/slack.go @@ -90,8 +90,7 @@ func (s *Slack) Post(workload string, namespace string, message string, fields [ err := postMessage(s.URL, payload) if err != nil { - return err + return fmt.Errorf("postMessage failed: %w", err) } - return nil } diff --git a/pkg/notifier/teams.go b/pkg/notifier/teams.go index 63e6ade3f..c08381ca0 100644 --- a/pkg/notifier/teams.go +++ b/pkg/notifier/teams.go @@ -70,7 +70,7 @@ func (s *MSTeams) Post(workload string, namespace string, message string, fields err := postMessage(s.URL, payload) if err != nil { - return err + return fmt.Errorf("postMessage failed: %w", err) } return nil From f34d94a912f839391b27103f722ecff1473b541d Mon Sep 17 00:00:00 2001 From: mathetake Date: Sun, 8 Mar 2020 13:12:56 +0900 Subject: [PATCH 08/11] pkg/loadtester: improve error handling messages --- pkg/loadtester/bash.go | 2 +- pkg/loadtester/helm.go | 2 +- pkg/loadtester/helmv3.go | 2 +- pkg/loadtester/task_ngrinder.go | 42 ++++++++++++++++++++------------- 4 files changed, 29 insertions(+), 19 deletions(-) diff --git a/pkg/loadtester/bash.go b/pkg/loadtester/bash.go index 8631c6f00..eccc9d19e 100644 --- a/pkg/loadtester/bash.go +++ b/pkg/loadtester/bash.go @@ -24,7 +24,7 @@ func (task *BashTask) Run(ctx context.Context) (bool, error) { if err != nil { task.logger.With("canary", task.canary).Errorf("command failed %s %v %s", task.command, err, out) - return false, fmt.Errorf(" %v %s", err, out) + return false, fmt.Errorf("command %s failed: %s: %w", task.command, out, err) } else { if task.logCmdOutput { fmt.Printf("%s\n", out) diff --git a/pkg/loadtester/helm.go b/pkg/loadtester/helm.go index 591bfc7c3..17661139f 100644 --- a/pkg/loadtester/helm.go +++ b/pkg/loadtester/helm.go @@ -27,7 +27,7 @@ func (task *HelmTask) Run(ctx context.Context) (bool, error) { out, err := cmd.CombinedOutput() if err != nil { task.logger.With("canary", task.canary).Errorf("command failed %s %v %s", task.command, err, out) - return false, fmt.Errorf(" %v %s", err, out) + return false, fmt.Errorf("command %s failed: %s: %w", task.command, out, err) } else { if task.logCmdOutput { fmt.Printf("%s\n", out) diff --git a/pkg/loadtester/helmv3.go b/pkg/loadtester/helmv3.go index 0970b55d0..3d824559f 100644 --- a/pkg/loadtester/helmv3.go +++ b/pkg/loadtester/helmv3.go @@ -27,7 +27,7 @@ func (task *HelmTaskv3) Run(ctx context.Context) (bool, error) { out, err := cmd.CombinedOutput() if err != nil { task.logger.With("canary", task.canary).Errorf("command failed %s %v %s", task.command, err, out) - return false, fmt.Errorf(" %v %s", err, out) + return false, fmt.Errorf("command %s failed: %s: %w", task.command, out, err) } else { if task.logCmdOutput { fmt.Printf("%s\n", out) diff --git a/pkg/loadtester/task_ngrinder.go b/pkg/loadtester/task_ngrinder.go index 9d7e8b86f..c2c98862b 100644 --- a/pkg/loadtester/task_ngrinder.go +++ b/pkg/loadtester/task_ngrinder.go @@ -29,16 +29,16 @@ func init() { } baseUrl, err := url.Parse(server) if err != nil { - return nil, errors.New(fmt.Sprintf("invalid url: %s", server)) + return nil, fmt.Errorf("invalid url: %s: %w", server, err) } cloneId, err := strconv.Atoi(clone) if err != nil { - return nil, errors.New("metadata clone must be integer") + return nil, fmt.Errorf("metadata clone must be integer: %w", err) } passwdDecoded, err := base64.StdEncoding.DecodeString(passwd) if err != nil { - return nil, errors.New("metadata auth provided is invalid, base64 encoded username:password required") + return nil, fmt.Errorf("metadata password is invalid: %w", err) } interval, err := time.ParseDuration(pollInterval) if err != nil { @@ -90,7 +90,8 @@ func (task *NGrinderTask) Run(ctx context.Context) bool { url := task.CloneAndStartEndpoint().String() result, err := task.request("POST", url, ctx) if err != nil { - task.logger.With("canary", task.canary).Errorf("failed to clone and start ngrinder test %s: %s", url, err.Error()) + task.logger.With("canary", task.canary). + Errorf("failed to clone and start ngrinder test %s: %s", url, err.Error()) return false } id := result["id"] @@ -134,26 +135,35 @@ func (task *NGrinderTask) PollStatus(ctx context.Context) bool { // send request, handle error, and eavl response json func (task *NGrinderTask) request(method, url string, ctx context.Context) (map[string]interface{}, error) { task.logger.Debugf("send %s request to %s", method, url) - req, _ := http.NewRequest(method, url, nil) + req, err := http.NewRequest(method, url, nil) + if err != nil { + return nil, fmt.Errorf("http.NewRequest failed: %w", err) + } + req.SetBasicAuth(task.username, task.passwd) if ctx != nil { req = req.WithContext(ctx) } resp, err := http.DefaultClient.Do(req) - if resp != nil { - defer resp.Body.Close() - } if err != nil { - task.logger.Errorf("bad request: %s", err.Error()) - return nil, err + task.logger.Errorf("request failed: %s", err.Error()) + return nil, fmt.Errorf("request failed: %w", err) } + defer resp.Body.Close() + respBytes, err := ioutil.ReadAll(resp.Body) - res := make(map[string]interface{}) - err = json.Unmarshal(respBytes, &res) if err != nil { - task.logger.Errorf("bad response, %s ,json expected:\n %s", err.Error(), string(respBytes)) - } else if success, ok := res["success"]; ok && success == false { - err = errors.New(res["message"].(string)) + return nil, fmt.Errorf("reading response body failed: %w", err) + } + + res := make(map[string]interface{}) + if err := json.Unmarshal(respBytes, &res); err != nil { + task.logger.Errorf("json unmarshalling failed: %s \n %s", err.Error(), string(respBytes)) + return nil, fmt.Errorf("json unmarshalling failed: %w for %s", err, string(respBytes)) + } + + if success, ok := res["success"]; ok && success == false { + return nil, fmt.Errorf("request failed: %s", string(respBytes)) } - return res, err + return res, nil } From ce89a2494753c2d9264f30811052b82577d1c3c4 Mon Sep 17 00:00:00 2001 From: mathetake Date: Sun, 8 Mar 2020 13:38:27 +0900 Subject: [PATCH 09/11] pkg/canary: refator error handling --- pkg/canary/deployment_controller.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/pkg/canary/deployment_controller.go b/pkg/canary/deployment_controller.go index ea5153d3f..41ff1b570 100644 --- a/pkg/canary/deployment_controller.go +++ b/pkg/canary/deployment_controller.go @@ -117,13 +117,12 @@ func (c *DeploymentController) Promote(cd *flaggerv1.Canary) error { // update HPA if cd.Spec.AutoscalerRef != nil { - switch cd.Spec.AutoscalerRef.Kind { - case "HorizontalPodAutoscaler": + if cd.Spec.AutoscalerRef.Kind == "HorizontalPodAutoscaler" { if err := c.reconcilePrimaryHpa(cd, false); err != nil { return fmt.Errorf( "reconcilePrimaryHpa for %s.%s failed: %w", primaryName, cd.Namespace, err) } - default: + } else { return fmt.Errorf("cd.Spec.AutoscalerRef.Kind is invalid: %s", cd.Spec.AutoscalerRef.Kind) } } From 22f860a3a38e32e5edf37a7147e57b8cd29abf56 Mon Sep 17 00:00:00 2001 From: mathetake Date: Sun, 8 Mar 2020 16:06:53 +0900 Subject: [PATCH 10/11] refactor pkg/controller --- cmd/flagger/main.go | 1 - pkg/controller/controller.go | 3 -- pkg/controller/scheduler.go | 32 +++++++++---------- .../scheduler_daemonset_fixture_test.go | 1 - .../scheduler_deployment_fixture_test.go | 1 - 5 files changed, 15 insertions(+), 23 deletions(-) diff --git a/cmd/flagger/main.go b/cmd/flagger/main.go index f12fa83c9..708ab504d 100644 --- a/cmd/flagger/main.go +++ b/cmd/flagger/main.go @@ -183,7 +183,6 @@ func main() { c := controller.NewController( kubeClient, - meshClient, flaggerClient, infos, controlLoopInterval, diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index 673dc5d15..0b92c458a 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -35,7 +35,6 @@ const controllerAgentName = "flagger" // Controller is managing the canary objects and schedules canary deployments type Controller struct { kubeClient kubernetes.Interface - istioClient clientset.Interface flaggerClient clientset.Interface flaggerInformers Informers flaggerSynced cache.InformerSynced @@ -62,7 +61,6 @@ type Informers struct { func NewController( kubeClient kubernetes.Interface, - istioClient clientset.Interface, flaggerClient clientset.Interface, flaggerInformers Informers, flaggerWindow time.Duration, @@ -89,7 +87,6 @@ func NewController( ctrl := &Controller{ kubeClient: kubeClient, - istioClient: istioClient, flaggerClient: flaggerClient, flaggerInformers: flaggerInformers, flaggerSynced: flaggerInformers.CanaryInformer.Informer().HasSynced, diff --git a/pkg/controller/scheduler.go b/pkg/controller/scheduler.go index 0a2004916..1dd9a1015 100644 --- a/pkg/controller/scheduler.go +++ b/pkg/controller/scheduler.go @@ -27,26 +27,26 @@ func (c *Controller) scheduleCanaries() { stats := make(map[string]int) c.canaries.Range(func(key interface{}, value interface{}) bool { - canary := value.(*flaggerv1.Canary) + cn := value.(*flaggerv1.Canary) // format: . name := key.(string) - current[name] = fmt.Sprintf("%s.%s", canary.Spec.TargetRef.Name, canary.Namespace) + current[name] = fmt.Sprintf("%s.%s", cn.Spec.TargetRef.Name, cn.Namespace) job, exists := c.jobs[name] // schedule new job for existing job with different analysis interval or non-existing job - if (exists && job.GetCanaryAnalysisInterval() != canary.GetAnalysisInterval()) || !exists { + if (exists && job.GetCanaryAnalysisInterval() != cn.GetAnalysisInterval()) || !exists { if exists { job.Stop() } newJob := CanaryJob{ - Name: canary.Name, - Namespace: canary.Namespace, + Name: cn.Name, + Namespace: cn.Namespace, function: c.advanceCanary, done: make(chan bool), - ticker: time.NewTicker(canary.GetAnalysisInterval()), - analysisInterval: canary.GetAnalysisInterval(), + ticker: time.NewTicker(cn.GetAnalysisInterval()), + analysisInterval: cn.GetAnalysisInterval(), } c.jobs[name] = newJob @@ -54,11 +54,11 @@ func (c *Controller) scheduleCanaries() { } // compute canaries per namespace total - t, ok := stats[canary.Namespace] + t, ok := stats[cn.Namespace] if !ok { - stats[canary.Namespace] = 1 + stats[cn.Namespace] = 1 } else { - stats[canary.Namespace] = t + 1 + stats[cn.Namespace] = t + 1 } return true }) @@ -75,7 +75,8 @@ func (c *Controller) scheduleCanaries() { for canaryName, targetName := range current { for name, target := range current { if name != canaryName && target == targetName { - c.logger.With("canary", canaryName).Errorf("Bad things will happen! Found more than one canary with the same target %s", targetName) + c.logger.With("canary", canaryName). + Errorf("Bad things will happen! Found more than one canary with the same target %s", targetName) } } } @@ -111,8 +112,8 @@ func (c *Controller) advanceCanary(name string, namespace string, skipLivenessCh } // init Kubernetes router - router := c.routerFactory.KubernetesRouter(cd.Spec.TargetRef.Kind, labelSelector, map[string]string{}, ports) - if err := router.Initialize(cd); err != nil { + kubeRouter := c.routerFactory.KubernetesRouter(cd.Spec.TargetRef.Kind, labelSelector, map[string]string{}, ports) + if err := kubeRouter.Initialize(cd); err != nil { c.recordEventWarningf(cd, "%v", err) return } @@ -128,7 +129,7 @@ func (c *Controller) advanceCanary(name string, namespace string, skipLivenessCh meshRouter := c.routerFactory.MeshRouter(provider) // create or update svc - if err := router.Reconcile(cd); err != nil { + if err := kubeRouter.Reconcile(cd); err != nil { c.recordEventWarningf(cd, "%v", err) return } @@ -206,7 +207,6 @@ func (c *Controller) advanceCanary(name string, namespace string, skipLivenessCh } if err := canaryController.SyncStatus(cd, status); err != nil { c.recordEventWarningf(cd, "%v", err) - return } return } @@ -305,7 +305,6 @@ func (c *Controller) advanceCanary(name string, namespace string, skipLivenessCh if ok := c.runPreRolloutHooks(cd); !ok { if err := canaryController.SetStatusFailedChecks(cd, cd.Status.FailedChecks+1); err != nil { c.recordEventWarningf(cd, "%v", err) - return } return } @@ -313,7 +312,6 @@ func (c *Controller) advanceCanary(name string, namespace string, skipLivenessCh if ok := c.runAnalysis(cd); !ok { if err := canaryController.SetStatusFailedChecks(cd, cd.Status.FailedChecks+1); err != nil { c.recordEventWarningf(cd, "%v", err) - return } return } diff --git a/pkg/controller/scheduler_daemonset_fixture_test.go b/pkg/controller/scheduler_daemonset_fixture_test.go index 8d2cdd6d1..65570ac8e 100644 --- a/pkg/controller/scheduler_daemonset_fixture_test.go +++ b/pkg/controller/scheduler_daemonset_fixture_test.go @@ -90,7 +90,6 @@ func newDaemonSetFixture(c *flaggerv1.Canary) daemonSetFixture { ctrl := &Controller{ kubeClient: kubeClient, - istioClient: flaggerClient, flaggerClient: flaggerClient, flaggerInformers: fi, flaggerSynced: fi.CanaryInformer.Informer().HasSynced, diff --git a/pkg/controller/scheduler_deployment_fixture_test.go b/pkg/controller/scheduler_deployment_fixture_test.go index 74a7321ef..be7c10dd6 100644 --- a/pkg/controller/scheduler_deployment_fixture_test.go +++ b/pkg/controller/scheduler_deployment_fixture_test.go @@ -92,7 +92,6 @@ func newDeploymentFixture(c *flaggerv1.Canary) fixture { ctrl := &Controller{ kubeClient: kubeClient, - istioClient: flaggerClient, flaggerClient: flaggerClient, flaggerInformers: fi, flaggerSynced: fi.CanaryInformer.Informer().HasSynced, From a32bd63edac42293ec7966aa6dc8eb6fd940a3e9 Mon Sep 17 00:00:00 2001 From: mathetake Date: Sun, 8 Mar 2020 16:30:50 +0900 Subject: [PATCH 11/11] pkg/metrics/providers/datadog: improve request failure error message --- pkg/metrics/providers/datadog.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pkg/metrics/providers/datadog.go b/pkg/metrics/providers/datadog.go index 6ef898b43..c71423ca1 100644 --- a/pkg/metrics/providers/datadog.go +++ b/pkg/metrics/providers/datadog.go @@ -105,7 +105,7 @@ func (p *DatadogProvider) RunQuery(query string) (float64, error) { defer cancel() r, err := http.DefaultClient.Do(req.WithContext(ctx)) if err != nil { - return 0, err + return 0, fmt.Errorf("request failed: %w", err) } defer r.Body.Close() @@ -155,8 +155,9 @@ func (p *DatadogProvider) IsOnline() (bool, error) { defer cancel() r, err := http.DefaultClient.Do(req.WithContext(ctx)) if err != nil { - return false, err + return false, fmt.Errorf("request failed: %w", err) } + defer r.Body.Close() b, err := ioutil.ReadAll(r.Body)