Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions apis/v1/binding_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,10 @@ type ResourceBindingSpec struct {
// it points to the name of the leading snapshot of the index group.
ResourceSnapshotName string `json:"resourceSnapshotName"`

// PolicySnapshotName is the name of the policy snapshot that this resource binding & resource snapshot points to.
// It is required to decide whether the binding needs to be deleted or not without talking to API server.
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I remember that ResourceSnapshot has a reference to PolicySnapshot, correct?

Additionally, since we cannot tell whether a PolicySnapshot is the latest from looking at the name, we have to talk to the API server (or actually, cache) any way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, but we only store the resourceSnapshot name here, but not include the policySnapshot name. :(

We will get the latest policySnapshot name by calling API server.
This field is to quickly figure out which policySnapshot the resourceSnapsht of the binding points to.
otherwise, we need to call API server by GET resourceSnapshot.

Copy link
Contributor

@michaelawyu michaelawyu Jun 7, 2023

Choose a reason for hiding this comment

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

Ah, but the client in use will query the cache for the GET ResourceSnapshot call, right? It will never reach API server. Besides, doesn't the rollout controller have to retrieve ResourceSnapshot any way?

Copy link
Contributor

Choose a reason for hiding this comment

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

Does the controller needs to get the ResourceSnapshot? If so, then I am not sure we need this field as the policySnapshotName is in the ResourceSnapshot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope, it does not need to query the resourceSnapshot for each binding.

The PolicySnapshotName will be used in two places:

  1. use to check if the current binding is old or not and then delete the old bindings for the ReCreate rollout strategy, in line 144, https://github.com/zhiying-lin/fleet/blob/rollout-controller/pkg/controllers/clusterresourcerollout/clusterresourcerollout_controller.go#L144
  2. use to check if the the current binding is using latest policy, then update the resource of the bindings, in line 160 https://github.com/zhiying-lin/fleet/blob/rollout-controller/pkg/controllers/clusterresourcerollout/clusterresourcerollout_controller.go#L160

Copy link
Contributor

Choose a reason for hiding this comment

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

Please see the message in chat. We don't need this field for saving trips to API server (the trip will not happen because of caches), but we do need it for a different reason.

PolicySnapshotName string `json:"policySnapshotName"`

// TargetCluster is the name of the cluster that the scheduler assigns the resources to.
TargetCluster string `json:"targetCluster"`
}
Expand Down
2 changes: 1 addition & 1 deletion apis/v1/commons.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ type ClusterState string

const (
// Unprefixed labels/annotations are reserved for end-users
// we will add a fleet.azure.com to designate these labels/annotations as official fleet labels/annotations.
// we will add a fleet.azure.com to designate these labels/annotations/finalizers as official fleet labels/annotations/finalizers.
// See https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#label-selector-and-annotation-conventions
fleetPrefix = "fleet.azure.com/"

Expand Down
9 changes: 7 additions & 2 deletions apis/v1/resourceSnapshot_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@ const (

// NumberOfResourceSnapshotsAnnotation is the annotation that contains the total number of resource snapshots.
NumberOfResourceSnapshotsAnnotation = fleetPrefix + "numberOfResourceSnapshots"

// ClusterResourceSnapshotFinalizer will be added by the clusterResourceRollout controller to clean up the clusterResourceBindings
// before the resource snapshot is deleted.
ClusterResourceSnapshotFinalizer = fleetPrefix + "clusterresourcebinding-cleanup"
)

// +genclient
Expand Down Expand Up @@ -67,8 +71,9 @@ type ResourceSnapShotSpec struct {
// +required
SelectedResources []ResourceContent `json:"selectedResources"`

// PolicySnapShotName is the name of the policy snapshot that this resource snapshot is pointing to.
PolicySnapShotName string `json:"policySnapShotName"`
// PolicySnapshotName is the name of the policy snapshot that this resource snapshot is pointing to.
// +required
PolicySnapshotName string `json:"policySnapshotName"`
}

// ResourceContent contains the content of a resource
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
/*
Copyright (c) Microsoft Corporation.
Licensed under the MIT license.
*/

package clusterresourcerollout

import (
"context"
"time"

apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/client-go/tools/record"
"k8s.io/klog/v2"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"

fleetv1 "go.goms.io/fleet/apis/v1"
)

// Reconciler reconciles the active ClusterResourceSnapshot object.
type Reconciler struct {
client.Client
recorder record.EventRecorder
}

// Reconcile rollouts the resources by updating/deleting the clusterResourceBindings.
func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
name := req.NamespacedName
crs := fleetv1.ClusterResourceSnapshot{}
crsKRef := klog.KRef(name.Namespace, name.Name)

startTime := time.Now()
klog.V(2).InfoS("Reconciliation starts", "clusterResourceSnapshot", crsKRef)
defer func() {
latency := time.Since(startTime).Milliseconds()
klog.V(2).InfoS("Reconciliation ends", "clusterResourceSnapshot", crsKRef, "latency", latency)
}()

if err := r.Client.Get(ctx, name, &crs); err != nil {
if apierrors.IsNotFound(err) {
klog.V(4).InfoS("Ignoring NotFound clusterResourceSnapshot", "clusterResourceSnapshot", crsKRef)
return ctrl.Result{}, nil
}
klog.ErrorS(err, "Failed to get clusterResourceSnapshot", "clusterResourceSnapshot", crsKRef)
return ctrl.Result{}, err
}

if crs.ObjectMeta.DeletionTimestamp != nil {
return r.handleDelete(ctx, &crs)
}

// register finalizer
if !controllerutil.ContainsFinalizer(&crs, fleetv1.ClusterResourceSnapshotFinalizer) {
controllerutil.AddFinalizer(&crs, fleetv1.ClusterResourceSnapshotFinalizer)
if err := r.Update(ctx, &crs); err != nil {
klog.ErrorS(err, "Failed to add mcs finalizer", "clusterResourceSnapshot", crsKRef)
return ctrl.Result{}, err
}
}
return r.handleUpdate(ctx, &crs)
}

func (r *Reconciler) handleDelete(_ context.Context, _ *fleetv1.ClusterResourceSnapshot) (ctrl.Result, error) {
return ctrl.Result{}, nil
}

func (r *Reconciler) handleUpdate(_ context.Context, _ *fleetv1.ClusterResourceSnapshot) (ctrl.Result, error) {
return ctrl.Result{}, nil
}

// SetupWithManager sets up the controller with the Manager.
func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error {
r.recorder = mgr.GetEventRecorderFor("clusterResourceRollout")
return ctrl.NewControllerManagedBy(mgr).
For(&fleetv1.ClusterResourceSnapshot{}).
Complete(r)
}