Adds default resources for fetcher pod #500
Conversation
executor/newdeploy/newdeploy.go
Outdated
| mincpu, err := resource.ParseQuantity("10m") | ||
| minmem, err := resource.ParseQuantity("16Mi") | ||
| maxcpu, err := resource.ParseQuantity("40m") | ||
| maxmem, err := resource.ParseQuantity("128Mi") |
There was a problem hiding this comment.
you can move this outside of the function (either ignore the errors as they are hardcoded anyway, or check in the init(), or wrap them in a helper function (like MustParseQuantity))
executor/newdeploy/newdeploy.go
Outdated
| @@ -87,6 +113,12 @@ func (deploy *NewDeploy) createOrGetDeployment(fn *crd.Function, env *crd.Enviro | |||
| return nil, err | |||
| } | |||
|
|
|||
| fetcherResources, err := deploy.getFetcherResources() | |||
| if err != nil { | |||
| log.Printf("Error while parsing fetcher resources: %v", err) | |||
There was a problem hiding this comment.
Why print here, rather than wrapping the err?
executor/poolmgr/gp.go
Outdated
| @@ -715,3 +723,27 @@ func (gp *GenericPool) destroy() error { | |||
|
|
|||
| return nil | |||
| } | |||
|
|
|||
| func (gp *GenericPool) getFetcherResources() (v1.ResourceRequirements, error) { | |||
There was a problem hiding this comment.
Extract this to a common/util file + it doesn't seem to be needed to attach this function the GenericPool.
executor/newdeploy/newdeploy.go
Outdated
| @@ -214,6 +246,10 @@ func (deploy *NewDeploy) createOrGetHpa(hpaName string, execStrategy *fission.Ex | |||
| return existingHpa, err | |||
| } | |||
|
|
|||
| if depl == nil { | |||
| return nil, errors.New("Failed to create HPA, found empty deployment") | |||
There was a problem hiding this comment.
Minor: try to stick with idiomatic go: error messages should not be capitalized.
|
Thanks, @erwinvaneyk ! |
executor/poolmgr/gp.go
Outdated
| @@ -715,3 +723,27 @@ func (gp *GenericPool) destroy() error { | |||
|
|
|||
| return nil | |||
| } | |||
|
|
|||
| func (gp *GenericPool) getFetcherResources() (v1.ResourceRequirements, error) { | |||
| //TBD Hardcoded as of now, should be configurable? | |||
There was a problem hiding this comment.
Yes, it really ought to be configurable. You could write a quick GetenvWithDefault function and maybe use that? Something like resource.ParseQuantity(GetenvWithDefault("FETCHER_MINCPU", "5m"))
There was a problem hiding this comment.
Ok to do in a later change if you like.
The fetcher needs a relatively smaller set of resources and does not have to be same as the function container/defaults. This change adds defaults for fetcher containers in function pods.
This PR adds default resources for Fetcher pod for both executors - pool manager and new deployment (And fixes #478 ). Currently, the resource numbers are based on experimentation. In future maybe we could look at the numbers from monitoring (Prometheus?)
Also, the getFetcherResources method is duplicated in both executors. On one hand, I wanted different fetcher resources for both but also wanted to avoid code duplication. I can not move the method up at executor level as it will cause cyclical dependency of packages, maybe create a util package within executor and then import in both executors?
This change is