Arvados

Ready for review at baa1f256924655d67b704f35981e9839743fab99 on branch 16739-concurrent-node-create-throttle

I think you could skip CheckRateLimitError (which would log a slightly misleading message "suspending remote calls due to rate-limit error") and do this

logger.Info("reached MaxConcurrentNodeCreateOps")
wp.instanceSet.throttleCreate.ErrorUntil(errors.New("reached MaxConcurrentNodeCreateOps"), time.Now().Add(5*time.Second), wp.notify)
return false

...also eliminating the new quotaerror type.

Log message should probably use the snakecase found in config since that's what admin will need to look for.

How about "MaxConcurrentInstanceCreateOps" instead of ...NodeCreateOps? I think we should stick with one term for a given thing in config keys & docs -- the section is called CloudVMs and other config keys use the term "instances", which is probably best since Azure/GCP/AWS all refer to "virtual machine instances".

(Some of the comments already say "workers" where they should probably say "instances" instead... but at least they're just comments.)

Nit: The "(azure)" stuff in the config-defaults comments could be misconstrued to mean that the feature only applies to Azure. Maybe more explicit, like "This is recommended by Azure in certain scenarios (see {link}) and can be used with other cloud providers too, if desired."?

Meanwhile, in existing code... I noticed the worker struct had its own throttleCreate and throttleInstances, unused except for a bogus call to wp.throttleCreate.Error() in Create() that clearly should have been checking wp.instanceSet.throttleCreate.Error() instead. Pushed a fix to your branch.

Rest LGTM. I like the test!

Tom Clegg wrote:

I think you could skip CheckRateLimitError (which would log a slightly misleading message "suspending remote calls due to rate-limit error") and do this

[...]

...also eliminating the new quotaerror type.

Ah, nice, I didn't like having to add that type.

Log message should probably use the snakecase found in config since that's what admin will need to look for.

Yes!

How about "MaxConcurrentInstanceCreateOps" instead of ...NodeCreateOps? I think we should stick with one term for a given thing in config keys & docs -- the section is called CloudVMs and other config keys use the term "instances", which is probably best since Azure/GCP/AWS all refer to "virtual machine instances".

Sure, done. Consistency is good.

(Some of the comments already say "workers" where they should probably say "instances" instead... but at least they're just comments.)

I didn't fix that here, not sure which comments you meant.

Nit: The "(azure)" stuff in the config-defaults comments could be misconstrued to mean that the feature only applies to Azure. Maybe more explicit, like "This is recommended by Azure in certain scenarios (see {link}) and can be used with other cloud providers too, if desired."?

Yes, that's better, fixed.

Meanwhile, in existing code... I noticed the worker struct had its own throttleCreate and throttleInstances, unused except for a bogus call to wp.throttleCreate.Error() in Create() that clearly should have been checking wp.instanceSet.throttleCreate.Error() instead. Pushed a fix to your branch.

Thanks!

Rest LGTM. I like the test!

Cool. The change to throttleCreate.ErrorUntil meant that I needed a way to reset the error in the test, so I added a ResetError() function on throttle.

Ready for another look at a5fef23f2863cd0183ff596f4579110e2ddb3b3d on branch 16739-concurrent-node-create-throttle

Cool. The change to throttleCreate.ErrorUntil meant that I needed a way to reset the error in the test, so I added a ResetError() function on throttle.

I removed that function in b108bdfa0f3c74239fa565a1d14db945eb4dcf18 and set the err value directly in the test.

a-d-c install doc page needs s/MaxConcurrentNodeCreateOps/MaxConcurrentInstanceCreateOps/

Rest LGTM, thanks!

Tom Clegg wrote:

a-d-c install doc page needs s/MaxConcurrentNodeCreateOps/MaxConcurrentInstanceCreateOps/

Rest LGTM, thanks!

Fixed and merged, thanks.