refactor: testing idea to wrap coscheduling

[vsoch]

This is the "skeleton" of a new idea to wrap coscheduling, where we will add in the logic for fluence only where it is needed, likely in the PodGroup (in the new fluence/core/core that wraps the same in coscheduling). This is just a skeleton because we are deploying the sidecar with the wrapped scheduling and absolutely no logic ported over to AskFlux. I think I have a sense of where to put this, but wanted to save this vanilla/skeleton state in case we need to go back to it. Note that it did not work to have fluence inherit the functions from coscheduler, so I opted for a strategy of adding it as a helper field, and then just using it when necessary (a call to f.cosched).

Note that this has no logic to work yet, so the tests will obviously fail. I want to open it so I can clearly see changes here (and update in logical, incremental steps).

Update: this doesn't seem to work, so we will need to duplicate a lot. I pushed again and have included all functions, and also restored functionality for the pod group to be cleaned up by the controller (easier development for me).

Second update: this is now working without clogging for these experiments: https://github.com/converged-computing/operator-experiments/tree/main/google/scheduler/run7

Update: this has been updated with #74 which:

• has support to re-enable activating siblings
• uses a branch of fluxion-go updated for go 1.21 and kubernetes 1.29
• updates go to 1.21 here

We will want to, before we do a final merge here, to merge flux-framework/fluxion-go#8 and then do a release that we update the digest for here. Note that fluence seems to be working OK so we likely can do this soon. My next step is working on other tools to supplement the experiments. But I'm not sure fluence in master (as a new build) would work now with upstream (actually I am sure it will not unless an older version is used).

This will close #71 #73 #59

[vsoch]

I'm testing another idea to only allow scheduling the first in the group, that way the next in the group get seen right after that. It's too bad we can't selectively grab pods from the queue (or maybe we can)? Otherwise we could just get any pod scheduled and then assign (actually assign, not just declare the node and wait for the pod to show up again) the pod to the node.

[vsoch]

It's running now (and been running). I can't back this up with data, but it seems to not be doing the "freeze for a while and then kick in" like an old car thing. Will update if/when it finishes. I doubled the size of the experiment so it's size chonkmaster.

Side note - we need a chonker scale for experiments. 🐱 🐈

LAMMPS?   A Fine Boi!
AMD?      A Heckin' Chonker
MuMMI?    OH LAWD HE COMIN'!

[vsoch]

I'm not sure why there are conflicts here now - going to try working on a separate branch and see what's up.

[vsoch]

Ah, no need - it was rebased with main and the PR was open to another (now deprecated) branch. Updating the PR branch to be against main resolved that (and showed the other large amount of work that preceded this one).

[vsoch]

@milroy I went through and changed most of the short variables (e.g., pgMgr) to be fully written out. I left a few that are convention for operators (reconcilers) like "mgr" (manager) and "req" (request). The one I didn't change from the original code was "ps" primarily because I don't know what that means. Let me know if there are others you'd like expanded. Thanks for starting to take a look!

[milroy]

I'm in the process of reviewing this PR, but it is taking quite a while. There's a lot of churn in the commits, e.g., fluence.go is updated 17 times across commits. That makes it difficult to track the desired final state and also causes me to wonder if my comments will be obviated by a later commit.

[vsoch]

I'm in the process of reviewing this PR, but it is taking quite a while. There's a lot of churn in the commits, e.g., fluence.go is updated 17 times across commits.

I'm really sorry it's so big - it's added up over many months, and understandably that sucks to review. I won't push anything until you are finished, and we can talk about the best way to work on it to minimize reviewer pain.

That makes it difficult to track the desired final state

Can you focus on review the final state (not each commit)?

and also causes me to wonder if my comments will be obviated by a later commit.

I think if I were to push the comments would say outdated, but they wouldn't go away (we could still see them).

[milroy]

I made quite a few comments and am afraid some of them may not be relevant for the final file states of the PR. I'll try to find those and close them.

In general, this looks really good and won't require much to merge. I think the main issues to address are single-character variable names (not including files that were automatically generated), and choices for primitive integer types.

This PR represents a ton of excellent work; thank you!

[milroy]

Double-checking the expected behavior given the syntax:

Less(pod1, pod2) should return true if pod1 < pod2 by one of the three criteria. return prio1 > prio2 Returns true if pod1 priority is greater than pod2. That means pod1 < pod2 which should mean pod1 comes before pod2.

[vsoch]

I think the function needs to return true if:

• for priority, pod 1 has higher priority (larger number) than pod 2, so pod1 > pod2.
• for timestamp, if pod 1 has a group timestamp that is before the group timestamp for pod 2 (there is a function Before for that)

We can see example with priority and falling back to timestamp when they are equal here.

Here is the current docstring for our function.

// Less is used to sort pods in the scheduling queue in the following order.
// 1. Compare the priorities of Pods.
// 2. Compare the initialization timestamps of PodGroups or Pods.
// 3. Compare the keys of PodGroups/Pods: <namespace>/<podname>.

In summary we do the same - first looking at priority (returning true if pod 1 has a higher number) and then considering timestamp and returning true if pod group 1 was created before group 2. If the timestamps are equal, then we sort based on the group name, which should provide consistent ordering (and there is likely no "right" answer for which one should come first).

[milroy]

I was checking because we have pod1 < pod2 if pod1.priority > pod2.priority, which indeed makes sense.

[milroy]

Is it worth creating a temporary file just to turn it back into a string?

[vsoch]

Absolutely not! I refactored this in another variant of this API, but (when I was working on it here) I didn't have enough confidence to question the original design decision. I thought there must be a reason, so I left it. I'll try removing it (it simplifies the code a ton and also we don't write a file we don't need) and see if tests pass (or if there is something I'm missing).

[milroy]

I think these should be int64s. They are in Fluxion, and there will likely be cases where the number of vertices approaches the max expressible int32.

[vsoch]

Working on now. One question I have is why for the name, here

flux-k8s/src/fluence/jgf/jgf.go

Lines 116 to 158 in 9f24f36

	func (g *Fluxjgf) MakeSubnet(index int, ip string) string {
	newnode := node{
	Id: strconv.Itoa(g.Elements),
	Metadata: nodeMetadata{
	Type: "subnet",
	Basename: ip,
	Name: ip + strconv.Itoa(g.Elements),
	Id: index,
	Uniq_id: g.Elements,
	Rank: -1,
	Exclusive: false,
	Unit: "",
	Size: 1,
	Paths: map[string]string{
	"containment": "",
	},
	},
	}
	g.addNode(newnode)
	return newnode.Id
	}
	func (g *Fluxjgf) MakeNode(index int, exclusive bool, subnet string) string {
	newnode := node{
	Id: strconv.Itoa(g.Elements),
	Metadata: nodeMetadata{
	Type: "node",
	Basename: subnet,
	Name: subnet + strconv.Itoa(g.Elements),
	Id: g.Elements,
	Uniq_id: g.Elements,
	Rank: -1,
	Exclusive: exclusive,
	Unit: "",
	Size: 1,
	Paths: map[string]string{
	"containment": "",
	},
	},
	}
	g.addNode(newnode)
	return newnode.Id
	}

we use the g.Elements length, but then for others we use an index?

flux-k8s/src/fluence/jgf/jgf.go

Lines 160 to 180 in 9f24f36

	func (g *Fluxjgf) MakeSocket(index int, name string) string {
	newnode := node{
	Id: strconv.Itoa(g.Elements),
	Metadata: nodeMetadata{
	Type: "socket",
	Basename: name,
	Name: name + strconv.Itoa(index),
	Id: index,
	Uniq_id: g.Elements,
	Rank: -1,
	Exclusive: false,
	Unit: "",
	Size: 1,
	Paths: map[string]string{
	"containment": "",
	},
	},
	}
	g.addNode(newnode)
	return newnode.Id
	}

See "Name" in each of the above. I think this would do good to have a comment to explain. My thinking is that maybe there is a subtle distinction between 'socket id in the scope of other sockets" vs "id in the scope of the entire graph" but I think the Name should be the first.

[vsoch]

The containment paths should also be a path from the cluster root to the named resource. I'm not sure if fluxion needs to use it, if not there is no issue that it's empty. If yes, this might be a source of error.

flux-k8s/src/fluence/jgf/jgf.go

Lines 173 to 175 in 9f24f36

	Paths: map[string]string{
	"containment": "",
	},

[vsoch]

okay I'm making a meta issue for us to tackle after this issue here - this entire function I think needs some refactor, but it should be a scoped PR after this one.

[milroy]

My thinking is that maybe there is a subtle distinction between 'socket id in the scope of other sockets" vs "id in the scope of the entire graph" but I think the Name should be the first.

You are correct. See an example here: https://github.com/flux-framework/flux-sched/blob/master/t/data/resource/jgfs/tiny.json.

The containment paths should also be a path from the cluster root to the named resource. I'm not sure if fluxion needs to use it, if not there is no issue that it's empty. If yes, this might be a source of error.

Fluxion hasn't needed to use it in Fluence yet because paths are used for subsystem checking, and more importantly establishing existence and uniqueness when mutating the resource graph (i.e., elasticity). We should add correct path generation in a follow-up PR.

[vsoch]

@milroy commit with response to review is here: cbeffce

I left the comments unresolved that need further conversation, or just the ones that were more fun that you should see. ⚾ 🪝 I do see some issues with the design of the JGF, but I'm not sure if there are functional implications. I updated an issue here to reflect those thoughts: #71 (and please add other bullets to it). Let's discuss the comments here as much as we can, and (if we can meet this Wednesday) finish up discussion then. I will do one final test of experiments I've run before using fluence to ensure that everything is still working before we merge.

[milroy]

Thank you very much for the quick and detailed work addressing my comments. This is a tremendous improvement, and makes Fluence far more useful and stable.

I'm approving the PR, but let's discuss a few points and test the changes as you suggested before merging.

[vsoch]

To update from my post in slack (which will be lost into the black hole of segfaults):

Fluence experiments are done - I did three sizes each one batch and varying iterations where one iteration == 5 jobs ranging in size from 2 to 6. Small was 3 of those, medium was 5, and large was 10. No clogging in any case, and the one flaw we have (that we know about) is that when the queue gets much longer, you have to wait longer to cycle through the entire thing and schedule the next job. So the large size took about 38 minutes. But the default scheduler would fail after a few minutes (clog) so we are optimizing for that - no clogging.

And a follow up question I had this morning to think about: why do we use the Kubernetes queue, period? Why can't we do what flux does with the planner and schedule the job for a set of nodes in the future? Then we wouldn't need to process through the same jobs continuously. It would improve things quite a bit I think. I suspect we are missing a flux component.

I think we are good to merge if someone wants to do the honors, or I can. It's not perfect and there is more to do, but this is a first step. 👣

[milroy]

And a follow up question I had this morning to think about: why do we use the Kubernetes queue, period? Why can't we do what flux does with the planner and schedule the job for a set of nodes in the future? Then we wouldn't need to process through the same jobs continuously. It would improve things quite a bit I think. I suspect we are missing a flux component.

We've started discussing improvements and changes to Fluence along these lines, and let's continue in a separate issue. For now, I'm merging this PR.