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

⚠️ CAPI v1 support #307

Merged
merged 1 commit into from
Apr 1, 2022
Merged

Conversation

detiber
Copy link
Member

@detiber detiber commented Feb 7, 2022

What this PR does / why we need it:

Updates cluster-api-provider-packet to support Cluster API v1 (v1beta1 api types)

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 #220
Fixes #245

TODO:

  • Update release workflows and tooling to reflect new changes
  • Cleanup commit history
  • Documentation for upgrade
    • General upgrade docs
    • Multitenancy changes
  • Release notes

Replaces #269 and #289

Blocked on: kubernetes-sigs/cloud-provider-equinix-metal#230, kubernetes-sigs/cloud-provider-equinix-metal#234

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 7, 2022
@k8s-ci-robot k8s-ci-robot added sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 7, 2022
@davidspek
Copy link
Contributor

@detiber I've been watching this PR for a while now and I'm wondering if I can provide some help to get this ready for merging.

@detiber
Copy link
Member Author

detiber commented Mar 7, 2022

@detiber I've been watching this PR for a while now and I'm wondering if I can provide some help to get this ready for merging.

@davidspek Thanks for reaching out. The biggest outstanding item is for getting the changes to cloud-provider-equinix-metal merged and a new release for that published (kubernetes-sigs/cloud-provider-equinix-metal#232, kubernetes-sigs/cloud-provider-equinix-metal#230).

Outside of that, I think the next biggest thing is making sure the documentation is updated, including how to upgrade from v1alpha3/v0.3.x. The big thing to highlight here is that the v1alpha3/v0.3.x multitenancy support (namespaced based) is not supported out of the box with v1beta1/v1+ (https://cluster-api.sigs.k8s.io/developer/providers/v1alpha3-to-v1alpha4.html#multi-tenancy)

We'd also welcome any additional testing/validation that you are willing to do.

@davidspek
Copy link
Contributor

davidspek commented Mar 7, 2022

@detiber I'll look into this more in a couple days. I was able to get a dev environment setup and deploy about 6 clusters using this branch so it does look like it's working properly.

Another thing I'd like to do is solve the todos for using kube-vip. Should that be a separate PR after this release, or do you think that should be included for this release?

@davidspek
Copy link
Contributor

@detiber I've gotten kube-vip integrated and working for HA control planes. There only does seem to be an issue with clusterctl generate cluster that messes up the YAML formatting of the preKubeadmCommands and postKubeadmCommands. For now I'm going to try and find a way around the problem, but it likely needs to be fixed upstream as well.

If I find a way around the formatting issues, do you think the kube-vip integration should be part of this PR?

@davidspek
Copy link
Contributor

I've got the formatting issue that occurs when running clusterctl generate cluster now solved as well, so it should be possible to add this to the current PR.

@cprivitere
Copy link
Member

Hi @davidspek, can you put in a PR against https://github.com/detiber/cluster-api-provider-packet so we can see the changes?

@davidspek
Copy link
Contributor

@cprivite Sure, I'll create a PR in tomorrow morning. The changes are actually quite minimal.

@davidspek
Copy link
Contributor

@cprivite @detiber I just created the PR quickly before I head off to bed so you can already take a look at the changes.

detiber#65

@davidspek
Copy link
Contributor

davidspek commented Mar 9, 2022

I've just noticed that the CCM starts trying to assign the control plane EIP to one of the nodes, which it shouldn't do when using kube-vip. I've laid out the issue and possible solutions in kubernetes-sigs/cloud-provider-equinix-metal#230 (comment).

I've updated my PR so that the CCM doesn't take control of the control plane EIP anymore.

@detiber detiber self-assigned this Mar 15, 2022
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 22, 2022
- Update dependencies:
  - GitHub Actions:
    - Bump actions/checkout from 2.3.5 to 3
    - Bump actions/cache from 2 to 3
  - bump cobra to v1.4.0
  - Bump cluster api to 1.1.3 and bump associated dependencies as a result
  - Update CPEM deployment to latest
  - Update calico, use kustomize from upstream source, and override images to use quay.io instead of dockerhub

- Remove separate deployment for webhooks
- Add configuration for cpem to use host ip for heatlh checks
- Add retries into cluster template for installing CPEM
- Add upstream e2e tests
- Update conformance spec skip serial tests like upstream does
- Update ci and build workflows to make better use of caching
- Add upgrade and multitenancy documentation

Co-authored-by: Chris Privitere <cprivite@users.noreply.github.com>
Signed-off-by: Jason DeTiberus <detiber@users.noreply.github.com>
@detiber detiber changed the title [WIP] ⚠️ CAPI v1 support ⚠️ CAPI v1 support Apr 1, 2022
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 1, 2022
@detiber
Copy link
Member Author

detiber commented Apr 1, 2022

/assign @displague

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.

All these passing tests are a thing of beauty, https://github.com/detiber/cluster-api-provider-packet/runs/5793843612?check_suite_focus=true .

Thanks for your dedication to the project and this effort, @detiber!

This was a massive lift and it's greatly appreciated. 🚀

I left some nitpicks on TODO items, but I don't see why we can't address those (and release notes) post-merge ahead of release.

push:
branches:
- main
- capi-v1 # TODO: Used for testing my fork, remove prior to merge
Copy link
Member

Choose a reason for hiding this comment

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

Let's clear out these "prior to merge" todos

name: Cleanup
on:
schedule:
- cron: "0 */2 * * *" # TODO: Run every 4 hours to soak test, should be less frequent before merge (weekly/daily/???)
Copy link
Member

@displague displague Apr 1, 2022

Choose a reason for hiding this comment

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

another "before merge" TODO

@displague
Copy link
Member

/honk

@k8s-ci-robot
Copy link
Contributor

@displague:
goose image

In response to this:

/honk

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.

@displague
Copy link
Member

displague commented Apr 1, 2022

/approve
/lgtm

practicing for #314

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: detiber, 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:

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

@displague
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 1, 2022
@k8s-ci-robot k8s-ci-robot merged commit 365fddb into kubernetes-sigs:main Apr 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ClusterAPI stucks when Provisioning failure happens Some resources missing the cluster-api category
5 participants