Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ADR 46: Build a common Task Runner image #217

Merged
merged 1 commit into from
Nov 22, 2024

Conversation

chmeliik
Copy link
Contributor

Related to https://issues.redhat.com/browse/STONEBLD-2982. Building the RHTAP task runner image was the final impulse, but I've felt the need for a unified runner image for some time now.

ADR/0046-common-task-runner-image.md Outdated Show resolved Hide resolved
Comment on lines +95 to +96
The Task Runner image does not replace the more specialized use-case-oriented images,
but they can use it as a base image if desirable.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have any recommendation on when it makes sense to create these specialized images? Or how narrowly focused they should be?

I proposed adding an additional endpoint to the build-trusted-artifacts image, but the contract team deemed that it would be better to put it somewhere else.

@zregvart @lcarva , fyi. I feel like this ADR is the opposite direction of what you were proposing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The recommendation could be this: if you're thinking of including a tool in the Task Runner image and it does not follow semver and/or isn't easily installable as a standalone tool, don't include it.

As an example, that would mean these images should be kept standalone

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I proposed adding an additional endpoint to the build-trusted-artifacts image, but the contract team deemed that it would be better to put it somewhere else.

Yeah, because of the nature of the build-trusted-artifacts image/repo. The proposal here is to create a multi-tool repo/image, which by definition has a much wider scope.

The recommendation could be this: if you're thinking of including a tool in the Task Runner image and it does not follow semver and/or isn't easily installable as a standalone tool, don't include it.

IMO the current state of those tools reflect the current state of getting code to run in the Konflux tasks. If we have a well-defined list of requirement for what it takes to be added to the runner image, then I expect these tools to be motivated to abide to them, especially if they are reasonable.

I expect a tool like oras to be compiled from source when building the runner image. Much like it's current done in https://github.com/konflux-ci/oras-container. The source is referenced via a git submodule. Would this be a reasonable approach for all of the tools?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The source is referenced via a git submodule. Would this be a reasonable approach for all of the tools?

With git submodules, it's kind of hard to to follow versions (or even to know what version you are building). Renovate just bumps them to the latest commit in main.

For tools written in Go, I was thinking of experimenting with a tools.go-based approach. We wouldn't include them as submodules, just declare them as dependencies. That gets you proper versioning for free. (That probably doesn't belong in this ADR though, as long as the image knows exactly what version of each tool it includes, it's fine)

If we have a well-defined list of requirement for what it takes to be added to the runner image, then I expect these tools to be motivated to abide to them, especially if they are reasonable.

💯

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the suggested requirements for tool inclusion

Copy link
Contributor

@lcarva lcarva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on the idea. It's clearly an improvement of the current process. The only concern is an image with multiple tools may become difficult to maintain over time for different reasons. I think that's ok. We can reassess the decision at that point.

ADR/0046-common-task-runner-image.md Show resolved Hide resolved
ADR/0046-common-task-runner-image.md Show resolved Hide resolved
Comment on lines +95 to +96
The Task Runner image does not replace the more specialized use-case-oriented images,
but they can use it as a base image if desirable.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I proposed adding an additional endpoint to the build-trusted-artifacts image, but the contract team deemed that it would be better to put it somewhere else.

Yeah, because of the nature of the build-trusted-artifacts image/repo. The proposal here is to create a multi-tool repo/image, which by definition has a much wider scope.

The recommendation could be this: if you're thinking of including a tool in the Task Runner image and it does not follow semver and/or isn't easily installable as a standalone tool, don't include it.

IMO the current state of those tools reflect the current state of getting code to run in the Konflux tasks. If we have a well-defined list of requirement for what it takes to be added to the runner image, then I expect these tools to be motivated to abide to them, especially if they are reasonable.

I expect a tool like oras to be compiled from source when building the runner image. Much like it's current done in https://github.com/konflux-ci/oras-container. The source is referenced via a git submodule. Would this be a reasonable approach for all of the tools?

Tasks have lower resource requirements because it's no longer necessary to split
them into steps.

The Task Runner image is larger than any of the individual images used by the Tasks
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also (maybe a stretch) there is a higher chance that the image will already be cached in the node.

Copy link
Contributor Author

@chmeliik chmeliik Nov 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realized that "Pipelines pull each image at most once" is not true, all the Tasks in a Pipeline don't usually run on the same node. I still think the common image would reduce overall image pull rates though. Rephrased


By no longer relying on a Tekton-specific feature (steps), most Tasks become nothing
more than a bash script wrapped in some YAML. It enables a saner approach to authoring
Tasks. Write a bash script that works on your machine, wrap it in a bunch of YAML,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although I agree with this, it seems to be contrary to this conversation. Are you saying that it's ok to have a mixture of random scripts in the runner image?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exceptions would still need to exist.

use-trusted-artifact / create-trusted-artifact steps would still be needed (until TA becomes a standalone tool and the TA-generation tool takes that into account, if ever)

The build-source-image task would still use the image tailor-built for that one task, until the script becomes a standalone tool (if ever).

I'll rephrase this to make it clearer

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Softened the wording, added an example where we would still want to use steps

@chmeliik
Copy link
Contributor Author

@arewm if this is ready for merge, could you merge this please?

And what are the next steps? I suppose this needs a Konflux Feature?

@chmeliik
Copy link
Contributor Author

Unless @lcarva you want to re-check if your concerns have been resolved

@lcarva
Copy link
Contributor

lcarva commented Nov 22, 2024

Let's merge.

@arewm arewm merged commit 6416632 into konflux-ci:main Nov 22, 2024
1 check passed
@chmeliik chmeliik deleted the common-task-image branch November 22, 2024 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants