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

Add new flag '--reconciliation-timeout' #218

Closed

Conversation

VadBuka
Copy link

@VadBuka VadBuka commented Nov 27, 2024

Description of your changes

This pull request introduces a new reconciliationTimeout flag to the Crossplane project. This feature allows users to specify a timeout duration for the reconciliation process, addressing the issue of indefinite waits during resource reconciliation. This enhancement ensures that the reconciliation function completes within a defined timeframe, improving reliability and efficiency in resource management. The timeout is cumulative for all calls within a single reconciliation cycle, allowing necessary operations such as error reporting and status updates to be completed before the timeout is enforced.

Fixes #217

I have:

  • Read and followed Crossplane's [contribution process].
  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

I built the provider and deployed it locally using the following configuration:

apiVersion: pkg.crossplane.io/v1beta1
kind: DeploymentRuntimeConfig
metadata:
  name: crossplane-cloudplatform-argocd
spec:
  deploymentTemplate:
    spec:
      selector: {}
      template:
        spec:
          containers:
          - name: package-runtime
            args:
            - --reconciliation-timeout=3m
            - --debug

I tested the feature with an application using the Helmfile plugin, where Helmfile rendering typically takes more than 1 minute. With the default reconciliation timeout set to 60 seconds, longer rendering processes would fail. By setting the reconciliation timeout to more than 60 seconds, I was able to successfully create an application with manifest generation taking over a minute, without encountering any errors.

Helmfile example:

/your-helmfile-directory
  ├── delay.sh
  └── helmfile.yaml
#!/bin/bash
echo "Starting delay..."
for ((i=60; i>0; i--)); do
  echo "$i seconds left"
  sleep 1
done
echo "Delay completed."
repositories:
  - name: bitnami
    url: https://charts.bitnami.com/bitnami

releases:
  - name: my-nginx-release
    namespace: default
    chart: bitnami/nginx
    version: 15.4.0  # Specify a stable version
    hooks:
      - events: ["prepare"]
        command: "./delay.sh"
        showlogs: true

@VadBuka VadBuka changed the title Add new reconciliation timeout Add new flag '--reconciliation-timeout' Nov 27, 2024
@VadBuka VadBuka marked this pull request as ready for review November 27, 2024 12:58
@VadBuka VadBuka marked this pull request as draft November 27, 2024 14:23
@VadBuka VadBuka marked this pull request as ready for review November 27, 2024 14:23
@MisterMX
Copy link
Collaborator

I understand the need to specify a custom timeout for specific reconcilers. However, I don't think we are going to merge this proposed solution., For once because it only effects the application controller but the flag implies it to be applied to all controllers. And for another it seems easier to me that we should just increase the hardcoded timeout for applications in total to, say 5 minutes or longer, instead of making it configurable.

@VadBuka
Copy link
Author

VadBuka commented Nov 28, 2024

/re

I understand the need to specify a custom timeout for specific reconcilers. However, I don't think we are going to merge this proposed solution., For once because it only effects the application controller but the flag implies it to be applied to all controllers. And for another it seems easier to me that we should just increase the hardcoded timeout for applications in total to, say 5 minutes or longer, instead of making it configurable.

If we set the timeout to a hardcoded 5 minutes, can it be merged then?

@MisterMX
Copy link
Collaborator

MisterMX commented Dec 2, 2024

If we set the timeout to a hardcoded 5 minutes, can it be merged then?

Yes, I guess that is reasonable.

@VadBuka
Copy link
Author

VadBuka commented Dec 2, 2024

Thank you for the review. I will close the PR and created a new one - #219

@VadBuka
Copy link
Author

VadBuka commented Dec 2, 2024

closed

@VadBuka VadBuka closed this Dec 2, 2024
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.

Introduce Configurable Reconciliation Timeout
2 participants