feat: add pod clique pod index to pod names#634
Conversation
steved
force-pushed
the
steved/pod-name-pod-index
branch
2 times, most recently
from
ab1c617 to
d161433
Compare
steved
changed the title
steved
force-pushed
the
steved/pod-name-pod-index
branch
from
d161433 to
814344e
Compare
steved
force-pushed
the
steved/pod-name-pod-index
branch
from
814344e to
02d199a
Compare
steved
changed the title
steved
requested review from
Ronkahn21,
danbar2,
gflarity,
sanjaychatterjee,
shayasoolin and
unmarshall
as code owners
| if len(podGroup.PodReferences) == 0 { | ||
| continue | ||
| } | ||
| podRefName := podGroup.PodReferences[0].Name |
There was a problem hiding this comment.
The podgroup name is already the fully qualified podclique name (since a09e370) so I think it was just an oversight that this still parses the pod references. Happy to back it out.
| resourceNameLength := len(pcsName) + len(pclqName) | ||
| // <pcs-name>-<pcs-index>-<pclq-name>-<pod-index>-<random> | ||
| func validatePodNameConstraints(pcsName string, pcsReplicas int32, pcsgName string, pcsgReplicas int32, pclqName string, pclqReplicas int32) error { | ||
| pclqOwnerNameReplica := apicommon.ResourceNameReplica{Name: pcsName, Replica: int(max(0, pcsReplicas-1))} |
There was a problem hiding this comment.
why are you using the max between 0 to max replicas instead of the exact pcs replica index? same question for all usages of max replicas in the ResourceNameReplica builder
There was a problem hiding this comment.
Some of these could be removed. Podclique replicas must be >= 1, but PCS replicas can be 0. This was to ensure we validate the exact index (replicas: 10 creates PCS 0 - 9) and we don't validate against -1 if replicas: 0 is set.
|
|
||
| // MaxReplicas returns the maximum number of replicas for this PodClique. | ||
| func (p PodCliqueSpec) MaxReplicas() int32 { | ||
| replicas := p.Replicas |
There was a problem hiding this comment.
Small question — this is a bit asymmetric with
PodCliqueScalingGroupConfig.MaxReplicas() below, which floors at 1 via
max(1, ptr.Deref(p.Replicas, 0)). Is that intentional (e.g. zero replicas is
a valid scaled-to-zero state for a PodClique), or would it be worth flooring
here too for consistency? Happy either way — just wanted to flag it. Thanks!
There was a problem hiding this comment.
Ah yeah, they do have slightly different behaviors. 0 isn't valid for replicas and the field is required, but the default for scaling configs is only set in the API so maybe we just trust that and deref it directly.
4 participants
What type of PR is this?
/kind feature
What this PR does / why we need it:
For better at-a-glance visibility, we'd like to encode the PCLQ pod index in the pod name.
Which issue(s) this PR fixes:
Fixes #635
Special notes for your reviewer:
GenerateNameand into Grove. Rather than appending a 5-char suffix, we can rely on the randomness of 3 characters since pod names can only conflict with their same index./scaleAPI callsDoes this PR introduce a API change?
Additional documentation e.g., enhancement proposals, usage docs, etc.:
N/A