-
Notifications
You must be signed in to change notification settings - Fork 58
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
feat: use 4h timeout default rh-advisories and rh-push-to-registry-redhat-io pipelines #792
base: development
Are you sure you want to change the base?
feat: use 4h timeout default rh-advisories and rh-push-to-registry-redhat-io pipelines #792
Conversation
Hi @creydr. Thanks for your PR. I'm waiting for a konflux-ci member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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-sigs/prow repository. |
/cc @ralphbean |
/ok-to-test |
You mean "without having RELEASE-1291 in place", right? It's fine with me, let's see what Johnny thinks. But could you please do the same for the push-to-registry-redhat-io pipeline? We try to keep them in sync with the exception of having/not-having the advisory stuff. Also, you'll need to fix your commit message - see here: https://github.com/konflux-ci/release-service-catalog/blob/development/CONTRIBUTING.md#commit-message-formatting-and-standards |
3235f13
to
b051eb7
Compare
Oh, yes. Updated the PR description
Updated rh-push-to-registry-redhat-io as well
Updated |
/ok-to-test |
I don't really get doing this for all tasks. Can you explain? For example, I would never expect |
b051eb7
to
3142d3d
Compare
Agree with Johnny. Having a general timeout sounds bad to me to be honest. Maybe timeouts have to be raised but they shouldn't be treated in the same way. |
The current default is 2 hours. You could say the same today - that 2 hours don't make sense in some cases. Raising all of them is just the easiest way to improve the situation - otherwise it's a lot more work to analyse how much is reasonable for each task. Besides, as was discussed in Slack recently, the main issue is currently that taskruns can be waiting for the PV to be freed up for a long time and this will eat away from the timeout value. So it doesn't matter if a task itself never takes more than 5 minutes - it can still time out if some other task blocks it (uses the shared PV). All of this is meant as a temporary measure until that thing is addressed. Of course in the case of |
/ok-to-test |
We do not have 2 hour timeouts set for every task in our pipeline definitions. If we did, then the diff would just be changing a 2 to a 4. So I disagree on that. But I do agree that setting a 2 hour timeout for most of the tasks makes no sense. Nowhere in the commit or PR does it say this is a temporary workaround. So, for those reasons, I did not approve. |
@creydr: The following test has Failed, say /retest to rerun failed tests.
Inspecting Test ArtifactsTo inspect your test artifacts, follow these steps:
mkdir -p oras-artifacts
cd oras-artifacts
oras pull quay.io/konflux-test-storage/konflux-team/release-service-catalog:konflux-e2e-tests-catalog-44tht Test results analysis🚨 Failed to provision a cluster, see the log for more details: Click to view logsINFO: Log in to your Red Hat account... INFO: Configure AWS Credentials... WARN: The current version (1.2.47) is not up to date with latest rosa cli released version (1.2.49). WARN: It is recommended that you update to the latest version. INFO: Logged in as 'konflux-ci-418295695583' on 'https://api.openshift.com' INFO: Create ROSA with HCP cluster... WARN: The current version (1.2.47) is not up to date with latest rosa cli released version (1.2.49). WARN: It is recommended that you update to the latest version. INFO: Creating cluster 'kx-d0b8565e81' INFO: To view a list of clusters and their status, run 'rosa list clusters' INFO: Cluster 'kx-d0b8565e81' has been created. INFO: Once the cluster is installed you will need to add an Identity Provider before you can login into the cluster. See 'rosa create idp --help' for more information.
Nodes:
INFO: Preparing to create operator roles. |
…egistry-rh-io pipelines Explicitly set the timeout for taskruns in the rh-advisories and rh-push-to-registry-redhat-io pipelines to 4h and thus override the cluster default of (currently) 2h. This is especially helpful for larger components which are running into issues related to RELEASE-1291. Signed-off-by: Christoph Stäbler <[email protected]>
3142d3d
to
4f17876
Compare
New changes are detected. LGTM label has been removed. |
What I meant is that 2 hours is the cluster default, so we currently have 2 hours even for tasks that are not expected to take more than a minute. But I guess that won't change your stance :)
I think it's implied by mentioning that this is needed because RELEASE-1291 is not fixed. But I might be wrong. |
By default the task timeout in the release pipelines are 2h. This is especially short for larger applications without having RELEASE-1291 in place.
This PR sets the timeout for the tasks in the rh-advisories pipeline to 4h (what we've done for the OpenShift Serverless release).
Also updated
rh-push-to-registry-redhat-io
as you try to keep those pipelines in sync.An alternative would be to set the 4h timeout globally for all tasks in https://github.com/redhat-appstudio/infra-deployments