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

Ensure ownerRefs and refs are continuously reconciled #2075

Closed
3 tasks done
sbueringer opened this issue Jul 24, 2023 · 7 comments
Closed
3 tasks done

Ensure ownerRefs and refs are continuously reconciled #2075

sbueringer opened this issue Jul 24, 2023 · 7 comments
Assignees

Comments

@sbueringer
Copy link
Member

sbueringer commented Jul 24, 2023

There are various refs and ownerRefs managed by CAPV. The goal of this issue is to ensure that:

  • they are always bumped to the latest apiVersion
  • they are re-reconciled if the ownerRefs are dropped (which can e.g. happen via backup/restore)

We should also implement an e2e tests similar to the one in core CAPI to ensure this functionality doesn't break (quickstart + verification).

Some notes:

  • Let's ensure this also covers Node IPAM (maybe in a second iteration)
  • We should consider extending the core CAPI & CAPV e2e test to also ensure finalizers are re-applied

Tasks

@sbueringer
Copy link
Member Author

/assign @killianmuldoon
(as discussed)

@sbueringer
Copy link
Member Author

/assign @killianmuldoon

@killianmuldoon
Copy link
Contributor

killianmuldoon commented Aug 23, 2023

Added a tasklist to track the smaller tasks involved here. Will probably create issues for some of the subtasks when I scope and start them.

@killianmuldoon
Copy link
Contributor

AFAICT there are no references to objects that include an APIVersion in CAPV except for the bootstrapRef in the VSphereVM:

This Ref is always of type Secret from corev1 so we don't have to update the ref.

There are additional references to Secret objects in CAPV - e.g. in the VSphereCluster and VSphereClusterIdentity, but the apiVersion is not part of those references.

@killianmuldoon
Copy link
Contributor

That leaves the last follow-up items:

  1. Let's ensure this also covers Node IPAM (maybe in a second iteration)
    Currently not tested as we have no tests for node IPAM. Should be added as part of that test. We can be confident it's unecessary for deprecating v1alpha3 and v1alpha4 as the IPAM code was added well after v1beta1 was created. Additionally the code for creating the IPAddressClaims looks good - i.e. is using EnsureOwnerRef.

  2. We should consider extending the core CAPI & CAPV e2e test to also ensure finalizers are re-applied
    Will open issues for this.

/close

@k8s-ci-robot
Copy link
Contributor

@killianmuldoon: Closing this issue.

In response to this:

That leaves the last follow-up items:

  1. Let's ensure this also covers Node IPAM (maybe in a second iteration)
    Currently not tested as we have no tests for node IPAM. Should be added as part of that test. We can be confident it's unecessary for deprecating v1alpha3 and v1alpha4 as the IPAM code was added well after v1beta1 was created. Additionally the code for creating the IPAddressClaims looks good - i.e. is using EnsureOwnerRef.

  2. We should consider extending the core CAPI & CAPV e2e test to also ensure finalizers are re-applied
    Will open issues for this.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@sbueringer
Copy link
Member Author

Thank you! Sounds good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants