Skip to content
This repository was archived by the owner on Sep 9, 2020. It is now read-only.

Retry apply if conflict is detected#639

Merged
bryanl merged 1 commit intoksonnet:masterfrom
bryanl:retry-apply-if-possible
Jun 25, 2018
Merged

Retry apply if conflict is detected#639
bryanl merged 1 commit intoksonnet:masterfrom
bryanl:retry-apply-if-possible

Conversation

@bryanl
Copy link
Member

@bryanl 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 a team June 22, 2018 17:01
@coveralls
Copy link

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

@bryanl bryanl force-pushed the retry-apply-if-possible branch 2 times, most recently from 4a34191 to 5d31fb4 Compare June 25, 2018 18:58

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

Choose a reason for hiding this comment

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

should we also have this in here:

var _ objectMerger = (*defaultObjectMerger)(nil)


return string(newobj.GetUID()), nil
}
time.Sleep(a.conflictTimeout)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

actions.OptionSkipGc: viper.GetBool(vApplySkipGc),
}

fmt.Println("extract jsonnet flag")
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this worth logging instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it was a debugging artifact.

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

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes.

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 ksonnet#619

Signed-off-by: bryanl <bryanliles@gmail.com>
@bryanl bryanl force-pushed the retry-apply-if-possible branch from 5d31fb4 to 97a91b5 Compare June 25, 2018 23:00
@bryanl bryanl merged commit d3e2d4c into ksonnet:master Jun 25, 2018
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.

3 participants