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

add status.labelSelector field to unitedDeployment to support scale sub-resource #1314

Merged
merged 1 commit into from
Jun 15, 2023
Merged

add status.labelSelector field to unitedDeployment to support scale sub-resource #1314

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
8 changes: 6 additions & 2 deletions apis/apps/v1alpha1/uniteddeployment_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,12 @@ limitations under the License.
package v1alpha1

import (
"github.com/openkruise/kruise/apis/apps/v1beta1"
appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/intstr"

"github.com/openkruise/kruise/apis/apps/v1beta1"
)

// UpdateStrategyType is a string enumeration type that enumerates
Expand Down Expand Up @@ -230,6 +231,9 @@ type UnitedDeploymentStatus struct {
// Records the information of update progress.
// +optional
UpdateStatus *UpdateStatus `json:"updateStatus,omitempty"`

// LabelSelector is label selectors for query over pods that should match the replica count used by HPA.
LabelSelector string `json:"labelSelector,omitempty"`
}

// UnitedDeploymentCondition describes current state of a UnitedDeployment.
Expand Down Expand Up @@ -267,7 +271,7 @@ type UpdateStatus struct {
// +k8s:openapi-gen=true
// +kubebuilder:object:root=true
// +kubebuilder:subresource:status
// +kubebuilder:subresource:scale:specpath=.spec.replicas,statuspath=.status.replicas,selectorpath=.status.selector
// +kubebuilder:subresource:scale:specpath=.spec.replicas,statuspath=.status.replicas,selectorpath=.status.labelSelector
// +kubebuilder:resource:shortName=ud
// +kubebuilder:printcolumn:name="DESIRED",type="integer",JSONPath=".spec.replicas",description="The desired number of pods."
// +kubebuilder:printcolumn:name="CURRENT",type="integer",JSONPath=".status.replicas",description="The number of currently all pods."
Expand Down
6 changes: 5 additions & 1 deletion config/crd/bases/apps.kruise.io_uniteddeployments.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1181,6 +1181,10 @@ spec:
description: CurrentRevision, if not empty, indicates the current
version of the UnitedDeployment.
type: string
labelSelector:
description: LabelSelector is label selectors for query over pods
that should match the replica count used by HPA.
type: string
observedGeneration:
description: ObservedGeneration is the most recent generation observed
for this UnitedDeployment. It corresponds to the UnitedDeployment's
Expand Down Expand Up @@ -1234,7 +1238,7 @@ spec:
storage: true
subresources:
scale:
labelSelectorPath: .status.selector
labelSelectorPath: .status.labelSelector
specReplicasPath: .spec.replicas
statusReplicasPath: .status.replicas
status: {}
Expand Down
23 changes: 17 additions & 6 deletions pkg/controller/uniteddeployment/uniteddeployment_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,6 @@ import (
"fmt"
"reflect"

appsv1alpha1 "github.com/openkruise/kruise/apis/apps/v1alpha1"
appsv1beta1 "github.com/openkruise/kruise/apis/apps/v1beta1"
"github.com/openkruise/kruise/pkg/controller/uniteddeployment/adapter"
utilclient "github.com/openkruise/kruise/pkg/util/client"
utildiscovery "github.com/openkruise/kruise/pkg/util/discovery"
"github.com/openkruise/kruise/pkg/util/ratelimiter"
appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/errors"
Expand All @@ -40,6 +34,14 @@ import (
"sigs.k8s.io/controller-runtime/pkg/manager"
"sigs.k8s.io/controller-runtime/pkg/reconcile"
"sigs.k8s.io/controller-runtime/pkg/source"

appsv1alpha1 "github.com/openkruise/kruise/apis/apps/v1alpha1"
appsv1beta1 "github.com/openkruise/kruise/apis/apps/v1beta1"
"github.com/openkruise/kruise/pkg/controller/uniteddeployment/adapter"
"github.com/openkruise/kruise/pkg/util"
utilclient "github.com/openkruise/kruise/pkg/util/client"
utildiscovery "github.com/openkruise/kruise/pkg/util/discovery"
"github.com/openkruise/kruise/pkg/util/ratelimiter"
)

func init() {
Expand Down Expand Up @@ -231,6 +233,14 @@ func (r *ReconcileUnitedDeployment) Reconcile(_ context.Context, request reconci
return reconcile.Result{}, err
}

selector, err := util.ValidatedLabelSelectorAsSelector(instance.Spec.Selector)
Copy link
Member

Choose a reason for hiding this comment

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

just using ValidatedLabelSelectorAsSelector is not enough to identify some label selector errors, you can call LabelSelectorAsSelector and check if there is any error. Similar code is in the cloneset validation validateCloneSetSpec

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean validate UnitedDeploymentSpec? https://github.com/openkruise/kruise/blob/9e74506ca800877f5bf63723efa33521a3533c46/pkg/webhook/uniteddeployment/validating/uniteddeployment_validation.go#LL52C60-L52C60

Copy link
Member

Choose a reason for hiding this comment

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

yes, it seems the uniteddeployment webhook already validate the selector, is it necessary to validate again the controller

Copy link
Contributor Author

@diannaowa diannaowa Jun 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

yes, it seems the uniteddeployment webhook already validate the selector, is it necessary to validate again the controller

Unnecessary, but we need converts the LabelSelector api type into internalSelector, i think.

if err != nil {
klog.Errorf("Error converting UnitedDeployment %s selector: %v", request, err)
// This is a non-transient error, so don't retry.
return reconcile.Result{}, nil
}
newStatus.LabelSelector = selector.String()

return r.updateStatus(instance, newStatus, oldStatus, nameToSubset, nextReplicas, nextPartitions, currentRevision, updatedRevision, collisionCount, control)
}

Expand Down Expand Up @@ -412,6 +422,7 @@ func (r *ReconcileUnitedDeployment) updateUnitedDeployment(ud *appsv1alpha1.Unit
oldStatus.UpdatedReadyReplicas == newStatus.UpdatedReadyReplicas &&
oldStatus.CurrentRevision == newStatus.CurrentRevision &&
oldStatus.CollisionCount == newStatus.CollisionCount &&
oldStatus.LabelSelector == newStatus.LabelSelector &&
ud.Generation == newStatus.ObservedGeneration &&
reflect.DeepEqual(oldStatus.SubsetReplicas, newStatus.SubsetReplicas) &&
reflect.DeepEqual(oldStatus.UpdateStatus, newStatus.UpdateStatus) &&
Expand Down