Add support for auto deployment for historicals #36
Conversation
|
|
1 similar comment
|
|
| @@ -11496,6 +11533,23 @@ spec: | |||
| status: | |||
| description: DruidStatus defines the observed state of Druid | |||
| properties: | |||
| HistoricalStatus: | |||
There was a problem hiding this comment.
| HistoricalStatus: | |
| historicalStatus: |
| return nil | ||
| } | ||
|
|
||
| m.Status.Historical.Replica = obj.(*appsv1.StatefulSet).Status.CurrentReplicas |
There was a problem hiding this comment.
If m.Status.Historical.Replica is used later on to scale down back to original replicas, then its better to create this at the beginning. Also use statefulset.spec.replicas as opposed to status.
There was a problem hiding this comment.
Also this feels redundant as you have already updated this field here
| logger_drain_historical.Info("Waiting for pods to drain", "name", m.Name, "namespace", m.Namespace) | ||
| return nil | ||
| } | ||
| //delete corresponding nodegrabbers |
There was a problem hiding this comment.
This can be implemented only if we use local storage. Which suggests that we need some kind of check to see if its a local storage instance or not.
| for m.Status.Historical.DecommissionedPods != nil { | ||
| //get the PVC for the pod to be disabled and delete it once the pod is deleted | ||
| for _, pod := range podList { | ||
| if pod.(*v1.Pod).Name == m.Status.Historical.DecommissionedPods[0] { |
There was a problem hiding this comment.
Suppose the podList has pods with 2,1,0 ordinal numbers and m.Status.Historical.DecommissionedPods has pods 0,1,2 . In this case only the pod 0 would be deleted, but the PVC of 1 and 2 would also be deleted which is not ideal.
Why not have a simple for loop over the pods in m.Status.Historical.DecommissionedPods and delete them?
| } | ||
| } | ||
| } | ||
| for it := startPod; it <= endPod; it++ { |
There was a problem hiding this comment.
After you delete the PVCs, the new pod which was deleted just before this block, would see the reference to the PVC and report that the PVC is missing and hence can't schedule. This is why we may need an additional round of deletion of the pods we deleted earlier.
|
|
||
| func deployHistorical(sdk client.Client, m *v1alpha1.Druid, nodeSpec *v1alpha1.DruidNodeSpec, nodeSpecUniqueStr string, emitEvent EventEmitter, batchSize int32, baseUrl string) error { | ||
| // patch the updateStrategy with onDelete | ||
| err := patchUpdateStrategy(sdk, m, nodeSpec, onDelete, emitEvent) |
There was a problem hiding this comment.
This means we would be patching the update strategy on every run. Can we patch it only at the start of an deployment process? Also, make sure to revert the patching done here after the deployment.
| return err | ||
| } | ||
|
|
||
| if obj.(*appsv1.StatefulSet).Status.ReadyReplicas != obj.(*appsv1.StatefulSet).Status.CurrentReplicas { |
There was a problem hiding this comment.
Add a similar check before starting the deployment to see if there's already another deployment in progress. You can use status.updateRevision and status.currentRevision to check for the same.
If there's another deployment in progress, dont proceed at all.
There was a problem hiding this comment.
I'll just list some of the pending items on this PR;
- Calling the deployer function of historicals from the handler
- Ensuring that the deployer function is only called when the deployment type is node change, otherwise the normal rolling update strategy should take care of it.
- Deployment can only be triggered when there's no on going updates in the cluster. This also means that if some other component is being rolled which is ranked higher than the historicals, it should be allowed to complete and only then trigger a deployment.
- Ensure the state of the cluster before the deployment is same as after the deployment with respect to the resources created or modified.
- We need to make the operator code for historical deployment as idempotent as possible. States can be built on each run using the config and status objects as needed.
- Actions like patching statefulset update strategy which is called during the start should only be called once in the life cycle. The next time its called, it should be after the deployment for reverting the changes.
rbankar7
force-pushed
the
rban/historical-auto-deploy
branch
from
f25c0b2
to
dc5f766
Compare
Fixes #XXXX.
Description
This PR has:
Key changed/added files in this PR
MyFooOurBarTheirBaz