-
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
Feature/k8s executor #87
base: feature/remote-worker
Are you sure you want to change the base?
Conversation
private Worker createK8sWorker(Integer poolId, String address, String namespace, String volumeName) { | ||
String id = "k8s-executor-" + UUID.randomUUID().toString(); | ||
K8sClusterResource k8sClusterResource = new DefaultK8sClusterResource(id, address); | ||
Specification resourceSpec = null; |
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.
just to make a note. Maybe we want it to be filled with the requirements we talked about, right? I mean, I think that this is the place we want to specify them.
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.
As we discussed before, there are two ways to specify the amount of CPU and RAM consumed by the POD container in the k8s cluster worker, the minimum (requests
) and the maximum (limits
), I believe that here can enter the maximum information, but the minimum does not fit, as the client that details this information. I will add a TODO to add something to that specification in the future.
} | ||
|
||
if (wasSuccessful(job)) { | ||
successTask(task); |
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 this method can be better named. The reader should know the method's job by its name..
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.
Done
successTask(task); | ||
taskExecutionResult = getSuccessResultInstance(tasksListSize); | ||
} else { | ||
failTask(task); |
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.
same thing
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.
Done
} | ||
|
||
private boolean wasSuccessful(V1Job job) { | ||
Integer successedQuant = job.getStatus().getSucceeded(); |
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 you mean succeededAmount, right?
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.
Yes, I updated
private String namespace; | ||
|
||
@Transient | ||
private BatchV1Api batchApi; |
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.
indentation seems broken 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.
Done
return commands; | ||
} | ||
|
||
@Override |
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.
indentation seems broken 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.
Done
|
||
public interface K8sClient { | ||
|
||
//FIXME what does it return? |
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.
should it be removed?
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.
Yes, removed
|
||
public class K8sConstants { | ||
public static final String JSON_CLUSTER_CAPACITY_KEY = "capacity"; | ||
public static final String JSON_CLUSTER_ADDRESS_KEY = "address"; |
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.
it is get from a general conf file? If so, I guess you could rename this properties to something more expressive like: "k8s_cluster_address" or something like that, what do you think?
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 assuming that these props lives in a "arrebol.conf" file. If that's not the case, do not consider my suggestion.
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.
Done
@@ -0,0 +1,19 @@ | |||
{ |
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 see now. Why a json file instead of a conf file? This structure is more complex then a simple conf file, don't you think? Just a note, though.
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 followed the same structure used for arrebol.json
, do you think it is better to change that?
Fixed #85
The Java library mentioned in #85 was used and there are 4 properties in the arrebol.json file:
poolId
: maximum number of jobs to be sent / executed in the k8s clusteraddress
: access address to the k8s clusternamespace
: namespace to be used in the k8s clustervolumeName
: name of the volume to be added to the job and used to execute it (optional)