-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add usable interfaces to create new tasks #16
Conversation
…tion instead of having to write manually the CRDs
…est facilities, add test CRDs
pkg/tasks/create.go
Outdated
args := &controlapi.JobArguments{} | ||
if rackName != "" { | ||
args.RackName = rackName | ||
} | ||
|
||
return args |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] isn't this just the same as:
return &controlapi.JobArguments{ RackName: rackName }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it is since it's not a pointer.
pkg/tasks/cluster.go
Outdated
generatedName := fmt.Sprintf("%s-%s-%d-%d", kcName, command, time.Now().Unix(), rand.Int31()) | ||
if len(generatedName) > 63 { | ||
generatedName = generatedName[:63] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[thought] When the K8ssandraTask controller creates CassandraTask
instances, it uses the naming pattern:
k8ssandraTaskName + "-" + dcName
So we should account for that. Although we don't necessarily have all the existing datacenter names here, so it's probably better to do it in k8ssandra-operator (there's no length check right now).
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think both places will need a cutting function. Even here, I'm wondering if I should rather cut the kcName instead of the randomized parts, since if someone has kcName with length 63, this would always create the same name and we end up with problems. I'll modify this functionality (and I can send a PR to k8ssandra-operator to cut the name there too).
I'd say the randomized part is the most important one (just have to be careful that it doesn't start with a number), we can control the output with kubectl get k8ssandratasks
to include the DC+Cluster information.
|
||
// Move | ||
|
||
// GarbageCollect |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[question] Are we missing the CreateMoveTask functions here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what uses the move functionality at this point, but no - I intentionally omitted that since the original tickets wanted just a subset of tasks to be exposed.
I think exposing the move might require more knowledge of how it's used (by some automation I guess?)
This allows to create new K8ssandraTasks or CassandraTasks by use of the k8ssandra-client API, with some basic validation done already here (and not just in cass-operator after Kubernetes object is already created).
Fixes #19