Hi! We are currently trying to add automatic tracing through datadog to the pods started through Flyte. To do so, we would need to set a few environment variables and mount a volume. While we can set the environment variables in the config, there is no way to add volumes here. Is there any other way that we're missing? The only other thing I could think of is switching all of our tasks to pod tasks, where we would have control over the whole spec, but that is something we would like to avoid if possible. Also happy to contribute here if this is something you'd see a need for.

Hey @Bernhard Stadlbauer! I think you should be able to use the new(ish) default PodTemplate integration for this. Essentially, the one-off configurations in the flytek8s configuration you linked get applied to the PodSpec whenever we create a Pod in Flyte. That configuration now includes a

DefaultPodTemplateName

option. Basically FlytePropeller looks for a PodTemplate in the namespace where the Pod is being created (e.x.

flytesnacks-deployment

for the project-domain be default), if that doesn't exist then it looks for a PodTemplate in the namespace that FlytePropeller is deployed in (typically

flyte

) and uses that PodSpec as the base for configuring Pods. So PodTemplate values will be overridden by the flytek8s configuration options, which will be overridden by specific task configuration.

Hi @Dan Rammer (hamersaw)! Thanks for the note on the pod-template, that seems like a really nice addition 👍 We tried it out, and it worked out of the box, the only small issue we're facing is that we would like to mount a volume into our main container, e.g. along the lines of:

apiVersion: v1
kind: PodTemplate
metadata:
  name: flyte-default-template
  namespace: {{ .Release.Namespace }}
template:
  metadata:
  spec:
    volumes:
      - hostPath:
          path: /var/run/datadog/
        name: apmsocketpath
    containers:
      - name: noop
        image: <http://docker.io/rwgrim/docker-noop|docker.io/rwgrim/docker-noop> # (<http://docker.io/rwgrim/docker-noop>)
        volumeMounts:
          - name: apmsocketpath
            mountPath: /var/run/datadog

but as the

noop

container gets completely overwritten (if I'm not mistaken, the related code is here) this won't work. Are we missing something here? One thing we could think of is to merge container specs instead of overwriting them?

Oh OK, this totally makes sense. Have you thought about what this might look like for pods with multiple containers? There are certainly instances where we launch some kind of sidecar - are we applying the container spec to all containers? Or matching on a container name (I think the primary container name is hardcoded)?

I think while we could imagine usecases where you'd like to inject the volume into all containers within the pod (e.g. to get traces of all of them), for this particular usecase it might be enough to only match the primary container

Perfect. So it looks like when we use the Pod plugin we set an annotation for the primary container name here, so we know the primary container for pod tasks. However, when we use a plain container task the container name is set to the task name here. I think we either (1) set the annotation for the primary container name on container tasks or (2) use a default container name for container tasks so that we have a deterministic name. Then we can easily merge the primary container with the container Spec in the PodTemplate as you suggested. Does this sound reasonable to you? I'm open to other ideas as well. Would you be interested in contributing this? I'm happy to help.

Hello! I’m working with Bernhard on this, we’re interested in contributing to this change, but I first want to see if we’re understanding things right. For option (1) that you proposed, since the primary container name would still be dependent on the task name and the container name specified in the PodTemplate is static, it seems like we still wouldn’t be able to match the containers by name in the way we want? It seems like we would have to go with option (2), by setting the containerName here to some constant, ensuring the name in our PodTemplate matches that constant, and then finding the container in basePodSpec here that matches the name and merging on that. (Let me know if that makes sense! I’m still just getting up to speed with Flyte 🙇 )

Hey @Ailin Yu! What I was thinking with option 1 is to set a Pod annotation similar to the Pod plugin. So here we set an annotation on the pod so that "primary_container_name" points to the primary container name. This is currently used to determine the pod status because secondary containers may run forever, but the pod is completed when the primary container completes. I was thinking that even though the container name in the container plugin will by dynamically set, we can include a similar pod annotation for the "primary_container_name" which points to the single container. This could be done right around where you linked setting the container name. Then we can use the annotation as "alias" or sorts to get the primary container. This opens us up to allowing for multiple configurable containers in the default PodTemplate and just mapping them. I'd be happy to meet up and discuss this if it is not clear, sometimes in person is much easier.

Thanks for the detail! I think I follow… This way in BuildPodWithSpec we would know the annotation key and can use that to get the container name that we want to merge on? I think I’m also not sure of the best way to add an annotation in that function since it has the taskContainer in scope but not the pod, I see you can get annotations through

TaskExecMetadata.GetAnnotations()

but that feels like maybe a bit of an incorrect way to use a getter function

Sure, I haven't looked too deep into the specific implementation details. The gist is this needs to work similarly for the container and pod plugins. I would support any approach that is clean and extensible to support specifying multiple container configurations.

Sounds good to me! Yes feel free to assign it to me, thanks for making the issue!

I think you're on the right track. I was thinking we could add a function (like

getPrimaryContainerName

to the

podBuilder

interface here . This would, of course need to be implemented for the container (use the task name as you suggested) and sidecar (refactor the current code) plugins. Then something like below in the BuildResource function:

podSpec, err := builder.buildPodSpec(ctx, task, taskCtx)
	if err != nil {
		return nil, err
	}

	podSpec.ServiceAccountName = flytek8s.GetServiceAccountNameFromTaskExecutionMetadata(taskCtx.TaskExecutionMetadata())
    primaryContainerName, err := builder.getPrimaryContainerName(ctx, task, taskCtx)
    if err != nil {
        return nil, err
    }

	podTemplate := flytek8s.DefaultPodTemplateStore.LoadOrDefault(taskCtx.TaskExecutionMetadata().GetNamespace())
	pod, err := flytek8s.BuildPodWithSpec(podTemplate, podSpec, primaryContainerName)
	if err != nil {
		return nil, err
	}

    pod.Annotations[PrimaryContainerName] = primaryContainerName

Then we can pass the

primaryContainerName

as a string to the

BuildPodWithSpec

function and don't have to rely on annotations, but we still ensure the annotation is set. This will take a little bit of refactoring, but it should be fairly straight forward. We can certainly meetup as well to iron out any issues in the dev environment and sort through this.

Ah got it! I had thought a bit about passing the container name directly yesterday too but wasn’t sure what would be better! I think I’ll need to look at

podBuilder

a bit more to understand how its used currently, with regards to refactoring. Otherwise I’m just wrestling with my dev setup right now to try to get a test task to try out!