🐛 [0.2] Exclude conversion-data annotation when MD reconciles

[vincepri]

Signed-off-by: Vince Prignano [email protected]

What this PR does / why we need it:
This PR fixes an issue that was causing controller to fight with each other when a management cluster had both v1alpha2 and v1alpha3 running.

The MachineDeployment controller has has logic to sync annotations from a MachineDeployment to its linked MachineSets as seen in:

cluster-api/controllers/machinedeployment_sync.go

Line 112 in a799265

annotationsUpdated := mdutil.SetNewMachineSetAnnotations(d, msCopy, newRevision, true, logger)

The SetNewMachineSetAnnotations (regardless of what the names says) actually syncs up annotations on a new MachineSet or an existing one, in particular when this function is called

cluster-api/controllers/mdutil/util.go

Line 202 in a799265

annotationChanged := copyDeploymentAnnotationsToMachineSet(deployment, newMS)

annotation are copied, and if there has been changes it returns true and the MachineSet is ultimately patched.

All annotations are usually copied, with the exception for some listed in

cluster-api/controllers/mdutil/util.go

Lines 141 to 147 in a799265

	var annotationsToSkip = map[string]bool{
	v1.LastAppliedConfigAnnotation: true,
	clusterv1.RevisionAnnotation: true,
	clusterv1.RevisionHistoryAnnotation: true,
	clusterv1.DesiredReplicasAnnotation: true,
	clusterv1.MaxReplicasAnnotation: true,
	}

.

With the release of v1alpha3, we added the conversion-data annotation that is used internally to restore fields that would otherwise be lost during the conversion to an older version. This annotation caused the MD to always think that a change needs to be made:

1. MachineDeployment sees that the MachineSet has the wrong conversion-data value (because it's set from the conversion webhook)
2. MachineDeployment copies its own conversion-data (as shown above), increases the revision history

   cluster-api/controllers/mdutil/util.go

   Line 241 in a799265

   newMS.Annotations[clusterv1.RevisionHistoryAnnotation] = strings.Join(oldRevisions, ",")

   , and issues a patch.
3. Webhook conversion kicks in, but removes the conversion-data field

   cluster-api/util/conversion/conversion.go

   Line 125 in a799265

   delete(from.GetAnnotations(), DataAnnotation)

4. Webhook saves the updated object in etcd.
5. Webhook conversion kicks in and converts the object from v1alpha3 to v1alpha2, overwriting the conversion-data field.
6. Go to step 1

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [vincepri]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[vincepri]

/hold

[vincepri]

/milestone v0.2.x

[vincepri]

/assign @ncdc

[vincepri]

We'll need to forward-port this fix as well

[DheerajSShetty]

Without this change v1alpha2 MachineDeployments in a management cluster running capi-webhooks would fail to resize, update.
Used the fix here to check Creation and upgrade of a MachineDeployment and it worked fine

[ncdc]

@vincepri can you explain the details about the fighting? I also think a comment in the code about why we're including this annotation in the skip list would be helpful for future-us 😄

[vincepri]

@ncdc updated the first post explaining my findings, I'll update and add a comment to the code as well

[vincepri]

/hold cancel

[ncdc]

/lgtm