Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

perf: set sync-timeout #3381

Merged
merged 1 commit into from
Apr 23, 2021
Merged

perf: set sync-timeout #3381

merged 1 commit into from
Apr 23, 2021

Conversation

jumpeiMano
Copy link
Contributor

@jumpeiMano jumpeiMano commented Dec 7, 2020

Edit: Resolves #3278

I modified sync.go to use SyncTimeout in Daemon.Sync function.
Because sometimes I have saw errors like this.

caller=loop.go:108 component=sync-loop err="loading resources from repo: 
error executing generator command \"kustomize build .\" from file \".flux.yaml\": context deadline exceeded
error output:
generated output:\n"

@jumpeiMano
Copy link
Contributor Author

@squaremo @stefanprodan Cloud you please check this PR? 🙇

@yebyen
Copy link
Contributor

yebyen commented Dec 13, 2020

Hi @jumpeiMano I wonder if you can elaborate on why this change is needed? I see you've provided some context already.

What is timing out, or is there a point at which the timeout was not being honored that this change fixes? How does this fix it? Could we add a test that shows how it was failing before, or can we give some explanation for why this change is needed?

Flux v1 is in maintenance mode, but I'm sure if this fixes something important it can be included in a next release soon!

@jumpeiMano
Copy link
Contributor Author

jumpeiMano commented Dec 14, 2020

@yebyen Thank you for your comment!

why this change is needed?

I am using the patchUpdated feature of flux v1. The command I have written in the .flux.yaml is below:

ls -d */ | xargs -I % sh -c 'echo ---; kustomize build %;'

This kustomize build command's result is sometimes huge. Often, there is a timeout error here, as described in the description.

In flux v1, there are some timeout-related flags, like git-timeout and sync-timeout, but neither of these flags are applied to this command execution, and the default context created by context.Background() is used, I have no way to extend the timeout value. Therefore, I have now made a fix so that the sync-timeout value is adopted in the Sync() function.

@dholbach dholbach added the blocked-dco Needs DCO to be fixed up label Jan 6, 2021
@kingdonb kingdonb added this to the 1.22.1 milestone Feb 22, 2021
@kingdonb kingdonb modified the milestones: 1.22.1, 1.22.0 Mar 11, 2021
@kingdonb
Copy link
Member

Hi, I'd really like to include this in 1.22.0 if it solves some weird timeout situations.

Can you add your --sign-off to the commit with

git commit --amend --signoff

and

git push --force-with-lease origin set-sync-timeout

per the details posted through DCO bot in the checks?

No need to rebase, as I will rebase anyway into #3442,
I should be able to force push into this branch, (but I cannot add your sign-off.)

@jumpeiMano
Copy link
Contributor Author

@kingdonb Thank you for your reviewing! Done!! d0ff266

@kingdonb kingdonb removed the blocked-dco Needs DCO to be fixed up label Mar 16, 2021
@kingdonb kingdonb modified the milestones: 1.22.0, 1.22.1 Mar 17, 2021
@kingdonb kingdonb modified the milestones: 1.22.1, 1.22.2 Mar 31, 2021
@yebyen
Copy link
Contributor

yebyen commented Apr 4, 2021

I think this resolves #3372

We should be able to test and close both of these in Flux 1.23.0, it may turn out to take a couple of weeks more, past the milestone assigned date though.

@kingdonb kingdonb self-requested a review April 16, 2021 21:27
@kingdonb kingdonb self-assigned this Apr 16, 2021
@kingdonb
Copy link
Member

I'm planning on merging this right after #3250 and cutting a release 1.22.2 that will include this change today.

Signed-off-by: jumpeiMano <jumpeim37@gmail.com>
Signed-off-by: Kingdon Barrett <kingdon@weave.works>
Copy link
Member

@kingdonb kingdonb left a comment

Choose a reason for hiding this comment

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

LGTM

@kingdonb kingdonb merged commit cf97726 into fluxcd:master Apr 23, 2021
@kingdonb kingdonb mentioned this pull request Apr 23, 2021
kingdonb pushed a commit that referenced this pull request Jul 11, 2021
This reverts commit cf97726, reversing
changes made to 5777baa.
kingdonb pushed a commit that referenced this pull request Jul 11, 2021
This reverts commit cf97726, reversing
changes made to 5777baa.

Signed-off-by: Kingdon Barrett <yebyen@gmail.com>
@kingdonb
Copy link
Member

kingdonb commented Jul 11, 2021

This PR may have caused an issue that is reported in #3500

If there is any insight you can provide, or an updated/different PR that does not cause the reported ephemeral storage growth issue (unbounded) when timeout is engaging, it would be preferable to fix forward than to simply revert the PR.

@jumpeiMano Do you mind taking a look at this issue, and letting me know if you can confirm the details reported there?

It appears this was a red herring, the culprit is most likely an upgrade to Kustomize.

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

Successfully merging this pull request may close these issues.

consistent timeout on fluxctl sync or fluxctl release (No way to debug)
4 participants