From e69f4f7ba5f7404ae36353d87b726c6a6136846d Mon Sep 17 00:00:00 2001 From: Tomash Sidei <43379202+tomashibm@users.noreply.github.com> Date: Mon, 16 May 2022 14:30:59 +0300 Subject: [PATCH] Fix bug when VCL is not loaded when only the container and not the whole pod is restarted (#69) * Fix bug when VCL is not loaded when only the container and not the whole pod is restarted. * Fire an event and log the case when a backend pod is found but couldn't find the port definition we're looking for. * Fire an event if both a templated and not templated version of a VCL file is set in the VCL ConfigMap. Signed-off-by: Tomash Sidei Co-authored-by: Craig Ingram * Fix varnish pods watcher predicate. Remove endpoint selector env var config for varnish controller. Now the selector is taken from the CR we get from the API. That allows to avoid rolling upgrades when the backend selector has changed. --- api/v1alpha1/zz_generated.deepcopy.go | 2 +- .../controller/varnishcluster_statefulset.go | 2 -- .../varnishcluster_statefulset_test.go | 7 ---- pkg/varnishcontroller/config/config.go | 34 +++++++------------ .../controller/controller.go | 26 ++++++++------ pkg/varnishcontroller/controller/endpoints.go | 17 ++++++++-- pkg/varnishcontroller/events/events.go | 2 ++ 7 files changed, 46 insertions(+), 44 deletions(-) diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 22632825..5de49480 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -7,7 +7,7 @@ package v1alpha1 import ( appsv1 "k8s.io/api/apps/v1" - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" "k8s.io/api/policy/v1beta1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/intstr" diff --git a/pkg/varnishcluster/controller/varnishcluster_statefulset.go b/pkg/varnishcluster/controller/varnishcluster_statefulset.go index 305767ba..bd967e43 100644 --- a/pkg/varnishcluster/controller/varnishcluster_statefulset.go +++ b/pkg/varnishcluster/controller/varnishcluster_statefulset.go @@ -18,7 +18,6 @@ import ( v1 "k8s.io/api/core/v1" kerrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/intstr" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" @@ -218,7 +217,6 @@ func (r *ReconcileVarnishCluster) reconcileStatefulSet(ctx context.Context, inst }, }, Env: []v1.EnvVar{ - {Name: "ENDPOINT_SELECTOR_STRING", Value: labels.SelectorFromSet(endpointSelector).String()}, {Name: "NAMESPACE", Value: instance.Namespace}, {Name: "POD_NAME", ValueFrom: &v1.EnvVarSource{FieldRef: &v1.ObjectFieldSelector{APIVersion: "v1", FieldPath: "metadata.name"}}}, {Name: "NODE_NAME", ValueFrom: &v1.EnvVarSource{FieldRef: &v1.ObjectFieldSelector{APIVersion: "v1", FieldPath: "spec.nodeName"}}}, diff --git a/pkg/varnishcluster/controller/varnishcluster_statefulset_test.go b/pkg/varnishcluster/controller/varnishcluster_statefulset_test.go index f0a6f29d..830ba1e5 100644 --- a/pkg/varnishcluster/controller/varnishcluster_statefulset_test.go +++ b/pkg/varnishcluster/controller/varnishcluster_statefulset_test.go @@ -9,8 +9,6 @@ import ( vcapi "github.com/ibm/varnish-operator/api/v1alpha1" - "k8s.io/apimachinery/pkg/labels" - "github.com/gogo/protobuf/proto" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -117,11 +115,6 @@ var _ = Describe("statefulset", func() { varnishControllerContainer, err := getContainerByName(podSpec, vcapi.VarnishControllerName) Expect(err).ToNot(HaveOccurred()) Expect(varnishControllerContainer.Image).To(Equal("ibmcom/varnish-controller:test")) - Expect(varnishControllerContainer.Env).To(ContainElement(v1.EnvVar{Name: "ENDPOINT_SELECTOR_STRING", Value: labels.SelectorFromSet(map[string]string{ - vcapi.LabelVarnishOwner: vcName, - vcapi.LabelVarnishComponent: vcapi.VarnishComponentNoCacheService, - vcapi.LabelVarnishUID: string(newVC.UID), - }).String()})) Expect(varnishControllerContainer.Env).To(ContainElement(v1.EnvVar{Name: "NAMESPACE", Value: vcNamespace})) Expect(varnishControllerContainer.Env).To(ContainElement(v1.EnvVar{Name: "POD_NAME", ValueFrom: &v1.EnvVarSource{FieldRef: &v1.ObjectFieldSelector{APIVersion: "v1", FieldPath: "metadata.name"}}})) Expect(varnishControllerContainer.Env).To(ContainElement(v1.EnvVar{Name: "VARNISH_CLUSTER_NAME", Value: vcName})) diff --git a/pkg/varnishcontroller/config/config.go b/pkg/varnishcontroller/config/config.go index 0a4d88af..18470a0d 100644 --- a/pkg/varnishcontroller/config/config.go +++ b/pkg/varnishcontroller/config/config.go @@ -5,7 +5,6 @@ import ( "strconv" "time" - "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/types" "github.com/caarlos0/env/v6" @@ -17,21 +16,19 @@ const VCLConfigDir = "/etc/varnish" // Config that reads in env variables type Config struct { - EndpointSelectorString string `env:"ENDPOINT_SELECTOR_STRING,required"` - Namespace string `env:"NAMESPACE,required"` - PodName string `env:"POD_NAME,required"` - NodeName string `env:"NODE_NAME,required"` - VarnishClusterName string `env:"VARNISH_CLUSTER_NAME,required"` - VarnishClusterUID types.UID `env:"VARNISH_CLUSTER_UID,required"` - VarnishClusterGroup string `env:"VARNISH_CLUSTER_GROUP,required"` - VarnishClusterVersion string `env:"VARNISH_CLUSTER_VERSION,required"` - VarnishClusterKind string `env:"VARNISH_CLUSTER_KIND,required"` - VarnishAdmArgs []string `env:"VARNISHADM_ARGS" envDefault:"-S /etc/varnish-secret/secret -T 127.0.0.1:6082" envSeparator:" " ` - VarnishPingTimeout time.Duration `env:"VARNISHADM_PING_TIMEOUT" envDefault:"90s"` - VarnishPingDelay time.Duration `env:"VARNISHADM_PING_DELAY" envDefault:"200ms"` - LogFormat string `env:"LOG_FORMAT,required"` - LogLevel zapcore.Level `env:"LOG_LEVEL,required"` - EndpointSelector labels.Selector + Namespace string `env:"NAMESPACE,required"` + PodName string `env:"POD_NAME,required"` + NodeName string `env:"NODE_NAME,required"` + VarnishClusterName string `env:"VARNISH_CLUSTER_NAME,required"` + VarnishClusterUID types.UID `env:"VARNISH_CLUSTER_UID,required"` + VarnishClusterGroup string `env:"VARNISH_CLUSTER_GROUP,required"` + VarnishClusterVersion string `env:"VARNISH_CLUSTER_VERSION,required"` + VarnishClusterKind string `env:"VARNISH_CLUSTER_KIND,required"` + VarnishAdmArgs []string `env:"VARNISHADM_ARGS" envDefault:"-S /etc/varnish-secret/secret -T 127.0.0.1:6082" envSeparator:" " ` + VarnishPingTimeout time.Duration `env:"VARNISHADM_PING_TIMEOUT" envDefault:"90s"` + VarnishPingDelay time.Duration `env:"VARNISHADM_PING_DELAY" envDefault:"200ms"` + LogFormat string `env:"LOG_FORMAT,required"` + LogLevel zapcore.Level `env:"LOG_LEVEL,required"` } // Load uses the caarlos0/env library to read in environment variables into a struct @@ -63,10 +60,5 @@ func Load() (*Config, error) { return &c, errors.WithStack(err) } - c.EndpointSelector, err = labels.Parse(c.EndpointSelectorString) - if err != nil { - return &c, errors.Wrapf(err, "could not parse endpoint selector: %s", c.EndpointSelectorString) - } - return &c, nil } diff --git a/pkg/varnishcontroller/controller/controller.go b/pkg/varnishcontroller/controller/controller.go index 55f04eac..59159f0f 100644 --- a/pkg/varnishcontroller/controller/controller.go +++ b/pkg/varnishcontroller/controller/controller.go @@ -2,6 +2,7 @@ package controller import ( "context" + "fmt" "strings" "time" @@ -44,13 +45,10 @@ type PodInfo struct { // SetupVarnishReconciler creates a new VarnishCluster Controller and adds it to the Manager with default RBAC. The Manager will set fields on the Controller // and Start it when the Manager is Started. func SetupVarnishReconciler(mgr manager.Manager, cfg *config.Config, varnish varnishadm.VarnishAdministrator, metrics *metrics.VarnishControllerMetrics, logr *logger.Logger) error { - backendsLabels, err := labels.ConvertSelectorToLabelsMap(cfg.EndpointSelectorString) - if err != nil { - return err - } - + // stub, backends selector will be set and updated on reconcile + backendsSelector := labels.SelectorFromSet(labels.Set{}) backendNamespacePredicate := predicates.NewNamespacesMatcherPredicate([]string{cfg.Namespace}, logr) - backendLabelsPredicate := predicates.NewLabelMatcherPredicate(backendsLabels.AsSelector(), logr) + backendLabelsPredicate := predicates.NewLabelMatcherPredicate(backendsSelector, logr) r := &ReconcileVarnish{ config: cfg, @@ -90,7 +88,7 @@ func SetupVarnishReconciler(mgr manager.Manager, cfg *config.Config, varnish var varnishPodsSelector := labels.SelectorFromSet(labels.Set{ v1alpha1.LabelVarnishOwner: cfg.VarnishClusterName, - v1alpha1.LabelVarnishComponent: v1alpha1.VarnishComponentCacheService, + v1alpha1.LabelVarnishComponent: v1alpha1.VarnishComponentVarnish, v1alpha1.LabelVarnishUID: string(cfg.VarnishClusterUID), }) builder.Watches( @@ -202,8 +200,10 @@ func (r *ReconcileVarnish) reconcileWithContext(ctx context.Context, request rec for fileName, contents := range templatizedFiles { if _, found := newFiles[fileName]; found { - // TODO: probably want to create event for this - return reconcile.Result{}, errors.Errorf("ConfigMap has %s and %s.tmpl entries. Cannot include file and template with same name", fileName, fileName) + errMsg := fmt.Sprintf("VCL ConfigMap %s has %s and %s.tmpl entries. Cannot include file and template with same name", + *vc.Spec.VCL.ConfigMapName, fileName, fileName) + r.eventHandler.Warning(vc, events.EventReasonInvalidVCLConfigMap, errMsg) + return reconcile.Result{}, errors.Errorf(errMsg) } newFiles[fileName] = contents } @@ -218,7 +218,13 @@ func (r *ReconcileVarnish) reconcileWithContext(ctx context.Context, request rec return reconcile.Result{}, errors.WithStack(err) } - if filesTouched { + configName, err := r.varnish.GetActiveConfigurationName() + if err != nil { + return reconcile.Result{}, err + } + + // reload if files changed, or we didn't load the VCL yet (happens when only the container restarted and not the whole pod) + if filesTouched || configName == "boot" { if err = r.reconcileVarnish(ctx, vc, pod, cm); err != nil { return reconcile.Result{}, errors.WithStack(err) } diff --git a/pkg/varnishcontroller/controller/endpoints.go b/pkg/varnishcontroller/controller/endpoints.go index 99726951..90ffd337 100644 --- a/pkg/varnishcontroller/controller/endpoints.go +++ b/pkg/varnishcontroller/controller/endpoints.go @@ -2,8 +2,11 @@ package controller import ( "context" + "fmt" "sort" + "github.com/ibm/varnish-operator/pkg/varnishcontroller/events" + "github.com/ibm/varnish-operator/api/v1alpha1" vclabels "github.com/ibm/varnish-operator/pkg/labels" @@ -36,7 +39,7 @@ func (r *ReconcileVarnish) getBackendEndpoints(ctx context.Context, vc *v1alpha1 namespaces = vc.Spec.Backend.Namespaces } - backendList, portNumber, err := r.getPodsInfo(ctx, namespaces, labels.SelectorFromSet(vc.Spec.Backend.Selector), *vc.Spec.Backend.Port) + backendList, portNumber, err := r.getPodsInfo(ctx, vc, namespaces, labels.SelectorFromSet(vc.Spec.Backend.Selector), *vc.Spec.Backend.Port) if err != nil { return nil, 0, 0, 0, errors.WithStack(err) } @@ -102,7 +105,7 @@ func (r *ReconcileVarnish) getVarnishEndpoints(ctx context.Context, vc *v1alpha1 varnishLables := labels.SelectorFromSet(vclabels.CombinedComponentLabels(vc, v1alpha1.VarnishComponentVarnish)) varnishPort := intstr.FromString(v1alpha1.VarnishPortName) - varnishEndpoints, _, err := r.getPodsInfo(ctx, []string{r.config.Namespace}, varnishLables, varnishPort) + varnishEndpoints, _, err := r.getPodsInfo(ctx, vc, []string{r.config.Namespace}, varnishLables, varnishPort) if err != nil { return nil, errors.WithStack(err) } @@ -110,7 +113,7 @@ func (r *ReconcileVarnish) getVarnishEndpoints(ctx context.Context, vc *v1alpha1 return varnishEndpoints, nil } -func (r *ReconcileVarnish) getPodsInfo(ctx context.Context, namespaces []string, labels labels.Selector, validPort intstr.IntOrString) ([]PodInfo, int32, error) { +func (r *ReconcileVarnish) getPodsInfo(ctx context.Context, vc *v1alpha1.VarnishCluster, namespaces []string, labels labels.Selector, validPort intstr.IntOrString) ([]PodInfo, int32, error) { var pods []v1.Pod for _, namespace := range namespaces { listOptions := []client.ListOption{ @@ -139,9 +142,11 @@ func (r *ReconcileVarnish) getPodsInfo(ctx context.Context, namespaces []string, continue } + portFound := false for _, container := range pod.Spec.Containers { for _, containerPort := range container.Ports { if containerPort.ContainerPort == validPort.IntVal || containerPort.Name == validPort.StrVal { + portFound = true portNumber = containerPort.ContainerPort var backendWeight = 1.0 nodeLabels, err := r.getNodeLabels(ctx, pod.Spec.NodeName) @@ -154,6 +159,12 @@ func (r *ReconcileVarnish) getPodsInfo(ctx context.Context, namespaces []string, } } } + + if !portFound { + errMsg := fmt.Sprintf("Backend pod %s/%s ignored since none of its containers have port %q defined", pod.Namespace, pod.Name, validPort.String()) + r.eventHandler.Warning(vc, events.EventReasonBackendIgnored, errMsg) + r.logger.Warnf(errMsg) + } } // sort slices so they also look the same for the code using it diff --git a/pkg/varnishcontroller/events/events.go b/pkg/varnishcontroller/events/events.go index 742e2663..bee560ab 100644 --- a/pkg/varnishcontroller/events/events.go +++ b/pkg/varnishcontroller/events/events.go @@ -11,6 +11,8 @@ const ( EventReasonReloadError EventReason = "ReloadError" EventReasonVCLCompilationError EventReason = "VCLCompilationError" + EventReasonInvalidVCLConfigMap EventReason = "InvalidVCLConfigMap" + EventReasonBackendIgnored EventReason = "BackendIgnored" annotationSourcePod string = "sourcePod" )