Implementing Operator downscaling functionality #901
Conversation
…cePartitions spec
|
Fixes issue: #179 |
…mio-operator into feature/add-downscaling-support
| err = r.Status().Update(ctx, hc) | ||
| if err != nil { | ||
| r.Log.Error(err, "failed to update cluster status") | ||
| return reconcile.Result{}, err | ||
| } | ||
| r.Log.Info(fmt.Sprintf("removing pod %s containing vhost %d", pod.Name, vhost)) | ||
| if err := r.Delete(ctx, &pod); err != nil { // delete pod before unregistering node | ||
| return reconcile.Result{}, r.logErrorAndReturn(err, fmt.Sprintf("failed to delete pod %s for vhost %d!", pod.Name, vhost)) | ||
| } |
There was a problem hiding this comment.
What happens if the r.Status().Update(ctx, hc) call succeeds, but then the r.Delete(ctx, &pod) fails? Or perhaps we update status, and then due to the operator being restarted, it doesn't get to do the pod deletion?
There was a problem hiding this comment.
If I read the code correctly, it would essentially "just" mean it'll append the ID one more time to the Status, and then try to do the pod deletion again? Perhaps it should only do the append operation if the vhost is not already in the list? Mostly to prevent some sort of edge case that appends the same vhost over and over, where it fails to actually delete the pod.
There was a problem hiding this comment.
That's fine. If the delete fails after the update succeeds, then the node will remain marked at Evicted and at the next reconcile run, it will try to delete the pod again.
As per the check at line 2228, the node will NOT be unregistered if it's still alive (if the pod is not deleted).
Edit: Nvm, I see your point. I'll add a check before the insert.
api/v1alpha1/humiocluster_types.go
Outdated
| // Default: false | ||
| // Preview: this feature is in a preview state | ||
| //+kubebuilder:default=false | ||
| EnableDownscalingFeature bool `json:"enableDownscalingFeature,omitempty"` |
There was a problem hiding this comment.
This may be nitpicky but if we plan on having opt-in features, perhaps we should create a feature flag type for this purpose?
FeatureFlags could then hold a Downscaling field that is a bool, or could be a string that we parse.
In general if we can keep the list of top level fields as minimal as possible I think that is cleaner. Especially if they are just holding a boolean.
There was a problem hiding this comment.
Great idea. I've implemented it, now I'm testing it, along with the eviction comment.
| continue | ||
| } | ||
| vhost := nodeIdToPodMap[pod.GetName()] | ||
| err = r.HumioClient.SetIsBeingEvicted(ctx, humioHttpClient, req, vhost, true) |
There was a problem hiding this comment.
This section makes me nervous. Do we have any protection in preventing runaway evictions?
For example, what if SetIsBeingEvicted returns an error but still executes the eviction, or something else happens (operator restart, etc) during this time after we mark for eviction but before we mark the pod as being evicted?
I think we should do some validation before this step, or change things where we're forced to be a little more careful. For example, in the reconciliation we could:
First check against the humio API to see if any nodes are marked for eviction. If they are, ensure the labels/annotations are set. If it sets the annotation, close the reconciliation loop. If it is already set, then continue.
Second step is to check if there are any pods with the label/annotation and check this against the humio API. If it doesn't match, then trigger the eviction for the pods that have the label and restart the reconciliation loop. If it does match, then continue with matchPodsToHosts() and starting the eviction process for a node assuming a node can be safely evicted.
Add logic to restrict a max number of concurrent evictions by default and possibly offer a way to override via the spec, possibly a max per zone?
This way if someone evicts via the UI then it will eventually lead to the pod being replaced, and we will not inadvertently evict all nodes in the cluster.
… scaling up the cluster as old pods don't have the new eviction annotation set
No description provided.