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

[CarterFendley]

IMPORTANT

Design changes have been made since the original description, see this comment for updated usage

---

---

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

[hbelmiro]

/lgtm

[CarterFendley]

Modifications have been made to address this issue found by @gregsheremeta, thanks for pointing that out! Since the instance was one where the driver sets the log level of the launcher a design change was made to have one unified PIPELINE_LOG_LEVEL env var to prevent the somewhat confusing implementations of passing the LAUNCHER_LOG_LEVEL as a command line argument to the driver (or similar implementations).

The new usage is to update the environment variable on the ml-pipelines deployment to the following.

    spec:
      containers:
      - env:
        - name: PIPELINE_LOG_LEVEL
          value: "3"

Importantly, as before, it is important that 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.

After these updates, the main container now also runs the launcher with configured log level.

@hbelmiro @HumairAK, or any others: Please let me know if you have any additional feedback on this PR. Apologies for the delay in the patch!

[HumairAK]

/lgtm
/approve

[google-oss-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: droctothorpe, HumairAK

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

The pull request process is described here

Needs approval from an approver in each of these files:

• backend/OWNERS [HumairAK]
• manifests/kustomize/OWNERS [HumairAK]

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