Add Restart task to CassandraTasks #394
Conversation
| @@ -69,6 +73,12 @@ const ( | |||
| CommandScrub CassandraCommand = "scrub" | |||
| ) | |||
|
|
|||
| const ( | |||
| KeyspaceArgument string = "keyspace_name" | |||
There was a problem hiding this comment.
How is this related to a restart?
| ) | ||
|
|
||
| // Cleanup functionality | ||
|
|
||
| func callCleanup(nodeMgmtClient httphelper.NodeMgmtClient, pod *corev1.Pod, taskConfig *TaskConfiguration) (string, error) { | ||
| // TODO Add more arguments configurations | ||
| keyspaceName := "" | ||
| if keyspace, found := taskConfig.Arguments["keyspace_name"]; found { | ||
| if keyspace, found := taskConfig.Arguments[api.KeyspaceArgument]; found { |
There was a problem hiding this comment.
How is this related to a restart?
There was a problem hiding this comment.
This is mostly superceded by #395 , just to ensure we don't have magic strings around the code. Same for previous comment.
| @@ -27,7 +32,7 @@ func callCleanup(nodeMgmtClient httphelper.NodeMgmtClient, pod *corev1.Pod, task | |||
| func callCleanupSync(nodeMgmtClient httphelper.NodeMgmtClient, pod *corev1.Pod, taskConfig *TaskConfiguration) error { | |||
| // TODO Add more arguments configurations | |||
| keyspaceName := "" | |||
| if keyspace, found := taskConfig.Arguments["keyspace_name"]; found { | |||
| if keyspace, found := taskConfig.Arguments[api.KeyspaceArgument]; found { | |||
There was a problem hiding this comment.
How is this related to a restart?
There was a problem hiding this comment.
The code mostly looks good to me, aside from a Result return that isn't used when invoking the rolling restart on the STS, unless I'm missing something.
It could be causing the only thing that looks problematic when testing this feature, which is that the completion time gets set right away when the CassandraTask gets created.
We'd need the completion time to be set only when the rolling restart is completed.
| return ctrl.Result{}, err | ||
| } | ||
|
|
||
| res, err = r.restartSts(ctx, sts, taskConfig) |
There was a problem hiding this comment.
The res value doesn't seem to be used at all. Maybe that's what could be causing prematurely setting the completion time?
There was a problem hiding this comment.
Isn't that res used on the line 333? I don't see it being overwritten anywhere.
There was a problem hiding this comment.
Isn't it overwritten on line 320?
res, failed, completed, err = r.reconcileEveryPodTask(ctx, dc, taskConfig)
There was a problem hiding this comment.
Or is break JobDefinition sending us straight to line 333?
There was a problem hiding this comment.
break JobDefinition should break us from the for() clause and thus line 320 should never be run.
burmanm
force-pushed
the
restart_task
branch
from
032a9eb to
2673975
Compare
What this PR does:
Creates a new option that does rolling restart outside cads-operator's controller.
Which issue(s) this PR fixes:
Fixes #385
Checklist