-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
enhancement (CollaSet): CollaSet supports scaling and updating #22
Conversation
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
type ByPartition struct { | ||
// Partition controls the update progress by indicating how many pods should be updated. | ||
// Defaults to nil (all pods will be updated) | ||
Partition *int32 `json:"partition,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What will happen if partition is 0?
no pods will be updated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
currently
If partition is 0, no pods will be updated
if partition is not set, all pods will be updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is nice to put the description into the comment
@Eikykun all fixed |
apis/apps/v1alpha1/collaset_types.go
Outdated
// Replicas is the most recently observed number of replicas. | ||
Replicas int32 `json:"replicas,omitempty"` | ||
|
||
// The number of pods in current version. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pods in updated version
may be more clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
Status corev1.ConditionStatus `json:"status,omitempty"` | ||
|
||
// Last time the condition transitioned from one status to another. | ||
LastTransitionTime metav1.Time `json:"last_transition_time,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*metav1.Time
may be better, unless this data is never empty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is ok. It is not allowed to be nil.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Acording to convension, the json tag should be lastTransitionTime
apis/apps/v1alpha1/collaset_types.go
Outdated
// +kubebuilder:printcolumn:name="CURRENT_REVISION",type="string",JSONPath=".status.currentRevision",description="The current revision." | ||
// +kubebuilder:printcolumn:name="UPDATED_REVISION",type="string",JSONPath=".status.updatedRevision",description="The updated revision." | ||
// +kubebuilder:printcolumn:name="AGE",type="date",JSONPath=".metadata.creationTimestamp" | ||
// +resource:path=collasets,strategy=CollaSetStrategy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
strategy in this marker is not necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should standardize annotation and label key prefix to kafed.kusionstack.io or {something}.kafed.kusionstack.io
follow these rules:
If the lablel is used only by collaset, the prefix should be collaset.kafed.kusionstack.io
If the label is shared in kafed, the prefix should be kafed.kusionstack.io
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point
fixed a few labels involved in CollaSet commit
return pod.Status.Phase == corev1.PodSucceeded || pod.Status.Phase == corev1.PodFailed | ||
} | ||
|
||
func NewPodFrom(cs *appsv1alpha1.CollaSet, revision *appsv1.ControllerRevision) (*corev1.Pod, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This func is not used, maybe have been moved to pod_utils.go
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this method is removed
// TODO use cache | ||
pod, err := controllerutils.NewPodFrom(set, metav1.NewControllerRef(set, appsv1alpha1.GroupVersion.WithKind("CollaSet")), revision) | ||
if err != nil { | ||
return fmt.Errorf("fail to new pod from revision %s: %s", updatedRevision.Name, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
revision.Name
is right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
// allocate new Pod a instance ID | ||
newPod.Labels[appsv1alpha1.PodInstanceIDLabelKey] = fmt.Sprintf("%d", availableIDContext.ID) | ||
|
||
if pod, err := sc.podControl.CreatePod(newPod, updatedRevision); err == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will create pod use revision
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
return false, nil | ||
} | ||
|
||
pod.GetLabels()[appsv1alpha1.PodScalingInLabelKey] = fmt.Sprintf("%d", time.Now().UnixNano()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This lable will be deleted in the right time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this unused label
// do delete Pod resource | ||
succCount, err = controllerutils.SlowStartBatch(len(podCh), controllerutils.SlowStartInitialBatchSize, false, func(i int, _ error) error { | ||
pod := <-podCh | ||
if err := sc.podControl.DeletePod(pod.Pod); err == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe block below is easy to understand.
if err := sc.podControl.DeletePod(pod.Pod); err != nil {
return err
}
if err := collasetutils.ActiveExpectations.ExpectDelete(set, expectations.Pod, pod.Name); err != nil {
return err
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
if err != nil { | ||
collasetutils.AddOrUpdateCondition(newStatus, appsv1alpha1.CollaSetScale, err, "ScaleOutFailed", err.Error()) | ||
return succCount > 0, err | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These else
s is not needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
return calculateStatus(instance, newStatus, updatedRevision, podWrappers, syncErr), syncErr | ||
} | ||
|
||
func doSync(instance *appsv1alpha1.CollaSet, updatedRevision *appsv1.ControllerRevision, revisions []*appsv1.ControllerRevision, newStatus *appsv1alpha1.CollaSetStatus) ([]*collasetutils.PodWrapper, *appsv1alpha1.CollaSetStatus, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Giving some comments in this core func will be very nice.❤️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment added
@zoumo @shaofan-hs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that this test file is empty, delete it pls
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
PodAvailableConditionsAnnotation = "kafed.kusionstack.io/available-conditions" // indicate the available conditions of a pod | ||
// --- Begin: Annotations for RuleSet --- | ||
|
||
AnnotationPodSkipRuleConditions = "apps.kafed.io/skip-rule-conditions" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this prefix so different?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is not used. removed
|
||
revisionManager = revision.NewRevisionManager(mgr.GetClient(), mgr.GetScheme(), &revisionOwnerAdapter{}) | ||
podControl = podcontrol.NewRealPodControl(mgr.GetClient(), mgr.GetScheme()) | ||
syncControl = synccontrol.NewRealSyncControl(mgr.GetClient(), podControl, recorder) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is It more appropriate to put these three variable into CollaSetReconciler ?
It is so strange to init global var in a struct construct function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
…le in case of using native K8s
} | ||
|
||
return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lint
simplify
} else {
return r.SetExpectations(controllerKey, resourceVersion)
}
return nil
to
return r.SetExpectations(controllerKey, resourceVersion)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't fix. This will break the original logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
1. Does this PR affect any open issues?(Y/N) and add issue references (e.g. "fix #123", "re #123".):
re #9
2. What is the scope of this PR (e.g. component or file name):
3. Provide a description of the PR(e.g. more details, effects, motivations or doc link):
This PR implements basical feature of CollaSet:
4. Are there any breaking changes?(Y/N) and describe the breaking changes(e.g. more details, motivations or doc link):
5. Are there test cases for these changes?(Y/N) select and add more details, references or doc links:
6. Release note
Please refer to Release Notes Language Style Guide to write a quality release note.