Arvados

12708-balance-storage-classes @ 3ce3d9f884aed70cc84155554defe614a1bcfaaa

https://ci.curoverse.com/job/developer-run-tests/689/

• keep-balance needs to take all relevant collections' storage_classes_desired fields into account when deciding to pull/trash replicas of a given block.
• When sending pull requests, keep-balance should supply (in "mount_uuid") the UUID of a mount that offers the appropriate storage classes.

• keep-balance's log should indicate whether all desired storage classes are achieved. (TBD: how much detail / how should details be reported?) (can't really tell the difference between "no pulls because repl is OK" and "repl not OK but there's nowhere left to pull to")
• keep-balance's log should indicate whether all desired storage classes are achievable.

Need to figure out the most useful way to report these things. Meanwhile "move to desired storage classes where possible" is working.

• func (bal *Balancer) setupCaches() {
  

nit: The purpose of this method seems to be to fill in mountsByClass but I wouldn't use the word "cache", it is more like a "lookup table" (so suggesting setupCaches() → setupLookupTables()

• As a minor optimization, you could calculate the number of slots ahead of time, so that on each run of "balanceBlock" the slots can be allocated once with correct capacity to avoid any reallocation.

• I see that a newly written block can't be trashed until the TTL expires. I also see that it preferentially pulls blocks to slots with the desired storage class. This means blocks written to "default" but intended for a different storage class will tend to be over-replicated for some period of time, which is counter-productive if the goal of storage classes is cost savings. Maybe the sorting should always prefer read-only slots? Ideally the SDKs will write to the intended storage class directly (#7929 and #7930).

• We need a documentation page in the admin/ sections about using keep-balance. I suppose it would be good to merge #13282 first.

• We discussed that a collection can specify multiple storage classes, in which case the block is supposed to go only to mounts that satisfy all the storage classes. As a simplifying assumption we're only allowing one storage class. If I squint a little bit I think I see a way to handle multiple storage classes without having to totally overhaul the "distribute to slots" algorithm, but if you have any thoughts on that, it might make a nice TODO comment for whenever we need to implement that feature.

• I know this is not part of the ticket but I really hate the notation 0=have<want, it is not common mathematical notation and it is important for admins to clearly understand what the report is saying, could we please print something less ambiguous like (0=have)<want or have<want & have=0 (this could also be addressed with a keep-balance admin page).

    bal.logf("%s lost (0=have<want)", bal.stats.lost)
    bal.logf("%s underreplicated (0<have<want)", bal.stats.underrep)
    bal.logf("%s just right (have=want)", bal.stats.justright)
    bal.logf("%s overreplicated (have>want>0)", bal.stats.overrep)
    bal.logf("%s unreferenced (have>want=0, new)", bal.stats.unref)
    bal.logf("%s garbage (have>want=0, old)", bal.stats.garbage)

• What's your sense of how this is all going to scale when it needs to balance ten million blocks?

How would we do an integration test for this? Ideally we should demonstrate it successfully moving blocks on an actual cluster before we try and put it into production...

Peter Amstutz wrote:

nit: The purpose of this method seems to be to fill in mountsByClass but I wouldn't use the word "cache", it is more like a "lookup table" (so suggesting setupCaches() → setupLookupTables()

Agreed, done.

• As a minor optimization, you could calculate the number of slots ahead of time, so that on each run of "balanceBlock" the slots can be allocated once with correct capacity to avoid any reallocation.

Done.

• I see that a newly written block can't be trashed until the TTL expires. I also see that it preferentially pulls blocks to slots with the desired storage class. This means blocks written to "default" but intended for a different storage class will tend to be over-replicated for some period of time, which is counter-productive if the goal of storage classes is cost savings.

"Everything is written to default, and later moved to cheaper storage" is the first use case we want to address, and in that case temporary over-replication is inevitable. Aside from that, no amount of client-side support would have been particularly useful without keep-balance support anyway -- keep-balance would just end up unbalancing whatever the client managed to achieve.

Maybe the sorting should always prefer read-only slots?

It does prefer read-only slots if they satisfy the requirements. Do you mean it should prefer read-only slots that don't satisfy the requirements? That would be an optimization if there was a strict ordering of classes. But I don't think it would work if you had classes like "more-private" and "more-durable": "you asked me to put a copy in more-durable, but we already have a copy in a readonly more-private volume, so I didn't bother making a more-durable copy".

• We discussed that a collection can specify multiple storage classes, in which case the block is supposed to go only to mounts that satisfy all the storage classes. As a simplifying assumption we're only allowing one storage class. If I squint a little bit I think I see a way to handle multiple storage classes without having to totally overhaul the "distribute to slots" algorithm, but if you have any thoughts on that, it might make a nice TODO comment for whenever we need to implement that feature.

Added.

• I know this is not part of the ticket but I really hate the notation 0=have<want, it is not common mathematical notation and it is important for admins to clearly understand what the report is saying, could we please print something less ambiguous like (0=have)<want or have<want & have=0 (this could also be addressed with a keep-balance admin page).

Improving the report sounds great but I'm not sure that's a good place to start. Anyway, since this isn't part of the ticket, I think we should leave it alone for now.

• What's your sense of how this is all going to scale when it needs to balance ten million blocks?

Time is dominated by retrieving collections from API server, and I don't expect that to change here. A map[string]int is bigger than an int, so memory use will be higher. I've added a TODO for pooling Desired maps, so we can reduce memory per megablock if it becomes an issue.

(Aside: The ultimate scaling device is sharding the blocks: balance [0-7].*, then [8-f].*, etc. This makes each balancing cycle cost more CPU time, but scales to an arbitrary number of blocks with a given amount of memory per keep-balance machine.)

12708-balance-storage-classes @ 2957b917aaaefc1485e1c5293d413f0931b3030a

12708-balance-storage-classes LGTM

I'd like to make an additional request for a keep-balance documentation page for the "admin" section of the docs. Among other things, it should explain to ops how to use storage classes. (Maybe also your recent adventures in "how to make sure data gets trashed")

12708-report-storage-classes @ 5012102bb1854af9ae591c755feb2ea9c874b708

https://ci.curoverse.com/job/developer-run-tests/709/

I was going to ask for logging test additions, but I see that there aren’t any of those about logged stats, are they worth the effort?
Other than that, it lgtm.