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

Retry apply if conflict is detected #639

Merged
merged 1 commit into from Jun 25, 2018

Conversation

Projects
None yet
3 participants
@bryanl
Member

bryanl commented Jun 22, 2018

When applying objects to a cluster, there a small chance that a
conflict could arise. Instead of failing instantly, sleep for one
second, and retry up to five times.

Fixes #619

Signed-off-by: bryanl bryanliles@gmail.com

@bryanl bryanl requested a review from ksonnet/reviewers Jun 22, 2018

@coveralls

This comment has been minimized.

coveralls commented Jun 22, 2018

Pull Request Test Coverage Report for Build 1045

  • 111 of 150 (74.0%) changed or added relevant lines in 7 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.6%) to 71.731%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/cluster/merger.go 5 6 83.33%
pkg/cluster/managed_annotation.go 15 19 78.95%
pkg/cluster/upsert.go 43 49 87.76%
pkg/cluster/apply.go 30 58 51.72%
Files with Coverage Reduction New Missed Lines %
pkg/cluster/apply.go 2 43.61%
Totals Coverage Status
Change from base Build 1040: 0.6%
Covered Lines: 10708
Relevant Lines: 14928

💛 - Coveralls
// defaultObjectMerger merges an object with an object already in the cluster. This
// will ensure that important cluster values aren't overwritten.
type defaultObjectMerger struct {

This comment has been minimized.

@underrun

underrun Jun 25, 2018

Collaborator

should we also have this in here:

var _ objectMerger = (*defaultObjectMerger)(nil)
return string(newobj.GetUID()), nil
}
time.Sleep(a.conflictTimeout)

This comment has been minimized.

@underrun

underrun Jun 25, 2018

Collaborator

is it worth trying exponential backoff? 1 second is generally a long time. do you have any statistics on how frequently one retry is enough? i know 1 - 5 seconds isn't a terrible amount of time, but things add up and commandline and returning to the users quickly is nice.

also would be nice to have a debug level log entry to know a retry happened.

This comment has been minimized.

@bryanl

bryanl Jun 25, 2018

Member

That's an interesting thought. I modeled it this way based on what I thought I saw in kubectl. This is something that we should possibly explore in the future.

@@ -107,7 +105,6 @@ func newApplyCmd(a app.App) *cobra.Command {
actions.OptionSkipGc: viper.GetBool(vApplySkipGc),
}
fmt.Println("extract jsonnet flag")

This comment has been minimized.

@underrun

underrun Jun 25, 2018

Collaborator

is this worth logging instead?

This comment has been minimized.

@bryanl

bryanl Jun 25, 2018

Member

No, it was a debugging artifact.

@@ -109,24 +72,54 @@ type Apply struct {
// these make it easier to test Apply.

This comment has been minimized.

@underrun

underrun Jun 25, 2018

Collaborator

are all of the below (especially the things you added) just things to make testing easier - are any of them essential to Apply outside the config except for adding easy testing?

This comment has been minimized.

@bryanl
Retry apply if conflict is detected
When applying objects to a cluster, there a small chance that a
conflict could arise. Instead of failing instantly, sleep for one
second, and retry up to five times.

Fixes #619

Signed-off-by: bryanl <bryanliles@gmail.com>

@bryanl bryanl merged commit d3e2d4c into ksonnet:master Jun 25, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
signed-off-by Commit has Signed-off-by
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment