Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
CecileRobertMichon opened this issue Nov 21, 2023 · 3 comments · Fixed by #9791
Closed

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

CecileRobertMichon opened this issue Nov 21, 2023 · 3 comments · Fixed by #9791
Assignees
Labels
area/machinepool Issues or PRs related to machinepools triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@CecileRobertMichon
Copy link
Contributor

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 #9619 (comment)

@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Nov 21, 2023
@CecileRobertMichon
Copy link
Contributor Author

Following discussion #9725 (comment):

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 (#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
Copy link
Member

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Nov 22, 2023
@sbueringer sbueringer added the area/machinepool Issues or PRs related to machinepools label Nov 22, 2023
@CecileRobertMichon
Copy link
Contributor Author

/assign

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/machinepool Issues or PRs related to machinepools triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants