adding scale subresource so that HPA can be used with the cluster #394
Conversation
|
/gcbrun |
|
/gcbrun |
| @@ -412,6 +412,18 @@ type JobSpec struct { | |||
| SecurityContext *corev1.PodSecurityContext `json:"securityContext,omitempty"` | |||
| } | |||
|
|
|||
| // HPASpec defines properties of a HPA for the cluster. | |||
| type HPASpec struct { | |||
| // Taskmanager lower limit for the number of pods. | |||
There was a problem hiding this comment.
Better to define the default value for optional fields.
There was a problem hiding this comment.
D you mean a default value for the HPASpec or for the struct fields?
There was a problem hiding this comment.
I mean the fields, e.g., if the user doesn't specify the value, what is the default value / behavior?
| @@ -519,7 +534,10 @@ type FlinkClusterComponentsStatus struct { | |||
| JobManagerIngress *JobManagerIngressStatus `json:"jobManagerIngress,omitempty"` | |||
|
|
|||
| // The state of TaskManager StatefulSet. | |||
| TaskManagerStatefulSet FlinkClusterComponentState `json:"taskManagerStatefulSet"` | |||
| TaskManagerStatefulSet TaskManagerStatefulSetStatus `json:"taskManagerStatefulSet"` | |||
There was a problem hiding this comment.
Thanks for fixing this!
| MaxReplicas int32 `json:"maxReplicas"` | ||
|
|
||
| // Taskmanager target average CPU utilization | ||
| TargetCPUUtilizationPercentage *int32 `json:"targetCPUUtilizationPercentage,omitempty"` |
There was a problem hiding this comment.
nit: "Percentage" seems to be unnecessary, can we remove it here and below?
There was a problem hiding this comment.
I mostly agree, but it does disambiguate whether is a percentage, proportion or even absolute value. It also matches the autoscaling k8s api.
| @@ -103,6 +104,11 @@ func (reconciler *ClusterReconciler) reconcile() (ctrl.Result, error) { | |||
| return ctrl.Result{}, err | |||
| } | |||
|
|
|||
| err = reconciler.reconcileHPA() | |||
There was a problem hiding this comment.
Can we put HPA at the end? because if it fails, other reconciliation can be done unaffected.
| } | ||
| return nil | ||
| } | ||
| reconciler.log.Info("JobManager ingress already exists, no action") |
| if err != nil { | ||
| log.Error(err, "Failed to delete HPA") | ||
| } else { | ||
| log.Info("Ingress HPA") |
There was a problem hiding this comment.
Wrong log message "Ingress".
|
Thank for the review @functicons. Yep if we're happy with the approach I'll modify the user guide and see if I can put some tests together 👍 |
|
@Padarn go ahead, thanks! |
|
@functicons while I was working on this I realised something (I should have seen this before): We will also need to scale the jobs parallelism for it to be useful. This gets a bit confusing because parallelism doesn't make sense unless you are using a job right now. I'd suggest for this version:
this feels a bit ugly, but without deeper thought, I'm not sure how it would be best to work. Thouhts? |
|
@Padarn |
|
after battling with this for some time, I realised it was simpler to separate this PR between the HPA and the scale subresource I've made a new PR here: #425 |
WIP - addresses #389