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.

Sign up for GitHub

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

Jump to bottom

✨ CAPI 1.5 support #674

Merged
merged 2 commits into from
Dec 18, 2023
Merged

✨ CAPI 1.5 support #674

merged 2 commits into from
Dec 18, 2023

Conversation

cprivitere
Copy link
Member

@cprivitere cprivitere commented Nov 21, 2023 •
edited
Loading

What this PR does / why we need it:
This PR is for upgrading CAPP to 1.5.X CAPI support.
This shall include dependency bumps as well as any new feature support we want/need to add.

  • remove support for packet-ccm
  • reduce tag length (capp instead of cluster-api-provider-packet)
  • use quay.io repo by default
  • fix lint errors
  • bump to capi 1.5.4
  • upgrade metal-go to v0.29.0
  • bump go version of Dockerfile to 1.20.11
  • fix return types for new CR version
  • add new logging calls more like upstream has
  • update rbac for packet-controller
  • better logging of unimplemented webhooks
  • add new clusterproxy methods to wrappedclusterproxy in e2e testing
  • bump e2e config capi versions
  • Update e2e yaml files to only test v1beta1
  • revert e2e ci config to standard version numbers
  • remove e2e clusterctl upgrade test as we dropped v1alpha3 support

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #611

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 21, 2023
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 21, 2023
@cprivitere
Copy link
Member Author

Current status is that while things compile just fine, creating a cluster results in nothing happening. The reconciler never seems to submit the API requests to create a device on Equinix Metal.

Attempting to debug this with metal go's debug options results in a SIGSEGV nil pointer error.

@cprivitere
Copy link
Member Author

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 22, 2023
@cprivitere
Copy link
Member Author

@displague What would your thoughts be about removing the support for older packet-ccm cloud provider entirely with this PR and only supporting equinixmetal. The method of introspection we used to figure out which cloud provider folks had installed:

  • Doesn't work with the newer controller runtime code
  • Is a bit of a hack where we had to swallow client errors to get it to work
  • Would require a lot of weird refactoring and eating of wrapped errors to work now
  • Doesn't seem needed at this point as who is even using the older packet-ccm at this point?
  • Is not how upstream docker handles it (they just set providerid prefix to docker:////) or another provider like azure handles it (they just straight up go with the UUID and no prefix, it seems).

@cprivitere cprivitere mentioned this pull request Dec 6, 2023
Merged
ctreatma added a commit to equinix-labs/metal-go that referenced this pull request Dec 6, 2023
@cprivitere @ctreatma
feat: patch bgpsession spec to remove status enum (#178)
cd21e3a 
While testing the new cluster API 1.5 changes in
kubernetes-sigs/cluster-api-provider-packet#674
We discovered the API was returning down,down for the bgpsessionstatus
after enabling BGP on a device. This didn't match any of the values in
the enum and would return an error.
@ctreatma  recommends we remove this enum and go back to just a string.

---------

Signed-off-by: Chris Privitere <23177737+cprivitere@users.noreply.github.com>
Co-authored-by: Charles Treatman <ctreatman@equinix.com>
@cprivitere
Copy link
Member Author

/unhold

@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Dec 6, 2023
@cprivitere
chore(deps): bump v1beta1 version
82c9317 
Signed-off-by: Chris Privitere <23177737+cprivitere@users.noreply.github.com>
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 6, 2023
@cprivitere
Copy link
Member Author

/hold issues with e2e tests still

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 7, 2023
@displague
Copy link
Member

displague commented Dec 7, 2023 •
edited
Loading

Doesn't seem needed at this point as who is even using the older packet-ccm at this point?

That's the only qualifier that I think we need to consider. If we were to remove support for that, I would recommend doing it in a separate PR. CAPI 1.5 upgrade PRs tend to be lighter. Removing packet-ccm support would make review harder.

opened #681 for that concern

@displague displague mentioned this pull request Dec 7, 2023
Closed
displague
displague reviewed Dec 7, 2023
View reviewed changes
test/e2e/suite_test.go Outdated Show resolved Hide resolved
@displague
Copy link
Member

displague commented Dec 7, 2023 •
edited
Loading

After looking over the changes, I'm fine with including #681 in this PR. The changes are almost entirely line removal.

@cprivitere cprivitere force-pushed the capi-1.5 branch 2 times, most recently from a7ce993 to 2d761fe Compare December 13, 2023 21:38
@cprivitere
breaking: Update to CAPI 1.5
826abc7 
- remove support for packet-ccm
- reduce tag length (capp instead of cluster-api-provider-packet)
- use quay.io repo by default
- fix lint errors
- bump to capi 1.5.4
- upgrade metal-go to v0.29.0
- bump go version of Dockerfile to 1.20.11
- fix return types for new CR version
- add new logging calls more like upstream has
- update rbac for packet-controller
- better logging of unimplemented webhooks
- add new clusterproxy methods to wrappedclusterproxy in e2e testing
- bump e2e config capi versions
- Update e2e yaml files to only test v1beta1
- revert e2e ci config to standard version numbers
- remove e2e clusterctl upgrade test as we dropped v1alpha3 support

Signed-off-by: Chris Privitere <23177737+cprivitere@users.noreply.github.com>
@cprivitere
Copy link
Member Author

/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 14, 2023
displague
displague reviewed Dec 18, 2023
View reviewed changes
pkg/cloud/packet/util.go
machineUIDTag = "cluster-api-provider-packet:machine-uid"
clusterIDTag = "cluster-api-provider-packet:cluster-id"
namespaceTag = "cluster-api-provider-packet:namespace"
machineUIDTag = "capp:machine-uid"
Copy link
Member

@displague displague Dec 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is a breaking change that will need to be called out in release notes with upgrade steps (along with the ramifications).

Presumably, changing the tags that are used in determining what to manage will cause the service to disown existing resources (without cleanup) and provision new resources. Is this for EIPs and servers? Control plane nodes too/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recognize that this was needed to fit more restrictive tag lengths in EM API resources.

displague
displague approved these changes Dec 18, 2023
View reviewed changes
Copy link
Member

@displague displague left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 18, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cprivitere, displague

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 [cprivitere,displague]

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

@cprivitere cprivitere added the release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. label Dec 18, 2023
@k8s-ci-robot k8s-ci-robot merged commit 9cb902b into kubernetes-sigs:main Dec 18, 2023
5 checks passed
@cprivitere cprivitere deleted the capi-1.5 branch December 18, 2023 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@displague displague displague approved these changes

@deitch deitch Awaiting requested review from deitch

@detiber detiber Awaiting requested review from detiber

Assignees

@displague displague

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CAPI v1.5.0 released, update to support it
3 participants
@cprivitere @displague @k8s-ci-robot