Skip to content

Optional Runtime Parameters Fix #12377

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
+1,234 −14
Open

Optional Runtime Parameters Fix #12377

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f83114e
Debug optional secret/configmap skip
Oct 16, 2025
e76c16f
Add optional Kubernetes inputs test pipeline
sduvvuri1603 Oct 17, 2025
50c1a3f
Gate optional Kubernetes helpers behind flags
sduvvuri1603 Oct 20, 2025
59046cf
Fix lint warning in node affinity skip
sduvvuri1603 Oct 20, 2025
43683f2
Remove standalone secrets/configmaps demo
sduvvuri1603 Oct 21, 2025
15a56da
Remove CA certificates copy
sduvvuri1603 Oct 23, 2025
eea578c
Remove CA certificates copy
sduvvuri1603 Oct 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 41 additions & 10 deletions backend/src/v2/driver/k8s.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package driver
import (
"context"
"encoding/json"
"errors"
"fmt"
"time"

Expand Down Expand Up @@ -178,22 +179,31 @@ func extendPodSpecPatch(
// Get node selector information
if kubernetesExecutorConfig.GetNodeSelector() != nil {
var nodeSelector map[string]string
// skipNodeSelector marks when the node selector input resolved to a null optional
// value. In that case we avoid appending an empty selector to the pod spec.
skipNodeSelector := false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be useful to have a comment here explaining what skipNodeSelector means in this context

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added an explanatory comment

if kubernetesExecutorConfig.GetNodeSelector().GetNodeSelectorJson() != nil {
err := resolveK8sJsonParameter(ctx, opts, dag, pipeline, mlmd,
kubernetesExecutorConfig.GetNodeSelector().GetNodeSelectorJson(), inputParams, &nodeSelector)
if err != nil {
return fmt.Errorf("failed to resolve node selector: %w", err)
if errors.Is(err, ErrResolvedParameterNull) {
skipNodeSelector = true
} else {
return fmt.Errorf("failed to resolve node selector: %w", err)
}
}
} else {
nodeSelector = kubernetesExecutorConfig.GetNodeSelector().GetLabels()
}

if setOnTaskConfig[pipelinespec.TaskConfigPassthroughType_KUBERNETES_NODE_SELECTOR] {
taskConfig.NodeSelector = nodeSelector
}
if !skipNodeSelector {
if setOnTaskConfig[pipelinespec.TaskConfigPassthroughType_KUBERNETES_NODE_SELECTOR] {
taskConfig.NodeSelector = nodeSelector
}

if setOnPod[pipelinespec.TaskConfigPassthroughType_KUBERNETES_NODE_SELECTOR] {
podSpec.NodeSelector = nodeSelector
if setOnPod[pipelinespec.TaskConfigPassthroughType_KUBERNETES_NODE_SELECTOR] {
podSpec.NodeSelector = nodeSelector
}
}
}

Expand All @@ -209,6 +219,9 @@ func extendPodSpecPatch(
resolvedParam, err := resolveInputParameter(ctx, dag, pipeline, opts, mlmd,
toleration.GetTolerationJson(), inputParams)
if err != nil {
if errors.Is(err, ErrResolvedParameterNull) {
continue // Skip applying the patch for this null/optional parameter
}
return fmt.Errorf("failed to resolve toleration: %w", err)
}

Expand Down Expand Up @@ -278,6 +291,9 @@ func extendPodSpecPatch(
resolvedSecretName, err := resolveInputParameterStr(ctx, dag, pipeline, opts, mlmd,
secretAsVolume.SecretNameParameter, inputParams)
if err != nil {
if errors.Is(err, ErrResolvedParameterNull) {
continue
}
return fmt.Errorf("failed to resolve secret name: %w", err)
}
secretName = resolvedSecretName.GetStringValue()
Expand Down Expand Up @@ -337,6 +353,9 @@ func extendPodSpecPatch(
resolvedSecretName, err := resolveInputParameterStr(ctx, dag, pipeline, opts, mlmd,
secretAsEnv.SecretNameParameter, inputParams)
if err != nil {
if errors.Is(err, ErrResolvedParameterNull) {
continue
}
return fmt.Errorf("failed to resolve secret name: %w", err)
}
secretName = resolvedSecretName.GetStringValue()
Expand All @@ -363,12 +382,15 @@ func extendPodSpecPatch(
for _, configMapAsVolume := range kubernetesExecutorConfig.GetConfigMapAsVolume() {
var configMapName string
if configMapAsVolume.ConfigMapNameParameter != nil {
resolvedSecretName, err := resolveInputParameterStr(ctx, dag, pipeline, opts, mlmd,
resolvedConfigMapName, err := resolveInputParameterStr(ctx, dag, pipeline, opts, mlmd,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was your reasoning behind refactoring resolvedSecretName to resolvedConfigMapName?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I renamed to resolvedConfigMapName here because this block is inside the ConfigMap handling path (GetConfigMapAsVolume/GetConfigMapAsEnv) and the value we resolve is the ConfigMap name, so the old identifier was a copy/paste holdover from the secret path and made the code harder to follow. Updating the name keeps it aligned with the ConfigMap resource we’re actually working with.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@HumairAK What are your thoughts here? It looks like the current implementation was added by you in the driver logic refactor: #11885

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got reply from Humair that it is a typo and can be ignored

configMapAsVolume.ConfigMapNameParameter, inputParams)
if err != nil {
if errors.Is(err, ErrResolvedParameterNull) {
continue
}
return fmt.Errorf("failed to resolve configmap name: %w", err)
}
configMapName = resolvedSecretName.GetStringValue()
configMapName = resolvedConfigMapName.GetStringValue()
} else if configMapAsVolume.ConfigMapName != "" {
configMapName = configMapAsVolume.ConfigMapName
} else {
Expand Down Expand Up @@ -424,12 +446,15 @@ func extendPodSpecPatch(

var configMapName string
if configMapAsEnv.ConfigMapNameParameter != nil {
resolvedSecretName, err := resolveInputParameterStr(ctx, dag, pipeline, opts, mlmd,
resolvedConfigMapName, err := resolveInputParameterStr(ctx, dag, pipeline, opts, mlmd,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above ^

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I renamed to resolvedConfigMapName here because this block is inside the ConfigMap handling path (GetConfigMapAsVolume/GetConfigMapAsEnv) and the value we resolve is the ConfigMap name, so the old identifier was a copy/paste holdover from the secret path and made the code harder to follow. Updating the name keeps it aligned with the ConfigMap resource we’re actually working with.

configMapAsEnv.ConfigMapNameParameter, inputParams)
if err != nil {
if errors.Is(err, ErrResolvedParameterNull) {
continue
}
return fmt.Errorf("failed to resolve configmap name: %w", err)
}
configMapName = resolvedSecretName.GetStringValue()
configMapName = resolvedConfigMapName.GetStringValue()
} else if configMapAsEnv.ConfigMapName != "" {
configMapName = configMapAsEnv.ConfigMapName
} else {
Expand All @@ -456,6 +481,9 @@ func extendPodSpecPatch(
resolvedSecretName, err := resolveInputParameterStr(ctx, dag, pipeline, opts, mlmd,
imagePullSecret.SecretNameParameter, inputParams)
if err != nil {
if errors.Is(err, ErrResolvedParameterNull) {
continue
}
return fmt.Errorf("failed to resolve image pull secret name: %w", err)
}
secretName = resolvedSecretName.GetStringValue()
Expand Down Expand Up @@ -588,6 +616,9 @@ func extendPodSpecPatch(
err := resolveK8sJsonParameter(ctx, opts, dag, pipeline, mlmd,
nodeAffinityTerm.GetNodeAffinityJson(), inputParams, &k8sNodeAffinity)
if err != nil {
if errors.Is(err, ErrResolvedParameterNull) {
continue
}
return fmt.Errorf("failed to resolve node affinity json: %w", err)
}

Expand Down
Loading
Loading