Skip to content
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.

Sign up for GitHub

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

chore: simplify comparison and variable declaration #1209

Merged
merged 1 commit into from
Mar 9, 2023
Merged

chore: simplify comparison and variable declaration #1209

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions pkg/control/sidecarcontrol/sidecarset_control.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ func (c *commonControl) UpdatePodAnnotationsInUpgrade(changedContainers []string
// if it is changed, indicates the update is complete.
// format: sidecarset.name -> appsv1alpha1.InPlaceUpdateState
sidecarUpdateStates := make(map[string]*pub.InPlaceUpdateState)
if stateStr, _ := pod.Annotations[SidecarsetInplaceUpdateStateKey]; len(stateStr) > 0 {
if stateStr := pod.Annotations[SidecarsetInplaceUpdateStateKey]; len(stateStr) > 0 {
if err := json.Unmarshal([]byte(stateStr), &sidecarUpdateStates); err != nil {
klog.Errorf("parse pod(%s/%s) annotations[%s] value(%s) failed: %s",
pod.Namespace, pod.Name, SidecarsetInplaceUpdateStateKey, stateStr, err.Error())
Expand Down Expand Up @@ -136,7 +136,6 @@ func (c *commonControl) UpdatePodAnnotationsInUpgrade(changedContainers []string
sidecarUpdateStates[sidecarSet.Name] = inPlaceUpdateState
by, _ := json.Marshal(sidecarUpdateStates)
pod.Annotations[SidecarsetInplaceUpdateStateKey] = string(by)
return
}

// only check sidecar container is consistent
Expand Down
6 changes: 2 additions & 4 deletions pkg/controller/advancedcronjob/advancedcronjob_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,11 +200,9 @@ func createNode(nodeName string) *v1.Node {
}

func createJob(jobName string, template appsv1alpha1.CronJobTemplate) *appsv1alpha1.AdvancedCronJob {
var historyLimit int32
historyLimit = 3
var historyLimit int32 = 3

var paused bool
paused = false
paused := false
job1 := &appsv1alpha1.AdvancedCronJob{
ObjectMeta: metav1.ObjectMeta{
Name: jobName,
Expand Down
19 changes: 7 additions & 12 deletions pkg/controller/broadcastjob/broadcastjob_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,10 +123,7 @@ func add(mgr manager.Manager, r reconcile.Reconciler) error {
}

// Watch for changes to Node
if err = c.Watch(&source.Kind{Type: &corev1.Node{}}, &enqueueBroadcastJobForNode{reader: mgr.GetCache()}); err != nil {
return err
}
return nil
return c.Watch(&source.Kind{Type: &corev1.Node{}}, &enqueueBroadcastJobForNode{reader: mgr.GetCache()})
}

var _ reconcile.Reconciler = &ReconcileBroadcastJob{}
Expand Down Expand Up @@ -178,7 +175,8 @@ func (r *ReconcileBroadcastJob) Reconcile(_ context.Context, request reconcile.R
addLabelToPodTemplate(job)

if IsJobFinished(job) {
if isPast, leftTime := pastTTLDeadline(job); isPast {
isPast, leftTime := pastTTLDeadline(job)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why you need to change here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to write something like

if !isPast && leftTime>0 { return xxx }

klog.Infof(xxx)
...
return xxx

But I was not sure of its correctness, so I reverted it, but this modification was kept.

if isPast {
klog.Infof("deleting the job %s", job.Name)
err = r.Delete(context.TODO(), job)
if err != nil {
Expand Down Expand Up @@ -245,9 +243,8 @@ func (r *ReconcileBroadcastJob) Reconcile(_ context.Context, request reconcile.R
failed := int32(len(failedPods))
succeeded := int32(len(succeededPods))

var desired int32
desiredNodes, restNodesToRunPod, podsToDelete := getNodesToRunPod(nodes, job, existingNodeToPodMap)
desired = int32(len(desiredNodes))
desired := int32(len(desiredNodes))
klog.Infof("%s/%s has %d/%d nodes remaining to schedule pods", job.Namespace, job.Name, len(restNodesToRunPod), desired)
klog.Infof("Before broadcastjob reconcile %s/%s, desired=%d, active=%d, failed=%d", job.Namespace, job.Name, desired, active, failed)
job.Status.Active = active
Expand Down Expand Up @@ -302,7 +299,7 @@ func (r *ReconcileBroadcastJob) Reconcile(_ context.Context, request reconcile.R
} else {
// Job is still active
if len(podsToDelete) > 0 {
//should we remove the pods without nodes associated, the podgc controller will do this if enabled
// should we remove the pods without nodes associated, the podgc controller will do this if enabled
failed, active, err = r.deleteJobPods(job, podsToDelete, failed, active)
if err != nil {
klog.Errorf("failed to deleteJobPods for job %s,", job.Name)
Expand Down Expand Up @@ -391,21 +388,19 @@ func (r *ReconcileBroadcastJob) reconcilePods(job *appsv1alpha1.BroadcastJob,
restNodesToRunPod []*corev1.Node, active, desired int32) (int32, error) {

// max concurrent running pods
var parallelism int32
var err error
parallelismInt, err := intstr.GetValueFromIntOrPercent(intstr.ValueOrDefault(job.Spec.Parallelism, intstr.FromInt(1<<31-1)), int(desired), true)
if err != nil {
return active, err
}
parallelism = int32(parallelismInt)
parallelism := int32(parallelismInt)

// The rest pods to run
rest := int32(len(restNodesToRunPod))
var errCh chan error
if active > parallelism {
// exceed parallelism limit
r.recorder.Eventf(job, corev1.EventTypeWarning, "TooManyActivePods", "Number of active pods exceed parallelism limit")
//TODO should we remove the extra pods ? it may just finish by its own.
// TODO should we remove the extra pods ? it may just finish by its own.

} else if active < parallelism {
// diff is the current number of pods to run in this reconcile loop
Expand Down
4 changes: 2 additions & 2 deletions pkg/controller/broadcastjob/broadcastjob_utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,10 +133,10 @@ func validateControllerRef(controllerRef *metav1.OwnerReference) error {
if len(controllerRef.Kind) == 0 {
return fmt.Errorf("controllerRef has empty Kind")
}
if controllerRef.Controller == nil || *controllerRef.Controller != true {
if controllerRef.Controller == nil || !*controllerRef.Controller {
return fmt.Errorf("controllerRef.Controller is not set to true")
}
if controllerRef.BlockOwnerDeletion == nil || *controllerRef.BlockOwnerDeletion != true {
if controllerRef.BlockOwnerDeletion == nil || !*controllerRef.BlockOwnerDeletion {
return fmt.Errorf("controllerRef.BlockOwnerDeletion is not set")
}
return nil
Expand Down
6 changes: 3 additions & 3 deletions pkg/controller/cloneset/cloneset_predownload_image.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,12 +61,12 @@ func (r *ReconcileCloneSet) createImagePullJobsForInPlaceUpdate(cs *appsv1alpha1
// ignore if all Pods update in one batch
var partition, maxUnavailable int
if cs.Spec.UpdateStrategy.Partition != nil {
if pValue, err := util.CalculatePartitionReplicas(cs.Spec.UpdateStrategy.Partition, cs.Spec.Replicas); err != nil {
pValue, err := util.CalculatePartitionReplicas(cs.Spec.UpdateStrategy.Partition, cs.Spec.Replicas)
if err != nil {
klog.Errorf("CloneSet %s/%s partition value is illegal", cs.Namespace, cs.Name)
return err
} else {
partition = pValue
}
partition = pValue
}
maxUnavailable, _ = intstrutil.GetValueFromIntOrPercent(
intstrutil.ValueOrDefault(cs.Spec.UpdateStrategy.MaxUnavailable, intstrutil.FromString(appsv1alpha1.DefaultCloneSetMaxUnavailable)), int(*cs.Spec.Replicas), false)
Expand Down
6 changes: 3 additions & 3 deletions pkg/controller/persistentpodstate/persistent_pod_state_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,10 +299,10 @@ func parseStsPodIndex(podName string) (int, error) {
}

// map[string]*corev1.Pod -> map[Pod.Name]*corev1.Pod
func (p *ReconcilePersistentPodState) getPodsAndStatefulset(persistentPodState *appsv1alpha1.PersistentPodState) (map[string]*corev1.Pod, *innerStatefulset, error) {
func (r *ReconcilePersistentPodState) getPodsAndStatefulset(persistentPodState *appsv1alpha1.PersistentPodState) (map[string]*corev1.Pod, *innerStatefulset, error) {
inner := &innerStatefulset{}
ref := persistentPodState.Spec.TargetReference
workload, err := p.finder.GetScaleAndSelectorForRef(ref.APIVersion, ref.Kind, persistentPodState.Namespace, ref.Name, "")
workload, err := r.finder.GetScaleAndSelectorForRef(ref.APIVersion, ref.Kind, persistentPodState.Namespace, ref.Name, "")
if err != nil {
klog.Errorf("persistentPodState(%s/%s) fetch statefulSet(%s) failed: %s", persistentPodState.Namespace, persistentPodState.Name, ref.Name, err.Error())
return nil, nil, err
Expand All @@ -314,7 +314,7 @@ func (p *ReconcilePersistentPodState) getPodsAndStatefulset(persistentPodState *
inner.DeletionTimestamp = workload.Metadata.DeletionTimestamp

// DisableDeepCopy:true, indicates must be deep copy before update pod objection
pods, _, err := p.finder.GetPodsForRef(ref.APIVersion, ref.Kind, persistentPodState.Namespace, ref.Name, true)
pods, _, err := r.finder.GetPodsForRef(ref.APIVersion, ref.Kind, persistentPodState.Namespace, ref.Name, true)
if err != nil {
klog.Errorf("list persistentPodState(%s/%s) pods failed: %s", persistentPodState.Namespace, persistentPodState.Name, err.Error())
return nil, nil, err
Expand Down
20 changes: 4 additions & 16 deletions pkg/controller/podunavailablebudget/podunavailablebudget_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,10 +140,7 @@ func add(mgr manager.Manager, r reconcile.Reconciler) error {
UpdateFunc: func(e event.UpdateEvent) bool {
old := e.ObjectOld.(*apps.Deployment)
new := e.ObjectNew.(*apps.Deployment)
if *old.Spec.Replicas != *new.Spec.Replicas {
return true
}
return false
return *old.Spec.Replicas != *new.Spec.Replicas
},
DeleteFunc: func(deleteEvent event.DeleteEvent) bool {
return true
Expand All @@ -157,10 +154,7 @@ func add(mgr manager.Manager, r reconcile.Reconciler) error {
UpdateFunc: func(e event.UpdateEvent) bool {
old := e.ObjectOld.(*kruiseappsv1beta1.StatefulSet)
new := e.ObjectNew.(*kruiseappsv1beta1.StatefulSet)
if *old.Spec.Replicas != *new.Spec.Replicas {
return true
}
return false
return *old.Spec.Replicas != *new.Spec.Replicas
},
DeleteFunc: func(deleteEvent event.DeleteEvent) bool {
return true
Expand All @@ -174,10 +168,7 @@ func add(mgr manager.Manager, r reconcile.Reconciler) error {
UpdateFunc: func(e event.UpdateEvent) bool {
old := e.ObjectOld.(*kruiseappsv1alpha1.CloneSet)
new := e.ObjectNew.(*kruiseappsv1alpha1.CloneSet)
if *old.Spec.Replicas != *new.Spec.Replicas {
return true
}
return false
return *old.Spec.Replicas != *new.Spec.Replicas
},
DeleteFunc: func(deleteEvent event.DeleteEvent) bool {
return true
Expand All @@ -191,10 +182,7 @@ func add(mgr manager.Manager, r reconcile.Reconciler) error {
UpdateFunc: func(e event.UpdateEvent) bool {
old := e.ObjectOld.(*apps.StatefulSet)
new := e.ObjectNew.(*apps.StatefulSet)
if *old.Spec.Replicas != *new.Spec.Replicas {
return true
}
return false
return *old.Spec.Replicas != *new.Spec.Replicas
},
DeleteFunc: func(deleteEvent event.DeleteEvent) bool {
return true
Expand Down
2 changes: 1 addition & 1 deletion pkg/controller/sidecarset/sidecarset_hotupgrade.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ func (p *Processor) flipHotUpgradingContainers(control sidecarcontrol.SidecarCon
p.recorder.Eventf(pod, corev1.EventTypeWarning, "ResetContainerFailed", fmt.Sprintf("reset sidecar container image empty failed: %s", err.Error()))
return err
}
p.recorder.Eventf(pod, corev1.EventTypeNormal, "ResetContainerSucceed", fmt.Sprintf("reset sidecar container image empty successfully"))
p.recorder.Eventf(pod, corev1.EventTypeNormal, "ResetContainerSucceed", "reset sidecar container image empty successfully")
}
return nil
}
Expand Down
8 changes: 4 additions & 4 deletions pkg/controller/workloadspread/workloadspread_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -499,7 +499,7 @@ func (r *ReconcileWorkloadSpread) calculateWorkloadSpreadStatus(ws *appsv1alpha1
// set the generation in the returned status
status := appsv1alpha1.WorkloadSpreadStatus{}
status.ObservedGeneration = ws.Generation
//status.ObservedWorkloadReplicas = workloadReplicas
// status.ObservedWorkloadReplicas = workloadReplicas
status.SubsetStatuses = make([]appsv1alpha1.WorkloadSpreadSubsetStatus, len(ws.Spec.Subsets))
scheduleFailedPodMap := make(map[string][]*corev1.Pod)

Expand All @@ -512,7 +512,7 @@ func (r *ReconcileWorkloadSpread) calculateWorkloadSpreadStatus(ws *appsv1alpha1
oldSubsetStatusMap[oldSubsetStatuses[i].Name] = &oldSubsetStatuses[i]
}

var rescheduleCriticalSeconds int32 = 0
var rescheduleCriticalSeconds int32
if ws.Spec.ScheduleStrategy.Type == appsv1alpha1.AdaptiveWorkloadSpreadScheduleStrategyType &&
ws.Spec.ScheduleStrategy.Adaptive != nil &&
ws.Spec.ScheduleStrategy.Adaptive.RescheduleCriticalSeconds != nil {
Expand Down Expand Up @@ -590,7 +590,7 @@ func (r *ReconcileWorkloadSpread) calculateWorkloadSpreadSubsetStatus(ws *appsv1
}
oldDeletingPods = oldSubsetStatus.DeletingPods
}
var active int32 = 0
var active int32

for _, pod := range pods {
// remove this Pod from creatingPods map because this Pod has been created.
Expand Down Expand Up @@ -667,7 +667,7 @@ func (r *ReconcileWorkloadSpread) calculateWorkloadSpreadSubsetStatus(ws *appsv1
func (r *ReconcileWorkloadSpread) UpdateWorkloadSpreadStatus(ws *appsv1alpha1.WorkloadSpread,
status *appsv1alpha1.WorkloadSpreadStatus) error {
if status.ObservedGeneration == ws.Status.ObservedGeneration &&
//status.ObservedWorkloadReplicas == ws.Status.ObservedWorkloadReplicas &&
// status.ObservedWorkloadReplicas == ws.Status.ObservedWorkloadReplicas &&
apiequality.Semantic.DeepEqual(status.SubsetStatuses, ws.Status.SubsetStatuses) {
return nil
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/webhook/resourcedistribution/validating/resourcedistribution_create_update_handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ func (h *ResourceDistributionCreateUpdateHandler) validateResourceDistributionSp
return
}
// deserialize old resource if need
var oldResource runtime.Object = nil
var oldResource runtime.Object
if oldObj != nil {
oldResource, errs = DeserializeResource(&oldObj.Spec.Resource, fldPath)
allErrs = append(allErrs, errs...)
Expand Down