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: 2 additions & 2 deletions api/v1beta1/cluster_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ type Topology struct {

// ControlPlaneTopology specifies the parameters for the control plane nodes in the cluster.
type ControlPlaneTopology struct {
// Metadata is the metadata applied to the machines of the ControlPlane.
// Metadata is the metadata applied to ControlPlane and the machines of the ControlPlane.
Copy link
Member

Choose a reason for hiding this comment

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

I'm still mulling about this, but if we are going to propagate labels/annotations to objects down the chain I think we should propagate labels/annotations to bootstrap and infrastructure template as well; this implies also keeping such labels and annotations in sync (TBD how given that this should work no matter is using a cluster class or not)

Copy link
Contributor Author

@MaxFedotov MaxFedotov Sep 9, 2022

Choose a reason for hiding this comment

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

do you consider it a part of this PR, or is it better to create a separate issue\PR to track this activity?

// At runtime this metadata is merged with the corresponding metadata from the ClusterClass.
//
// This field is supported if and only if the control plane provider template
Expand Down Expand Up @@ -133,7 +133,7 @@ type WorkersTopology struct {
// MachineDeploymentTopology specifies the different parameters for a set of worker nodes in the topology.
// This set of nodes is managed by a MachineDeployment object whose lifecycle is managed by the Cluster controller.
type MachineDeploymentTopology struct {
// Metadata is the metadata applied to the machines of the MachineDeployment.
// Metadata is the metadata applied to the MachineDeployment and the machines of the MachineDeployment.
// At runtime this metadata is merged with the corresponding metadata from the ClusterClass.
// +optional
Metadata ObjectMeta `json:"metadata,omitempty"`
Expand Down
4 changes: 2 additions & 2 deletions api/v1beta1/zz_generated.openapi.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 7 additions & 7 deletions config/crd/bases/cluster.x-k8s.io_clusters.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 7 additions & 0 deletions internal/contract/controlplane.go
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,13 @@ func (c *ControlPlaneContract) IsScaling(obj *unstructured.Unstructured) (bool,
return false, nil
}

// Metadata provides access to the metadata of a ControlPlane object.
func (c *ControlPlaneContract) Metadata() *Metadata {
return &Metadata{
path: Path{"metadata"},
}
}

// ControlPlaneMachineTemplate provides a helper struct for working with MachineTemplate in ClusterClass.
type ControlPlaneMachineTemplate struct{}

Expand Down
62 changes: 38 additions & 24 deletions internal/controllers/topology/cluster/desired_state.go
Original file line number Diff line number Diff line change
Expand Up @@ -112,11 +112,16 @@ func computeInfrastructureCluster(_ context.Context, s *scope.Scope) (*unstructu
cluster := s.Current.Cluster
currentRef := cluster.Spec.InfrastructureRef

labels := map[string]string{}
labels[clusterv1.ClusterLabelName] = cluster.Name
labels[clusterv1.ClusterTopologyOwnedLabel] = ""

infrastructureCluster, err := templateToObject(templateToInput{
template: template,
templateClonedFromRef: templateClonedFromRef,
cluster: cluster,
namePrefix: fmt.Sprintf("%s-", cluster.Name),
labels: labels,
currentObjectRef: currentRef,
// Note: It is not possible to add an ownerRef to Cluster at this stage, otherwise the provisioning
// of the infrastructure cluster starts no matter of the object being actually referenced by the Cluster itself.
Expand Down Expand Up @@ -176,11 +181,28 @@ func (r *Reconciler) computeControlPlane(ctx context.Context, s *scope.Scope, in
cluster := s.Current.Cluster
currentRef := cluster.Spec.ControlPlaneRef

// Compute the labels and annotations to be applied to KubeadmControlPlane and Machines.
// We merge the labels and annotations from topology and ClusterClass.
topologyMetadata := s.Blueprint.Topology.ControlPlane.Metadata
clusterClassMetadata := s.Blueprint.ClusterClass.Spec.ControlPlane.Metadata

controlPlaneLabels := mergeMap(topologyMetadata.Labels, clusterClassMetadata.Labels)
controlPlaneAnnotations := mergeMap(topologyMetadata.Annotations, clusterClassMetadata.Annotations)

// Add the cluster-name and the topology owned labels, so they are propagated down to Machines.
if controlPlaneLabels == nil {
controlPlaneLabels = map[string]string{}
}
controlPlaneLabels[clusterv1.ClusterLabelName] = cluster.Name
controlPlaneLabels[clusterv1.ClusterTopologyOwnedLabel] = ""

controlPlane, err := templateToObject(templateToInput{
template: template,
templateClonedFromRef: templateClonedFromRef,
cluster: cluster,
namePrefix: fmt.Sprintf("%s-", cluster.Name),
labels: controlPlaneLabels,
annotations: controlPlaneAnnotations,
currentObjectRef: currentRef,
// Note: It is not possible to add an ownerRef to Cluster at this stage, otherwise the provisioning
// of the ControlPlane starts no matter of the object being actually referenced by the Cluster itself.
Expand All @@ -205,22 +227,10 @@ func (r *Reconciler) computeControlPlane(ctx context.Context, s *scope.Scope, in
return nil, errors.Wrap(err, "failed to spec.machineTemplate.infrastructureRef in the ControlPlane object")
}

// Compute the labels and annotations to be applied to ControlPlane machines.
// We merge the labels and annotations from topology and ClusterClass.
// We also add the cluster-name and the topology owned labels, so they are propagated down to Machines.
topologyMetadata := s.Blueprint.Topology.ControlPlane.Metadata
clusterClassMetadata := s.Blueprint.ClusterClass.Spec.ControlPlane.Metadata

machineLabels := mergeMap(topologyMetadata.Labels, clusterClassMetadata.Labels)
if machineLabels == nil {
machineLabels = map[string]string{}
}
machineLabels[clusterv1.ClusterLabelName] = cluster.Name
machineLabels[clusterv1.ClusterTopologyOwnedLabel] = ""
if err := contract.ControlPlane().MachineTemplate().Metadata().Set(controlPlane,
&clusterv1.ObjectMeta{
Labels: machineLabels,
Annotations: mergeMap(topologyMetadata.Annotations, clusterClassMetadata.Annotations),
Labels: controlPlaneLabels,
Annotations: controlPlaneAnnotations,
}); err != nil {
return nil, errors.Wrap(err, "failed to set spec.machineTemplate.metadata in the ControlPlane object")
}
Expand Down Expand Up @@ -512,6 +522,10 @@ func computeMachineDeployment(_ context.Context, s *scope.Scope, desiredControlP
return nil, errors.Wrapf(err, "failed to compute version for %s", machineDeploymentTopology.Name)
}

// Compute labels for MachineDeployment and Machines
machineDeploymentLabels := mergeMap(machineDeploymentTopology.Metadata.Labels, machineDeploymentBlueprint.Metadata.Labels)
machineDeploymentAnnotations := mergeMap(machineDeploymentTopology.Metadata.Annotations, machineDeploymentBlueprint.Metadata.Annotations)

// Compute the MachineDeployment object.
gv := clusterv1.GroupVersion
desiredMachineDeploymentObj := &clusterv1.MachineDeployment{
Expand All @@ -527,8 +541,8 @@ func computeMachineDeployment(_ context.Context, s *scope.Scope, desiredControlP
ClusterName: s.Current.Cluster.Name,
Template: clusterv1.MachineTemplateSpec{
ObjectMeta: clusterv1.ObjectMeta{
Labels: mergeMap(machineDeploymentTopology.Metadata.Labels, machineDeploymentBlueprint.Metadata.Labels),
Annotations: mergeMap(machineDeploymentTopology.Metadata.Annotations, machineDeploymentBlueprint.Metadata.Annotations),
Labels: machineDeploymentLabels,
Annotations: machineDeploymentAnnotations,
},
Spec: clusterv1.MachineSpec{
ClusterName: s.Current.Cluster.Name,
Expand Down Expand Up @@ -562,7 +576,10 @@ func computeMachineDeployment(_ context.Context, s *scope.Scope, desiredControlP
labels[clusterv1.ClusterLabelName] = s.Current.Cluster.Name
labels[clusterv1.ClusterTopologyOwnedLabel] = ""
labels[clusterv1.ClusterTopologyMachineDeploymentLabelName] = machineDeploymentTopology.Name
desiredMachineDeploymentObj.SetLabels(labels)
desiredMachineDeploymentObj.SetLabels(mergeMap(labels, machineDeploymentLabels))

// Apply Annotations
desiredMachineDeploymentObj.SetAnnotations(machineDeploymentAnnotations)

// Set the selector with the subset of labels identifying controlled machines.
// NOTE: this prevents the web hook to add cluster.x-k8s.io/deployment-name label, that is
Expand Down Expand Up @@ -710,6 +727,8 @@ type templateToInput struct {
cluster *clusterv1.Cluster
namePrefix string
currentObjectRef *corev1.ObjectReference
labels map[string]string
annotations map[string]string
// OwnerRef is an optional OwnerReference to attach to the cloned object.
ownerRef *metav1.OwnerReference
}
Expand All @@ -718,20 +737,15 @@ type templateToInput struct {
// of adding required labels (cluster, topology), annotations (clonedFrom)
// and assigning a meaningful name (or reusing current reference name).
func templateToObject(in templateToInput) (*unstructured.Unstructured, error) {
// NOTE: The cluster label is added at creation time so this object could be read by the ClusterTopology
// controller immediately after creation, even before other controllers are going to add the label (if missing).
labels := map[string]string{}
labels[clusterv1.ClusterLabelName] = in.cluster.Name
labels[clusterv1.ClusterTopologyOwnedLabel] = ""

Comment on lines -723 to -726
Copy link
Contributor

Choose a reason for hiding this comment

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

How about keeping this and doing a mergeMap with in.Labels and using that as in the GenerateTemplate function. This way we wont have to duplicate this everywhere templateToObjects is used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MaxFedotov the proposal is now merged. Proposal here. The proposal contains all the details for the questions on this PR.

Do you have bandwidth to continue working on this? I am happy to pick up the pending work here, in case you do not have bandwidth 🙂

Hi @ykakarap! I would be grateful if you take this on. Unfortunately i don't have free time right now to continue with it :(

// Generate the object from the template.
// NOTE: OwnerRef can't be set at this stage; other controllers are going to add OwnerReferences when
// the object is actually created.
object, err := external.GenerateTemplate(&external.GenerateTemplateInput{
Template: in.template,
TemplateRef: in.templateClonedFromRef,
Namespace: in.cluster.Namespace,
Labels: labels,
Labels: in.labels,
Annotations: in.annotations,
ClusterName: in.cluster.Name,
OwnerRef: in.ownerRef,
})
Expand Down
40 changes: 37 additions & 3 deletions internal/controllers/topology/cluster/desired_state_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,8 @@ func TestComputeControlPlane(t *testing.T) {
cluster: scope.Current.Cluster,
templateRef: blueprint.ClusterClass.Spec.ControlPlane.Ref,
template: blueprint.ControlPlane.Template,
labels: mergeMap(blueprint.Topology.ControlPlane.Metadata.Labels, blueprint.ClusterClass.Spec.ControlPlane.Metadata.Labels),
annotations: mergeMap(blueprint.Topology.ControlPlane.Metadata.Annotations, blueprint.ClusterClass.Spec.ControlPlane.Metadata.Annotations),
currentRef: nil,
obj: obj,
})
Expand Down Expand Up @@ -349,6 +351,8 @@ func TestComputeControlPlane(t *testing.T) {
cluster: scope.Current.Cluster,
templateRef: blueprint.ClusterClass.Spec.ControlPlane.Ref,
template: blueprint.ControlPlane.Template,
labels: mergeMap(blueprint.Topology.ControlPlane.Metadata.Labels, blueprint.ClusterClass.Spec.ControlPlane.Metadata.Labels),
annotations: mergeMap(blueprint.Topology.ControlPlane.Metadata.Annotations, blueprint.ClusterClass.Spec.ControlPlane.Metadata.Annotations),
currentRef: nil,
obj: obj,
})
Expand Down Expand Up @@ -391,18 +395,38 @@ func TestComputeControlPlane(t *testing.T) {
cluster: scope.Current.Cluster,
templateRef: blueprint.ClusterClass.Spec.ControlPlane.Ref,
template: blueprint.ControlPlane.Template,
labels: mergeMap(blueprint.Topology.ControlPlane.Metadata.Labels, blueprint.ClusterClass.Spec.ControlPlane.Metadata.Labels),
annotations: mergeMap(blueprint.Topology.ControlPlane.Metadata.Annotations, blueprint.ClusterClass.Spec.ControlPlane.Metadata.Annotations),
currentRef: nil,
obj: obj,
})
gotMetadata, err := contract.ControlPlane().MachineTemplate().Metadata().Get(obj)

gotMachineTemplateMetadata, err := contract.ControlPlane().MachineTemplate().Metadata().Get(obj)
g.Expect(err).ToNot(HaveOccurred())

gotControlPlaneMetadata, err := contract.ControlPlane().Metadata().Get(obj)
g.Expect(err).ToNot(HaveOccurred())

expectedLabels := mergeMap(scope.Current.Cluster.Spec.Topology.ControlPlane.Metadata.Labels, blueprint.ClusterClass.Spec.ControlPlane.Metadata.Labels)
expectedLabels[clusterv1.ClusterLabelName] = cluster.Name
expectedLabels[clusterv1.ClusterTopologyOwnedLabel] = ""
g.Expect(gotMetadata).To(Equal(&clusterv1.ObjectMeta{

expectedAnnotations := mergeMap(scope.Current.Cluster.Spec.Topology.ControlPlane.Metadata.Annotations, blueprint.ClusterClass.Spec.ControlPlane.Metadata.Annotations)

defaultControlPlaneAnnotations := map[string]string{
clusterv1.TemplateClonedFromNameAnnotation: blueprint.ClusterClass.Spec.ControlPlane.Ref.Name,
clusterv1.TemplateClonedFromGroupKindAnnotation: blueprint.ClusterClass.Spec.ControlPlane.Ref.GroupVersionKind().GroupKind().String(),
}
expectedControlPlaneAnnotations := mergeMap(defaultControlPlaneAnnotations, expectedAnnotations)

g.Expect(gotMachineTemplateMetadata).To(Equal(&clusterv1.ObjectMeta{
Labels: expectedLabels,
Annotations: expectedAnnotations,
}))

g.Expect(gotControlPlaneMetadata).To(Equal(&clusterv1.ObjectMeta{
Labels: expectedLabels,
Annotations: mergeMap(scope.Current.Cluster.Spec.Topology.ControlPlane.Metadata.Annotations, blueprint.ClusterClass.Spec.ControlPlane.Metadata.Annotations),
Annotations: expectedControlPlaneAnnotations,
}))

assertNestedField(g, obj, version, contract.ControlPlane().Version().Path()...)
Expand Down Expand Up @@ -444,6 +468,8 @@ func TestComputeControlPlane(t *testing.T) {
templateRef: blueprint.ClusterClass.Spec.ControlPlane.Ref,
template: blueprint.ControlPlane.Template,
currentRef: scope.Current.Cluster.Spec.ControlPlaneRef,
labels: mergeMap(blueprint.Topology.ControlPlane.Metadata.Labels, blueprint.ClusterClass.Spec.ControlPlane.Metadata.Labels),
annotations: mergeMap(blueprint.Topology.ControlPlane.Metadata.Annotations, blueprint.ClusterClass.Spec.ControlPlane.Metadata.Annotations),
obj: obj,
})
})
Expand Down Expand Up @@ -1361,6 +1387,9 @@ func TestComputeMachineDeployment(t *testing.T) {

g.Expect(actualMd.Labels).To(HaveKeyWithValue(clusterv1.ClusterTopologyMachineDeploymentLabelName, "big-pool-of-machines"))
g.Expect(actualMd.Labels).To(HaveKey(clusterv1.ClusterTopologyOwnedLabel))
g.Expect(actualMd.Labels).To(HaveKeyWithValue("foo", "baz"))
g.Expect(actualMd.Labels).To(HaveKeyWithValue("fizz", "buzz"))
g.Expect(actualMd.Annotations).To(HaveKeyWithValue("annotation-1", "annotation-1-val"))
g.Expect(controllerutil.ContainsFinalizer(actualMd, clusterv1.MachineDeploymentTopologyFinalizer)).To(BeTrue())

g.Expect(actualMd.Spec.Selector.MatchLabels).To(HaveKey(clusterv1.ClusterTopologyOwnedLabel))
Expand All @@ -1369,6 +1398,7 @@ func TestComputeMachineDeployment(t *testing.T) {
g.Expect(actualMd.Spec.Template.ObjectMeta.Labels).To(HaveKeyWithValue("foo", "baz"))
g.Expect(actualMd.Spec.Template.ObjectMeta.Labels).To(HaveKeyWithValue("fizz", "buzz"))
g.Expect(actualMd.Spec.Template.ObjectMeta.Labels).To(HaveKey(clusterv1.ClusterTopologyOwnedLabel))
g.Expect(actualMd.Spec.Template.ObjectMeta.Annotations).To(HaveKeyWithValue("annotation-1", "annotation-1-val"))
g.Expect(actualMd.Spec.Template.Spec.InfrastructureRef.Name).ToNot(Equal("linux-worker-inframachinetemplate"))
g.Expect(actualMd.Spec.Template.Spec.Bootstrap.ConfigRef.Name).ToNot(Equal("linux-worker-bootstraptemplate"))
})
Expand Down Expand Up @@ -1415,11 +1445,15 @@ func TestComputeMachineDeployment(t *testing.T) {

g.Expect(actualMd.Labels).To(HaveKeyWithValue(clusterv1.ClusterTopologyMachineDeploymentLabelName, "big-pool-of-machines"))
g.Expect(actualMd.Labels).To(HaveKey(clusterv1.ClusterTopologyOwnedLabel))
g.Expect(actualMd.Labels).To(HaveKeyWithValue("foo", "baz"))
g.Expect(actualMd.Labels).To(HaveKeyWithValue("fizz", "buzz"))
g.Expect(actualMd.Annotations).To(HaveKeyWithValue("annotation-1", "annotation-1-val"))
g.Expect(controllerutil.ContainsFinalizer(actualMd, clusterv1.MachineDeploymentTopologyFinalizer)).To(BeFalse())

g.Expect(actualMd.Spec.Template.ObjectMeta.Labels).To(HaveKeyWithValue("foo", "baz"))
g.Expect(actualMd.Spec.Template.ObjectMeta.Labels).To(HaveKeyWithValue("fizz", "buzz"))
g.Expect(actualMd.Spec.Template.ObjectMeta.Labels).To(HaveKey(clusterv1.ClusterTopologyOwnedLabel))
g.Expect(actualMd.Spec.Template.ObjectMeta.Annotations).To(HaveKeyWithValue("annotation-1", "annotation-1-val"))
g.Expect(actualMd.Spec.Template.Spec.InfrastructureRef.Name).To(Equal("linux-worker-inframachinetemplate"))
g.Expect(actualMd.Spec.Template.Spec.Bootstrap.ConfigRef.Name).To(Equal("linux-worker-bootstraptemplate"))
})
Expand Down