CA: refactor utils related to NodeInfos

[towca]

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

This is a part of Dynamic Resource Allocation (DRA) support in Cluster Autoscaler. There were multiple very similar utils related to copying and sanitizing NodeInfos scattered around the CA codebase. Instead of adding similar DRA handling to all of them separately, they're consolidated into a single location that will be later adapted to handle DRA.

Which issue(s) this PR fixes:

The CA/DRA integration is tracked in kubernetes/kubernetes#118612, this is just part of the implementation.

Special notes for your reviewer:

The first commit in the PR is just a squash of #7466, and it shouldn't be a part of this review. The PR will be rebased on top of master after #7466 is merged.

This is intended to be a no-op refactor. It was extracted from #7350 after #7447, and #7466.

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

https://github.com/kubernetes/enhancements/blob/9de7f62e16fc5c1ea3bd40689487c9edc7fa5057/keps/sig-node/4381-dra-structured-parameters/README.md

[towca]

/assign @MaciekPytel
/assign @jackfrancis
/hold

[towca]

/assign @BigDarkClown

[jackfrancis]

yikes

[jackfrancis]

should be if caErr != nil { here?

(also do we need to define a new caErr variable here instead of just re-using err?)

[towca]

Ugh my bad, good catch!

As to why we need 2 variables:

• GetNodeInfo returns the error interface, so err has type error.
• TemplateNodeInfoFromExampleNodeInfo returns the errors.AutoscalerError interface.
• We need to return the errors.AutoscalerError interface from Process().
• We could technically assign the TemplateNodeInfoFromExampleNodeInfo error of type errors.AutoscalerError to err, since errors.AutoscalerError is a superset of error. But then we'd have to wrap it in errors.AutoscalerError right back to return it, which is IMO confusing.

[jackfrancis]

ACK, all makes sense!

[jackfrancis]

Because the NewNodeInfo constructor only sets a node object if the passed in node is not nil:

	if node != nil {
		result.schedNodeInfo.SetNode(node)
	}

... invoking n.Node().DeepCopy(), inline like this might be (theoretically) subject to a nil pointer exception

[jackfrancis]

nevermind, you can ignore this comment

// Node returns overall information about this node.
func (n *NodeInfo) Node() *v1.Node {
	if n == nil {
		return nil
	}
	return n.node
}

[towca]

Yeah this should be okay because DeepCopy() gracefully handles nil receivers. Added a comment for clarity and more test cases to cover this scenario.

[jackfrancis]

is this error response too generic?

[towca]

Good point, relying on the CloudProvider error being descriptive enough doesn't sound like a good idea. Added a prefix.

This also required making AddPrefix() wrap the error instead of just modifying the message and implementing Unwrap() so that we can use errors.Is() to check if SanitizedTemplateNodeInfoFromNodeGroup failed because of cloudprovider.ErrNotImplemented. Added a separate commit for that to this PR, and changed the check in MixedTemplateNodeInfoProvider from == to errors.Is().

[jackfrancis]

I don't think we need to assign this to a var

[towca]

Definitely, this must be a leftover from old code - removed.

[jackfrancis]

Not sure if I love the term "example" here. Would TemplateNodeInfoFromNode or TemplateNodeInfoFromRealNode or TemplateNodeInfoFromRealNodeInfo work? Then we'd document like this

// TemplateNodeInfoFromNode returns a template NodeInfo object based on a NodeInfo from a real node on the cluster. The template is sanitized, and only

	// We need to sanitize the node before determining the DS pods, since taints are checked there, and
	// we might need to filter some out during sanitization.
	sanitizedNode := sanitizeNodeInfo(realNode, newNodeNameBase, randSuffix, &taintConfig)

// No need to sanitize the expected pods again - they either come from sanitizedNode and were sanitized above,

etc.

I think my observation is that the word "example" suggests something non-real, mock object, something like that.

[towca]

Interesting point about "example" sugesting something not real, I haven't really thought about it this way. I went with Bartek's naming suggestions which are very similar to yours. WDYT?

[BigDarkClown]

It is nice that we are introducing pod name sanitization, but I am not the fan of just adding a suffix here. As we are not constrained here, what would you think about doing something like just adding a random suffix always? This would also simplify the interface a bit.

[towca]

I explained why I think consistent suffixes within a NodeInfo are important in the response to your comment on sanitizeNodeInfo. WDYT?

[BigDarkClown]

+1, I am convinced :)

[BigDarkClown]

I don't like this interface, mainly the newNodeNameBase and namesSuffix. It seems confusing and overworded, I need to fully understand the internal implementation to be able to get what it does to node and pod names. I think a better option would be to just go with newNodeName and change the logic for pod names to not depend on the namesSuffix.

[towca]

By interface do you mean the sanitizeNodeInfo signature? I know it's not ideal, but the function is very short and local (I intentionally kept it unexported and left these details out from the public functions).

IMO having the suffix passed separately so that we can apply the same one to all of the names we're changing is very helpful for debugging. If you're looking at a template NodeInfo (e.g. in the CA debugging snapshot, or in a live debugger), you can easily see which pods were sanitized together with the Node (they have the same suffix as the Node), and distinguish them from "real" pods which got added later. This gets even more helpful when we start sanitizing DRA objects with a bunch of names that all need a suffix. I actually added this after losing an hour debugging a unit test and getting confused by the suffixes.

The local sanitizeNodeInfo being slightly more confusing seems to me like a fair price to pay for the added debuggability.

[BigDarkClown]

I agree these are very good arguments, this will improve stuff from the debugging perspective. +1 from me.

[BigDarkClown]

I am not a fan of naming of all the functions now. Fresh/Template/Example does not mean much, they just add to confusion. As the user of these I would still need to go to implementation and checkout what it does exactly, the interface is not great because of that.

I think if we restrict ourselves to few key-words ("Template", "Sanitized", "DeepCopy"), we would make it easier to understand what is going on.

My proposal:

# Template methods, best with the same interface [(nodeGroup)/(nodeInfo, ngName)[, DSs, forceDs, taintConfig
TemplateNodeInfoFromNodeGroupTemplate -> SanitizedTemplateNodeInfoFromNodeGroup
TemplateNodeInfoFromExampleNodeInfo -> SanitizedTemplateNodeInfoFromNodeInfo

# DeepCopy methods
FreshNodeInfoFromTemplateNodeInfo -> nodeInfo.SanitizedDeepCopy(nodeName)
nodeInfo.DeepCopy stays the same

What do you think? IMO it would be more self descriptive.

[towca]

Fair enough, "sanitize" is basically a recognized term in CA while "fresh" and "example" aren't. Went with your naming suggestions!

I'd prefer to keep all of the sanitization logic in a single place though (especially because otherwise we'd have to export sanitizeNodeInfo which I want to avoid), so I renamed FreshNodeInfoFromTemplateNodeInfo to NodeInfoSanitizedDeepCopy instead of making it a NodeInfo method. WDYT?

[BigDarkClown]

I agree that while having nodeInfo.SanitizedDeepCopy would be nice, it is better to keep it in the same place instead of splitting the sanitization into multiple places. LGTM.

[BigDarkClown]

Changes LGTM, I've added hold but feel free to remove once you are comfortable to do so.

/lgtm
/approve
/hold

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BigDarkClown, towca

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:

• cluster-autoscaler/OWNERS [BigDarkClown,towca]

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

[towca]

@jackfrancis Just waiting for your LGTM now then

[jackfrancis]

/lgtm

[jackfrancis]

/hold cancel