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

KFLUXINFRA-745: ConfigMap Settings for the Default Resource Requests and Limits #4249

Closed

Conversation

manish-jangra
Copy link
Contributor

@manish-jangra manish-jangra commented Aug 2, 2024

Pipeline ConfigMap Settings for the Default Resource Requests and Limits.

These values are not tested and are for testing purposes to see if it enforces resource requests and limits for the PipelineRin containers with no resource requests and limits set.

Copy link

openshift-ci bot commented Aug 2, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: manish-jangra

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved label Aug 2, 2024
Copy link
Contributor

@arewm arewm left a comment

Choose a reason for hiding this comment

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

I don't think that these changes will have any impact on the build container task as the task already specifies default. If you want to increase the default for specific steps within the task, I think you can use a prefix-* key: as indicated in tektoncd/pipeline#7003.

@gabemontero
Copy link
Contributor

I also suspect my earlier guidance in slack won't do what is desired here. Not 100% why, but that configMapGenerator in the kustomize.yaml seem to only deal with the config maps in the tekton-results namespace, not the config maps in the openshift-pipelines namesapce (which has the config-default cm).

I'm searching the internet trying to figure out this bit of gitops skulduggery.

Also, any PR like this that updates staging has to run hack/deploy-config.sh. I post an example of what I mean if I figure out how to ge the gitops to work.

@gabemontero
Copy link
Contributor

OK @manish-jangra @arewm I figured out and havee got an example of the correct gitops mechanics for updating config-defaults in openshift-pipelines updated in #4250

Apologies for my incorrect suggestion yesterday.

I'll leave it to you @manish-jangra to update this PR to follow this pattern, as well as work with @arewm if the use of default is find, or if you need some prefix filtering.

@manish-jangra
Copy link
Contributor Author

manish-jangra commented Aug 5, 2024

Thank you @gabemontero , your proposed solution sounds reasonable.

@arewm is there any fixed pattern for tasks naming conventions in Konflux PipelineRuns which can be used as a prefix?

@arewm
Copy link
Contributor

arewm commented Aug 5, 2024

is there any fixed pattern for tasks naming conventions in Konflux PipelineRuns which can be used as a prefix?

Are the pods not named according to the name of the task? While some users may customize this name, I assume that the default name we set on the pipeline (i.e. build-container should be a pretty consistent prefix)?

@manish-jangra
Copy link
Contributor Author

build-container isn't that a pod with 9+ containers running in it? It says containers inside the pod check it here

@arewm
Copy link
Contributor

arewm commented Aug 5, 2024

@manish-jangra, yes there are a lot of containers/steps running within the pod. If the current primary use case for deploying this fix is to try and improve the resource requests for all runs of the build pipeline, then we could just increase it for all steps in the task. If we do not want that, then I don't think that there is any other universal prefix for all steps which we are trying to target.

@jhutar
Copy link
Member

jhutar commented Aug 5, 2024

I would vote only merging this after konflux-ci/build-definitions#1247 (to cover other tasks that do not have their requests/limits set).

If we push this one before, that would make step-build container to run with limit of 0.5 CPU that would be a regression for some big projects.

Another way might be to only make this to define requests, as current 10m on CPU is not good for OpenShift scheduler or nodes auto-scaler.

@openshift-merge-robot
Copy link
Collaborator

PR needs rebase.

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-sigs/prow repository.

Copy link

openshift-ci bot commented Aug 20, 2024

@manish-jangra: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/appstudio-e2e-tests cc6391c link true /test appstudio-e2e-tests

Full PR test history. Your PR dashboard.

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-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants