WIP: HALON-855: Inconsistent Mero drive status

[1468ca0b-2a64-4fb4-8e52-ea5806644b4c]

Created by: rajanikantchirmade

Add tentative fix.

TODO

• [ ] abort of SNS operation should reset states of involved storage devices [context]
• [ ] set PoolRepairStatus in ruleRebalanceStart [context]

[vvv]

closed

[rajanikantchirmade]

But it only change "PoolRepairStatus" not drive status.

[rajanikantchirmade]

marked the task abort of SNS operation should reset states of involved storage devices [context] as incomplete

[rajanikantchirmade]

marked the task abort of SNS operation should reset states of involved storage devices [context] as completed

[vvv]

changed the description

[vvv]

marked as a Work In Progress

[vvv]

1. Where PoolRepairStatus used to be set before this change?

I've found out that in the original code (master) PoolRepairStatus is being set in mkRepairStartOperation.mkRepairOperationStarted.

Sorry for the noise.

[rajanikantchirmade]

1. Where PoolRepairStatus used to be set before this change?

We did not changed code related to this. But we can check it on master by manually triggering abort using hctl command.

2. Shouldn't similar change be made in ruleRebalanceStart?

Yes, you are right.

[rajanikantchirmade]

Yes, this fix need to cover rebalance too.

[vvv]

@rajanikant.chirmade Please answer the remaining comments and propose a solution for the “sdevs remain in SDSInhibited SDSRepairing state” problem.

[vvv]

added 1 commit

• e028c618 - mkRepairStartOperation.mkRepairOperationStarted: handle error

Compare with previous version

[vvv]

changed this line in version 7 of the diff

[vvv]

changed this line in version 7 of the diff

[vvv]

@rajanikant.chirmade ?

[vvv]

added 1 commit

• aff15c40 - Refactor ruleOnSnsOperationQuiesceFailure

Compare with previous version

[vvv]

changed this line in version 6 of the diff

[vvv]

changed this line in version 6 of the diff

[vvv]

changed this line in version 6 of the diff

[vvv]

[nit] Use blank lines to separate sections of code.

[vvv]

1. Where PoolRepairStatus used to be set before this change?
2. Shouldn't similar change be made in ruleRebalanceStart?

[vvv]

added 1 commit

• 6e389ed5 - s/abortRepairFromProc/abortSnsOperations/; document & refactor

Compare with previous version

[vvv]

changed this line in version 5 of the diff

[vvv]

changed this line in version 5 of the diff

[vvv]

changed this line in version 5 of the diff

[vvv]

Document.

[vvv]

                        , getConfObjState sdev rg `elem` [ M0_NC_REPAIR
                                                         , M0_NC_REBALANCE
                                                         ]

?

[vvv]

What about M0_NC_REBALANCE? I think we want to abort it as well. In fact, we want to abort any ongoing SNS operation.

[vvv]

Since abortRepairFromProc is called for any timed-out process (see ruleProcessKeepaliveReply), it is quite possible that proc does not host CST_IOS service. In this case pools will be null and Log.DEBUG "Repair not running" message will be printed.

Are we okay with that?

[vvv]

added 1 commit

• 01eaa618 - Refactor ruleProcessKeepaliveReply

Compare with previous version

[vvv]

changed this line in version 4 of the diff

[vvv]

changed this line in version 4 of the diff

[vvv]

changed this line in version 4 of the diff

[vvv]

Prelude. is not needed.

[vvv]

Suggestion: add a type signature.

    getProcs :: [(Fid, M0.TimeSpec)] -> Graph -> [(M0.Process, M0.TimeSpec)]
    getProcs fids rg = [ (p, t)
                       | (fid, t) <- fids
                       , Just p <- [M0.lookupConfObjByFid fid rg]
                       ]

[vvv]

[nit] Remove braces.

[vvv]

added 3 commits

• 9ac55f14 - 1 commit from branch master
• 43ee86e7 - HALON-855: Inconsistent Mero drive status
• 17e051e4 - Replace getSDevPool with poolFromSdevM

Compare with previous version

[vvv]

assigned to @rajanikant.chirmade

[vvv]

@rajanikant.chirmade You've mentioned some problem with this patch. Can you describe what exactly is not working?

AFAIU, the $M0_CLUSTER file you test against looks like this:

clovis-apps: [ client1.local ]
confds:      [ cmu.local ]
ssus:
  - host:  ssu1.local
    disks: /dev/sd[b-h]
  - host:  ssu2.local
    disks: /dev/sd[b-h]

How do you test the patch? What happens? What is the expected behaviour?

[vvv]

uuid is unnecessary, we can do fine without auxiliary identifier.

[vvv]

fromJust is dangerous and should generally be avoided. It will throw runtime error if prs is Nothing.

[vvv]

Use explicit import list, i.e.

import           HA.RecoveryCoordinator.Castor.Drive.Rules.Repair (abortRepairFromProc)

[vvv]

[nit] Parentheses are not needed here.

[vvv]

added 6 commits

• 6e39a9ad...1deafefd - 4 commits from branch master
• c422ff58 - HALON-855: Inconsistent Mero drive status
• 108402d9 - Replace getSDevPool with poolFromSdevM

Compare with previous version

[chumakd]

assigned to @valery.vorotyntsev

[1468ca0b-2a64-4fb4-8e52-ea5806644b4c]

Created by: rajanikantchirmade

Yes, this need to be fix. Cherry picked your patch. Thanks.

[1468ca0b-2a64-4fb4-8e52-ea5806644b4c]

Created by: vvv

What if several sdevs of the proc are being repaired (M0_NC_REPAIR)? What if two of those sdevs belong different pools?