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 --git-timeout configuration flags #1416

Merged
merged 5 commits into from Oct 4, 2018
Merged

Conversation

@hiddeco
Copy link
Member

hiddeco commented Oct 1, 2018

Closes #1414

This PR adds a --git-time-out --git-timeout flag to both fluxd and the helm operator.

I was unsure what to do with the gitOpTimeout constant in daemon/loop.go:25. If someone could give me guidance in this I am more than happy to make additional changes.

Resulting builds from changes in this PR can be pulled and tested from:
hiddeco/flux:1414-git-timeout-9dc6b2181744
hiddeco/helm-operator:1414-git-timeout-9dc6b2181744

@sfrique

This comment has been minimized.

Copy link
Contributor

sfrique commented Oct 1, 2018

I don't know who takes care of the helm chart, but it would be nice to add the option there as well.
the current values -> https://github.com/weaveworks/flux/blob/master/chart/flux/values.yaml has quite a few options.

I tested and it fixed the problem I was having, thanks a lot.


// Chart releases sync due to Custom Resources changes -------------------------------
{
mainLogger.Log("info", "Attempting to clone repo ...", "url", gitRemote.URL)
ctx, cancel := context.WithTimeout(context.Background(), 1*time.Minute)
ctx, cancel := context.WithCancel(context.Background())

This comment has been minimized.

Copy link
@justinbarrick

justinbarrick Oct 3, 2018

Collaborator

Why did we drop the timeout here?

This comment has been minimized.

Copy link
@hiddeco

hiddeco Oct 4, 2018

Author Member

Because every git operation (step) has its own timeout set. If we leave the timeout here it could cut off early instead of respecting the user’s configuration flag.

Copy link
Member

squaremo left a comment

I have reservations over using the same timeout for all git operations -- increasing the timeout for git clone might mean that other operations take much longer to fail. But I don't know that's a problem, and it can be addressed if it turns out to be. Meanwhile, this is worth it just for removing all the ad-hoc constants :-)

One request, as commented.

@@ -93,6 +93,7 @@ func main() {
gitSkipMessage = fs.String("git-ci-skip-message", "", "additional text for commit messages, useful for skipping builds in CI. Use this to supply specific text, or set --git-ci-skip")

gitPollInterval = fs.Duration("git-poll-interval", 5*time.Minute, "period at which to poll git repo for new commits")
gitTimeOut = fs.Duration("git-time-out", 20*time.Second, "duration after which git operations time out")

This comment has been minimized.

Copy link
@squaremo

squaremo Oct 4, 2018

Member

"Timeout" as a noun is usually one word -- "I gave the operation a timeout." I suggest making the flag --git-timeout, and spelling variables likewise.

This comment has been minimized.

Copy link
@hiddeco

hiddeco Oct 4, 2018

Author Member

I have reservations over using the same timeout for all git operations -- increasing the timeout for git clone might mean that other operations take much longer to fail.

You are not alone with this, I had/have the same concerns. This can be solved eventually by adding a second flag that defines a budget for all operations together (each operation may take x but all together it may not take longer than y).


As for the spelling - will make adjustments later today.

@hiddeco hiddeco changed the title Add --git-time-out configuration flags Add --git-timeout configuration flags Oct 4, 2018
Copy link
Member

squaremo left a comment

Grand, thanks Hidde 🐝

@@ -190,7 +190,7 @@ The following tables lists the configurable parameters of the Weave Flux chart a
| `git.label` | Label to keep track of sync progress, used to tag the Git branch | `flux-sync`
| `git.ciSkip` | Append "[ci skip]" to commit messages so that CI will skip builds | `false`
| `git.pollInterval` | Period at which to poll git repo for new commits | `5m`
| `git.timeOut` | Duration after which git operations time out | `20s`
| `git.timeout` | Duration after which git operations timeout | `20s`

This comment has been minimized.

Copy link
@squaremo

squaremo Oct 4, 2018

Member

Now I feel like a pedant, but the verb is "to time out"

This comment has been minimized.

Copy link
@hiddeco

hiddeco Oct 4, 2018

Author Member

Never drop out of high school to pursue your dreams as an engineer kids, it will bite you later

Changed like it never happened

@hiddeco hiddeco force-pushed the hiddeco:1414-git-timeout branch from 5978219 to b9c21dc Oct 4, 2018
@hiddeco hiddeco merged commit ebc78e3 into fluxcd:master Oct 4, 2018
1 check passed
1 check passed
ci/circleci: build Your tests passed on CircleCI!
Details
@hiddeco hiddeco deleted the hiddeco:1414-git-timeout branch Oct 4, 2018
@latchmihay

This comment has been minimized.

Copy link

latchmihay commented Feb 25, 2019

the gitOpTimeout constant in daemon/loop.go:25 which is currently 15s gets hit on slower/remote networks, and it waits for entire loop cycle until its retried.

Is it possible for it to use--git-timeout argument for gitOpTimeout constant. I understand the concerns that it will slow everything down to timeout, but it should be fine to whoever is changing it.

Having the --git-timeout default of 20s to gitOpTimeout constant shouldn't be that big of a deal since its additional 5s

@hiddeco

This comment has been minimized.

Copy link
Member Author

hiddeco commented Feb 25, 2019

@latchmihay your reasoning makes sense to me and the additional 5s probably does not hurt anyone.

I will turn it into a PR tomorrow and see what others think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.