Skip to content

Make rep ping value timeout configurable #211

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Make rep ping value timeout configurable #211

wants to merge 1 commit into from

Conversation

@aemengo
Copy link

@aemengo aemengo commented Oct 17, 2016

We noticed that PCF Dev repeatedly failed to complete the rep ping step within the allotted time. We'd like it to be configurable.

Signed-off-by: Anthony Emengo [email protected]

@cfdreddbot
Copy link

cfdreddbot commented Oct 17, 2016

Hey aemengo!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you and the commit authors have already signed the CLA.

1 similar comment
@cfdreddbot
Copy link

cfdreddbot commented Oct 17, 2016

Hey aemengo!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you and the commit authors have already signed the CLA.

@cf-gitbot
Copy link

cf-gitbot commented Oct 17, 2016

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/132625721

The labels on this github issue will be updated when the story is started.

@emalm
Copy link
Contributor

emalm commented Oct 31, 2016

Thanks, @aemengo! Rather than have this timeout be independently configurable, I think it would make more sense for it to match the value of either the garden healthcheck timeout or the healthcheck period (

diego-release/jobs/rep/spec

Lines 148 to 153 in 6fcb7d5

diego.executor.garden_healthcheck.interval:
description: "Frequency for healtchecking garden"
default: "10m"
diego.executor.garden_healthcheck.timeout:
description: "Maximum allowed time for garden healthcheck"
default: "10m"
). Do you think you could try that, keeping in mind that those values are string representations of Golang durations instead of an integral number of seconds?

Thanks,
Eric

@aemengo
Copy link
Author

aemengo commented Oct 31, 2016

Yes absolutely, it would work perfectly fine for us.

@bdshroyer
Copy link

bdshroyer commented Nov 17, 2016

Hi @aemengo,

Any update on this?

Thanks,

Brandon

@aemengo
Copy link
Author

aemengo commented Nov 21, 2016

@bdshroyer @ematpl As I'm understanding, I should update the PR with a bash representation of the golang time.Duration garden values passed in through the spec. This seems rather unwieldy to me. Is there a more straightforward approach that we can think of here? I worry that a lot of context and explanation would be necessary for a change like that.

@emalm
Copy link
Contributor

emalm commented Nov 28, 2016

Hey, @aemengo, I'm saying that I think the value of diego.executor.garden_healthcheck.timeout is the appropriate value to use for this timeout as well. That BOSH property is currently specified in a way that makes sense for the rep to interpret as a Golang time.Duration, but for that to work in the bash pre-start script it likely has to be converted into seconds. I don't think it needs to handle all cases: hours, minutes, and seconds would be appropriate. Alternately, if there's a reasonable way to introduce a property with a numerical value in seconds to be used in both cases and to deprecate the diego.executor.garden_healthcheck.timeout property, that could work.

Thanks,
Eric

@mdelillo mdelillo force-pushed the make-rep-ping-value-configurable branch from 868abf9 to 6756721 Compare December 22, 2016 19:26
@mdelillo
Copy link
Contributor

mdelillo commented Dec 22, 2016

@ematpl @bdshroyer We've updated the PR to use make-rep-ping-value-configurable

@mdelillo && @chhhavi

@bdshroyer
Copy link

bdshroyer commented Dec 23, 2016

Merged by diego-release/6e98527.

@bdshroyer bdshroyer closed this Dec 23, 2016
sunjayBhatia added a commit that referenced this pull request Nov 21, 2019
@sunjayBhatia
bump bbs executor locket opencensus-proto go-control-plane protoc-gen…
1e997f4 
…-validate googleapis protobuf protoc-gen-validate

[finishes #169766863](https://www.pivotaltracker.com/story/show/169766863)

Submodule src/code.cloudfoundry.org/bbs c8e9aac..0cb4cac:
  > Regenerate models with latest protobuf
Submodule src/code.cloudfoundry.org/executor 71aaf90..739290b:
  > Updates for latest envoy go-control-plane
Submodule src/code.cloudfoundry.org/locket fc205fac2..b8526ba7b:
  > Regenerate models with latest protobuf
Submodule src/github.com/census-instrumentation/opencensus-proto 000000000...d89fa54de (new submodule)
Submodule src/github.com/envoyproxy/go-control-plane 8a91fb26f..34c8be46e:
  > Mirrored from envoyproxy/envoy @ 274f17cabe7f49f105c82919937ab710b9428261
  > Mirrored from envoyproxy/envoy @ 5536ce73026fcbf1c176c09d409a981d67981047
  > Mirrored from envoyproxy/envoy @ df6d3bc453167a8e9fd29662280859b4f56f0af8
  > Mirrored from envoyproxy/envoy @ 8e368e4bdfa8f220c5f4cb03ca61587ae1a3d118
  > Mirrored from envoyproxy/envoy @ 466c00b47c199601f81d45e7001fdd3840439d6e
  > Fix generic comparisons on protobuf messages (#241)
  > Mirrored from envoyproxy/envoy @ 443bc3345b0e3db99a3df03d52f317697b99d5d7
  > Adding support to shutdown handler (#239)
  > Mirrored from envoyproxy/envoy @ 20ca0ae3bdd9c2a69194203f5e1d2eca92ce2b48
  > Mirrored from envoyproxy/envoy @ cf74f816933d1350d7c588a3b8478dd399ce3d18
  > Mirrored from envoyproxy/envoy @ 0aed05aecd17576b0dd96f3e4126acd1c24a02bc
  > Mirrored from envoyproxy/envoy @ d9fc7f7a2d5c2098e191adb2502c8e04917abdd1
  > Mirrored from envoyproxy/envoy @ d36adbb5c4aad13af21f109b86978b8478f54409
  > Mirrored from envoyproxy/envoy @ 174ba21bb88b7d50d0b7efeefd0c43268b2afd5e
  > Mirrored from envoyproxy/envoy @ 57748e72f79e14217980b09d20c17f865459231f
  > Mirrored from envoyproxy/envoy @ ee17a88621b374715590f4cf62b15b4258a3d21f
  > Mirrored from envoyproxy/envoy @ 2d7e2bbc1496b0d9d1d1462fe5ddee2f1eaf374f
  > Mirrored from envoyproxy/envoy @ 86420eb64a750fd6025cf5efc8b846c8fe63f0c9
  > Mirrored from envoyproxy/envoy @ a55621cb23f69aac44a6a1fc5ba1ffd1475293e5
  > Mirrored from envoyproxy/envoy @ b4ca6034aae03083e65b058beaebc7404d5cf456
  > Mirrored from envoyproxy/envoy @ 63455de2ee5ee9b916ae73a805f8177996ded618
  > Mirrored from envoyproxy/envoy @ 7846427d7aafb9e023c38bb6e704262f7c724c92
  > Mirrored from envoyproxy/envoy @ e2cf41606c42cd18b5ac5fd2add9cf983ece9172
  > Mirrored from envoyproxy/envoy @ 6b3df463f5289e13076b9a55d37e289661ed6f42
  > Mirrored from envoyproxy/envoy @ c63c1e83989a10512546d67df44629029d1be23c
  > Mirrored from envoyproxy/envoy @ fbd056aae8d74e8acae8880c0766af588a6c3583
  > Mirrored from envoyproxy/envoy @ 68ca6746d168c6cea26d21479ef50b2be9aaa25d
  > Mirrored from envoyproxy/envoy @ f68368f1a497d8f9254a714c2694cee88477438d
  > Mirrored from envoyproxy/envoy @ b331807fcfccee404863284b55d12da6e1775df0
  > Mirrored from envoyproxy/envoy @ 8e0b240b0e0abcda07b7b259178bdad60a699019
  > Mirrored from envoyproxy/envoy @ e3247539bcc39f3111287b479a85e51655a6f1db (#232)
  > Mirrored from envoyproxy/envoy @ 72fc3606045c519264c4358808ebb88fb82d947c (#231)
  > Implement RTDS (#229)
  > Transition to use golang/protobuf to support automated code generation (#226)
  > fixing readme import and type in example (#224)
  > add http_inspector to wellknown.go (#219)
  > fix accesslog example (#216)
  > update go-control-plane (#214)
  > Changes the HTTP status code to 304 (as opposed to 500) when a fetch is skipped due to the client's version already being up-to-date. (#209)
  > add run_docker.sh and update README (#211)
  > update to the latest (#208)
  > add RBAC to wellknown.go (#207)
  > update data-plane-api to 5ced57515167940657b58ba771de97d753119aea (#204)
  > Replace gogo/googleapis with istio.io/gogo-genproto (#202)
  > Update generated files (#201)
  > add OWNERS.md (#200)
  > Update and fix script (#199)
  > update data-plane-api (#192)
  > unpin envoy (#189)
  > update data-plane-api (#188)
  > Update protobuf generation script (#181)
  > introduce go modules (#179)
  > update data-plane-api to da8a43117df30a8439d6290a34e9e82e19006d1e (#178)
  > Update data-plane-api to 758fe69d11f5dba13f8297e4cd615ef4be0cbc4c (#177)
  > update dependencies (#169)
  > Add a GetSnapshot method to SnapshotCache interface (#163)
  > Vhds protobufs (#167)
  > update data-plane-api to 698cf2f460236d2e6f6787394025c0145ca07713 (#166)
  > Update data plane to 3cb3571f093602b7d6a06945906802666febcf66 (#162)
  > update data-plane-api to 4f4ee118c54b7a52393c7d45b94f6bd5040a7118 (#158)
  > Update data-plane-api to 2fcac33dc159d3f6bab7cf84865177f75d32ba05 (#157)
  > test: use typed configs (#156)
  > Feature: Add support for stale-bot (#145)
  > Allow Callbacks.OnStreamRequest() to return an error to close stream (#155)
  > Update data-plane-api to 0afc79c323bcc0044381e8edb1dbdb87d35d6948 (#154)
  > Create a WKT for the MySQL Proxy (#152)
  > add SDS support (#56)
  > Update data-plane-api and istio.io/gogo-genproto (#150)
  > README: fix indentation of Go code block (#147)
  > Testing: Move creation of linux-test bin to docker (#143)
  > Upgrade: Update golang from 1.10 to 1.11 (#144)
Submodule src/github.com/envoyproxy/protoc-gen-validate 000000000...9eff07ddf (new submodule)
Submodule src/github.com/golang/protobuf b4deda097..6c65a5562:
  > jsonpb: fix marshaling of Duration
  > Log parsing errors using log pkg (#851)
  > fix indentation in jsonpb with Any messages
  > protoc-gen-go/grpc: use status and code packages only if needed (#820)
  > protoc-gen-go/grpc: inline errUnimplemented function (#818)
  > Add UnimplementedServer for server interface (#785)
  > Delete the conformance test (#808)
  > proto: remove test dependency on experimental packages (#805)
  > ptypes: More cleanly construct a Timestamp (#797)
  > ptypes: Avoid assuming time.Duration is nanoseconds (#796)
  > protoc-gen-go: fix oneof name mangling regression (#782)
  > README.md: document how to build a specific version of protoc-gen-go (#764)
  > protoc-gen-go: generate XXX_OneofWrappers instead of XXX_OneofFuncs (#760)
  > all: update to v3.6.1 (#757)
  > proto: store extension values according to protobuf data model (#746)
  > proto: deprecate {Unm,M}arshalMessageSet{JSON} (#741)
  > all: increase minimum supported version to go1.9 (#742)
  > protoc-gen-go: reference publicly imported symbols directly
  > protoc-gen-go: add test for use of publicly imported symbol
  > protoc-gen-go: include groups in public import forwarders
  > protoc-gen-go: expand import_public test files
  > protoc-gen-go: normalize floating-point default values (#737)
  > protoc-gen-go: generate deprecation comments for oneof fields
  > protoc-gen-go: add test case for deprecated oneof field
  > protoc-gen-go: don't force space after "//" in comments (#725)
  > protoc-gen-go/generator: fix misspelling (#732)
  >  .travis.yml: include go1.11 (#731)
  > protoc-gen-go: only disambiguate predefined idents for local package names (#724)
  > protoc-gen-go: predeclared identifiers in cleanPackageName (#722)
  > protoc-gen-go: put all imports in one section (#720)
  > protoc-gen-go: more standard import organization (#719)
  > protoc-gen-go: generate descriptor before plugins (#718)
  > protoc-gen-go: simplify init function generation (#716)
  > protoc-gen-go: move package comments to before the package decl (#715)
  > protoc-gen-go: fix symbol names in comments on public imports
  > protoc-gen-go: additional tests for public imports
  > protoc-gen-go: Don't set the json tag on extension fields. (#713)
  > protoc-gen-go: add more spaces between methods (#712)
  > protoc-gen-go: add blank line after extension method (#711)
  > protoc-gen-go: put imports in a single import() block (#707)
  > proto: remove commented-out code (#704)
  > proto: replace exprs with matching consts from math pkg (#705)
  > proto: replace len(b)<=0 with len(b)==0 (#706)
  > protoc-gen-go: stop generating package "// import" comment (#701)
  > protoc-gen-go: use consistent receiver name for messages (#700)
  > go.mod: add module file (#699)
  > protoc-gen-go: add blank lines between various enum functions (#697)
  > proto: uniquify descriptor var based on filename alone (#696)
  > proto: deprecate Stats data structure (#677)
  > proto: return more useful error message in SetExtension (#685)
  > proto: fix handling of required fields after multiple violations (#679)
  > Update issue templates
  > README.md: remove usage of group in example (#673)
  > jsonpb: avoid unexported fields in hand-crafted message (#671)
  > proto: make invalid UTF-8 errors non-fatal (#660)
  > jsonpb: strictly document JSONPBMarshaler and JSONPBUnmarshaler behavior (#662)
  > proto: revert strict erroring of deterministic and custom marshalers (#658)
  > protoc-gen-go: refactor generator by splitting up generateMessage (#657)
  > proto: fix and cleanup test for deterministic marshal with custom marshalers (#656)
  > jsonpb: avoid copying string-valued map-keys (#654)
  > proto: return errors when using deterministic marshaling with custom Marshalers
  > README: fix markdown formatting issue
  > proto: fix quadratic behavior in nested map marshaling (#641)
  > jsonpb: handle map key and value properties properly (#645)
  > jsonpb: decode int32/uint32/float32/float64 strings (#630)
  > ptypes: ran gofmt on any_test.go
  > Build grpc generated code as part of tests (#634)
  > proto: revert UTF-8 validation for proto2 (#628)
  > jsonpb: error on scalar enum provided for repeated enums instead of panic
  > proto: mention field name in error message (#616)
  > jsonpb: fix unmarshaling JSON object with escaped string into Struct type (#612)
  > grpc: fix and improve interface comments for client/server types (#604)
  > LICENSE: Move title to README.md
  > proto: adjust documentation on RequiredNotSetError (#603)
  > grpc: document ctx closure best practices (#600)
  > protoc-gen-go: remove deprecated function in grpc (#592) (#602)
  > ptypes: optimize Is to avoid prefix scan (#601)

Co-authored-by: Amin Jamali <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

accepted

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@aemengo @cfdreddbot @cf-gitbot @emalm @bdshroyer @mdelillo