feat: a new pod naming rule for instanceset #9292
Conversation
8b49f68 to
e7a89c9
Compare
e7a89c9 to
15893fa
Compare
15893fa to
ff58e24
Compare
ff58e24 to
41c2834
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9292 +/- ##
==========================================
+ Coverage 60.09% 60.19% +0.10%
==========================================
Files 391 399 +8
Lines 47616 48034 +418
==========================================
+ Hits 28613 28916 +303
- Misses 16203 16302 +99
- Partials 2800 2816 +16
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| func (t *componentServiceTransformer) podsNameNSuffix(synthesizeComp *component.SynthesizedComponent) (map[string]string, error) { | ||
| podNames, err := generatePodNames(synthesizeComp) | ||
| func (t *componentServiceTransformer) podsNameNSuffix(synthesizeComp *component.SynthesizedComponent, runningITS *workloadsv1.InstanceSet) (map[string]string, error) { | ||
| podNames, err := component.GeneratePodNamesByITS(runningITS) |
There was a problem hiding this comment.
The running ITS should not be used to calculate the expected pods.
There was a problem hiding this comment.
Should use proto its? IMO using running its will eventually converge, just need another reconciliation loop.
There was a problem hiding this comment.
Another reconciliation loop can indeed bring all resources to the final state, e.g., (a1, b1, c1) -> (a2, b2, c2). However, in the current reconciliation loop, it will put the resources in an inconsistent state, something like: (a1, b1, c1) -> (a2, b2, c1), and the impact of this inconsistency is uncertain. The difference here lies in the inconsistency rather than the eventual consistency.
43ab64b to
8675be1
Compare
pkg/controller/instanceset/instancetemplate/pod_name_builder_separated.go
Show resolved
Hide resolved
pkg/controller/instanceset/instancetemplate/pod_name_builder_separated.go
Show resolved
Hide resolved
pkg/controller/instanceset/instancetemplate/pod_name_builder_separated.go
Show resolved
Hide resolved
pkg/controller/instanceset/instancetemplate/pod_name_builder_combined.go
Show resolved
Hide resolved
pkg/controller/instanceset/instancetemplate/pod_name_builder_combined.go
Show resolved
Hide resolved
pkg/controller/instanceset/instancetemplate/pod_name_builder_combined.go
Show resolved
Hide resolved
| return fmt.Errorf("ordinal(%v) must >= 0", item) | ||
| } | ||
| if ordinalSet.Has(item) { | ||
| return fmt.Errorf("duplicate ordinal(%v)", item) |
There was a problem hiding this comment.
Duplicates within the same template should be okay.
| for _, pod := range pods { | ||
| templateName, ok := pod.Labels[TemplateNameLabelKey] | ||
| if !ok { | ||
| return fmt.Errorf("unknown pod %v", klog.KObj(pod)) |
There was a problem hiding this comment.
Not all pods have a template.
There was a problem hiding this comment.
Pod without a template will get an empty name.
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
What would happen if the ordinals of two templates were (partially) exchanged?
There was a problem hiding this comment.
You mean the order two templates are changed? Then nothing should happen in theory. Template lists are sorted by template name before the pod name calculation.
There was a problem hiding this comment.
From: template a -> [0, 1, 2], template b -> [3, 4, 5], to: template a -> [0, 1, 3], template b -> [2, 4, 5].
There was a problem hiding this comment.
GenerateTemplateName2OrdinalMap will return the updated ordinal map.
| @@ -480,6 +480,10 @@ type ClusterComponentSpec struct { | |||
| // +listMapKey=name | |||
| Instances []InstanceTemplate `json:"instances,omitempty" patchStrategy:"merge,retainKeys" patchMergeKey:"name"` | |||
|
|
|||
| // +optional | |||
| // +kubebuilder:default=Separated | |||
| PodNamingRule PodNamingRule `json:"podNamingRule,omitempty"` | |||
There was a problem hiding this comment.
Why is this field defined outside the instance template?
There was a problem hiding this comment.
Then how does a user test the new naming rule? Use a feature gate?
There was a problem hiding this comment.
The question is why not defined it within the instance template?
There was a problem hiding this comment.
The naming rule applies to the whole instanceset, not just one instance template.
Changes related to OpsRequest will be implemented in another PR.