From 003c30410574879e6008562ac126bd7c1076fd74 Mon Sep 17 00:00:00 2001 From: Ta-Ching Chen Date: Mon, 2 Dec 2019 06:35:07 +0800 Subject: [PATCH] Prevent newdeploy updates deployment if no resources changed (#1452) We used to update timestamp in the deployment environment field in order to trigger a rolling update when the function referenced resources get updated. However, use timestamp means we are not able to avoid triggering a rolling update when executor tries to adopt orphaned deployment due to timestamp changed which is unwanted. In order to let executor adopt deployment without triggering a rolling update, we need an identical way to get a value that can reflect res- ources changed without affecting by time. To achieve this goal, the sum of the resource version of all referenced resources is a good fit for our scenario since the sum of the resource version is always the same as long as no resources changed. --- pkg/apis/fission.io/v1/const.go | 4 +- .../executortype/newdeploy/newdeploy.go | 65 ++++++++++++++++++- .../executortype/newdeploy/newdeploymgr.go | 15 +++-- 3 files changed, 74 insertions(+), 10 deletions(-) diff --git a/pkg/apis/fission.io/v1/const.go b/pkg/apis/fission.io/v1/const.go index 46215576..b85d77f1 100644 --- a/pkg/apis/fission.io/v1/const.go +++ b/pkg/apis/fission.io/v1/const.go @@ -22,8 +22,8 @@ const ( ) const ( - //LastUpdateTimestamp env variable is used for updating configmaps and secrets in pods - LastUpdateTimestamp string = "LASTUPDATE_TIMESTAMP" + // ResourceVersionCount env variable is used for updating configmaps and secrets in pods + ResourceVersionCount string = "RESOURCE_VERSION_COUNT" ) const ( diff --git a/pkg/executor/executortype/newdeploy/newdeploy.go b/pkg/executor/executortype/newdeploy/newdeploy.go index 44fd8e2f..3ebd46ba 100644 --- a/pkg/executor/executortype/newdeploy/newdeploy.go +++ b/pkg/executor/executortype/newdeploy/newdeploy.go @@ -18,6 +18,7 @@ package newdeploy import ( "fmt" + "strconv" "time" multierror "github.com/hashicorp/go-multierror" @@ -30,6 +31,7 @@ import ( "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/intstr" + "k8s.io/client-go/kubernetes" fv1 "github.com/fission/fission/pkg/apis/fission.io/v1" "github.com/fission/fission/pkg/executor/util" @@ -215,6 +217,11 @@ func (deploy *NewDeploy) getDeploymentSpec(fn *fv1.Function, env *fv1.Environmen // rollback, set RevisionHistoryLimit to 0 to disable this feature. revisionHistoryLimit := int32(0) + rvCount, err := referencedResourcesRVSum(deploy.kubernetesClient, fn.Metadata.Namespace, fn.Spec.Secrets, fn.Spec.ConfigMaps) + if err != nil { + return nil, err + } + container, err := util.MergeContainer(&apiv1.Container{ Name: fn.Metadata.Name, Image: env.Spec.Runtime.Image, @@ -232,8 +239,8 @@ func (deploy *NewDeploy) getDeploymentSpec(fn *fv1.Function, env *fv1.Environmen }, Env: []apiv1.EnvVar{ { - Name: fv1.LastUpdateTimestamp, - Value: time.Now().String(), + Name: fv1.ResourceVersionCount, + Value: fmt.Sprintf("%v", rvCount), }, }, // https://istio.io/docs/setup/kubernetes/additional-setup/requirements/ @@ -526,3 +533,57 @@ func (deploy *NewDeploy) cleanupNewdeploy(ns string, name string) error { return result.ErrorOrNil() } + +// referencedResourcesRVSum returns the sum of resource version of all resources the function references to. +// We used to update timestamp in the deployment environment field in order to trigger a rolling update when +// the function referenced resources get updated. However, use timestamp means we are not able to avoid tri- +// ggering a rolling update when executor tries to adopt orphaned deployment due to timestamp changed which +// is unwanted. In order to let executor adopt deployment without triggering a rolling update, we need an +// identical way to get a value that can reflect resources changed without affecting by the time. +// To achieve this goal, the sum of the resource version of all referenced resources is a good fit for our +// scenario since the sum of the resource version is always the same as long as no resources changed. +func referencedResourcesRVSum(client *kubernetes.Clientset, namespace string, secrets []fv1.SecretReference, cfgmaps []fv1.ConfigMapReference) (int, error) { + rvCount := 0 + + if len(secrets) > 0 { + list, err := client.CoreV1().Secrets(namespace).List(metav1.ListOptions{}) + if err != nil { + return 0, err + } + + objmap := make(map[string]apiv1.Secret) + for _, secret := range list.Items { + objmap[secret.Namespace+"/"+secret.Name] = secret + } + + for _, ref := range secrets { + s, ok := objmap[ref.Namespace+"/"+ref.Name] + if ok { + rv, _ := strconv.ParseInt(s.ResourceVersion, 10, 32) + rvCount += int(rv) + } + } + } + + if len(cfgmaps) > 0 { + list, err := client.CoreV1().ConfigMaps(namespace).List(metav1.ListOptions{}) + if err != nil { + return 0, err + } + + objmap := make(map[string]apiv1.ConfigMap) + for _, cfg := range list.Items { + objmap[cfg.Namespace+"/"+cfg.Name] = cfg + } + + for _, ref := range cfgmaps { + s, ok := objmap[ref.Namespace+"/"+ref.Name] + if ok { + rv, _ := strconv.ParseInt(s.ResourceVersion, 10, 32) + rvCount += int(rv) + } + } + } + + return rvCount, nil +} diff --git a/pkg/executor/executortype/newdeploy/newdeploymgr.go b/pkg/executor/executortype/newdeploy/newdeploymgr.go index 097cdcca..27577784 100644 --- a/pkg/executor/executortype/newdeploy/newdeploymgr.go +++ b/pkg/executor/executortype/newdeploy/newdeploymgr.go @@ -224,14 +224,17 @@ func (deploy *NewDeploy) RefreshFuncPods(logger *zap.Logger, f fv1.Function) err return err } - patch := fmt.Sprintf(`{"spec" : {"template": {"spec":{"containers":[{"name": "%s", "env":[{"name": "%s", "value": "%s"}]}]}}}}`, - f.Metadata.Name, - fv1.LastUpdateTimestamp, - time.Now().String()) - // Ideally there should be only one deployment but for now we rely on label/selector to ensure that condition for _, deployment := range dep.Items { - _, err := deploy.kubernetesClient.AppsV1().Deployments(deployment.ObjectMeta.Namespace).Patch(deployment.ObjectMeta.Name, + rvCount, err := referencedResourcesRVSum(deploy.kubernetesClient, deployment.Namespace, f.Spec.Secrets, f.Spec.ConfigMaps) + if err != nil { + return err + } + + patch := fmt.Sprintf(`{"spec" : {"template": {"spec":{"containers":[{"name": "%s", "env":[{"name": "%s", "value": "%v"}]}]}}}}`, + f.Metadata.Name, fv1.ResourceVersionCount, rvCount) + + _, err = deploy.kubernetesClient.AppsV1().Deployments(deployment.ObjectMeta.Namespace).Patch(deployment.ObjectMeta.Name, k8sTypes.StrategicMergePatchType, []byte(patch)) if err != nil {