Fixed Pipeline Schedules failing after upgrading

[hbelmiro]

Resolves: https://issues.redhat.com/browse/RHOAIENG-14265

This PR cherry picks 5 commits from upstream.

Checklist:

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

[jgarciao]

I've tested this PR using the dsp-api-server test image provided by @hbelmiro and scheduled workflows seem to be working fine.

• In the testing, I've manually modified an existing ScheduledWorkflow with the following command and after restarting the dsp-api-server pod it was updated removing the incorrect fields and it kept working correctly:

oc  patch scheduledworkflow  hello-world-every-2m89bbq  -n my-project  --type='merge' -p '{"spec":{"workflow":{"spec":{"dynamicKey1":"dynamicValue1","dynamicKey2":"dynamicValue2"}}}}'

• I've verified that this PR doesn't reintroduce the bug we found in Dashboard when listing Experiments and runs documented here: https://issues.redhat.com/browse/RHOAIENG-18581

[hbelmiro]

@HumairAK @mprahl can you PTAL?

[mprahl]

@hbelmiro could you please rebase this PR?

[hbelmiro]

@hbelmiro could you please rebase this PR?

@mprahl done.

[mprahl]

@hbelmiro could you please remind me what the last commit is fixing compared to what's in upstream?

[hbelmiro]

@hbelmiro could you please remind me what the last commit is fixing compared to what's in upstream?

@mprahl 40a10c4 fixes the ServiceAccount name, which in ODH we add the DSPA name as a suffix.

#129 may give more context.

[mprahl]

@hbelmiro could you please remind me what the last commit is fixing compared to what's in upstream?

@mprahl 40a10c4 fixes the ServiceAccount name, which in ODH we add the DSPA name as a suffix.

#129 may give more context.

Could you please explain why this default value for the service account is not working correctly?
https://github.com/opendatahub-io/data-science-pipelines-operator/blob/7f7f7852591c5389af0661ecea5059bc0d498c83/config/internal/apiserver/default/deployment.yaml.tmpl#L75-L76

[hbelmiro]

@hbelmiro could you please remind me what the last commit is fixing compared to what's in upstream?

@mprahl 40a10c4 fixes the ServiceAccount name, which in ODH we add the DSPA name as a suffix.
#129 may give more context.

Could you please explain why this default value for the service account is not working correctly? https://github.com/opendatahub-io/data-science-pipelines-operator/blob/7f7f7852591c5389af0661ecea5059bc0d498c83/config/internal/apiserver/default/deployment.yaml.tmpl#L75-L76

@mprahl I wasn't aware of that config. I'll take a look.

[hbelmiro]

@hbelmiro could you please remind me what the last commit is fixing compared to what's in upstream?

@mprahl 40a10c4 fixes the ServiceAccount name, which in ODH we add the DSPA name as a suffix.
#129 may give more context.

Could you please explain why this default value for the service account is not working correctly? https://github.com/opendatahub-io/data-science-pipelines-operator/blob/7f7f7852591c5389af0661ecea5059bc0d498c83/config/internal/apiserver/default/deployment.yaml.tmpl#L75-L76

@mprahl I wasn't aware of that config. I'll take a look.

@mprahl good catch!
The ServiceAccount is hardcoded in upstream. I'll send a PR to fix it and then we can cherry pick it and remove 40a10c4.

/hold

[mprahl]

@hbelmiro I think that hardcoded value is on purpose as it's supposed to get replaced here, so maybe it's not getting replaced properly.
https://github.com/kubeflow/pipelines/blob/915cc552f56359454b91870df0e5eea1ecda2218/backend/src/apiserver/template/template.go#L295-L306

[hbelmiro]

@hbelmiro I think that hardcoded value is on purpose as it's supposed to get replaced here, so maybe it's not getting replaced properly. https://github.com/kubeflow/pipelines/blob/915cc552f56359454b91870df0e5eea1ecda2218/backend/src/apiserver/template/template.go#L295-L306

@mprahl setDefaultServiceAccount is not being called because of the hardcoded value.
Besides that, I don't see a reason for having a hardcoded value if there's a config for a default value.

[mprahl]

@hbelmiro I think that hardcoded value is on purpose as it's supposed to get replaced here, so maybe it's not getting replaced properly. https://github.com/kubeflow/pipelines/blob/915cc552f56359454b91870df0e5eea1ecda2218/backend/src/apiserver/template/template.go#L295-L306

@mprahl setDefaultServiceAccount is not being called because of the hardcoded value. Besides that, I don't see a reason for having a hardcoded value if there's a config for a default value.

Oh, I see. Perhaps just removing the if condition would fix it?

[hbelmiro]

@hbelmiro I think that hardcoded value is on purpose as it's supposed to get replaced here, so maybe it's not getting replaced properly. https://github.com/kubeflow/pipelines/blob/915cc552f56359454b91870df0e5eea1ecda2218/backend/src/apiserver/template/template.go#L295-L306

@mprahl setDefaultServiceAccount is not being called because of the hardcoded value. Besides that, I don't see a reason for having a hardcoded value if there's a config for a default value.

Oh, I see. Perhaps just removing the if condition would fix it?

@mprahl Having pipeline-runner hardcoded doesn't make sense to me.
From my perspective, we should use DefaultPipelineRunnerServiceAccount since the beginning:

ServiceAccountName: common.GetStringConfigWithDefault(common.DefaultPipelineRunnerServiceAccountFlag, common.DefaultPipelineRunnerServiceAccount),

And keep the condition there (just for safety).

[mprahl]

@hbelmiro I think that hardcoded value is on purpose as it's supposed to get replaced here, so maybe it's not getting replaced properly. https://github.com/kubeflow/pipelines/blob/915cc552f56359454b91870df0e5eea1ecda2218/backend/src/apiserver/template/template.go#L295-L306

@mprahl setDefaultServiceAccount is not being called because of the hardcoded value. Besides that, I don't see a reason for having a hardcoded value if there's a config for a default value.

Oh, I see. Perhaps just removing the if condition would fix it?

@mprahl Having pipeline-runner hardcoded doesn't make sense to me. From my perspective, we should use DefaultPipelineRunnerServiceAccount since the beginning:

ServiceAccountName: common.GetStringConfigWithDefault(common.DefaultPipelineRunnerServiceAccountFlag, common.DefaultPipelineRunnerServiceAccount),

And keep the condition there (just for safety).

That seems okay too!

[hbelmiro]

/unhold

@mprahl can you please take another look?

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mprahl

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:

• OWNERS [mprahl]

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

[hbelmiro]

@mprahl can you please manually merge it?