Run dpl workflows from ECS in k8s - #850
Conversation
Sorry, something went wrong.
knopers8
left a comment
There was a problem hiding this comment.
i'm leaving some todo comments for myself, don't look Michal!!
Sorry, something went wrong.
| // jitOnK8sEnabled reports whether ECS may route JIT/DPL task classes to the | ||
| // K8s path. Missing/false means JIT keeps running through Mesos, same as | ||
| // before this bridge existed. | ||
| func jitOnK8sEnabled() bool { |
There was a problem hiding this comment.
that's not used anywhere, todo. I would even consider working towards having a global switch, affecting not just jitted DPL, but everything we run.
Sorry, something went wrong.
| tasks: | ||
| mtichak-ost.cern.ch: | ||
| # mtichak-ost: | ||
| mtichak-ost: |
There was a problem hiding this comment.
understand why it is changed again
Sorry, something went wrong.
| pod: | ||
| containers: | ||
| - args: | ||
| - source /etc/profile.d/o2.sh && o2-dpl-raw-proxy -b --session default --dataspec |
There was a problem hiding this comment.
not sure why it is all one big argument
Sorry, something went wrong.
| @@ -0,0 +1,110 @@ | |||
| apiVersion: aliecs.alice.cern/v1alpha1 | |||
There was a problem hiding this comment.
try to understand why we only have the QC CheckRunner template here, but not all of them. Maybe it's an example.
Sorry, something went wrong.
| images: | ||
| - name: environment-manager | ||
| newName: gitlab-registry.cern.ch/aliceo2group/dockerfiles/aliecs/environment-manager | ||
| newTag: latest |
There was a problem hiding this comment.
Try to understand why it's a part of this PR and why it's needed. Maybe it was removed just to be able to use a custom-built image instead of the latest.
Sorry, something went wrong.
| flag.BoolVar(&enableLeaderElection, "leader-elect", false, | ||
| "Enable leader election for controller manager. "+ | ||
| "Enabling this will ensure there is only one active controller manager.") | ||
| flag.IntVar(&maxConcurrentReconciles, "max-concurrent-reconciles", 1, |
There was a problem hiding this comment.
not sure why we needed to make it an option to run DPL
Sorry, something went wrong.
| envVars: | ||
| - O2_DETECTOR | ||
| - O2_PARTITION | ||
| - OCC_CONTROL_PORT |
There was a problem hiding this comment.
not clear why it's removed. after all we use the allocator only for JIT-generated templates.
Sorry, something went wrong.
|
|
||
| var clientsForContainers map[string]*OccClient = make(map[string]*OccClient) | ||
| // var clientsForContainers map[string]*OccClient = make(map[string]*OccClient) | ||
| var clientsForContainers sync.Map |
There was a problem hiding this comment.
i guess that's to allow for concurrent reconciles.
Sorry, something went wrong.
| } | ||
|
|
||
| stateReply, err := client.GetState(ctx) | ||
| stateReply, err := client.(*OccClient).GetState(ctx) |
There was a problem hiding this comment.
we could cast just after retrieving from the map.
Sorry, something went wrong.
| type TaskReference struct { | ||
| Name string `json:"name"` | ||
| TaskID string `json:"taskID,omitempty"` | ||
| NameSuffix string `json:"nameSuffix,omitempty"` |
There was a problem hiding this comment.
todo add comment that it's mostly for jit-blabla, so I don't scratch my head when refactoring.
Sorry, something went wrong.
|
Just a remark... I am not 100% sure that this is PR worthy without any changes. So if you think that something is bad, don't hesitate to call it out. I would do probably some refactoring myself. I just wanted you to see what I did about the dpl and friends |
Sorry, something went wrong.
Reviewers
At least 1 approving review is required to merge this pull request.
this should be multiple PRs, but I wanted to create this PR before I leave for holidays. However at least all independant issues are separate commits. If you have any questions, I will be responding via MM or maybe even in comments here.