Skip to content

Commit

Permalink
bug fix
Browse files Browse the repository at this point in the history
  • Loading branch information
@zhiying-lin
zhiying-lin committed Jul 12, 2023
1 parent 4c1526b commit 0b51676
Show file tree
Hide file tree
Showing 2 changed files with 81 additions and 20 deletions.
10 changes: 6 additions & 4 deletions pkg/controllers/clusterresourceplacement/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,9 +116,11 @@ func (r *Reconciler) handleUpdate(ctx context.Context, crp *fleetv1beta1.Cluster

func (r *Reconciler) getOrCreateClusterSchedulingPolicySnapshot(ctx context.Context, crp *fleetv1beta1.ClusterResourcePlacement) (*fleetv1beta1.ClusterSchedulingPolicySnapshot, error) {
crpKObj := klog.KObj(crp)
schedulingPolicy := *crp.Spec.Policy // will exclude the numberOfClusters
schedulingPolicy.NumberOfClusters = nil
policyHash, err := generatePolicyHash(&schedulingPolicy)
schedulingPolicy := crp.Spec.Policy
if schedulingPolicy != nil {
schedulingPolicy.NumberOfClusters = nil // will exclude the numberOfClusters
}
policyHash, err := generatePolicyHash(schedulingPolicy)
if err != nil {
klog.ErrorS(err, "Failed to generate policy hash of crp", "clusterResourcePlacement", crpKObj)
return nil, controller.NewUnexpectedBehaviorError(err)
Expand Down Expand Up @@ -168,7 +170,7 @@ func (r *Reconciler) getOrCreateClusterSchedulingPolicySnapshot(ctx context.Cont
},
},
Spec: fleetv1beta1.SchedulingPolicySnapshotSpec{
Policy: &schedulingPolicy,
Policy: schedulingPolicy,
PolicyHash: []byte(policyHash),
},
}
Expand Down
91 changes: 75 additions & 16 deletions pkg/controllers/clusterresourceplacement/controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,6 @@ func clusterResourcePlacementForTest() *fleetv1beta1.ClusterResourcePlacement {
Name: testName,
},
Spec: fleetv1beta1.ClusterResourcePlacementSpec{
Policy: placementPolicyForTest(),
ResourceSelectors: []fleetv1beta1.ClusterResourceSelector{
{
Group: corev1.GroupName,
Expand All @@ -101,9 +100,9 @@ func clusterResourcePlacementForTest() *fleetv1beta1.ClusterResourcePlacement {
}

func TestGetOrCreateClusterSchedulingPolicySnapshot(t *testing.T) {
wantPolicy := placementPolicyForTest()
wantPolicy.NumberOfClusters = nil
jsonBytes, err := json.Marshal(wantPolicy)
testPolicy := placementPolicyForTest()
testPolicy.NumberOfClusters = nil
jsonBytes, err := json.Marshal(testPolicy)
if err != nil {
t.Fatalf("failed to create the policy hash: %v", err)
}
Expand All @@ -115,13 +114,15 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot(t *testing.T) {
unspecifiedPolicyHash := []byte(fmt.Sprintf("%x", sha256.Sum256(jsonBytes)))
tests := []struct {
name string
policy *fleetv1beta1.PlacementPolicy
revisionHistoryLimit *int32
policySnapshots []fleetv1beta1.ClusterSchedulingPolicySnapshot
wantPolicySnapshots []fleetv1beta1.ClusterSchedulingPolicySnapshot
wantLatestSnapshotIndex int // index of the wantPolicySnapshots array
}{
{
name: "new clusterResourcePolicy and no existing policy snapshots owned by my-crp",
name: "new clusterResourcePolicy and no existing policy snapshots owned by my-crp",
policy: placementPolicyForTest(),
policySnapshots: []fleetv1beta1.ClusterSchedulingPolicySnapshot{
{
ObjectMeta: metav1.ObjectMeta{
Expand Down Expand Up @@ -168,15 +169,67 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot(t *testing.T) {
},
},
Spec: fleetv1beta1.SchedulingPolicySnapshotSpec{
Policy: wantPolicy,
Policy: testPolicy,
PolicyHash: policyHash,
},
},
},
wantLatestSnapshotIndex: 1,
},
{
name: "new clusterResourcePolicy (unspecified policy) and no existing policy snapshots owned by my-crp",
policySnapshots: []fleetv1beta1.ClusterSchedulingPolicySnapshot{
{
ObjectMeta: metav1.ObjectMeta{
Name: "another-crp-1",
Labels: map[string]string{
fleetv1beta1.PolicyIndexLabel: "1",
fleetv1beta1.IsLatestSnapshotLabel: "true",
fleetv1beta1.CRPTrackingLabel: "another-crp",
},
},
},
},
wantPolicySnapshots: []fleetv1beta1.ClusterSchedulingPolicySnapshot{
{
ObjectMeta: metav1.ObjectMeta{
Name: "another-crp-1",
Labels: map[string]string{
fleetv1beta1.PolicyIndexLabel: "1",
fleetv1beta1.IsLatestSnapshotLabel: "true",
fleetv1beta1.CRPTrackingLabel: "another-crp",
},
},
},
// new policy snapshot owned by the my-crp
{
ObjectMeta: metav1.ObjectMeta{
Name: fmt.Sprintf(fleetv1beta1.PolicySnapshotNameFmt, testName, 0),
Labels: map[string]string{
fleetv1beta1.PolicyIndexLabel: "0",
fleetv1beta1.IsLatestSnapshotLabel: "true",
fleetv1beta1.CRPTrackingLabel: testName,
},
OwnerReferences: []metav1.OwnerReference{
{
Name: testName,
BlockOwnerDeletion: pointer.Bool(true),
Controller: pointer.Bool(true),
APIVersion: fleetAPIVersion,
Kind: "ClusterResourcePlacement",
},
},
},
Spec: fleetv1beta1.SchedulingPolicySnapshotSpec{
PolicyHash: unspecifiedPolicyHash,
},
},
},
wantLatestSnapshotIndex: 1,
},
{
name: "new clusterResourcePolicy with invalidRevisionLimit and no existing policy snapshots owned by my-crp",
policy: placementPolicyForTest(),
revisionHistoryLimit: &invalidRevisionLimit,
policySnapshots: []fleetv1beta1.ClusterSchedulingPolicySnapshot{
{
Expand Down Expand Up @@ -224,7 +277,7 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot(t *testing.T) {
},
},
Spec: fleetv1beta1.SchedulingPolicySnapshotSpec{
Policy: wantPolicy,
Policy: testPolicy,
PolicyHash: policyHash,
},
},
Expand All @@ -233,6 +286,7 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot(t *testing.T) {
},
{
name: "crp policy has no change",
policy: placementPolicyForTest(),
revisionHistoryLimit: &singleRevisionLimit,
policySnapshots: []fleetv1beta1.ClusterSchedulingPolicySnapshot{
{
Expand All @@ -257,7 +311,7 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot(t *testing.T) {
},
},
Spec: fleetv1beta1.SchedulingPolicySnapshotSpec{
Policy: wantPolicy,
Policy: testPolicy,
PolicyHash: policyHash,
},
},
Expand Down Expand Up @@ -285,7 +339,7 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot(t *testing.T) {
},
},
Spec: fleetv1beta1.SchedulingPolicySnapshotSpec{
Policy: wantPolicy,
Policy: testPolicy,
PolicyHash: policyHash,
},
},
Expand All @@ -296,6 +350,7 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot(t *testing.T) {
name: "crp policy has changed and there is no active snapshot",
// It happens when last reconcile loop fails after setting the latest label to false and
// before creating a new policy snapshot.
policy: placementPolicyForTest(),
revisionHistoryLimit: &multipleRevisionLimit,
policySnapshots: []fleetv1beta1.ClusterSchedulingPolicySnapshot{
{
Expand Down Expand Up @@ -391,7 +446,7 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot(t *testing.T) {
},
},
Spec: fleetv1beta1.SchedulingPolicySnapshotSpec{
Policy: wantPolicy,
Policy: testPolicy,
PolicyHash: policyHash,
},
},
Expand All @@ -400,6 +455,7 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot(t *testing.T) {
},
{
name: "crp policy has changed and there is an active snapshot",
policy: placementPolicyForTest(),
revisionHistoryLimit: &singleRevisionLimit,
policySnapshots: []fleetv1beta1.ClusterSchedulingPolicySnapshot{
{
Expand Down Expand Up @@ -449,15 +505,16 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot(t *testing.T) {
},
},
Spec: fleetv1beta1.SchedulingPolicySnapshotSpec{
Policy: wantPolicy,
Policy: testPolicy,
PolicyHash: policyHash,
},
},
},
wantLatestSnapshotIndex: 0,
},
{
name: "crp policy has been changed and reverted back and there is no active snapshot",
name: "crp policy has been changed and reverted back and there is no active snapshot",
policy: placementPolicyForTest(),
policySnapshots: []fleetv1beta1.ClusterSchedulingPolicySnapshot{
{
ObjectMeta: metav1.ObjectMeta{
Expand Down Expand Up @@ -503,7 +560,7 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot(t *testing.T) {
},
},
Spec: fleetv1beta1.SchedulingPolicySnapshotSpec{
Policy: wantPolicy,
Policy: testPolicy,
PolicyHash: policyHash,
},
},
Expand Down Expand Up @@ -554,7 +611,7 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot(t *testing.T) {
},
},
Spec: fleetv1beta1.SchedulingPolicySnapshotSpec{
Policy: wantPolicy,
Policy: testPolicy,
PolicyHash: policyHash,
},
},
Expand All @@ -564,6 +621,7 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot(t *testing.T) {
{
name: "crp policy has not been changed and only the numberOfCluster is changed",
// cause no new policy snapshot is created, it does not trigger the history limit check.
policy: placementPolicyForTest(),
revisionHistoryLimit: &singleRevisionLimit,
policySnapshots: []fleetv1beta1.ClusterSchedulingPolicySnapshot{
{
Expand Down Expand Up @@ -611,7 +669,7 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot(t *testing.T) {
},
},
Spec: fleetv1beta1.SchedulingPolicySnapshotSpec{
Policy: wantPolicy,
Policy: testPolicy,
PolicyHash: policyHash,
},
},
Expand Down Expand Up @@ -662,7 +720,7 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot(t *testing.T) {
},
},
Spec: fleetv1beta1.SchedulingPolicySnapshotSpec{
Policy: wantPolicy,
Policy: testPolicy,
PolicyHash: policyHash,
},
},
Expand All @@ -674,6 +732,7 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot(t *testing.T) {
t.Run(tc.name, func(t *testing.T) {
ctx := context.Background()
crp := clusterResourcePlacementForTest()
crp.Spec.Policy = tc.policy
crp.Spec.RevisionHistoryLimit = tc.revisionHistoryLimit
objects := []client.Object{crp}
for i := range tc.policySnapshots {
Expand Down

0 comments on commit 0b51676

Please sign in to comment.