kubernetes-sigs / cluster-api

Home for Cluster API, a subproject of sig-cluster-lifecycle
https://cluster-api.sigs.k8s.io
Apache License 2.0
3.55k stars 1.3k forks source link

Switch to ssa.Patch to create machines in machinepool controller #9755

Closed CecileRobertMichon closed 10 months ago

CecileRobertMichon commented 11 months ago

I would recommend using the following here:

if err := ssa.Patch(ctx, r.Client, MachinePoolControllerName, machine); err != nil {

Otherwise you end up with co-ownership of fields between "manager" and "machinepool-controller" like here: https://storage.googleapis.com/kubernetes-jenkins/logs/periodic-cluster-api-e2e-main/1725100075312484352/artifacts/clusters/bootstrap/resources/quick-start-jwp00u/Machine/quick-start-loyh5w-mp-0-9svll-hk7jp.yaml

The consequence is that it's impossible to unset fields via "machinepool-controller"

_Originally posted by @sbueringer in https://github.com/kubernetes-sigs/cluster-api/pull/9619#discussion_r1395699740_

CecileRobertMichon commented 11 months ago

Following discussion https://github.com/kubernetes-sigs/cluster-api/pull/9725#discussion_r1399668133:

fake client doesn't support SSA. Sorry didn't think about that when recommending using SSA.

For reference. In KCP we previously had a flag to skip some optional SSA part until we migrated the tests over to envtest (https://github.com/kubernetes-sigs/cluster-api/issues/8393).

But I think we should aim for using SSA consistently before the v1.6.0 release. Otherwise we end up with strange issues in our release.

The ideal solution for this now is to migrate the affected tests to envtest.

This will require migrating tests to use envtest

sbueringer commented 11 months ago

/triage accepted

CecileRobertMichon commented 10 months ago

/assign