notify: Webhook endpoints can fail, but we must start the server. #4060

harshavardhana · 2017-04-07T02:36:55Z

Description

Ignore any network errors when registering a webhook
notifier during Minio startup sequence. This way server
can be started even if the webhook endpoint is not available
and unreachable.

Motivation and Context

Fixes #4050

How Has This Been Tested?

Manually with wrong config.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing tests passed.

mention-bot · 2017-04-07T02:36:57Z

@harshavardhana, thanks for your PR! By analyzing the history of the files in this pull request, we identified @donatello, @balamurugana and @krishnasrinivas to be potential reviewers.

codecov-io · 2017-04-07T03:05:04Z

Codecov Report

Merging #4060 into master will decrease coverage by 0.06%.
The diff coverage is 54.32%.

@@            Coverage Diff             @@
##           master    #4060      +/-   ##
==========================================
- Coverage   67.71%   67.64%   -0.07%     
==========================================
  Files         175      175              
  Lines       24044    24106      +62     
==========================================
+ Hits        16281    16307      +26     
- Misses       6643     6674      +31     
- Partials     1120     1125       +5

Impacted Files	Coverage Δ
cmd/utils.go	`84.96% <ø> (-0.76%)`	⬇️
cmd/event-notifier.go	`72.56% <ø> (ø)`	⬆️
cmd/notify-webhook.go	`67.72% <54.32%> (-17.68%)`	⬇️
cmd/fs-v1-background-append.go	`78.52% <0%> (-1.35%)`	⬇️
cmd/xl-v1-utils.go	`92.18% <0%> (+0.82%)`	⬆️
cmd/xl-v1-metadata.go	`72.85% <0%> (+0.95%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 60ea2b1...fd12f34. Read the comment docs.

vadmeste · 2017-04-07T09:51:56Z

cmd/notify-webhook.go

+
+		// Redo the request with the new redirect url if http 307
+		// is returned, quit the loop otherwise
+		if resp.StatusCode == http.StatusTemporaryRedirect {


Can you add a TODO here to check later if golang 1.8 follows 307 after POST ?
golang/go#7912

There is nothing do for since its already implemented in go1.8?

There is nothing do for since its already implemented in go1.8?

Yes

vadmeste · 2017-04-07T09:58:58Z

cmd/notify-webhook.go

+	// Set proper server user-agent.
+	req.Header.Set("User-Agent", globalServerUserAgent)
+
+	for {


Looks like 'for' here is only needed to follow redirect urls, can you also add a comment here ?

vadmeste · 2017-04-07T10:03:39Z

cmd/notify-webhook.go

+}
+
+// isNetErrorRetryable - is network error retryable.
+func isNetErrorRetryable(err error) bool {


This PR is supposed to ignore error like 'connection refused' for example when remote port is closed, can you add more comments to understand ?

Connection refused is already *net.OpError ..

Ignore any network errors when registering a webhook notifier during Minio startup sequence. This way server can be started even if the webhook endpoint is not available and unreachable. Fixes minio#4050

This PR fixes a regression introduced in minio#4060

This PR fixes a regression introduced in #4060

harshavardhana requested review from vadmeste and donatello April 7, 2017 02:37

harshavardhana added the dev-review label Apr 7, 2017

harshavardhana added this to the Edge cache milestone Apr 7, 2017

harshavardhana force-pushed the do-not-fail branch 2 times, most recently from dc3009d to 70eb344 Compare April 7, 2017 03:00

harshavardhana force-pushed the do-not-fail branch 2 times, most recently from d1ad709 to ae72ed0 Compare April 7, 2017 07:17

vadmeste reviewed Apr 7, 2017

View reviewed changes

harshavardhana force-pushed the do-not-fail branch 2 times, most recently from 5a19db4 to 58cfc4c Compare April 7, 2017 11:01

notify: Webhook endpoints can fail, but we start server.

fd12f34

Ignore any network errors when registering a webhook notifier during Minio startup sequence. This way server can be started even if the webhook endpoint is not available and unreachable. Fixes minio#4050

harshavardhana force-pushed the do-not-fail branch from 58cfc4c to fd12f34 Compare April 7, 2017 21:12

vadmeste approved these changes Apr 7, 2017

View reviewed changes

donatello approved these changes Apr 8, 2017

View reviewed changes

harshavardhana changed the title ~~notify: Webhook endpoints can fail, but we start server.~~ notify: Webhook endpoints can fail, but we must start server. Apr 8, 2017

harshavardhana changed the title ~~notify: Webhook endpoints can fail, but we must start server.~~ notify: Webhook endpoints can fail, but we must start the server. Apr 8, 2017

harshavardhana merged commit 6b4f368 into minio:master Apr 8, 2017

harshavardhana deleted the do-not-fail branch April 8, 2017 08:13

harshavardhana added a commit to harshavardhana/minio that referenced this pull request May 4, 2017

Webhook endpoints can fail, we must start the server.

4752430

This PR fixes a regression introduced in minio#4060

harshavardhana mentioned this pull request May 4, 2017

Webhook endpoints can fail, we must start the server. #4255

Merged

8 tasks

harshavardhana added a commit to harshavardhana/minio that referenced this pull request May 4, 2017

Webhook endpoints can fail, we must start the server.

d2c7dc0

This PR fixes a regression introduced in minio#4060

harshavardhana added a commit to harshavardhana/minio that referenced this pull request May 4, 2017

Webhook endpoints can fail, we must start the server.

340f00b

This PR fixes a regression introduced in minio#4060

deekoder pushed a commit that referenced this pull request May 4, 2017

Webhook endpoints can fail, we must start the server. (#4255)

df027a8

This PR fixes a regression introduced in #4060

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

notify: Webhook endpoints can fail, but we must start the server. #4060

notify: Webhook endpoints can fail, but we must start the server. #4060

harshavardhana commented Apr 7, 2017 •

edited

Loading

mention-bot commented Apr 7, 2017

codecov-io commented Apr 7, 2017 •

edited

Loading

vadmeste Apr 7, 2017

harshavardhana Apr 7, 2017

vadmeste Apr 7, 2017

vadmeste Apr 7, 2017

vadmeste Apr 7, 2017

harshavardhana Apr 7, 2017

notify: Webhook endpoints can fail, but we must start the server. #4060

notify: Webhook endpoints can fail, but we must start the server. #4060

Conversation

harshavardhana commented Apr 7, 2017 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

mention-bot commented Apr 7, 2017

codecov-io commented Apr 7, 2017 • edited Loading

Codecov Report

vadmeste Apr 7, 2017

Choose a reason for hiding this comment

harshavardhana Apr 7, 2017

Choose a reason for hiding this comment

vadmeste Apr 7, 2017

Choose a reason for hiding this comment

vadmeste Apr 7, 2017

Choose a reason for hiding this comment

vadmeste Apr 7, 2017

Choose a reason for hiding this comment

harshavardhana Apr 7, 2017

Choose a reason for hiding this comment

harshavardhana commented Apr 7, 2017 •

edited

Loading

codecov-io commented Apr 7, 2017 •

edited

Loading