Added a new helm parameter controllers.vdbMaxBackoffDuration #978
Conversation
cchen-vertica
requested review from
roypaulin,
fenic-fawkes,
HaoYang0000,
qindotguan and
LiboYu2
as code owners
pkg/opcfg/config.go
Outdated
| // GetVdbMaxBackoffDuration returns maximum backoff requeue duration in milliseconds for vdb controller | ||
| func GetVdbMaxBackoffDuration() int { | ||
| duration := lookupIntEnvVar("VDB_MAX_BACKOFF_DURATION", envCanNotExist) | ||
| if duration == 0 { |
There was a problem hiding this comment.
If we set the value VDB_MAX_BACKOFF_DURATION=0, should we expect 0ms duration?
There was a problem hiding this comment.
Yes, we do expect 0ms duration. I will change this one to "<0".
pkg/opcfg/config.go
Outdated
| // GetVdbMaxBackoffDuration returns maximum backoff requeue duration in milliseconds for vdb controller | ||
| func GetVdbMaxBackoffDuration() int { | ||
| duration := lookupIntEnvVar("VDB_MAX_BACKOFF_DURATION", envCanNotExist) | ||
| if duration == 0 { |
There was a problem hiding this comment.
| if duration == 0 { | |
| if duration <= 0 { |
| rateLimiter := workqueue.NewItemExponentialFailureRateLimiter(1*time.Millisecond, | ||
| time.Duration(opcfg.GetVdbMaxBackoffDuration())*time.Millisecond) |
There was a problem hiding this comment.
Let's not use opcfg here. Let' set the rate limiter in main.go and pass it to SetupWithManager:
main.go
rateLimiter := workqueue.NewItemExponentialFailureRateLimiter(....)
// Create the controller with custom options, including the rate limiter
options := controller.Options{
RateLimiter: rateLimiter,
}
}).SetupWithManager(mgr, options)
verticadb_controller.go
return ctrl.NewControllerManagedBy(mgr).
WithOptions(options).
For(&vapi.VerticaDB{}).
Owns(&corev1.ServiceAccount{}).
Owns(&rbacv1.Role{}).
| duration := lookupIntEnvVar("VDB_MAX_BACKOFF_DURATION", envCanNotExist) | ||
| if duration == 0 { | ||
| const envName = "VDB_MAX_BACKOFF_DURATION" | ||
| duration := lookupStringEnvVar(envName, envCanNotExist) |
There was a problem hiding this comment.
Why not use lookupIntEnvVar? If the issue is the default value, maybe we can change this to lookupStringEnvVar(envName string, mustExist bool, defaultValue int)?
There was a problem hiding this comment.
lookupIntEnvVar returns 0 when it cannot find the env var which cannot differentiate the case with user input 0. I don't want to update lookupIntEnvVar too much since it's called in many other places.
| } | ||
| valInt, err := strconv.Atoi(duration) | ||
| if err != nil || valInt < 0 { | ||
| dieIfNotValid(envName) |
There was a problem hiding this comment.
You should return the default value in this case too. see lookupIntEnvVar. Also use a constant for the default value.
There was a problem hiding this comment.
lookupIntEnvVar doesn't return any value when err != nil (dieIfNotValid() will exit the process). I think if the user provided the wrong value, the operator should exit and tell them their value isn't acceptable. We shouldn't replace their wrong value with a default value.
If the default value occurred more than once in the code, I will use a constant value, but it didn't so I just use a literal constant.
This PR introduces a new Helm parameter, controllers.vdbMaxBackoffDuration, to address the requeue-hang issue in revive_db. This parameter allows us to set a maximum requeue duration for the VDB controller, providing more control over the requeue behavior.