feat: check for available node resources before building injector pod

[chrishorton]

Description

Addresses #2144 to check if node has enough resources before the injector runs

Related Issue

Fixes #2144

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Other (security config, docs update, etc)

Checklist before merging

• Test, docs, adr added or updated as needed
• Contributor Guide Steps followed

[netlify]

✅ Deploy Preview for zarf-docs canceled.

Name	Link
🔨 Latest commit	5dec437
🔍 Latest deploy log	https://app.netlify.com/sites/zarf-docs/deploys/659f0413d23d4c000830b2d9

[Racer159]

Thanks for the PR! Looks like some of the logic needs updating and passing down the resources into GetImagesWithNodes from the cluster package would help keep the code drier.

[chrishorton]

Resolved all your suggestions in latest commit, thanks!

[Racer159]

Thanks for the changes! Logic looks good (will let the tests run to make sure) - since this is library code made some suggestions for naming though.

[chrishorton]

Pushed those changes!

[lucasrod16]

I think we should try to avoid changing the function signatures of GetAllImages and GetImagesWithNodes because they are public facing APIs.

var (
	injectorRequestedCPU    = resource.MustParse(".5")
	injectorRequestedMemory = resource.MustParse("64Mi")
	injectorLimitCPU        = resource.MustParse("1")
	injectorLimitMemory     = resource.MustParse("256Mi")
)

These variables are static and will likely not change often, so we could move them to the k8s package, make them public, and use them directly in the functions that need them instead of passing them in as parameters.

@Racer159 @Noxsios thoughts?

[chrishorton]

@lucasrod16 I would tend to agree with you on that. Wasn't sure about creating new global variables but not changing the function signature would be more ideal since it's public facing and would be a breaking change for usages.

[lucasrod16]

@chrishorton thanks for submitting this PR 🙂

[Racer159]

I think we should try to avoid changing the function signatures of GetAllImages and GetImagesWithNodes because they are public facing APIs.

var (
	injectorRequestedCPU    = resource.MustParse(".5")
	injectorRequestedMemory = resource.MustParse("64Mi")
	injectorLimitCPU        = resource.MustParse("1")
	injectorLimitMemory     = resource.MustParse("256Mi")
)

These variables are static and will likely not change often, so we could move them to the k8s package, make them public, and use them directly in the functions that need them instead of passing them in as parameters.

@Racer159 @Noxsios thoughts?

I suggested this direction since it would be relatively easier to implement than a larger refactor that is likely warranted for how the injector pod gets built - we only use these functions for this purpose so they could ultimately be tailored more to the injector node selection process and removed from k8s if we wanted. Currently IMO it would make more sense to move things into cluster than to k8s because no Zarf-specific logic is in k8s today (the only "zarf" thing it imports is helpers and that also has a rule to not import anything else from Zarf.

The injector* variables are global to the cluster package so we could mess things up in the future but they are private so people importing cluster won't be able to mess them up without something like go:linkname which is why I was ok with them being there for now (would just be up to the Zarf maintainers to make sure that they aren't modified).

I do agree the function signature is a little odd taking in the node resource quantities though so we could refactor both GetImagesWithNodes and GetAllImages into a function with a better signature (potentially something like func GetImagesFromAvailableNodes(timeoutDuration time.Duration, minResources corev1.ResourceList) (ImageNodeMap, error) that would check all resource types and be a bit better named.

[Racer159]

For historical context, these functions are also a bit appendix-like - they had more of a purpose back when Zarf performed more node checks on init but we removed some of those to reduce the permissions Zarf needs to run in a cluster.

[lucasrod16]

I suggested this direction since it would be relatively easier to implement than a larger refactor that is likely warranted for how the injector pod gets built - we only use these functions for this purpose so they could ultimately be tailored more to the injector node selection process and removed from k8s if we wanted. Currently IMO it would make more sense to move things into cluster than to k8s because no Zarf-specific logic is in k8s today (the only "zarf" thing it imports is helpers and that also has a rule to not import anything else from Zarf.

The injector* variables are global to the cluster package so we could mess things up in the future but they are private so people importing cluster won't be able to mess them up without something like go:linkname which is why I was ok with them being there for now (would just be up to the Zarf maintainers to make sure that they aren't modified).

I do agree the function signature is a little odd taking in the node resource quantities though so we could refactor both GetImagesWithNodes and GetAllImages into a function with a better signature (potentially something like func GetImagesFromAvailableNodes(timeoutDuration time.Duration, minResources corev1.ResourceList) (ImageNodeMap, error) that would check all resource types and be a bit better named.

Good point on moving things to the cluster package. After taking a closer look, GetImagesWithNodes and GetAllImages are defined in the k8s package but only used in the cluster package so I agree that makes more sense. We could do a gradual code repair for this logic by just moving the existing functions GetImagesWithNodes and GetAllImages to the cluster package for now and tackle it one small piece at a time.

[chrishorton]

That would work. When I was working on this, trying for an elegant solution (not changing func signatures) everything produced a circular dependency because of how the logic is intertwined and used in the two packages.

For historical context, these functions are also a bit appendix-like - they had more of a purpose back when Zarf performed more node checks on init but we removed some of those to reduce the permissions Zarf needs to run in a cluster.

I think this is also a good reason to refactor getAllImages and GetImagesWithNodes out of the k8s package.

Would be happy to work on this, if a new issue is created to refactor/migrate.

[chrishorton]

@chrishorton thanks for submitting this PR 🙂

Glad I can help a bit! I'm struggling to manually deploy a lot of our software over an air-gap so any tooling that helps is gold to me 😁

[Noxsios]

I don't have much to add aside from echoing that the relationship between pkg/k8s and pkg/cluster needs to be re-evaluated. A bit ago we moved pkg/cluster from internal to pkg (#2162) to support external usage of cluster. IMO cluster should be the de-facto library export and k8s's functions should be merged in (as cluster is a utility for interacting w/ Zarf specific cluster needs.). Users should be using the Kubernetes K8s lib otherwise.

Doing some cursory checks, I can also see that there are some public receivers on k8s that are not used in Zarf, IMO those should be removed during this refactor (yes a breaking change, but 99% of them are bare abstractions on top of the core k8s lib).

[Racer159]

Created this issue to capture the above feedback - #2222 (feel free to pick it up if you want @chrishorton !)

Going to approve this as is for now though with the intent that the tech debt gets addressed in the future.

[Racer159]

lgtm