Return possible states a heal operation #4045

krisis · 2017-04-04T15:38:05Z

Description

Background

Healing algorithm returns failure only if an attempt to heal any of the disks failed. It returns success even if all disks needing heal were offline.
In its current form, heal admin REST APIs returns the above boolean state making it impossible for clients like mc to distinguish between an object being healed completely, partially and not.

Solution

The fix (so far) is to return struct { HealedCount int, OfflineCount int} in addition to error for a heal-object and heal-upload operation. mc can make use of this to guide users better.

Future-proof solution

This brings us to whether it is right by design to provide internal details of the healing process in admin API. We return an enumerated constant in heal-object and heal-upload API that captures the 3 result states of the heal operation, namely, CompletelyHealed, PartiallyHealed and NotHealed. In addition, we would return error to indicate if the healing operation failed to due to other I/O related errors.

Motivation and Context

How Has This Been Tested?

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-04T15:38:13Z

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

codecov-io · 2017-04-04T15:56:51Z

Codecov Report

Merging #4045 into master will increase coverage by 0.01%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##           master    #4045      +/-   ##
==========================================
+ Coverage   68.23%   68.25%   +0.01%     
==========================================
  Files         174      174              
  Lines       24125    24133       +8     
==========================================
+ Hits        16461    16471      +10     
+ Misses       6553     6551       -2     
  Partials     1111     1111

Impacted Files	Coverage Δ
pkg/madmin/heal-commands.go	`0% <ø> (ø)`	⬆️
cmd/xl-v1-healing.go	`72.89% <100%> (-0.28%)`	⬇️
cmd/admin-handlers.go	`57.09% <83.33%> (+1.32%)`	⬆️
cmd/fs-v1-background-append.go	`78.52% <0%> (-1.35%)`	⬇️

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 d103d5f...d6abb4c. Read the comment docs.

deekoder · 2017-04-06T00:44:53Z

Can you update the API doc https://github.com/minio/minio/tree/master/docs/admin-api with the details?

deekoder

Change looks ok. But please update the admin-api docs as mentioned in the comments.

krisis · 2017-04-08T08:06:22Z

@deekoder I have added necessary admin-API docs.

harshavardhana · 2017-04-08T08:17:42Z

cmd/admin-handlers.go

+)
+
+// newHealResult - returns healResult given number of disks healed and
+// number of disks offline


Can you add a test case for this alone?

@harshavardhana Added a unit test.

harshavardhana · 2017-04-09T06:51:06Z

@deekoder @krisis has fixed the admin API docs can you review again?

harshavardhana · 2017-04-09T06:51:43Z

ping @vadmeste need your review here..

vadmeste · 2017-04-09T15:37:16Z

pkg/madmin/heal-commands.go

@@ -494,10 +494,21 @@ func (adm *AdminClient) HealUpload(bucket, object, uploadID string, dryrun bool)

 // HealResult - represents result of heal-object admin API.
 type HealResult struct {
-	HealedCount  int // number of disks that were healed.
-	OfflineCount int // number of disks that needed healing but were offline.
+	State HealState


Can you set a json tag here? json:"state"

vadmeste

One comment, LGTM otherwise

deekoder · 2017-04-11T18:34:40Z

I will schedule the gist review today with @abperiasamy

harshavardhana · 2017-04-11T20:15:40Z

I will schedule the gist review today with @abperiasamy

Is this PR waiting on the gist review? - we can merge this and proceed with gist review with a separate PR as a doc cleanup.

deekoder · 2017-04-12T21:59:07Z

cmd/admin-handlers.go

+
+const (
+	// notHealed - none of the disks healed
+	notHealed healState = iota


@abperiasamy : By prefixing heal, there is namespace pollution. Use healOK, healFailed, healPartial everywhere. Always group them with common prefix to help IDEs autocomplete.

deekoder · 2017-04-12T22:03:37Z

pkg/madmin/heal-commands.go

@@ -494,10 +494,21 @@ func (adm *AdminClient) HealUpload(bucket, object, uploadID string, dryrun bool)

 // HealResult - represents result of heal-object admin API.


@abperiasamy : healState is duplicated in cmd/admin-handlers.go

This is on purpose the struct on cmd/ cannot be used with madmin package.. Just like how minio-go has ListObjects struct which is similar to what server has as ListObjectInfo. All elements are same.

Since madmin is imported into mc it is not required for minio server to be vendorized in mc to get admin API working. So it is written like an independent SDK capable of parsing all the Admin API REST responses and requests.

harshavardhana · 2017-04-12T22:10:04Z

@abperiasamy : healState is duplicated in cmd/admin-handlers.go

This is on purpose the struct on cmd/ cannot be used with madmin package.. Just like how minio-go has ListObjects struct which is similar to what server has as ListObjects.

Since madmin is imported into mc it is not required for minio server to be vendorized in mc to get admin API working. So it is written like an independent SDK capable of parsing all the Admin API REST responses and requests.

harshavardhana · 2017-04-13T06:22:02Z

pkg/madmin/API.md

+
+| Value | Description |
+|---|---|
+|`NotHealed` | Object/Upload wasn't healed on any of the disks |


This should be fixed as well @krisis ?

@harshavardhana yes, thanks for catching that. I have updated the PR fixing this.

krisis · 2017-04-13T06:22:52Z

@deekoder I have changed the healState values to be HealNone, HealPartial and HealOK respectively.

deekoder requested review from harshavardhana and abperiasamy April 5, 2017 17:48

deekoder self-requested a review April 6, 2017 01:18

deekoder suggested changes Apr 6, 2017

View reviewed changes

krisis force-pushed the feat/madmin-heal branch from 52ebeb4 to 3afce69 Compare April 7, 2017 06:27

krisis changed the title ~~[need-review] Return possible states a heal operation~~ Return possible states a heal operation Apr 7, 2017

krisis force-pushed the feat/madmin-heal branch from 3afce69 to dc9f99c Compare April 7, 2017 06:31

krisis mentioned this pull request Apr 7, 2017

admin-api: Issue when heal is attempted while a node is down. #3980

Closed

krisis force-pushed the feat/madmin-heal branch from dc9f99c to e43f48f Compare April 8, 2017 07:36

harshavardhana requested changes Apr 8, 2017

View reviewed changes

krisis force-pushed the feat/madmin-heal branch from e43f48f to 1f2f383 Compare April 8, 2017 08:43

harshavardhana approved these changes Apr 8, 2017

View reviewed changes

harshavardhana requested review from donatello and removed request for abperiasamy April 9, 2017 06:50

harshavardhana requested review from vadmeste and removed request for donatello April 9, 2017 06:51

vadmeste reviewed Apr 9, 2017

View reviewed changes

vadmeste requested changes Apr 9, 2017

View reviewed changes

harshavardhana added the dev-review label Apr 9, 2017

krisis force-pushed the feat/madmin-heal branch from 1f2f383 to e70ce50 Compare April 10, 2017 04:04

vadmeste approved these changes Apr 12, 2017

View reviewed changes

deekoder reviewed Apr 12, 2017

View reviewed changes

krisis force-pushed the feat/madmin-heal branch from e70ce50 to e051317 Compare April 13, 2017 06:21

harshavardhana requested changes Apr 13, 2017

View reviewed changes

Return possible states a heal operation

d6abb4c

krisis force-pushed the feat/madmin-heal branch from e051317 to d6abb4c Compare April 13, 2017 06:26

harshavardhana approved these changes Apr 13, 2017

View reviewed changes

deekoder requested a review from krishnasrinivas April 13, 2017 15:39

deekoder approved these changes Apr 14, 2017

View reviewed changes

harshavardhana merged commit ca64b86 into minio:master Apr 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return possible states a heal operation #4045

Return possible states a heal operation #4045

krisis commented Apr 4, 2017 •

edited by harshavardhana

Loading

mention-bot commented Apr 4, 2017

codecov-io commented Apr 4, 2017 •

edited

Loading

deekoder commented Apr 6, 2017

deekoder left a comment

krisis commented Apr 8, 2017

harshavardhana Apr 8, 2017

krisis Apr 8, 2017

harshavardhana commented Apr 9, 2017

harshavardhana commented Apr 9, 2017

vadmeste Apr 9, 2017

vadmeste left a comment

deekoder commented Apr 11, 2017

harshavardhana commented Apr 11, 2017

deekoder Apr 12, 2017 •

edited

Loading

deekoder Apr 12, 2017

harshavardhana Apr 12, 2017 •

edited

Loading

harshavardhana commented Apr 12, 2017

harshavardhana Apr 13, 2017

krisis Apr 13, 2017

krisis commented Apr 13, 2017

		@@ -494,10 +494,21 @@ func (adm *AdminClient) HealUpload(bucket, object, uploadID string, dryrun bool)

		// HealResult - represents result of heal-object admin API.

Return possible states a heal operation #4045

Return possible states a heal operation #4045

Conversation

krisis commented Apr 4, 2017 • edited by harshavardhana Loading

Description

Background

Solution

Future-proof solution

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

mention-bot commented Apr 4, 2017

codecov-io commented Apr 4, 2017 • edited Loading

Codecov Report

deekoder commented Apr 6, 2017

deekoder left a comment

Choose a reason for hiding this comment

krisis commented Apr 8, 2017

harshavardhana Apr 8, 2017

Choose a reason for hiding this comment

krisis Apr 8, 2017

Choose a reason for hiding this comment

harshavardhana commented Apr 9, 2017

harshavardhana commented Apr 9, 2017

vadmeste Apr 9, 2017

Choose a reason for hiding this comment

vadmeste left a comment

Choose a reason for hiding this comment

deekoder commented Apr 11, 2017

harshavardhana commented Apr 11, 2017

deekoder Apr 12, 2017 • edited Loading

Choose a reason for hiding this comment

deekoder Apr 12, 2017

Choose a reason for hiding this comment

harshavardhana Apr 12, 2017 • edited Loading

Choose a reason for hiding this comment

harshavardhana commented Apr 12, 2017

harshavardhana Apr 13, 2017

Choose a reason for hiding this comment

krisis Apr 13, 2017

Choose a reason for hiding this comment

krisis commented Apr 13, 2017

krisis commented Apr 4, 2017 •

edited by harshavardhana

Loading

codecov-io commented Apr 4, 2017 •

edited

Loading

deekoder Apr 12, 2017 •

edited

Loading

harshavardhana Apr 12, 2017 •

edited

Loading