feat(backend): configurable log level for driver / launcher images

[CarterFendley]

Description of your changes:

This PR adds the ability to change log level in the driver / launcher containers. This is implemented in a similar pattern as the overrides for driver / launcher images. Specifically, you can add the following environment variables to the ml-pipeline deployments:

    spec:
      containers:
      - env:
        - name: DRIVER_LOG_LEVEL
          value: "3"
        - name: LAUNCHER_LOG_LEVEL
          value: "3"

Note: A numerical value such as the literal 3 not "3" here will be invalid deployment spec and validation on the spec will fail causing kubectl edit to reject it with the message: error: deployments.apps "ml-pipeline" is invalid.

Other minor alterations

1. In this commit two locations were updated to use the workflowCompiler.driverImage and workflowCompiler.launcherImage attributes which are populated here. This is a very minor change but seemed better to invoke only once and match other such usages (in importer.go and dag.go). If there are reasons this should be re-invoked, please let me know.
2. The --copy flag were moved into the arguments block, to match other implementations. Again, lmk if this is not wanted.

Feedback wanted

The environment variable for this is similar to the V2_LAUNCHER_IMAGE and V2_DRIVER_IMAGE but without the V2_ prefix. If anyone has preferences here, I do not, so happy to take any path.

Checklist:

• You have signed off your commits
• The title for your pull request (PR) should follow our title convention. Learn more about the pull request title convention used in this repository.

[google-oss-prow]

Hi @CarterFendley. Thanks for your PR.

I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[hbelmiro]

/ok-to-test

[droctothorpe]

LGTM. Thank you for tackling this, @CarterFendley!

[rimolive]

Maybe worth adding just one unit test to verify if setting both env vars will generate the Workflow yaml with the new flags.

[CarterFendley]

Maybe worth adding just one unit test to verify if setting both env vars will generate the Workflow yaml with the new flags.

Will do :)

[CarterFendley]

Okay in this commit I have updated the compiler tests with logic to optional take in environment variables and set them:

if tt.envVars != nil {
    for _, envVar := range tt.envVars {
	    parts := strings.Split(strings.ReplaceAll(envVar, " ", ""), "=")
	    os.Setenv(parts[0], parts[1])
    
	    // Unset after test cases has ended
	    defer os.Unsetenv(parts[0])
    }
}

To test cases and golden files have been added to test the logic included in this PR.

{
	jobPath:          "../testdata/hello_world.json",
	platformSpecPath: "",
	argoYAMLPath:     "testdata/with_logging/hello_world.yaml",
	envVars:          []string{"DRIVER_LOG_LEVEL=5", "LAUNCHER_LOG_LEVEL=5"},
},
{
	jobPath:          "../testdata/importer.json",
	platformSpecPath: "",
	argoYAMLPath:     "testdata/with_logging/importer.yaml",
	envVars:          []string{"DRIVER_LOG_LEVEL=5", "LAUNCHER_LOG_LEVEL=5"},
},

[rimolive]

/lgtm

[hbelmiro]

Thank you @CarterFendley.
Looks good. I just left a few minor comments.

[hbelmiro]

Any reason for this change besides optimization?

[CarterFendley]

No not really, just made it a bit concise to add flags and follows the pattern used in the other Container definitions for driver / launcher

[google-oss-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: droctothorpe
Once this PR has been reviewed and has the lgtm label, please ask for approval from rimolive. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• backend/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[hbelmiro]

/lgtm

[gregsheremeta]

I wanted to install this and run it, and also verify that it's backwards compatible. All looks good with that, but I noticed one problem. You missed one of the launcher invocations, roughly here

pipelines/backend/src/v2/driver/driver.go

Lines 389 to 405 in c2a7713

	launcherCmd := []string{
	component.KFPLauncherPath,
	// TODO(Bobgy): no need to pass pipeline_name and run_id, these info can be fetched via pipeline context and pipeline run context which have been created by root DAG driver.
	"--pipeline_name", pipelineName,
	"--run_id", runID,
	"--execution_id", fmt.Sprintf("%v", executionID),
	"--executor_input", string(executorInputJSON),
	"--component_spec", string(componentJSON),
	"--pod_name",
	fmt.Sprintf("$(%s)", component.EnvPodName),
	"--pod_uid",
	fmt.Sprintf("$(%s)", component.EnvPodUID),
	"--mlmd_server_address",
	fmt.Sprintf("$(%s)", component.EnvMetadataHost),
	"--mlmd_server_port",
	fmt.Sprintf("$(%s)", component.EnvMetadataPort),
	"--", // separater before user command and args

The symptom is that even though I set LAUNCHER_LOG_LEVEL to something other than 1, the user code (impl) container logs always say Setting log level to: '1'

Launcher is invoked twice, in two different ways 🤦.

1. First it's invoked to copy itself into the main container (you caught that invocation -- the one with --copy -- this runs in the kfp-launcher init-container)
2. Second, to actually launch the user's code (this runs in the main container). This second one is the one that is much more important to be able to control the logging in. That invocation is generated in the pod spec patch in driver.go. (This is all super arcane, so let me know if this isn't making sense.)

Once you add it, you should see it in the output of podSpecPatch in a container-driver log. Roughly:

I1018 20:34:19.156021      18 main.go:246] output podSpecPatch=
{"containers":[{"name":"main","image":"python:3.7","command":["/kfp-launcher/launch","--pipeline_name","my-pipeline","--run_id","SNIP","--execution_id","291","--executor_input","SNIP","--component_spec","SNIP","--pod_name","$(KFP_POD_NAME)","--pod_uid","$(KFP_POD_UID)","--mlmd_server_address","$(METADATA_GRPC_SERVICE_HOST)","--mlmd_server_port","$(METADATA_GRPC_SERVICE_PORT)","--", 
**(you'll see your log_level here...)**
],

And then the user code container log should respect the launcher log setting:
Setting log level to: 'whatever_you_set'

[hbelmiro]

@CarterFendley is there anything pending in this PR?

[CarterFendley]

@hbelmiro Yes, as noted by Greg, we want to make sure that the main container for the -impl pod also is setup to be configurable.

Had some discussion on slack about the possible implementations. I am OOO probably all of next week, will add that feature soon after I am back