Skip to content

Commit

Permalink
Add "await-sidecar-readiness" feature flag
Browse files Browse the repository at this point in the history
True by default, setting this to false will prevent the first Step in a
TaskRun from waiting on the "Ready" annotation before commencing. This
should allow tasks to be run in environments that don't support
DownwardAPI volumes.
  • Loading branch information
hWorblehat committed Jun 10, 2022
1 parent 88a71c3 commit 276e3a0
Show file tree
Hide file tree
Showing 7 changed files with 149 additions and 74 deletions.
8 changes: 8 additions & 0 deletions config/config-feature-flags.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,14 @@ data:
# See https://github.com/tektoncd/pipeline/issues/2791 for more
# info.
disable-creds-init: "false"
# Setting this flag to "false" will stop Tekton from waiting for a
# TaskRun's sidecar containers to be running before starting the first
# step. This will allow Tasks to be run in environments that don't
# support the DownwardAPI volume type, but may lead to unintended
# behaviour if sidecars are used.
#
# See https://github.com/tektoncd/pipeline/issues/4937 for more info.
await-sidecar-readiness: "true"
# This option should be set to false when Pipelines is running in a
# cluster that does not use injected sidecars such as Istio. Setting
# it to false should decrease the time it takes for a TaskRun to start
Expand Down
17 changes: 12 additions & 5 deletions docs/install.md
Original file line number Diff line number Diff line change
Expand Up @@ -355,11 +355,18 @@ To customize the behavior of the Pipelines Controller, modify the ConfigMap `fea
node in the cluster must have an appropriate label matching `topologyKey`. If some or all nodes
are missing the specified `topologyKey` label, it can lead to unintended behavior.

- `running-in-environment-with-injected-sidecars`: set this flag to `"true"` to allow the
Tekton controller to set the `tekton.dev/ready` annotation at pod creation time for
TaskRuns with no Sidecars specified. Enabling this option should decrease the time it takes for a TaskRun to
start running. However, for clusters that use injected sidecars e.g. istio
enabling this option can lead to unexpected behavior.
- `await-sidecar-readiness`: set this flag to `"false"` to allow the Tekton controller to start a
TasksRun's first step immediately without waiting for sidecar containers to be running first. Using
this option should decrease the time it takes for a TaskRun to start running, and will allow TaskRun
pods to be scheduled in environments that don't support [Downward API](https://kubernetes.io/docs/tasks/inject-data-application/downward-api-volume-expose-pod-information/)
volumes (e.g. some virtual kubelet implementations). However, this may lead to unexpected behaviour
with Tasks that use sidecars, or in clusters that use injected sidecars (e.g. Istio). Setting this flag
to `"false"` will mean the `running-in-environment-with-injected-sidecars` flag has no effect.

- `running-in-environment-with-injected-sidecars`: set this flag to `"false"` to allow the
Tekton controller to start a TasksRun's first step immediately if it has no Sidecars specified.
Using this option should decrease the time it takes for a TaskRun to start running.
However, for clusters that use injected sidecars (e.g. Istio) this can lead to unexpected behavior.

- `require-git-ssh-secret-known-hosts`: set this flag to `"true"` to require that
Git SSH Secrets include a `known_hosts` field. This ensures that a git remote server's
Expand Down
7 changes: 7 additions & 0 deletions pkg/apis/config/feature_flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ const (
DefaultDisableCredsInit = false
// DefaultRunningInEnvWithInjectedSidecars is the default value for "running-in-environment-with-injected-sidecars".
DefaultRunningInEnvWithInjectedSidecars = true
// DefaultAwaitSidecarReadiness is the default value for "await-sidecar-readiness".
DefaultAwaitSidecarReadiness = true
// DefaultRequireGitSSHSecretKnownHosts is the default value for "require-git-ssh-secret-known-hosts".
DefaultRequireGitSSHSecretKnownHosts = false
// DefaultEnableTektonOciBundles is the default value for "enable-tekton-oci-bundles".
Expand All @@ -61,6 +63,7 @@ const (
disableAffinityAssistantKey = "disable-affinity-assistant"
disableCredsInitKey = "disable-creds-init"
runningInEnvWithInjectedSidecarsKey = "running-in-environment-with-injected-sidecars"
awaitSidecarReadinessKey = "await-sidecar-readiness"
requireGitSSHSecretKnownHostsKey = "require-git-ssh-secret-known-hosts" // nolint: gosec
enableTektonOCIBundles = "enable-tekton-oci-bundles"
enableCustomTasks = "enable-custom-tasks"
Expand All @@ -81,6 +84,7 @@ type FeatureFlags struct {
ScopeWhenExpressionsToTask bool
EnableAPIFields string
SendCloudEventsForRuns bool
AwaitSidecarReadiness bool
EmbeddedStatus string
}

Expand Down Expand Up @@ -118,6 +122,9 @@ func NewFeatureFlagsFromMap(cfgMap map[string]string) (*FeatureFlags, error) {
if err := setFeature(runningInEnvWithInjectedSidecarsKey, DefaultRunningInEnvWithInjectedSidecars, &tc.RunningInEnvWithInjectedSidecars); err != nil {
return nil, err
}
if err := setFeature(awaitSidecarReadinessKey, DefaultAwaitSidecarReadiness, &tc.AwaitSidecarReadiness); err != nil {
return nil, err
}
if err := setFeature(requireGitSSHSecretKnownHostsKey, DefaultRequireGitSSHSecretKnownHosts, &tc.RequireGitSSHSecretKnownHosts); err != nil {
return nil, err
}
Expand Down
46 changes: 38 additions & 8 deletions pkg/apis/config/feature_flags_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,16 +35,25 @@ func TestNewFeatureFlagsFromConfigMap(t *testing.T) {
testCases := []testCase{
{
expectedConfig: &config.FeatureFlags{
RunningInEnvWithInjectedSidecars: config.DefaultRunningInEnvWithInjectedSidecars,
EnableAPIFields: "stable",
EmbeddedStatus: config.DefaultEmbeddedStatus,
DisableAffinityAssistant: false,
RunningInEnvWithInjectedSidecars: true,
RequireGitSSHSecretKnownHosts: false,

DisableCredsInit: config.DefaultDisableCredsInit,
AwaitSidecarReadiness: config.DefaultAwaitSidecarReadiness,
EnableTektonOCIBundles: config.DefaultEnableTektonOciBundles,
EnableCustomTasks: config.DefaultEnableCustomTasks,
EnableAPIFields: config.DefaultEnableAPIFields,
SendCloudEventsForRuns: config.DefaultSendCloudEventsForRuns,
EmbeddedStatus: config.DefaultEmbeddedStatus,
},
fileName: config.GetFeatureFlagsConfigName(),
},
{
expectedConfig: &config.FeatureFlags{
DisableAffinityAssistant: true,
RunningInEnvWithInjectedSidecars: false,
AwaitSidecarReadiness: false,
RequireGitSSHSecretKnownHosts: true,
EnableTektonOCIBundles: true,
EnableCustomTasks: true,
Expand All @@ -62,7 +71,12 @@ func TestNewFeatureFlagsFromConfigMap(t *testing.T) {
EnableTektonOCIBundles: true,
EnableCustomTasks: true,

DisableAffinityAssistant: config.DefaultDisableAffinityAssistant,
DisableCredsInit: config.DefaultDisableCredsInit,
RunningInEnvWithInjectedSidecars: config.DefaultRunningInEnvWithInjectedSidecars,
AwaitSidecarReadiness: config.DefaultAwaitSidecarReadiness,
RequireGitSSHSecretKnownHosts: config.DefaultRequireGitSSHSecretKnownHosts,
SendCloudEventsForRuns: config.DefaultSendCloudEventsForRuns,
EmbeddedStatus: config.DefaultEmbeddedStatus,
},
fileName: "feature-flags-enable-api-fields-overrides-bundles-and-custom-tasks",
Expand All @@ -73,7 +87,12 @@ func TestNewFeatureFlagsFromConfigMap(t *testing.T) {
EnableTektonOCIBundles: true,
EnableCustomTasks: true,

DisableAffinityAssistant: config.DefaultDisableAffinityAssistant,
DisableCredsInit: config.DefaultDisableCredsInit,
RunningInEnvWithInjectedSidecars: config.DefaultRunningInEnvWithInjectedSidecars,
AwaitSidecarReadiness: config.DefaultAwaitSidecarReadiness,
RequireGitSSHSecretKnownHosts: config.DefaultRequireGitSSHSecretKnownHosts,
SendCloudEventsForRuns: config.DefaultSendCloudEventsForRuns,
EmbeddedStatus: config.DefaultEmbeddedStatus,
},
fileName: "feature-flags-bundles-and-custom-tasks",
Expand All @@ -91,11 +110,7 @@ func TestNewFeatureFlagsFromConfigMap(t *testing.T) {

func TestNewFeatureFlagsFromEmptyConfigMap(t *testing.T) {
FeatureFlagsConfigEmptyName := "feature-flags-empty"
expectedConfig := &config.FeatureFlags{
RunningInEnvWithInjectedSidecars: true,
EnableAPIFields: "stable",
EmbeddedStatus: config.DefaultEmbeddedStatus,
}
expectedConfig := defaultFeatureFlags()
verifyConfigFileWithExpectedFeatureFlagsConfig(t, FeatureFlagsConfigEmptyName, expectedConfig)
}

Expand Down Expand Up @@ -159,3 +174,18 @@ func verifyConfigFileWithExpectedFeatureFlagsConfig(t *testing.T, fileName strin
t.Errorf("NewFeatureFlagsFromConfigMap(actual) = %v", err)
}
}

func defaultFeatureFlags() *config.FeatureFlags {
return &config.FeatureFlags{
DisableAffinityAssistant: config.DefaultDisableAffinityAssistant,
DisableCredsInit: config.DefaultDisableCredsInit,
RunningInEnvWithInjectedSidecars: config.DefaultRunningInEnvWithInjectedSidecars,
AwaitSidecarReadiness: config.DefaultAwaitSidecarReadiness,
RequireGitSSHSecretKnownHosts: config.DefaultRequireGitSSHSecretKnownHosts,
EnableTektonOCIBundles: config.DefaultEnableTektonOciBundles,
EnableCustomTasks: config.DefaultEnableCustomTasks,
EnableAPIFields: config.DefaultEnableAPIFields,
SendCloudEventsForRuns: config.DefaultSendCloudEventsForRuns,
EmbeddedStatus: config.DefaultEmbeddedStatus,
}
}
1 change: 1 addition & 0 deletions pkg/apis/config/testdata/feature-flags-all-flags-set.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ metadata:
data:
disable-affinity-assistant: "true"
running-in-environment-with-injected-sidecars: "false"
await-sidecar-readiness: "false"
require-git-ssh-secret-known-hosts: "true"
enable-tekton-oci-bundles: "true"
enable-custom-tasks: "true"
Expand Down
24 changes: 10 additions & 14 deletions pkg/pod/pod.go
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,8 @@ func (b *Builder) Build(ctx context.Context, taskRun *v1beta1.TaskRun, taskSpec
)
volumeMounts := []corev1.VolumeMount{binROMount}
implicitEnvVars := []corev1.EnvVar{}
alphaAPIEnabled := config.FromContextOrDefaults(ctx).FeatureFlags.EnableAPIFields == config.AlphaAPIFields
featureFlags := config.FromContextOrDefaults(ctx).FeatureFlags
alphaAPIEnabled := featureFlags.EnableAPIFields == config.AlphaAPIFields

// Add our implicit volumes first, so they can be overridden by the user if they prefer.
volumes = append(volumes, implicitVolumes...)
Expand Down Expand Up @@ -183,7 +184,7 @@ func (b *Builder) Build(ctx context.Context, taskRun *v1beta1.TaskRun, taskSpec
return nil, err
}

readyImmediately := isPodReadyImmediately(ctx, taskSpec.Sidecars)
readyImmediately := isPodReadyImmediately(*featureFlags, taskSpec.Sidecars)

if alphaAPIEnabled {
stepContainers, err = orderContainers(credEntrypointArgs, stepContainers, &taskSpec, taskRun.Spec.Debug, !readyImmediately)
Expand Down Expand Up @@ -377,18 +378,13 @@ func makeLabels(s *v1beta1.TaskRun) map[string]string {
}

// isPodReadyImmediately 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 isPodReadyImmediately(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.
cfg := config.FromContextOrDefaults(ctx)
return !cfg.FeatureFlags.RunningInEnvWithInjectedSidecars
// controller should consider the Pod "Ready" as soon as it's deployed.
// This will add the `Ready` annotation when creating the Pod,
// and prevent the first step from waiting for the annotation to appear before starting.
func isPodReadyImmediately(featureFlags config.FeatureFlags, sidecars []v1beta1.Sidecar) bool {
return !featureFlags.AwaitSidecarReadiness ||
// If the TaskRun has sidecars, we must wait for them
(len(sidecars) == 0 && !featureFlags.RunningInEnvWithInjectedSidecars)
}

func runMount(i int, ro bool) corev1.VolumeMount {
Expand Down
120 changes: 73 additions & 47 deletions pkg/pod/pod_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ var (
return k == ReleaseAnnotation
}
featureInjectedSidecar = "running-in-environment-with-injected-sidecars"
featureAwaitSidecarReadiness = "await-sidecar-readiness"
featureFlagSetReadyAnnotationOnPodCreate = "enable-ready-annotation-on-pod-create"

defaultActiveDeadlineSeconds = int64(config.DefaultTimeoutMinutes * 60 * deadlineFactor)
Expand Down Expand Up @@ -2036,74 +2037,99 @@ func TestIsPodReadyImmediately(t *testing.T) {
sd := v1beta1.Sidecar{
Name: "a-sidecar",
}

getFeatureFlags := func(cfg map[string]string) *config.FeatureFlags {
flags, err := config.NewFeatureFlagsFromMap(cfg)
if err != nil {
t.Fatalf("Error building feature flags for testingL %s", err)
}
return flags
}

tcs := []struct {
description string
sidecars []v1beta1.Sidecar
configMap *corev1.ConfigMap
expected bool
description string
sidecars []v1beta1.Sidecar
featureFlags *config.FeatureFlags
expected bool
}{{
description: "Default behavior with sidecars present: Pod is not ready on create",
description: "Default behavior with sidecars present: Pod is not ready on create",
sidecars: []v1beta1.Sidecar{sd},
featureFlags: getFeatureFlags(map[string]string{}),
expected: false,
}, {
description: "Default behavior with no sidecars present: Pod is not ready on create",
sidecars: []v1beta1.Sidecar{},
featureFlags: getFeatureFlags(map[string]string{}),
expected: false,
}, {
description: "Setting await-sidecar-readiness to true and 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: config.GetFeatureFlagsConfigName(), Namespace: system.Namespace()},
Data: map[string]string{},
},
featureFlags: getFeatureFlags(map[string]string{
featureAwaitSidecarReadiness: "true",
featureInjectedSidecar: "true",
}),
expected: false,
}, {
description: "Default behavior with no sidecars present: Pod is not ready on create",
description: "Setting await-sidecar-readiness to true and 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: config.GetFeatureFlagsConfigName(), Namespace: system.Namespace()},
Data: map[string]string{},
},
featureFlags: getFeatureFlags(map[string]string{
featureAwaitSidecarReadiness: "true",
featureInjectedSidecar: "true",
}),
expected: false,
}, {
description: "Setting running-in-environment-with-injected-sidecars to true with sidecars present results in false",
description: "Setting await-sidecar-readiness to true and 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: config.GetFeatureFlagsConfigName(), Namespace: system.Namespace()},
Data: map[string]string{
featureInjectedSidecar: "true",
},
},
featureFlags: getFeatureFlags(map[string]string{
featureAwaitSidecarReadiness: "true",
featureInjectedSidecar: "false",
}),
expected: false,
}, {
description: "Setting running-in-environment-with-injected-sidecars to true with no sidecars present results in false",
description: "Setting await-sidecar-readiness to true and 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: config.GetFeatureFlagsConfigName(), Namespace: system.Namespace()},
Data: map[string]string{
featureInjectedSidecar: "true",
},
},
expected: false,
featureFlags: getFeatureFlags(map[string]string{
featureAwaitSidecarReadiness: "true",
featureInjectedSidecar: "false",
}),
expected: true,
}, {
description: "Setting running-in-environment-with-injected-sidecars to false with sidecars present results in false",
description: "Setting await-sidecar-readiness to false and running-in-environment-with-injected-sidecars to true with sidecars present results in true",
sidecars: []v1beta1.Sidecar{sd},
configMap: &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: system.Namespace()},
Data: map[string]string{
featureInjectedSidecar: "false",
},
},
expected: false,
featureFlags: getFeatureFlags(map[string]string{
featureAwaitSidecarReadiness: "false",
featureInjectedSidecar: "true",
}),
expected: true,
}, {
description: "Setting running-in-environment-with-injected-sidecars to false with no sidecars present results in true",
description: "Setting await-sidecar-readiness to false and running-in-environment-with-injected-sidecars to true with no sidecars present results in true",
sidecars: []v1beta1.Sidecar{},
configMap: &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: system.Namespace()},
Data: map[string]string{
featureInjectedSidecar: "false",
},
},
featureFlags: getFeatureFlags(map[string]string{
featureAwaitSidecarReadiness: "false",
featureInjectedSidecar: "true",
}),
expected: true,
}, {
description: "Setting await-sidecar-readiness to false and running-in-environment-with-injected-sidecars to false with sidecars present results in true",
sidecars: []v1beta1.Sidecar{sd},
featureFlags: getFeatureFlags(map[string]string{
featureAwaitSidecarReadiness: "false",
featureInjectedSidecar: "false",
}),
expected: true,
}, {
description: "Setting await-sidecar-readiness to false and running-in-environment-with-injected-sidecars to false with no sidecars present results in true",
sidecars: []v1beta1.Sidecar{},
featureFlags: getFeatureFlags(map[string]string{
featureAwaitSidecarReadiness: "false",
featureInjectedSidecar: "false",
}),
expected: true,
}}

for _, tc := range tcs {
t.Run(tc.description, func(t *testing.T) {
store := config.NewStore(logtesting.TestLogger(t))
store.OnConfigChanged(tc.configMap)
if result := isPodReadyImmediately(store.ToContext(context.Background()), tc.sidecars); result != tc.expected {
if result := isPodReadyImmediately(*tc.featureFlags, tc.sidecars); result != tc.expected {
t.Errorf("expected: %t Received: %t", tc.expected, result)
}
})
Expand Down

0 comments on commit 276e3a0

Please sign in to comment.