kueue: skip old platform param if new param exists #8849
Conversation
🤖 Gemini AI Assistant AvailableHi @sadlerap! I'm here to help with your pull request. You can interact with me using the following commands: Available Commands
How to Use
PermissionsOnly OWNER, MEMBER, or COLLABORATOR users can trigger my responses. This ensures secure and appropriate usage. This message was automatically added to help you get started with the Gemini AI assistant. Feel free to delete this comment if you don't need assistance. |
|
🤖 Hi @sadlerap, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
sadlerap
force-pushed
the
kueue-skip-old-params-staging
branch
from
738e195 to
38fc3a1
Compare
There was a problem hiding this comment.
please add tests for this change at https://github.com/redhat-appstudio/infra-deployments/blob/main/hack/test-tekton-kueue-config.py
sadlerap
force-pushed
the
kueue-skip-old-params-staging
branch
2 times, most recently
from
dc259bd to
b7aed2a
Compare
We came across a pipelinerun that was using the new parameter format to parameterize the old template format via a matrix definition. While it's likely that the pipelinerun is doing extra configuration here that it doesn't need to (it had `PLATFORM` defined in both matrix params and task params), tekton doesn't reject it. Therefore, we need to consider the possibility that these pipelineruns may exist - tekton-kueue will need to handle such pipelines without issues. The problem in the pipelinerun that caused issues was that the old platform parameter was populated with substitution data that cannot fit inside a label. The CEL expressions don't run against the data that has been substituted in (that would require us to monitor TaskRuns instead), so they would try to create labels with invalid characters, which would be rejected by kubernetes. Since the old platforms parameter will only contain garbage like this, and this can happen only if the new platform parameter is populated, only extract the old parameter if the new parameter isn't exist. See KONFLUX-9917 for further details. Fixes: KONFLUX-9917 Signed-off-by: Andy Sadler <[email protected]>
sadlerap
force-pushed
the
kueue-skip-old-params-staging
branch
from
b7aed2a to
f2a7498
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: enkeefe00, hugares, sadlerap 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:
Approvers can indicate their approval by writing |
openshift-merge-bot
bot
merged commit fce480f
into
redhat-appstudio:main
5 participants
If both the old and the new platform params exist in the pipelinerun, we should only pull from the new platform params. The old parameters are likely going to be something we can't interpret with CEL expressions if the new paramter is set. To avoid issues, only read from the old expressions if the new expressions are not set.
See KONFLUX-9917 for issues that have already cropped up in our production clusters.