you shouldn't return here, you should wrap the following block in an else

As we will use the supportedInstanceType instead of instance prefix now, the error throw our there should not because not supported issue. I throw the error directly here in new version

we need to check what the err is before we say that the instance isn't supported

I revised the topologySupportedInstance to check the supported instance types instead of the instance prefix, so the DescribeInstanceTopology api should supported here. I will throw error directly

revised

I think we probably just want to work with a list of fully-formed instance type names, prefix matching has some ambiguity and configuration footguns ("p4" would match all p4dn instances, for example). The list isn't going to change very often and we don't need the benefit of shorthand IMO

Do we want to be updating the cloud-provider each time there's a new AWS instance type, though?

would it be a bad idea to do some dynamic discover based on an DescribeInstanceTopology result? then cache the instance types' support with a TTL? from what i can tell the api just returns an empty list when the instance-id's type is not supported

I discussed this with Carter before. We don't want to trigger this api for all nodes, because some cx might have hundreds or thousands of nodes in their cluster, if we call this api for all nodes, it very likely trigger throttle issue.

And DescribeInstanceTopology only supported for very limited instances types which lots of cx might not use. check the supported instance type and regions here. https://docs.aws.amazon.com/AWSEC2/latest/APIReference/API_DescribeInstanceTopology.html

Do we want to be updating the cloud-provider each time there's a new AWS instance type, though?

We don't need to update cloud-provider, TopologySupportedInstanceTypes and TopologySupportedRegions is added in cloud-config where will accept the input in KCP CCM manifest. We only need to update the manifest with the cloud-config input.

I'm not clear how you would use this. How would you "query" the labels using a pod's nodeSelector, or a telemetry system, if the index of the networkNode appears in the label key?

after reading through the instance topology docs i think i have an idea how this works?

Assuming the goal is to run workloads that share the same topology, you'd need to know the network nodes that your capacity sits on first, then use that in a nodeSelector for the corresponding layer index. AFAIK there's no way to apply a nodeSelector based on some heuristic like "schedule pods to nodes where the label topology.k8s.aws/network-node-layer-1 matches", so this logic would have to be handled by a high level system. the only guarentee is that there are always at least 3 layers.

However, this scheme breaks down if network nodes can be part of different layers, which i couldn't immediately grok from the docs but i assumed wasn't possible.

this scheme breaks down if network nodes can be part of different layers

yep, if trees with different roots can overlap (share network nodes).

The long form doc is helpful, the API doc doesn’t even say that order is meaningful in the networkNodeSet. 😄

I’m wondering if the “layer” number needs to be in the labels. Is it enough to just say “schedule this pod within this networkNode” instead of “within this networkNode which is the layer 2 networkNode for the instance”?

—-

A primary motivation for this feature was observability, being able to correlate issues with something more granular than AZ. If you want to slice and dice your instance metrics using network nodes, should you care about the layer of the network node?

If we inverted this, so that the label key contains the networkNode and the value is the layer number, that would allow you to use the Exists operator in a nodeAffinity to place the pod within a networkNode, without needing the layer to also match: https://kubernetes.io/docs/reference/kubernetes-api/common-definitions/node-selector-requirement/#NodeSelectorRequirement

yep, if trees with different roots can overlap (share network nodes).

We can assume that's not the case for now.

A primary motivation for this feature was observability, being able to correlate issues with something more granular than AZ

Do you mean the feature to apply arbitrary labels from within the cloud provider? I think observability at the zone-id level is where we started, but I'd like to think we can help with scheduling here too. The next thing I'm thinking is a label that describes whether an instance has a GPU or not, so we don't need to schedule GPU pods (Device Plugin, EFA etc) on nodes that don't need it etc.

Assuming the goal is to run workloads that share the same topology, you'd need to know the network nodes that your capacity sits on first, then use that in a nodeSelector for the corresponding layer index.

Right that's what I was expecting you'd need to do. You don't know the topology until you launch the instance first, so it forces this ordering of sorts where you launch the instance first, you realize what your network nodes are, and you should ideally see a common network node across your instances. I think you could maybe define weighted node affinities across the network node layers so you pack it as close as possible?

this wrapper's logic seems strange, shouldn't we return the list of []*ec2.InstanceTopology instead of just the first item?

DescribeInstanceTopology can accept a list of instance IDs, if we pass only 1 instance ID, it will return 1 according topology info something like below.

"Instances": [
        {
            "InstanceId": "i-xxx",
            "InstanceType": "p5e.48xlarge",
            "NetworkNodes": [
                "nn-xxxxx",
                "nn-xxxxx",
                "nn-xxxxx"
            ],
            "AvailabilityZone": "us-west-2c",
            "ZoneId": "usw2-az3"
        }
    ]

But yeah, it seems more reasonable to return all instances in api and get the first instance in where trigger it and get result. Will revised in next

agreed, because we still allow the full ec2.DescribeInstanceTopologyInput payload which could specify more than one instance id

revised

IIUC, the info you're embedding into the key is about the layer of the network node in the node set

revised in next version

after reading through the instance topology docs i think i have an idea how this works?

Assuming the goal is to run workloads that share the same topology, you'd need to know the network nodes that your capacity sits on first, then use that in a nodeSelector for the corresponding layer index. AFAIK there's no way to apply a nodeSelector based on some heuristic like "schedule pods to nodes where the label topology.k8s.aws/network-node-layer-1 matches", so this logic would have to be handled by a high level system. the only guarentee is that there are always at least 3 layers.

However, this scheme breaks down if network nodes can be part of different layers, which i couldn't immediately grok from the docs but i assumed wasn't possible.