Skip to content

Commit

Permalink
Use configmap watcher for feature flags config
Browse files Browse the repository at this point in the history 
Taskrun reconciler now uses a configmap watcher for `feature-flags`
configmap to remove the need for an extra Kubernetes API call to get the
configmap's value every time a pod is being created.

Signed-off-by: Arash Deshmeh <adeshmeh@ca.ibm.com>
  • Loading branch information
@adshmh
adshmh committed May 22, 2020
1 parent 76b39ec commit d0fa286
Show file tree
Hide file tree
Showing 9 changed files with 388 additions and 93 deletions.
35 changes: 10 additions & 25 deletions pkg/pod/pod.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,30 +17,21 @@ limitations under the License.
package pod

import (
"context"
"fmt"
"os"
"path/filepath"

"github.com/tektoncd/pipeline/pkg/apis/config"
"github.com/tektoncd/pipeline/pkg/apis/pipeline"
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
"github.com/tektoncd/pipeline/pkg/names"
"github.com/tektoncd/pipeline/pkg/system"
"github.com/tektoncd/pipeline/pkg/version"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/client-go/kubernetes"
)

// GetFeatureFlagsConfigName returns the name of the configmap containing all
// customizations for the feature flags.
func GetFeatureFlagsConfigName() string {
if e := os.Getenv("CONFIG_FEATURE_FLAGS_NAME"); e != "" {
return e
}
return "feature-flags"
}

const (
// ResultsDir is the folder used by default to create the results file
ResultsDir = "/tekton/results"
Expand Down Expand Up @@ -86,7 +77,7 @@ var (

// MakePod converts TaskRun and TaskSpec objects to a Pod which implements the taskrun specified
// by the supplied CRD.
func MakePod(images pipeline.Images, taskRun *v1beta1.TaskRun, taskSpec v1beta1.TaskSpec, kubeclient kubernetes.Interface, entrypointCache EntrypointCache, overrideHomeEnv bool) (*corev1.Pod, error) {
func MakePod(ctx context.Context, images pipeline.Images, taskRun *v1beta1.TaskRun, taskSpec v1beta1.TaskSpec, kubeclient kubernetes.Interface, entrypointCache EntrypointCache, overrideHomeEnv bool) (*corev1.Pod, error) {
var initContainers []corev1.Container
var volumes []corev1.Volume
var volumeMounts []corev1.VolumeMount
Expand Down Expand Up @@ -190,7 +181,7 @@ func MakePod(images pipeline.Images, taskRun *v1beta1.TaskRun, taskSpec v1beta1.
// - sets container name to add "step-" prefix or "step-unnamed-#" if not specified.
// TODO(#1605): Remove this loop and make each transformation in
// isolation.
shouldOverrideWorkingDir := shouldOverrideWorkingDir(kubeclient)
shouldOverrideWorkingDir := shouldOverrideWorkingDir(ctx)
for i, s := range stepContainers {
if s.WorkingDir == "" && shouldOverrideWorkingDir {
stepContainers[i].WorkingDir = pipeline.WorkspaceDir
Expand Down Expand Up @@ -330,12 +321,9 @@ func getLimitRangeMinimum(namespace string, kubeclient kubernetes.Interface) (co
// but this is planned to change in an upcoming release.
//
// For further reference see https://github.com/tektoncd/pipeline/issues/2013
func ShouldOverrideHomeEnv(kubeclient kubernetes.Interface) bool {
configMap, err := kubeclient.CoreV1().ConfigMaps(system.GetNamespace()).Get(GetFeatureFlagsConfigName(), metav1.GetOptions{})
if err == nil && configMap != nil && configMap.Data != nil && configMap.Data[featureFlagDisableHomeEnvKey] == "true" {
return false
}
return true
func ShouldOverrideHomeEnv(ctx context.Context) bool {
cfg := config.FromContextOrDefaults(ctx)
return !cfg.FeatureFlags.DisableHomeEnvOverwrite
}

// shouldOverrideWorkingDir returns a bool indicating whether a Pod should have its
Expand All @@ -344,10 +332,7 @@ func ShouldOverrideHomeEnv(kubeclient kubernetes.Interface) bool {
// if not specified by the user, but this is planned to change in an upcoming release.
//
// For further reference see https://github.com/tektoncd/pipeline/issues/1836
func shouldOverrideWorkingDir(kubeclient kubernetes.Interface) bool {
configMap, err := kubeclient.CoreV1().ConfigMaps(system.GetNamespace()).Get(GetFeatureFlagsConfigName(), metav1.GetOptions{})
if err == nil && configMap != nil && configMap.Data != nil && configMap.Data[featureFlagDisableWorkingDirKey] == "true" {
return false
}
return true
func shouldOverrideWorkingDir(ctx context.Context) bool {
cfg := config.FromContextOrDefaults(ctx)
return !cfg.FeatureFlags.DisableWorkingDirOverwrite
}
64 changes: 17 additions & 47 deletions pkg/pod/pod_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,14 @@ limitations under the License.
package pod

import (
"context"
"fmt"
"os"
"path/filepath"
"strings"
"testing"

"github.com/google/go-cmp/cmp"
"github.com/tektoncd/pipeline/pkg/apis/config"
"github.com/tektoncd/pipeline/pkg/apis/pipeline"
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
"github.com/tektoncd/pipeline/pkg/system"
Expand All @@ -33,6 +34,7 @@ import (
"k8s.io/apimachinery/pkg/api/resource"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
fakek8s "k8s.io/client-go/kubernetes/fake"
logtesting "knative.dev/pkg/logging/testing"
)

var (
Expand Down Expand Up @@ -813,7 +815,7 @@ script-heredoc-randomly-generated-78c5n
// No entrypoints should be looked up.
entrypointCache := fakeCache{}

got, err := MakePod(images, tr, c.ts, kubeclient, entrypointCache, true)
got, err := MakePod(context.Background(), images, tr, c.ts, kubeclient, entrypointCache, true)
if err != nil {
t.Fatalf("MakePod: %v", err)
}
Expand Down Expand Up @@ -858,14 +860,14 @@ func TestShouldOverrideHomeEnv(t *testing.T) {
}{{
description: "Default behaviour: A missing disable-home-env-overwrite flag should result in true",
configMap: &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{Name: GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()},
ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()},
Data: map[string]string{},
},
expected: true,
}, {
description: "Setting disable-home-env-overwrite to false should result in true",
configMap: &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{Name: GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()},
ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()},
Data: map[string]string{
featureFlagDisableHomeEnvKey: "false",
},
Expand All @@ -874,55 +876,23 @@ func TestShouldOverrideHomeEnv(t *testing.T) {
}, {
description: "Setting disable-home-env-overwrite to true should result in false",
configMap: &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{Name: GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()},
ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()},
Data: map[string]string{
featureFlagDisableHomeEnvKey: "true",
},
},
expected: false,
}} {
t.Run(tc.description, func(t *testing.T) {
kubeclient := fakek8s.NewSimpleClientset(
tc.configMap,
)
if result := ShouldOverrideHomeEnv(kubeclient); result != tc.expected {
store := config.NewStore(logtesting.TestLogger(t))
store.OnConfigChanged(tc.configMap)
if result := ShouldOverrideHomeEnv(store.ToContext(context.Background())); result != tc.expected {
t.Errorf("Expected %t Received %t", tc.expected, result)
}
})
}
}

func TestGetFeatureFlagsConfigName(t *testing.T) {
for _, tc := range []struct {
description string
featureFlagEnvValue string
expected string
}{{
description: "Feature flags config value not set",
featureFlagEnvValue: "",
expected: "feature-flags",
}, {
description: "Feature flags config value set",
featureFlagEnvValue: "feature-flags-test",
expected: "feature-flags-test",
}} {
t.Run(tc.description, func(t *testing.T) {
original := os.Getenv("CONFIG_FEATURE_FLAGS_NAME")
defer t.Cleanup(func() {
os.Setenv("CONFIG_FEATURE_FLAGS_NAME", original)
})
if tc.featureFlagEnvValue != "" {
os.Setenv("CONFIG_FEATURE_FLAGS_NAME", tc.featureFlagEnvValue)
}
got := GetFeatureFlagsConfigName()
want := tc.expected
if got != want {
t.Errorf("GetFeatureFlagsConfigName() = %s, want %s", got, want)
}
})
}
}

func TestShouldOverrideWorkingDir(t *testing.T) {
for _, tc := range []struct {
description string
Expand All @@ -931,14 +901,14 @@ func TestShouldOverrideWorkingDir(t *testing.T) {
}{{
description: "Default behaviour: A missing disable-working-directory-overwrite flag should result in true",
configMap: &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{Name: GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()},
ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()},
Data: map[string]string{},
},
expected: true,
}, {
description: "Setting disable-working-directory-overwrite to false should result in true",
configMap: &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{Name: GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()},
ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()},
Data: map[string]string{
featureFlagDisableWorkingDirKey: "false",
},
Expand All @@ -947,18 +917,18 @@ func TestShouldOverrideWorkingDir(t *testing.T) {
}, {
description: "Setting disable-working-directory-overwrite to true should result in false",
configMap: &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{Name: GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()},
ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()},
Data: map[string]string{
featureFlagDisableWorkingDirKey: "true",
},
},
expected: false,
}} {
t.Run(tc.description, func(t *testing.T) {
kubeclient := fakek8s.NewSimpleClientset(
tc.configMap,
)
if result := shouldOverrideWorkingDir(kubeclient); result != tc.expected {
store := config.NewStore(logtesting.TestLogger(t))
store.OnConfigChanged(tc.configMap)
ctx := store.ToContext(context.Background())
if result := shouldOverrideWorkingDir(ctx); result != tc.expected {
t.Errorf("Expected %t Received %t", tc.expected, result)
}
})
Expand Down
3 changes: 3 additions & 0 deletions pkg/reconciler/taskrun/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"context"
"time"

"github.com/tektoncd/pipeline/pkg/apis/config"
"github.com/tektoncd/pipeline/pkg/apis/pipeline"
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
pipelineclient "github.com/tektoncd/pipeline/pkg/client/injection/client"
Expand Down Expand Up @@ -105,6 +106,8 @@ func NewController(namespace string, images pipeline.Images) func(context.Contex
Handler: controller.HandleAll(impl.EnqueueControllerOf),
})

c.configStore = config.NewStore(c.Logger.Named("config-store"))
c.configStore.WatchConfigs(cmw)
return impl
}
}
17 changes: 13 additions & 4 deletions pkg/reconciler/taskrun/taskrun.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/client-go/tools/cache"
"knative.dev/pkg/apis"
"knative.dev/pkg/configmap"
"knative.dev/pkg/controller"
"knative.dev/pkg/tracker"
)
Expand All @@ -56,6 +57,11 @@ const (
taskRunAgentName = "taskrun-controller"
)

type configStore interface {
ToContext(ctx context.Context) context.Context
WatchConfigs(w configmap.Watcher)
}

// Reconciler implements controller.Reconciler for Configuration resources.
type Reconciler struct {
*reconciler.Base
Expand All @@ -71,6 +77,7 @@ type Reconciler struct {
timeoutHandler *reconciler.TimeoutSet
metrics *Recorder
pvcHandler volumeclaim.PvcHandler
configStore configStore
}

// Check that our Reconciler implements controller.Reconciler
Expand All @@ -88,6 +95,8 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error {
return nil
}

ctx = c.configStore.ToContext(ctx)

// Get the Task Run resource with this namespace/name
original, err := c.taskRunLister.TaskRuns(namespace).Get(name)
if k8serrors.IsNotFound(err) {
Expand Down Expand Up @@ -374,7 +383,7 @@ func (c *Reconciler) reconcile(ctx context.Context, tr *v1beta1.TaskRun,
tr.Spec.Workspaces = taskRunWorkspaces
}

pod, err = c.createPod(tr, rtr)
pod, err = c.createPod(ctx, tr, rtr)
if err != nil {
c.handlePodCreationError(tr, err)
return nil
Expand Down Expand Up @@ -533,7 +542,7 @@ func (c *Reconciler) failTaskRun(tr *v1beta1.TaskRun, reason, message string) er

// createPod creates a Pod based on the Task's configuration, with pvcName as a volumeMount
// TODO(dibyom): Refactor resource setup/substitution logic to its own function in the resources package
func (c *Reconciler) createPod(tr *v1beta1.TaskRun, rtr *resources.ResolvedTaskResources) (*corev1.Pod, error) {
func (c *Reconciler) createPod(ctx context.Context, tr *v1beta1.TaskRun, rtr *resources.ResolvedTaskResources) (*corev1.Pod, error) {
ts := rtr.TaskSpec.DeepCopy()
inputResources, err := resourceImplBinding(rtr.Inputs, c.Images)
if err != nil {
Expand Down Expand Up @@ -589,12 +598,12 @@ func (c *Reconciler) createPod(tr *v1beta1.TaskRun, rtr *resources.ResolvedTaskR
}

// Check if the HOME env var of every Step should be set to /tekton/home.
shouldOverrideHomeEnv := podconvert.ShouldOverrideHomeEnv(c.KubeClientSet)
shouldOverrideHomeEnv := podconvert.ShouldOverrideHomeEnv(ctx)

// Apply creds-init path substitutions.
ts = resources.ApplyCredentialsPath(ts, podconvert.CredentialsPath(shouldOverrideHomeEnv))

pod, err := podconvert.MakePod(c.Images, tr, *ts, c.KubeClientSet, c.entrypointCache, shouldOverrideHomeEnv)
pod, err := podconvert.MakePod(ctx, c.Images, tr, *ts, c.KubeClientSet, c.entrypointCache, shouldOverrideHomeEnv)
if err != nil {
return nil, fmt.Errorf("translating TaskSpec to Pod: %w", err)
}
Expand Down
Loading

0 comments on commit d0fa286

Please sign in to comment.