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 Jun 6, 2020
1 parent 10b2f9a commit 64da2b5
Show file tree
Hide file tree
Showing 12 changed files with 429 additions and 134 deletions.
52 changes: 14 additions & 38 deletions pkg/pod/pod.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,14 @@ 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"
"github.com/tektoncd/pipeline/pkg/workspace"
corev1 "k8s.io/api/core/v1"
Expand All @@ -33,25 +33,10 @@ import (
"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"

featureInjectedSidecar = "running-in-environment-with-injected-sidecars"
featureFlagDisableHomeEnvKey = "disable-home-env-overwrite"
featureFlagDisableWorkingDirKey = "disable-working-directory-overwrite"
featureFlagConfigMapName = "feature-flags"
featureFlagSetReadyAnnotationOnPodCreate = "enable-ready-annotation-on-pod-create"

taskRunLabelKey = pipeline.GroupName + pipeline.TaskRunLabelKey
)

Expand Down Expand Up @@ -90,7 +75,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 @@ -194,7 +179,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 @@ -253,7 +238,7 @@ func MakePod(images pipeline.Images, taskRun *v1beta1.TaskRun, taskSpec v1beta1.
podAnnotations := taskRun.Annotations
podAnnotations[ReleaseAnnotation] = ReleaseAnnotationValue

if shouldAddReadyAnnotationOnPodCreate(taskSpec.Sidecars, kubeclient) {
if shouldAddReadyAnnotationOnPodCreate(ctx, taskSpec.Sidecars) {
podAnnotations[readyAnnotation] = readyAnnotationValue
}

Expand Down Expand Up @@ -369,12 +354,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 @@ -383,28 +365,22 @@ 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
}

// shouldAddReadyAnnotationonPodCreate returns a bool indicating whether the
// controller should add the `Ready` annotation when creating the Pod. We cannot
// add the annotation if Tekton is running in a cluster with injected sidecars
// or if the Task specifies any sidecars.
func shouldAddReadyAnnotationOnPodCreate(sidecars []v1beta1.Sidecar, kubeclient kubernetes.Interface) bool {
func shouldAddReadyAnnotationOnPodCreate(ctx context.Context, sidecars []v1beta1.Sidecar) bool {
// If the TaskRun has sidecars, we cannot set the READY annotation early
if len(sidecars) > 0 {
return false
}
// If the TaskRun has no sidecars, check if we are running in a cluster where sidecars can be injected by other
// controllers.
configMap, err := kubeclient.CoreV1().ConfigMaps(system.GetNamespace()).Get(GetFeatureFlagsConfigName(), metav1.GetOptions{})
if err == nil && configMap != nil && configMap.Data != nil && configMap.Data[featureInjectedSidecar] == "false" {
return true
}
return false
cfg := config.FromContextOrDefaults(ctx)
return !cfg.FeatureFlags.RunningInEnvWithInjectedSidecars
}
92 changes: 35 additions & 57 deletions pkg/pod/pod_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,15 @@ limitations under the License.
package pod

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

"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
"github.com/tektoncd/pipeline/pkg/apis/config"
"github.com/tektoncd/pipeline/pkg/apis/pipeline"
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1"
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
Expand All @@ -35,6 +36,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 All @@ -47,6 +49,10 @@ var (
ignoreReleaseAnnotation = func(k string, v string) bool {
return k == ReleaseAnnotation
}
featureInjectedSidecar = "running-in-environment-with-injected-sidecars"
featureFlagDisableHomeEnvKey = "disable-home-env-overwrite"
featureFlagDisableWorkingDirKey = "disable-working-directory-overwrite"
featureFlagSetReadyAnnotationOnPodCreate = "enable-ready-annotation-on-pod-create"
)

func TestMakePod(t *testing.T) {
Expand Down Expand Up @@ -980,11 +986,14 @@ script-heredoc-randomly-generated-78c5n
}}} {
t.Run(c.desc, func(t *testing.T) {
names.TestingSeed()
kubeclient := fakek8s.NewSimpleClientset(
store := config.NewStore(logtesting.TestLogger(t))
store.OnConfigChanged(
&corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{Name: featureFlagConfigMapName, Namespace: system.GetNamespace()},
ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()},
Data: c.featureFlags,
},
)
kubeclient := fakek8s.NewSimpleClientset(
&corev1.ServiceAccount{ObjectMeta: metav1.ObjectMeta{Name: "default", Namespace: "default"}},
&corev1.ServiceAccount{ObjectMeta: metav1.ObjectMeta{Name: "service-account", Namespace: "default"},
Secrets: []corev1.ObjectReference{{
Expand Down Expand Up @@ -1026,7 +1035,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(store.ToContext(context.Background()), images, tr, c.ts, kubeclient, entrypointCache, true)
if err != nil {
t.Fatalf("MakePod: %v", err)
}
Expand Down Expand Up @@ -1077,14 +1086,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 @@ -1093,55 +1102,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 @@ -1150,14 +1127,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 @@ -1166,18 +1143,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 All @@ -1199,23 +1176,23 @@ func TestShouldAddReadyAnnotationonPodCreate(t *testing.T) {
description: "Default behavior with sidecars present: Ready annotation not set on pod create",
sidecars: []v1beta1.Sidecar{sd},
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: false,
}, {
description: "Default behavior with no sidecars present: Ready annotation not set on pod create",
sidecars: []v1beta1.Sidecar{},
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: false,
}, {
description: "Setting running-in-environment-with-injected-sidecars to true with sidecars present results in false",
sidecars: []v1beta1.Sidecar{sd},
configMap: &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{Name: GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()},
ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()},
Data: map[string]string{
featureInjectedSidecar: "true",
},
Expand All @@ -1225,7 +1202,7 @@ func TestShouldAddReadyAnnotationonPodCreate(t *testing.T) {
description: "Setting running-in-environment-with-injected-sidecars to true with no sidecars present results in false",
sidecars: []v1beta1.Sidecar{},
configMap: &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{Name: GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()},
ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()},
Data: map[string]string{
featureInjectedSidecar: "true",
},
Expand All @@ -1235,7 +1212,7 @@ func TestShouldAddReadyAnnotationonPodCreate(t *testing.T) {
description: "Setting running-in-environment-with-injected-sidecars to false with sidecars present results in false",
sidecars: []v1beta1.Sidecar{sd},
configMap: &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{Name: GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()},
ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()},
Data: map[string]string{
featureInjectedSidecar: "false",
},
Expand All @@ -1245,7 +1222,7 @@ func TestShouldAddReadyAnnotationonPodCreate(t *testing.T) {
description: "Setting running-in-environment-with-injected-sidecars to false with no sidecars present results in true",
sidecars: []v1beta1.Sidecar{},
configMap: &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{Name: GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()},
ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()},
Data: map[string]string{
featureInjectedSidecar: "false",
},
Expand All @@ -1255,8 +1232,9 @@ func TestShouldAddReadyAnnotationonPodCreate(t *testing.T) {

for _, tc := range tcs {
t.Run(tc.description, func(t *testing.T) {
kubclient := fakek8s.NewSimpleClientset(tc.configMap)
if result := shouldAddReadyAnnotationOnPodCreate(tc.sidecars, kubclient); result != tc.expected {
store := config.NewStore(logtesting.TestLogger(t))
store.OnConfigChanged(tc.configMap)
if result := shouldAddReadyAnnotationOnPodCreate(store.ToContext(context.Background()), tc.sidecars); result != tc.expected {
t.Errorf("expected: %t Received: %t", tc.expected, result)
}
})
Expand Down
13 changes: 5 additions & 8 deletions pkg/reconciler/pipelinerun/affinity_assistant.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,14 @@ limitations under the License.
package pipelinerun

import (
"context"
"fmt"

"github.com/tektoncd/pipeline/pkg/apis/config"
"github.com/tektoncd/pipeline/pkg/apis/pipeline"
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1"
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
"github.com/tektoncd/pipeline/pkg/pod"
"github.com/tektoncd/pipeline/pkg/reconciler/volumeclaim"
"github.com/tektoncd/pipeline/pkg/system"
"github.com/tektoncd/pipeline/pkg/workspace"
appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"
Expand Down Expand Up @@ -217,10 +217,7 @@ func affinityAssistantStatefulSet(name string, pr *v1beta1.PipelineRun, claimNam
// be created for each PipelineRun that use workspaces with PersistentVolumeClaims
// as volume source. The default behaviour is to enable the Affinity Assistant to
// provide Node Affinity for TaskRuns that share a PVC workspace.
func (c *Reconciler) isAffinityAssistantDisabled() bool {
configMap, err := c.KubeClientSet.CoreV1().ConfigMaps(system.GetNamespace()).Get(pod.GetFeatureFlagsConfigName(), metav1.GetOptions{})
if err == nil && configMap != nil && configMap.Data != nil && configMap.Data[featureFlagDisableAffinityAssistantKey] == "true" {
return true
}
return false
func (c *Reconciler) isAffinityAssistantDisabled(ctx context.Context) bool {
cfg := config.FromContextOrDefaults(ctx)
return cfg.FeatureFlags.DisableAffinityAssistant
}
Loading

0 comments on commit 64da2b5

Please sign in to comment.