Refactor Context into separate cloudup and nodeup types

[johngmyers]

This separates the Context, Task, and related types to distinguish those used in cloudup from those used in nodeup. Followup PRs will move some fields only used in cloudup into CloudupContext.

The basics of this refactor are:

type Context[T SubContext] struct {
	 ...

	T T
}

type SubContext interface {
	CloudupContext | NodeupContext
}
type CloudupContext struct{}
type NodeupContext struct{}

type Task[T SubContext] interface {
	Run(*Context[T]) error
}

type ModelBuilderContext[T SubContext] struct {
	Tasks              map[string]Task[T]
	LifecycleOverrides map[string]Lifecycle
}

[johngmyers]

/retest

[olemarkus]

I really like this change, but since it introduces generics and such, I'll let others the opportunity to voice their opinions as well.

/lgtm

[hakman]

Let's keep this open until @justinsb has a chance to take a look. Also, keep in mind that KubeCon NA is next week.

[justinsb]

Is there a broader goal here @johngmyers ? I do think we should adopt generics, and I do like the idea of separating the cloud and node tasks from each other. But right now this puts relatively complex type signatures into every Task. I tried using a type alias (type NodeTask Task[NodeupContext]), but then go did not treat map[string]NodeTask the same as map[string]Task[NodeupContext]. That might get fixed over time, but right now this is made awkward by the current limitations of generics.

[johngmyers]

@justinsb the goal is fundamentally to make it clear through the type system that the two different types of tasks have different things available to them. When I was refactoring the PKI code I was particularly frustrated by the fact that, without generics, I couldn't represent through the type system that cloudup tasks could create new keypairs, whereas nodeup tasks could only read the keypairs that already existed.

I'm also a bit annoyed that nodeup tasks have to implement HasLifecycle, though this PR doesn't address that.

[justinsb]

OK, I think it's a good goal, the generic syntax is just pretty awkward right now. I'm almost inclined to see if we can just make them different types and not use generics, but when I've tried it ends up with problems on the execution engine and localtarget. So I appreciate that you actually got this working :-) Can we discuss more in office hours?

[johngmyers]

/hold
/kind office-hours

[justinsb]

I had a rough go in #14457, let's talk about the alternatives...

I would really like to use generics BTW, the place which really bugs me is if we can replace our reflection-backed dispatching on the Render / Find etc methods with something like

type NodeTask[T] interface {
  Find(c *fi.NodeContext) (*T, error)
  Render(c *fi.NodeContext, a,e, changes *T) (error)
...
}

[johngmyers]

We might be able to handle things like Find/Render with interfaces, like HasAddress is implemented.

[johngmyers]

Could we implement a DefaultDeltaTask struct which would be embedded by all the Tasks that want the DefaultDeltaRunbehavior?

type DefaultDeltaRun[T SubContext] struct {
   // Name *string ?
}

func (e *DefaultDeltaRun[T])) Run(c *fi.Context[T]) error {
  //Similar to body of current DefaultDeltaRunMethod
}

[johngmyers]

Might need to be

type DefaultDeltaRun[T HasFind, C SubContext] struct {
   // Name *string ?
}

func (e *DefaultDeltaRun[T, C])) Run(c *fi.Context[C]) error {
  //Similar to body of current DefaultDeltaRunMethod
}

[johngmyers]

To answer my question about DefaultDeltaTask:

I tried creating:

type DeltaRun struct{}

var _ Task = &DeltaRun{}

func (d *DeltaRun) Run(c *Context) error {
	return defaultDeltaRunMethod(d, c)
}

I also had to make fi.getDependencies() ignore anonymous struct fields.

The problem was that defaultDeltaRunMethod got a reference to the embedded empty struct, not the enclosing struct, so reflection didn't find any of the enclosing struct's methods.

I then tried:

type HasFind interface {
	Task
	Find(c *Context) (HasFind, error)
}

and then modifying (*executor) forkJoin():

			if hasFind, ok := ts.task.(HasFind); ok {
				results[index] = defaultDeltaRunMethod(hasFind, e.context)
			} else {
				results[index] = ts.task.Run(e.context)
			}

There were two problems:

1. Go doesn't permit union types to have methods, so I couldn't define Task itself to be a type that contains either Run or Find. That means HasFind still had to define a Run method (that would be never called).

2. In order for the (HasFind) type assertion to work, all the Find methods had to weaken the type of their first return value to HasFind. I couldn't find a way in Go to type-assert that it had to be a pointer to its own type.

In all, such a refactor doesn't look like it would gain much.

[justinsb]

I think I had a breakthrough ... I like the approach, my objection has always been about exposing the generic implementation to all the tasks etc.

While type definitions didn't work right (type CloudupTask Task[CloudupContext] confused the compiler), type aliases did work type CloudupTask = Task[CloudupContext].

I've sent a PR to this PR: johngmyers#21 ... although I'm not sure that the tests are running there, so I might have to send a WIP PR here instead.

Let me know what you think @johngmyers ... I can probably rebase if needed, but this would make me happy - we get generics, we split the types, and we don't have to repeat the generic type names everywhere.

I don't love my name CloudupSubContext, but one good thing there is that these names shouldn't escape outside of fi, so they can be awkward :-)

[johngmyers]

/retest

[johngmyers]

/hold cancel

[johngmyers]

Failing e2e tests should be fixed by #14800

[johngmyers]

/retest

[johngmyers]

@justinsb is this good to go now?

[justinsb]

I'm good with it if you are :-)

/approve
/lgtm

/hold to make sure you're happy too @johngmyers

This is really nice BTW - thanks for doing this!

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [justinsb]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[johngmyers]

/hold cancel