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

Jump to bottom

Feature/revision index #1733

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Feature/revision index #1733

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
38 changes: 38 additions & 0 deletions pkg/controller/sidecarset/sidecarset_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
appsv1alpha1 "github.com/openkruise/kruise/apis/apps/v1alpha1"
"github.com/openkruise/kruise/pkg/control/sidecarcontrol"

"github.com/openkruise/kruise/pkg/util/fieldindex"
appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand All @@ -17,6 +18,7 @@ import (
"k8s.io/apimachinery/pkg/util/intstr"
clientgoscheme "k8s.io/client-go/kubernetes/scheme"
"k8s.io/client-go/tools/record"
"k8s.io/kubernetes/pkg/apis/apps"
utilpointer "k8s.io/utils/pointer"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/client/fake"
Expand Down Expand Up @@ -137,6 +139,7 @@ func init() {
utilruntime.Must(appsv1.AddToScheme(scheme))
utilruntime.Must(appsv1alpha1.AddToScheme(scheme))
utilruntime.Must(corev1.AddToScheme(scheme))
utilruntime.Must(apps.AddToScheme(scheme))
Copy link
Member

Choose a reason for hiding this comment

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

there is no need to add unversioned scheme here, L139 already do the job

}

func getLatestPod(client client.Client, pod *corev1.Pod) (*corev1.Pod, error) {
Expand Down Expand Up @@ -184,6 +187,13 @@ func testUpdateWhenUseNotUpdateStrategy(t *testing.T, sidecarSetInput *appsv1alp

fakeClient := fake.NewClientBuilder().WithScheme(scheme).
WithObjects(sidecarSetInput, podInput).
WithIndex(&apps.ControllerRevision{}, fieldindex.IndexNameForOwnerRefUID, func(obj client.Object) []string {
var owners []string
for _, ref := range obj.GetOwnerReferences() {
owners = append(owners, string(ref.UID))
}
return owners
}).
WithStatusSubresource(&appsv1alpha1.SidecarSet{}).Build()
reconciler := ReconcileSidecarSet{
Client: fakeClient,
Expand Down Expand Up @@ -219,6 +229,13 @@ func testUpdateWhenSidecarSetPaused(t *testing.T, sidecarSetInput *appsv1alpha1.

fakeClient := fake.NewClientBuilder().WithScheme(scheme).
WithObjects(sidecarSetInput, podInput).
WithIndex(&apps.ControllerRevision{}, fieldindex.IndexNameForOwnerRefUID, func(obj client.Object) []string {
Copy link
Member

Choose a reason for hiding this comment

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

use appsv1.ControllerRevision{}

var owners []string
for _, ref := range obj.GetOwnerReferences() {
owners = append(owners, string(ref.UID))
}
return owners
}).
WithStatusSubresource(&appsv1alpha1.SidecarSet{}).Build()
reconciler := ReconcileSidecarSet{
Client: fakeClient,
Expand Down Expand Up @@ -254,6 +271,13 @@ func testUpdateWhenMaxUnavailableNotZero(t *testing.T, sidecarSetInput *appsv1al

fakeClient := fake.NewClientBuilder().WithScheme(scheme).
WithObjects(sidecarSetInput, podInput).
WithIndex(&apps.ControllerRevision{}, fieldindex.IndexNameForOwnerRefUID, func(obj client.Object) []string {
var owners []string
for _, ref := range obj.GetOwnerReferences() {
owners = append(owners, string(ref.UID))
}
return owners
}).
WithStatusSubresource(&appsv1alpha1.SidecarSet{}).Build()
reconciler := ReconcileSidecarSet{
Client: fakeClient,
Expand Down Expand Up @@ -289,6 +313,13 @@ func testUpdateWhenPartitionFinished(t *testing.T, sidecarSetInput *appsv1alpha1
}

fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(sidecarSetInput, podInput).
WithIndex(&apps.ControllerRevision{}, fieldindex.IndexNameForOwnerRefUID, func(obj client.Object) []string {
var owners []string
for _, ref := range obj.GetOwnerReferences() {
owners = append(owners, string(ref.UID))
}
return owners
}).
WithStatusSubresource(&appsv1alpha1.SidecarSet{}).Build()
reconciler := ReconcileSidecarSet{
Client: fakeClient,
Expand Down Expand Up @@ -324,6 +355,13 @@ func testRemoveSidecarSet(t *testing.T, sidecarSetInput *appsv1alpha1.SidecarSet
}

fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(sidecarSetInput, podInput).
WithIndex(&apps.ControllerRevision{}, fieldindex.IndexNameForOwnerRefUID, func(obj client.Object) []string {
var owners []string
for _, ref := range obj.GetOwnerReferences() {
owners = append(owners, string(ref.UID))
}
return owners
}).
WithStatusSubresource(&appsv1alpha1.SidecarSet{}).Build()
reconciler := ReconcileSidecarSet{
Client: fakeClient,
Expand Down
6 changes: 6 additions & 0 deletions pkg/util/fieldindex/register.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
appsv1alpha1 "github.com/openkruise/kruise/apis/apps/v1alpha1"
utildiscovery "github.com/openkruise/kruise/pkg/util/discovery"

appsv1 "k8s.io/api/apps/v1"
batchv1 "k8s.io/api/batch/v1"
v1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -65,6 +66,11 @@ func RegisterFieldIndexes(c cache.Cache) error {
if err = c.IndexField(context.TODO(), &v1.PersistentVolumeClaim{}, IndexNameForOwnerRefUID, ownerIndexFunc); err != nil {
return
}
// controller revision ownerReference
if err = c.IndexField(context.TODO(), &appsv1.ControllerRevision{}, IndexNameForOwnerRefUID, ownerIndexFunc); err != nil {
return
}

// ImagePullJob ownerReference
if err = c.IndexField(context.TODO(), &appsv1alpha1.ImagePullJob{}, IndexNameForOwnerRefUID, ownerIndexFunc); err != nil {
return
Expand Down
16 changes: 9 additions & 7 deletions pkg/util/history/controller_history.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,16 @@ import (
apps "k8s.io/api/apps/v1"
"k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/fields"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/types"
"k8s.io/client-go/util/retry"
"k8s.io/kubernetes/pkg/controller/history"
"sigs.k8s.io/controller-runtime/pkg/client"

utilclient "github.com/openkruise/kruise/pkg/util/client"
"github.com/openkruise/kruise/pkg/util/fieldindex"
)

// NewHistory returns an an instance of Interface that uses client to communicate with the API Server and lister to list ControllerRevisions.
Expand All @@ -44,16 +48,14 @@ type realHistory struct {
func (rh *realHistory) ListControllerRevisions(parent metav1.Object, selector labels.Selector) ([]*apps.ControllerRevision, error) {
// List all revisions in the namespace that match the selector
revisions := apps.ControllerRevisionList{}
err := rh.List(context.TODO(), &revisions, &client.ListOptions{Namespace: parent.GetNamespace(), LabelSelector: selector})
if err != nil {
return nil, err
opts := &client.ListOptions{
Copy link
Member

Choose a reason for hiding this comment

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

consider add DisableDeepCopy option

Copy link
Member

Choose a reason for hiding this comment

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

When use DisableDeepCopy, kruise will panic in e2e because some update codes in cloneset VCTHashEqual handler.
We should fix it. @shiyan2016

pkg/controller/cloneset/cloneset_controller.go:453

lastEqualRevision := equalRevisions[equalCount-1]
if !VCTHashEqual(lastEqualRevision, updateRevision) {
	klog.InfoS("Revision vct hash will be updated", "revisionName", lastEqualRevision.Name, "lastRevisionVCTHash", lastEqualRevision.Annotations[volumeclaimtemplate.HashAnnotation], "updateRevisionVCTHash", updateRevision.Annotations[volumeclaimtemplate.HashAnnotation])
	lastEqualRevision.Annotations[volumeclaimtemplate.HashAnnotation] = updateRevision.Annotations[volumeclaimtemplate.HashAnnotation]
}
// if the equivalent revision is not immediately prior we will roll back by incrementing the
// Revision of the equivalent revision
updateRevision, err = r.controllerHistory.UpdateControllerRevision(equalRevisions[equalCount-1], updateRevision.Revision)

Namespace: parent.GetNamespace(),
FieldSelector: fields.SelectorFromSet(fields.Set{fieldindex.IndexNameForOwnerRefUID: string(parent.GetUID())}),
}
err := rh.List(context.TODO(), &revisions, opts, utilclient.DisableDeepCopy)
var owned []*apps.ControllerRevision
for i := range revisions.Items {
ref := metav1.GetControllerOf(&revisions.Items[i])
if ref == nil || ref.UID == parent.GetUID() {
owned = append(owned, &revisions.Items[i])
}
owned = append(owned, &revisions.Items[i])
}
return owned, err
}
Expand Down
10 changes: 9 additions & 1 deletion pkg/util/history/controller_history_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,14 @@ import (

appsv1alpha1 "github.com/openkruise/kruise/apis/apps/v1alpha1"
"github.com/openkruise/kruise/pkg/util"
"github.com/openkruise/kruise/pkg/util/fieldindex"
apps "k8s.io/api/apps/v1"
v1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/util/uuid"
"k8s.io/kubernetes/pkg/controller/history"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/client/fake"
)

Expand Down Expand Up @@ -69,7 +71,13 @@ func TestRevisionHistory(t *testing.T) {
t.Fatalf("Failed to new controller revision: %v", err)
}

fakeClient := fake.NewClientBuilder().Build()
fakeClient := fake.NewClientBuilder().WithIndex(&apps.ControllerRevision{}, fieldindex.IndexNameForOwnerRefUID, func(obj client.Object) []string {
var owners []string
Copy link
Member

Choose a reason for hiding this comment

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

There are so many errors in other uts and can be fixed by using client with index.
I think we should wrap this code with a customized function and replace fake.NewClientBuilder().

for _, ref := range obj.GetOwnerReferences() {
owners = append(owners, string(ref.UID))
}
return owners
}).Build()
historyControl := NewHistory(fakeClient)

newCR, err := historyControl.CreateControllerRevision(parent, cr, parent.Status.CollisionCount)
Expand Down
Loading