FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

[OCTRL-1091] Integrate k8s client into ECS core by justonedev1 · Pull Request #833 · AliceO2Group/Control · GitHub

/ Control Public

[OCTRL-1091] Integrate k8s client into ECS core - #833

Merged
justonedev1 merged 8 commits into
masterfrom
OCTRL-1091-merge
Aug 12, 2026
ConversationCommits (8)ChecksFiles changed
Merged

[OCTRL-1091] Integrate k8s client into ECS core#833
justonedev1 merged 8 commits into
masterfrom
OCTRL-1091-merge

Conversation

justonedev1 commented Jul 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

This is actual implementation of the ECS Kubernetes bridge.

Before merging it needs the OCTRL-1090

The goal of this PR was to take existing code that deploys Mesos tasks and adjusts it for the kubernetes client usage, basically linking CRDs defined and managed by managers in control-operator to the existing ECS task infrastructure and classes. The code from managerk8s.go was inspired roughly accroding to this table (generated by llm, but I went through it):

managerk8s.go Mesos counterpart Location
createK8sTaskEntries — InboundChannels loop, IPC endpoint, ::global alias makeTaskForMesosResources — same loop, IPC/TCP handling, global alias scheduler.go:1346
buildK8sNodeTaskRefs — O2_ROLE/O2_SYSTEM env var injection loop makeTaskForMesosResources — fillEnvDefault loop for the same two vars scheduler.go:1458
buildK8sNodeTaskRefs — ArgsCLI trimming/filtering makeTaskForMesosResources — cmd.Arguments handling scheduler.go:1437
newTaskForKubernetes newTaskForMesosOffer manager.go:169
deployKubernetesTasks — overall flow acquireTasks — Mesos deployment block manager.go:550
configureK8sTasks — BuildPropertyMap per task, global bind map configureTasks — bind map building + BuildPropertyMaps manager.go:804
transitionAndWaitK8sEnvState — send transition, block until state reached transitionTasks — send Mesos command, block on response manager.go:901
killK8sTask doKillTask manager.go:1238
killK8sEnvironment releaseTasks manager.go:733
process — route watch event to roster task (called from watchK8sTasks) updateTaskState / updateTaskStatus manager.go:1071
watchK8sTasks — retry loop on watcher failure Mesos subscribe retry loop scheduler.go
nodeNameFromDescriptor — scan constraints for machine_id Mesos offer attribute matching / Satisfy(targetConstraints) scheduler.go:743
k8sEnvRegistry, createK8sEnvironmentCRD, waitForK8sEnvState, cmdEnvToK8sEnvVars (no direct Mesos equivalent) —

It contains multiple changes:

  • upgrading deps after rebasing on master
  • new controlmodes to distinguish between Kubernetes and Mesos tasks
  • k8s manager which translates existing mesos infrastructure to kubernetes (creation, updating, transitioning ,...)
  • small change for environment CRDs
  • control-workflow manifests for readout, stfbuilder-senderoutput and stfsender
  • simple documentation

justonedev1 changed the title [Octrl 1091] Integrate k8s client into ECS core [OCTRL 1091] Integrate k8s client into ECS core Jul 9, 2026
justonedev1 changed the title [OCTRL 1091] Integrate k8s client into ECS core [OCTRL-1091] Integrate k8s client into ECS core Jul 9, 2026
justonedev1 marked this pull request as ready for review July 9, 2026 15:13
justonedev1 requested a review from knopers8 as a code owner July 9, 2026 15:13
knopers8 requested changes Jul 22, 2026
View reviewed changes

knopers8 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks, i have comments mostly to naming, while the logic and approach seems OK for me.

task/manager.go becomes less clean due to placing kubernetes stuff there, but I wouldn't worry too much with creating a common abstraction for k8s + mesos, since this code is subject to further rewriting or starting it from scratch without mesos in the first place.

Comment thread core/task/manager.go Outdated
Comment thread docs/kubernetes_ecs.md Outdated
Comment thread core/task/manager.go Outdated
Comment thread core/task/managerk8s.go Outdated
m.roster.append(e.task)
}

envCRDName, err := m.createK8sEnvironmentCRD(ctx, envId, nodeToRefs)

knopers8 Jul 22, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I might be mistaken, but isn't it a custom object that we are creating, not Custom Resource Definition? My understanding is that a CRD defines what fields our custom objects should have at the new custom resource/endpoint.

Suggested change
envCRDName, err := m.createK8sEnvironmentCRD(ctx, envId, nodeToRefs)
envCRDName, err := m.createK8sEnvironmentObject(ctx, envId, nodeToRefs)

So if I am right, any occurence of "CRD" should be replaced here with "Object" or removed.

justonedev1 Aug 6, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

no this is actual CRD that we are creating and filling in

knopers8 Aug 10, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Well, doesn't that happen during make install? Why do we have to do it at environment creation?

Control/control-operator/Makefile

Line 202 in 763366e

install: manifests kustomize ## Install CRDs into the K8s cluster specified in ~/.kube/config.

justonedev1 Aug 11, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ah, I understand now. No I am not installing anything, cluster alreay needs to know about this CRD to create an instance of it (that is what install is for). But this code does the same as what you do if you use kubectl apply -f environment.yaml. It just describes the instance of environment by filling in the instance of Environment CRD with required tasks and other properties and applies it to the cluster.

knopers8 Aug 11, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Exactly. So my point was that Environment CRD is what we create by kubectl apply during make install. As in the doc:

Defining a CRD object creates a new custom resource with a name and schema that you specify.

Then:

A resource is an endpoint in the Kubernetes API that stores a collection of API objects of a certain kind; for example, the built-in pods resource contains a collection of Pod objects.

So what we are creating here is an object (instance) of the Environment custom resource.

justonedev1 Aug 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

yes, that is what I am saying and the reason for the name. If you use just "Object" it is misleading as in "what object" it is concretly defined as instance of CRD... I can rename it to createK8sEnvironmentCRDInstance, but I think that leaving CRD at all from the name is obscuring information from reader

justonedev1 Aug 11, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I changed naming to what we agreed in person

Comment thread core/task/managerk8s.go Outdated
justonedev1 requested a review from knopers8 August 6, 2026 10:52

justonedev1 commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator Author

Sorry for the noisy force-pushing. I was resolving conflicts before we decided on naming. But the change of naming is in it's own commit, so it should be easy to review.

knopers8 approved these changes Aug 11, 2026
View reviewed changes

knopers8 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

thank you

justonedev1 merged commit c97d5fd into master Aug 12, 2026
6 checks passed
justonedev1 deleted the OCTRL-1091-merge branch August 12, 2026 06:50
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

knopers8 knopers8 approved these changes

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

Back | FazBrowse Home | New Git URL