From a346e62b86d2e8d28aad72cd7be33a13d0049838 Mon Sep 17 00:00:00 2001 From: Filip Strozik Date: Mon, 29 Apr 2024 13:53:21 +0200 Subject: [PATCH] Improve fn tag calculation (#929) --- .../serverless/build_resources_test.go | 22 ++++++++- .../controllers/serverless/configmap.go | 2 +- .../controllers/serverless/deployment.go | 2 +- .../internal/controllers/serverless/fsm.go | 5 +- .../serverless/function_reconcile.go | 36 +++++++++++++- .../function_reconcile_asserts_test.go | 2 +- .../function_reconcile_gitops_test.go | 4 +- .../internal/controllers/serverless/job.go | 35 ++------------ .../controllers/serverless/system_state.go | 24 +++++----- .../internal/controllers/serverless/utils.go | 6 ++- .../controllers/serverless/utils_test.go | 48 +++++++++++-------- 11 files changed, 109 insertions(+), 77 deletions(-) diff --git a/components/serverless/internal/controllers/serverless/build_resources_test.go b/components/serverless/internal/controllers/serverless/build_resources_test.go index cff0c89e2..941f03902 100644 --- a/components/serverless/internal/controllers/serverless/build_resources_test.go +++ b/components/serverless/internal/controllers/serverless/build_resources_test.go @@ -102,7 +102,16 @@ func TestFunctionReconciler_buildDeployment(t *testing.T) { instance: *tt.args.instance, } - got := s.buildDeployment(buildDeploymentArgs{}, resourceCfg) + got := s.buildDeployment(buildDeploymentArgs{}, cfg{ + runtimeBaseImage: "some_image", + fn: FunctionConfig{ + ResourceConfig: ResourceConfig{ + Function: FunctionResourceConfig{ + Resources: resourceCfg, + }, + }, + }, + }) // deployment selector labels are equal with pod labels for key, value := range got.Spec.Selector.MatchLabels { @@ -204,7 +213,16 @@ func TestFunctionReconciler_buildDeploymentWithResources(t *testing.T) { t.Run(tt.name, func(t *testing.T) { s := systemState{instance: *tt.args.instance} - got := s.buildDeployment(buildDeploymentArgs{}, resourceCfg) + got := s.buildDeployment(buildDeploymentArgs{}, cfg{ + runtimeBaseImage: "some_image", + fn: FunctionConfig{ + ResourceConfig: ResourceConfig{ + Function: FunctionResourceConfig{ + Resources: resourceCfg, + }, + }, + }, + }) require.Len(t, got.Spec.Template.Spec.Containers, 1) require.Equal(t, tt.args.expectedResources, got.Spec.Template.Spec.Containers[0].Resources) diff --git a/components/serverless/internal/controllers/serverless/configmap.go b/components/serverless/internal/controllers/serverless/configmap.go index d0fdb7270..123780f54 100644 --- a/components/serverless/internal/controllers/serverless/configmap.go +++ b/components/serverless/internal/controllers/serverless/configmap.go @@ -26,7 +26,7 @@ func stateFnInlineCheckSources(ctx context.Context, r *reconciler, s *systemStat return nil, errors.Wrap(err, "while listing deployments") } - srcChanged := s.inlineFnSrcChanged(r.cfg.docker.PullAddress) + srcChanged := s.inlineFnSrcChanged(r.cfg.docker.PullAddress, r.cfg.runtimeBaseImage) if !srcChanged { cfgStatus := getConditionStatus(s.instance.Status.Conditions, serverlessv1alpha2.ConditionConfigurationReady) if cfgStatus == corev1.ConditionTrue { diff --git a/components/serverless/internal/controllers/serverless/deployment.go b/components/serverless/internal/controllers/serverless/deployment.go index 4d8f27b3b..0ffe17325 100644 --- a/components/serverless/internal/controllers/serverless/deployment.go +++ b/components/serverless/internal/controllers/serverless/deployment.go @@ -46,7 +46,7 @@ func stateFnCheckDeployments(ctx context.Context, r *reconciler, s *systemState) ImagePullAccountName: r.cfg.fn.ImagePullAccountName, } - expectedDeployment := s.buildDeployment(args, r.cfg.fn.ResourceConfig.Function.Resources) + expectedDeployment := s.buildDeployment(args, r.cfg) if len(s.deployments.Items) == 0 { return buildStateFnCreateDeployment(expectedDeployment), nil } diff --git a/components/serverless/internal/controllers/serverless/fsm.go b/components/serverless/internal/controllers/serverless/fsm.go index f98fc9587..ce0398f32 100644 --- a/components/serverless/internal/controllers/serverless/fsm.go +++ b/components/serverless/internal/controllers/serverless/fsm.go @@ -31,8 +31,9 @@ type k8s struct { } type cfg struct { - docker DockerConfig - fn FunctionConfig + runtimeBaseImage string + docker DockerConfig + fn FunctionConfig } // nolint diff --git a/components/serverless/internal/controllers/serverless/function_reconcile.go b/components/serverless/internal/controllers/serverless/function_reconcile.go index 7b589311e..a66f5c5a2 100644 --- a/components/serverless/internal/controllers/serverless/function_reconcile.go +++ b/components/serverless/internal/controllers/serverless/function_reconcile.go @@ -2,6 +2,9 @@ package serverless import ( "context" + "fmt" + "regexp" + "strings" "time" "github.com/pkg/errors" @@ -10,6 +13,7 @@ import ( appsv1 "k8s.io/api/apps/v1" autoscalingv1 "k8s.io/api/autoscaling/v1" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/tools/record" "k8s.io/client-go/util/workqueue" ctrl "sigs.k8s.io/controller-runtime" @@ -138,6 +142,16 @@ func (r *FunctionReconciler) Reconcile(ctx context.Context, request ctrl.Request contextLogger.Debug("starting state machine") + runtime := instance.Status.Runtime + if runtime == "" { + runtime = instance.Spec.Runtime + } + + latestRuntimeImage, err := r.getRuntimeImageFromConfigMap(ctx, instance.GetNamespace(), runtime) + if err != nil { + return ctrl.Result{}, errors.Wrap(err, "failed to fetch runtime image from config map") + } + stateReconciler := reconciler{ fn: r.initStateFunction, log: contextLogger, @@ -147,8 +161,9 @@ func (r *FunctionReconciler) Reconcile(ctx context.Context, request ctrl.Request statsCollector: r.statsCollector, }, cfg: cfg{ - fn: r.config, - docker: dockerCfg, + fn: r.config, + docker: dockerCfg, + runtimeBaseImage: latestRuntimeImage, }, gitClient: r.gitFactory.GetGitClient(contextLogger), } @@ -205,3 +220,20 @@ func (r *FunctionReconciler) readDockerConfig(ctx context.Context, instance *ser } } + +func (r *FunctionReconciler) getRuntimeImageFromConfigMap(ctx context.Context, namespace string, runtime serverlessv1alpha2.Runtime) (string, error) { + instance := &corev1.ConfigMap{} + dockerfileConfigMapName := fmt.Sprintf("dockerfile-%s", runtime) + err := r.client.Get(ctx, types.NamespacedName{Namespace: namespace, Name: dockerfileConfigMapName}, instance) + if err != nil { + return "", errors.Wrap(err, "while extracting correct config map for given runtime") + } + baseImage := instance.Data["Dockerfile"] + re := regexp.MustCompile(`base_image=.*`) + matchedLines := re.FindStringSubmatch(baseImage) + if len(matchedLines) == 0 { + return "", errors.Errorf("could not find the base image from %s", dockerfileConfigMapName) + } + runtimeImage := strings.TrimPrefix(matchedLines[0], "base_image=") + return runtimeImage, err +} diff --git a/components/serverless/internal/controllers/serverless/function_reconcile_asserts_test.go b/components/serverless/internal/controllers/serverless/function_reconcile_asserts_test.go index 53740cbbd..281db048b 100644 --- a/components/serverless/internal/controllers/serverless/function_reconcile_asserts_test.go +++ b/components/serverless/internal/controllers/serverless/function_reconcile_asserts_test.go @@ -119,7 +119,7 @@ func assertSuccessfulFunctionDeployment(t *testing.T, resourceClient resource.Cl instance: *function, } - g.Expect(deployment.Spec.Template.Spec.Containers[0].Image).To(gomega.Equal(s.buildImageAddress(regPullAddr))) + g.Expect(deployment.Spec.Template.Spec.Containers[0].Image).To(gomega.Equal(s.buildImageAddress(regPullAddr, "some_image"))) g.Expect(deployment.Spec.Template.Labels).To(gomega.HaveLen(8)) g.Expect(deployment.Spec.Template.Labels[serverlessv1alpha2.FunctionNameLabel]).To(gomega.Equal(function.Name)) g.Expect(deployment.Spec.Template.Labels[serverlessv1alpha2.PodAppNameLabel]).To(gomega.Equal(function.Name)) diff --git a/components/serverless/internal/controllers/serverless/function_reconcile_gitops_test.go b/components/serverless/internal/controllers/serverless/function_reconcile_gitops_test.go index 9a59cef78..a41e1ef08 100644 --- a/components/serverless/internal/controllers/serverless/function_reconcile_gitops_test.go +++ b/components/serverless/internal/controllers/serverless/function_reconcile_gitops_test.go @@ -319,7 +319,7 @@ func TestGitOpsWithContinuousGitCheckout(t *testing.T) { instance: *function, } - expectedImage := s.buildImageAddress("localhost:32132") + expectedImage := s.buildImageAddress("localhost:32132", "some_image") g.Expect(deployment).To(gomega.Not(gomega.BeNil())) g.Expect(deployment).To(haveSpecificContainer0Image(expectedImage)) g.Expect(deployment).To(haveLabelLen(8)) @@ -567,7 +567,7 @@ func TestGitOpsWithoutContinuousGitCheckout(t *testing.T) { instance: *function, } - expectedImage := s.buildImageAddress("localhost:32132") + expectedImage := s.buildImageAddress("localhost:32132", "some_image") g.Expect(deployment).To(gomega.Not(gomega.BeNil())) g.Expect(deployment).To(haveSpecificContainer0Image(expectedImage)) g.Expect(deployment).To(haveLabelLen(8)) diff --git a/components/serverless/internal/controllers/serverless/job.go b/components/serverless/internal/controllers/serverless/job.go index ec4f75beb..d177e8f8b 100644 --- a/components/serverless/internal/controllers/serverless/job.go +++ b/components/serverless/internal/controllers/serverless/job.go @@ -3,15 +3,12 @@ package serverless import ( "context" "fmt" - "regexp" - "strings" "time" batchv1 "k8s.io/api/batch/v1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" apilabels "k8s.io/apimachinery/pkg/labels" - "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" serverlessv1alpha2 "github.com/kyma-project/serverless/components/serverless/pkg/apis/serverless/v1alpha2" @@ -68,7 +65,7 @@ func buildStateFnCheckImageJob(expectedJob batchv1.Job) stateFn { return buildStatusUpdateStateFnWithCondition(condition), nil } - s.fnImage = s.buildImageAddress(r.cfg.docker.PullAddress) + s.fnImage = s.buildImageAddress(r.cfg.docker.PullAddress, r.cfg.runtimeBaseImage) diffRuntimeImage, err := functionRuntimeChanged(ctx, r, s) if err != nil { @@ -108,11 +105,7 @@ func functionRuntimeChanged(ctx context.Context, r *reconciler, s *systemState) return !result, nil } - latestRuntimeImage, err := getRuntimeImageFromConfigMap(ctx, r, s) - if err != nil { - return false, errors.Wrap(err, "while fetching runtime image from config map") - } - result := latestRuntimeImage == functionRuntimeImage + result := r.cfg.runtimeBaseImage == functionRuntimeImage return !result, nil } @@ -141,11 +134,6 @@ func buildStateFnRunJob(expectedJob batchv1.Job) stateFn { return nil, errors.Wrap(err, "while creating job") } - runtimeImage, err := getRuntimeImageFromConfigMap(ctx, r, s) - if err != nil { - return nil, errors.Wrap(err, "while extracting runtime fn-image from config map") - } - condition := serverlessv1alpha2.Condition{ Type: serverlessv1alpha2.ConditionBuildReady, Status: corev1.ConditionUnknown, @@ -154,28 +142,11 @@ func buildStateFnRunJob(expectedJob batchv1.Job) stateFn { Message: fmt.Sprintf("Job %s created", expectedJob.GetName()), } - s.instance.Status.RuntimeImage = runtimeImage + s.instance.Status.RuntimeImage = r.cfg.runtimeBaseImage return buildStatusUpdateStateFnWithCondition(condition), nil } } -func getRuntimeImageFromConfigMap(ctx context.Context, r *reconciler, s *systemState) (string, error) { - instance := &corev1.ConfigMap{} - dockerfileConfigMapName := fmt.Sprintf("dockerfile-%s", s.instance.Status.Runtime) - err := r.client.Get(ctx, types.NamespacedName{Namespace: s.instance.Namespace, Name: dockerfileConfigMapName}, instance) - if err != nil { - return "", errors.Wrap(err, "while extracting correct config map for given runtime") - } - baseImage := instance.Data["Dockerfile"] - re := regexp.MustCompile(`base_image=.*`) - matchedLines := re.FindStringSubmatch(baseImage) - if len(matchedLines) == 0 { - return "", errors.Errorf("could not find the base image from %s", dockerfileConfigMapName) - } - runtimeImage := strings.TrimPrefix(matchedLines[0], "base_image=") - return runtimeImage, err -} - func stateFnInlineDeleteJobs(ctx context.Context, r *reconciler, s *systemState) (stateFn, error) { r.log.Info("delete Jobs") diff --git a/components/serverless/internal/controllers/serverless/system_state.go b/components/serverless/internal/controllers/serverless/system_state.go index d25660051..680b0a497 100644 --- a/components/serverless/internal/controllers/serverless/system_state.go +++ b/components/serverless/internal/controllers/serverless/system_state.go @@ -66,20 +66,20 @@ func prometheusSvcAnnotations() map[string]string { "prometheus.io/scrape": "true", } } -func (s *systemState) buildImageAddress(registryAddress string) string { +func (s *systemState) buildImageAddress(registryAddress string, runtimeBase string) string { var imageTag string isGitType := s.instance.TypeOf(serverlessv1alpha2.FunctionTypeGit) if isGitType { - imageTag = calculateGitImageTag(&s.instance) + imageTag = calculateGitImageTag(&s.instance, runtimeBase) } else { - imageTag = calculateInlineImageTag(&s.instance) + imageTag = calculateInlineImageTag(&s.instance, runtimeBase) } return fmt.Sprintf("%s/%s-%s:%s", registryAddress, s.instance.Namespace, s.instance.Name, imageTag) } // TODO to self - create issue to refactor this -func (s *systemState) inlineFnSrcChanged(dockerPullAddress string) bool { - image := s.buildImageAddress(dockerPullAddress) +func (s *systemState) inlineFnSrcChanged(dockerPullAddress string, runtimeBase string) bool { + image := s.buildImageAddress(dockerPullAddress, runtimeBase) configurationStatus := getConditionStatus(s.instance.Status.Conditions, serverlessv1alpha2.ConditionConfigurationReady) rtm := fnRuntime.GetRuntime(s.instance.Spec.Runtime) fnLabels := s.functionLabels() @@ -240,7 +240,7 @@ func (s *systemState) buildGitJobRepoFetcherContainer(gitOptions git.Options, cf } func (s *systemState) buildJobExecutorContainer(cfg cfg, volumeMounts []corev1.VolumeMount) corev1.Container { - imageName := s.buildImageAddress(cfg.docker.PushAddress) + imageName := s.buildImageAddress(cfg.docker.PushAddress, cfg.runtimeBaseImage) args := append(cfg.fn.Build.ExecutorArgs, fmt.Sprintf("%s=%s", destinationArg, imageName), fmt.Sprintf("--context=dir://%s", workspaceMountPath)) @@ -414,8 +414,8 @@ type buildDeploymentArgs struct { ImagePullAccountName string } -func (s *systemState) buildDeployment(cfg buildDeploymentArgs, resourceConfig Resources) appsv1.Deployment { - imageName := s.buildImageAddress(cfg.DockerPullAddress) +func (s *systemState) buildDeployment(args buildDeploymentArgs, cfg cfg) appsv1.Deployment { + imageName := s.buildImageAddress(args.DockerPullAddress, cfg.runtimeBaseImage) const volumeName = "tmp-dir" emptyDirVolumeSize := resource.MustParse("100Mi") @@ -427,8 +427,8 @@ func (s *systemState) buildDeployment(cfg buildDeploymentArgs, resourceConfig Re deploymentEnvs := buildDeploymentEnvs( s.instance.GetName(), s.instance.GetNamespace(), - cfg.TraceCollectorEndpoint, - cfg.PublisherProxyAddress, + args.TraceCollectorEndpoint, + args.PublisherProxyAddress, ) envs = append(envs, deploymentEnvs...) @@ -468,7 +468,7 @@ func (s *systemState) buildDeployment(cfg buildDeploymentArgs, resourceConfig Re Name: functionContainerName, Image: imageName, Env: envs, - Resources: getDeploymentResources(s.instance, resourceConfig), + Resources: getDeploymentResources(s.instance, cfg.fn.ResourceConfig.Function.Resources), VolumeMounts: volumeMounts, /* In order to mark pod as ready we need to ensure the function is actually running and ready to serve traffic. @@ -514,7 +514,7 @@ func (s *systemState) buildDeployment(cfg buildDeploymentArgs, resourceConfig Re SecurityContext: restrictiveContainerSecurityContext(), }, }, - ServiceAccountName: cfg.ImagePullAccountName, + ServiceAccountName: args.ImagePullAccountName, } enrichPodSpecWithSecurityContext(&templateSpec, functionUser, functionUserGroup) diff --git a/components/serverless/internal/controllers/serverless/utils.go b/components/serverless/internal/controllers/serverless/utils.go index cc466454e..b35a326e0 100644 --- a/components/serverless/internal/controllers/serverless/utils.go +++ b/components/serverless/internal/controllers/serverless/utils.go @@ -363,22 +363,24 @@ func getCondition(conditions []serverlessv1alpha2.Condition, conditionType serve return serverlessv1alpha2.Condition{} } -func calculateInlineImageTag(instance *serverlessv1alpha2.Function) string { +func calculateInlineImageTag(instance *serverlessv1alpha2.Function, runtimeBase string) string { hash := sha256.Sum256([]byte(strings.Join([]string{ string(instance.GetUID()), fmt.Sprintf("%v", *instance.Spec.Source.Inline), instance.EffectiveRuntime(), + runtimeBase, }, "-"))) return fmt.Sprintf("%x", hash) } -func calculateGitImageTag(instance *serverlessv1alpha2.Function) string { +func calculateGitImageTag(instance *serverlessv1alpha2.Function, runtimeBase string) string { data := strings.Join([]string{ string(instance.GetUID()), instance.Status.Commit, instance.Status.BaseDir, instance.EffectiveRuntime(), + runtimeBase, }, "-") hash := sha256.Sum256([]byte(data)) return fmt.Sprintf("%x", hash) diff --git a/components/serverless/internal/controllers/serverless/utils_test.go b/components/serverless/internal/controllers/serverless/utils_test.go index 6e1f3838e..8f6cd9901 100644 --- a/components/serverless/internal/controllers/serverless/utils_test.go +++ b/components/serverless/internal/controllers/serverless/utils_test.go @@ -10,12 +10,14 @@ import ( func Test_calculateGitImageTag(t *testing.T) { tests := []struct { - name string - fn *serverlessv1alpha2.Function - want string + name string + fn *serverlessv1alpha2.Function + baseImage string + want string }{ { - name: "should use runtime", + name: "should use runtime", + baseImage: "nodejs18:test", fn: &serverlessv1alpha2.Function{ ObjectMeta: metav1.ObjectMeta{ UID: "fn-uuid", @@ -30,10 +32,11 @@ func Test_calculateGitImageTag(t *testing.T) { Runtime: "nodejs20", }, }, - want: "5e62e84b27afdcf23e9ea682a8ce44b693c4a3258e5b26bd038c60cd41eb60ee", + want: "5093e1e9a1b0c94c513bbec23b8291240ba988872353a024d1b0d5b2901d421c", }, { - name: "should use runtimeOverride", + name: "should use runtimeOverride", + baseImage: "nodejs18:test", fn: &serverlessv1alpha2.Function{ ObjectMeta: metav1.ObjectMeta{ UID: "fn-uuid", @@ -49,10 +52,11 @@ func Test_calculateGitImageTag(t *testing.T) { RuntimeImageOverride: "nodejs20", }, }, - want: "5e62e84b27afdcf23e9ea682a8ce44b693c4a3258e5b26bd038c60cd41eb60ee", + want: "5093e1e9a1b0c94c513bbec23b8291240ba988872353a024d1b0d5b2901d421c", }, { - name: "should use runtime when runtimeOverride is empty", + name: "should use runtime when runtimeOverride is empty", + baseImage: "nodejs18:test", fn: &serverlessv1alpha2.Function{ ObjectMeta: metav1.ObjectMeta{ UID: "fn-uuid", @@ -68,24 +72,26 @@ func Test_calculateGitImageTag(t *testing.T) { RuntimeImageOverride: "", }, }, - want: "5e62e84b27afdcf23e9ea682a8ce44b693c4a3258e5b26bd038c60cd41eb60ee", + want: "5093e1e9a1b0c94c513bbec23b8291240ba988872353a024d1b0d5b2901d421c", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - assert.Equal(t, tt.want, calculateGitImageTag(tt.fn)) + assert.Equal(t, tt.want, calculateGitImageTag(tt.fn, tt.baseImage)) }) } } func Test_calculateInlineImageTag(t *testing.T) { tests := []struct { - name string - fn *serverlessv1alpha2.Function - want string + name string + fn *serverlessv1alpha2.Function + baseImage string + want string }{ { - name: "should use runtime", + name: "should use runtime", + baseImage: "nodejs18:test", fn: &serverlessv1alpha2.Function{ ObjectMeta: metav1.ObjectMeta{ UID: "fn-uuid", @@ -106,10 +112,11 @@ func Test_calculateInlineImageTag(t *testing.T) { }, }, }, - want: "9f131e00ad3c6cfc5ca36f27df299eeeb2b08bcc4328782e79b69440b1b7aa2b", + want: "3773f4e6b48ccb71c2a43f4a7cbbeb98333701d823d8ff5917d8f7373c7abbcd", }, { - name: "should use runtimeOverride", + name: "should use runtimeOverride", + baseImage: "nodejs18:test", fn: &serverlessv1alpha2.Function{ ObjectMeta: metav1.ObjectMeta{ UID: "fn-uuid", @@ -131,10 +138,11 @@ func Test_calculateInlineImageTag(t *testing.T) { }, }, }, - want: "9f131e00ad3c6cfc5ca36f27df299eeeb2b08bcc4328782e79b69440b1b7aa2b", + want: "3773f4e6b48ccb71c2a43f4a7cbbeb98333701d823d8ff5917d8f7373c7abbcd", }, { - name: "should use runtime instead of runtimeOverride", + name: "should use runtime instead of runtimeOverride", + baseImage: "nodejs18:test", fn: &serverlessv1alpha2.Function{ ObjectMeta: metav1.ObjectMeta{ UID: "fn-uuid", @@ -156,12 +164,12 @@ func Test_calculateInlineImageTag(t *testing.T) { }, }, }, - want: "9f131e00ad3c6cfc5ca36f27df299eeeb2b08bcc4328782e79b69440b1b7aa2b", + want: "3773f4e6b48ccb71c2a43f4a7cbbeb98333701d823d8ff5917d8f7373c7abbcd", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - assert.Equal(t, tt.want, calculateInlineImageTag(tt.fn)) + assert.Equal(t, tt.want, calculateInlineImageTag(tt.fn, tt.baseImage)) }) } }