Skip to content

Commit

Permalink
address review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@ykakarap
ykakarap committed Feb 7, 2023
1 parent 674c38a commit 5dd7067
Show file tree
Hide file tree
Showing 2 changed files with 97 additions and 61 deletions.
95 changes: 77 additions & 18 deletions controllers/external/util.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,14 @@ package external

import (
"context"
"fmt"
"strings"

"github.com/pkg/errors"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/types"
"k8s.io/apiserver/pkg/storage/names"
"sigs.k8s.io/controller-runtime/pkg/client"

Expand Down Expand Up @@ -84,14 +86,6 @@ type CreateFromTemplateInput struct {
// Annotations is an optional map of annotations to be added to the object.
// +optional
Annotations map[string]string

// SSA when set to true will create the object using server side apply.
// When SSA is set to ture, SSAPatchOptions cannot be empty.
SSA bool

// SSAPatchOptions is used when using SSA to creat the object.
// The value is ignored when SSA is set to false.
SSAPatchOptions []client.PatchOption
}

// CreateFromTemplate uses the client and the reference to create a new object from the template.
Expand All @@ -109,16 +103,81 @@ func CreateFromTemplate(ctx context.Context, in *CreateFromTemplateInput) (*core
return nil, errors.Wrap(err, "failed to compute object form template")
}

if in.SSA {
// Create the external clone using SSA.
if err := in.Client.Patch(ctx, obj, client.Apply, in.SSAPatchOptions...); err != nil {
return nil, err
}
} else {
// Create the external clone.
if err := in.Client.Create(ctx, obj); err != nil {
return nil, err
}
if err := in.Client.Create(ctx, obj); err != nil {
return nil, err
}

return GetObjectReference(obj), nil
}

// ApplyFromTemplateInput is the input to ApplyFromTemplate.
type ApplyFromTemplateInput struct {
// Client is the controller runtime client.
Client client.Client

// TemplateRef is a reference to the template that needs to be cloned.
TemplateRef *corev1.ObjectReference

// Name is the Kubernetes name of the target object.
// If Name is empty a random name would be generated.
// +optional
Name string

// If UID is specified it will be used on the target object.
// +optional
UID types.UID

// Namespace is the Kubernetes namespace the cloned object should be created into.
Namespace string

// ClusterName is the cluster this object is linked to.
ClusterName string

// OwnerRef is an optional OwnerReference to attach to the cloned object.
// +optional
OwnerRef *metav1.OwnerReference

// Labels is an optional map of labels to be added to the object.
// +optional
Labels map[string]string

// Annotations is an optional map of annotations to be added to the object.
// +optional
Annotations map[string]string

// Manager is the name of the manager to when using SSA patch.
Manager string
}

// ApplyFromTemplate uses the client and the reference to create a new object from the template.
func ApplyFromTemplate(ctx context.Context, in *ApplyFromTemplateInput) (*corev1.ObjectReference, error) {
if in.Manager == "" {
return nil, fmt.Errorf("manager is not specified")
}
obj, err := ComputeObjectFromTemplate(ctx, &ComputeObjectFromTemplateInput{
Client: in.Client,
TemplateRef: in.TemplateRef,
Namespace: in.Namespace,
ClusterName: in.ClusterName,
OwnerRef: in.OwnerRef,
Labels: in.Labels,
Annotations: in.Annotations,
})
if err != nil {
return nil, errors.Wrap(err, "failed to compute object form template")
}
if in.Name != "" {
obj.SetName(in.Name)
}
obj.SetUID(in.UID)

// Apply the patch using SSA.
patchOptions := []client.PatchOption{
client.ForceOwnership,
client.FieldOwner(in.Manager),
}
if err := in.Client.Patch(ctx, obj, client.Apply, patchOptions...); err != nil {
return nil, err
}

return GetObjectReference(obj), nil
Expand Down
63 changes: 20 additions & 43 deletions internal/controllers/machineset/machineset_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -364,12 +364,14 @@ func (r *Reconciler) reconcile(ctx context.Context, cluster *clusterv1.Cluster,
return ctrl.Result{}, errors.Wrapf(err, "failed to get infra machine object %q for machine %q", klog.KRef(m.Namespace, m.Spec.InfrastructureRef.Name), klog.KObj(m))
}
// Adjust the managed fields to make the infra machine object SSA compatible.
if err := r.adjustManagedFields(ctx, infraMachine); err != nil {
if err := ssa.CleanUpManagedFieldsForSSACompatibility(ctx, infraMachine, machineSetManagerName, r.Client); err != nil {
return ctrl.Result{}, errors.Wrapf(err, "failed to update managedfields of infra machine %q for machine %q", klog.KRef(m.Namespace, m.Spec.InfrastructureRef.Name), klog.KObj(m))
}
updatedInfraMachine, err := external.ComputeObjectFromTemplate(ctx, &external.ComputeObjectFromTemplateInput{
_, err = external.ApplyFromTemplate(ctx, &external.ApplyFromTemplateInput{
Client: r.Client,
TemplateRef: &machineSet.Spec.Template.Spec.InfrastructureRef,
UID: infraMachine.GetUID(),
Name: infraMachine.GetName(),
Namespace: machineSet.Namespace,
ClusterName: machineSet.Spec.ClusterName,
OwnerRef: &metav1.OwnerReference{
Expand All @@ -380,22 +382,10 @@ func (r *Reconciler) reconcile(ctx context.Context, cluster *clusterv1.Cluster,
},
Labels: machineLabels(machineSet),
Annotations: machineSet.Spec.Template.Annotations,
Manager: machineSetManagerName,
})
if err != nil {
return ctrl.Result{}, errors.Wrapf(err, "failed to compute updated infra machine %q for machine %q", klog.KRef(m.Namespace, m.Spec.InfrastructureRef.Name), klog.KObj(m))
}
// Since the InfraMachine already exists, use that name.
// Set the UID to the existing infra machine object to ensure that we only update and avoid accidentally
// creating an infra machine.
// TODO: The resourceVersion of the updatedInfraMachine will be empty. Is that okay?
updatedInfraMachine.SetName(infraMachine.GetName())
updatedInfraMachine.SetUID(infraMachine.GetUID())
patchOptions := []client.PatchOption{
client.ForceOwnership,
client.FieldOwner(machineSetManagerName),
}
if err := r.Client.Patch(ctx, updatedInfraMachine, client.Apply, patchOptions...); err != nil {
return ctrl.Result{}, errors.Wrapf(err, "failed to update infra machine object %q for machine %q", klog.KRef(m.Namespace, m.Spec.InfrastructureRef.Name), klog.KObj(m))
return ctrl.Result{}, errors.Wrapf(err, "failed to update infra machine %q for machine %q", klog.KRef(m.Namespace, m.Spec.InfrastructureRef.Name), klog.KObj(m))
}
}

Expand All @@ -413,12 +403,14 @@ func (r *Reconciler) reconcile(ctx context.Context, cluster *clusterv1.Cluster,
return ctrl.Result{}, errors.Wrapf(err, "failed to get bootstrapconfig %q for machine %q", klog.KRef(m.Namespace, m.Spec.InfrastructureRef.Name), klog.KObj(m))
}
// Adjust the managed fields to make the bootstrap object SSA compatible.
if err := r.adjustManagedFields(ctx, bootstrapConfig); err != nil {
if err := ssa.CleanUpManagedFieldsForSSACompatibility(ctx, bootstrapConfig, machineSetManagerName, r.Client); err != nil {
return ctrl.Result{}, errors.Wrapf(err, "failed to update managedfields of bootstrapconfig %q for machine %q", klog.KRef(m.Namespace, m.Spec.InfrastructureRef.Name), klog.KObj(m))
}
updatedBootstrapConfig, err := external.ComputeObjectFromTemplate(ctx, &external.ComputeObjectFromTemplateInput{
_, err = external.ApplyFromTemplate(ctx, &external.ApplyFromTemplateInput{
Client: r.Client,
TemplateRef: machineSet.Spec.Template.Spec.Bootstrap.ConfigRef,
UID: bootstrapConfig.GetUID(),
Name: bootstrapConfig.GetName(),
Namespace: machineSet.Namespace,
ClusterName: machineSet.Spec.ClusterName,
OwnerRef: &metav1.OwnerReference{
Expand All @@ -429,22 +421,10 @@ func (r *Reconciler) reconcile(ctx context.Context, cluster *clusterv1.Cluster,
},
Labels: machineLabels(machineSet),
Annotations: machineSet.Spec.Template.Annotations,
Manager: machineSetManagerName,
})
if err != nil {
return ctrl.Result{}, errors.Wrapf(err, "failed to cumpute updated bootstrapconfig %q for machine %q", klog.KRef(m.Namespace, m.Spec.InfrastructureRef.Name), klog.KObj(m))
}
// Since the BootstrapConfig already exists, use that name.
// Set the UID to the existing BootstrapConfig to ensure that we only update and avoid accidentally
// creating an BootstrapConfig object.
// TODO: The resourceVersion of the updatedInfraMachine will be empty. Is that okay?
updatedBootstrapConfig.SetName(bootstrapConfig.GetName())
updatedBootstrapConfig.SetUID(bootstrapConfig.GetUID())
patchOptions := []client.PatchOption{
client.ForceOwnership,
client.FieldOwner(machineSetManagerName),
}
if err := r.Client.Patch(ctx, updatedBootstrapConfig, client.Apply, patchOptions...); err != nil {
return ctrl.Result{}, errors.Wrapf(err, "failed to update bootstrapconfig %q for machine %q", klog.KRef(m.Namespace, m.Spec.InfrastructureRef.Name), klog.KObj(m))
return ctrl.Result{}, errors.Wrapf(err, "failed to updated bootstrapconfig %q for machine %q", klog.KRef(m.Namespace, m.Spec.InfrastructureRef.Name), klog.KObj(m))
}
}

Expand Down Expand Up @@ -517,13 +497,8 @@ func (r *Reconciler) syncReplicas(ctx context.Context, ms *clusterv1.MachineSet,
err error
)

patchOptions := []client.PatchOption{
client.ForceOwnership,
client.FieldOwner(machineSetManagerName),
}

if ms.Spec.Template.Spec.Bootstrap.ConfigRef != nil {
bootstrapRef, err = external.CreateFromTemplate(ctx, &external.CreateFromTemplateInput{
bootstrapRef, err = external.ApplyFromTemplate(ctx, &external.ApplyFromTemplateInput{
Client: r.Client,
TemplateRef: ms.Spec.Template.Spec.Bootstrap.ConfigRef,
Namespace: machine.Namespace,
Expand All @@ -536,8 +511,7 @@ func (r *Reconciler) syncReplicas(ctx context.Context, ms *clusterv1.MachineSet,
Name: ms.Name,
UID: ms.UID,
},
SSA: true,
SSAPatchOptions: patchOptions,
Manager: machineSetManagerName,
})
if err != nil {
conditions.MarkFalse(ms, clusterv1.MachinesCreatedCondition, clusterv1.BootstrapTemplateCloningFailedReason, clusterv1.ConditionSeverityError, err.Error())
Expand All @@ -549,7 +523,7 @@ func (r *Reconciler) syncReplicas(ctx context.Context, ms *clusterv1.MachineSet,
log = log.WithValues(bootstrapRef.Kind, klog.KRef(bootstrapRef.Namespace, bootstrapRef.Name))
}

infraRef, err = external.CreateFromTemplate(ctx, &external.CreateFromTemplateInput{
infraRef, err = external.ApplyFromTemplate(ctx, &external.ApplyFromTemplateInput{
Client: r.Client,
TemplateRef: &ms.Spec.Template.Spec.InfrastructureRef,
Namespace: machine.Namespace,
Expand All @@ -562,8 +536,7 @@ func (r *Reconciler) syncReplicas(ctx context.Context, ms *clusterv1.MachineSet,
Name: ms.Name,
UID: ms.UID,
},
SSA: true,
SSAPatchOptions: patchOptions,
Manager: machineSetManagerName,
})
if err != nil {
conditions.MarkFalse(ms, clusterv1.MachinesCreatedCondition, clusterv1.InfrastructureTemplateCloningFailedReason, clusterv1.ConditionSeverityError, err.Error())
Expand All @@ -574,6 +547,10 @@ func (r *Reconciler) syncReplicas(ctx context.Context, ms *clusterv1.MachineSet,
log = log.WithValues(infraRef.Kind, klog.KRef(infraRef.Namespace, infraRef.Name))
machine.Spec.InfrastructureRef = *infraRef

patchOptions := []client.PatchOption{
client.ForceOwnership,
client.FieldOwner(machineSetManagerName),
}
if err := r.Client.Patch(ctx, machine, client.Apply, patchOptions...); err != nil {
log.Error(err, "Error while creating a machine")
r.recorder.Eventf(ms, corev1.EventTypeWarning, "FailedCreate", "Failed to create machine: %v", err)
Expand Down

0 comments on commit 5dd7067

Please sign in to comment.