🌱 Deduplicate AdoptMachinePorts and AdoptBastionPorts #1944
Conversation
k8s-ci-robot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mdbooth 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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
cncf-cla: yes
k8s-ci-robot
added
the
size/XL
✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
k8s-ci-robot
added
the
needs-rebase
k8s-ci-robot
removed
the
needs-rebase
|
/test pull-cluster-api-provider-openstack-e2e-full-test |
|
/test pull-cluster-api-provider-openstack-e2e-full-test |
k8s-ci-robot
added
the
needs-rebase
k8s-ci-robot
removed
the
needs-rebase
|
/test pull-cluster-api-provider-openstack-e2e-full-test |
k8s-ci-robot
added
size/L
k8s-ci-robot
added
size/XL
|
/test pull-cluster-api-provider-openstack-e2e-full-test |
|
/test pull-cluster-api-provider-openstack-e2e-full-test |
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
|
#1954 was the root cause of most of the full-test CI failures in this PR. This PR includes a change, which we needed to make anyway, which avoids the issue in most cases: we don't need to re-reconcile if we only adopted. However, the underlying issue remains. I'll push a separate PR to fix that. |
| @@ -25,7 +25,7 @@ import ( | |||
| func AdoptDependentMachineResources(scope *scope.WithLogger, baseName string, referencedResources *infrav1.ReferencedMachineResources, dependentResources *infrav1.DependentMachineResources) error { | |||
| networkingService, err := networking.NewService(scope) | |||
| if err != nil { | |||
| return false, err | |||
There was a problem hiding this comment.
Seems like this change should be in a different commit? It's syntax error otherwise.
|
Alright, I only found one issue with a change done in the last commit, when it should be done in the middle one. Let's fix that so that reverts would be possible? Or just squash the commits. |
Thanks. I'll tidy them up but keep them separate, as I've been trying to make smaller, functional commits. When you look at the next one in this series, though, you'll see how that's going 😟 |
Both of these methods rely on ReferencedMachineResources and DependentMachineResources, so they can be easily refactored to have a common implementation.
Unlike referenced resources, which specify a future intent, adoption only affects resources which have already been created. Successive executions will produce the same output, so there is no need to re-reconcile if we found orphaned resources. Also we seem to be hitting this a lot in practise, most likely due to the controller-runtime read-after-write cache inconsistency issue.
We can't resolve machine resources until the cluster is initialised.
|
I've cleaned that up and re-run the non-e2e tests against each commit individually. I also rebased on to main to pick up #1955, which is important for this series. |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
Both of these methods rely on ReferencedMachineResources and DependentMachineResources, so they can be easily refactored to have a common implementation.
Fixes: #1942
This PR is based on #1941, so we should merge that first.
TODO:
/hold