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

feat: re-introduce awsmanagedcluster #3797

Merged
merged 2 commits into from
Nov 16, 2022
Merged

feat: re-introduce awsmanagedcluster #3797

merged 2 commits into from
Nov 16, 2022

Conversation

richardcase
Copy link
Member

@richardcase richardcase commented Oct 25, 2022 •
edited
Loading

Signed-off-by: Richard Case [email protected]

What type of PR is this?

/kind feature
/kind api-change

What this PR does / why we need it:

This change brings back AWSManagedCluster and changes all the templates to use this. This is required for CAPA to support ClusterClass for EKS

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 #3507

Special notes for your reviewer:

Checklist:

  • squashed commits
  • includes documentation
  • adds unit tests
  • adds or updates e2e tests

Tasks:

  • Add AWSManagedCluster API definition
  • Add AWSManagedCluster controller
  • Add conditions to controller
  • Update EKS templates to use AWSManagedCluster
  • Update the e2e to use AWSManagedCluster
  • Update the docs to use AWSManagedCluster
  • Ensure follow on issues are created
  • Add GC to AWSManagedCluster

@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. kind/feature Categorizes issue or PR as related to a new feature. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 25, 2022
@k8s-ci-robot k8s-ci-robot added needs-priority size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 25, 2022
@richardcase
Copy link
Member Author

/milestone v2.0.0

@k8s-ci-robot k8s-ci-robot added this to the v2.0.0 milestone Oct 25, 2022
@richardcase richardcase removed this from the v2.0.0 milestone Oct 25, 2022
@pydctw
Copy link
Contributor

pydctw commented Oct 27, 2022

Is the plan to go with Option 2 or is it an intermediate step before moving to Option 3 of the proposal?

@richardcase
Copy link
Member Author

Is the plan to go with Option 2 or is it an intermediate step before moving to Option 3 of the proposal?

Initially, option 2 realistically. For CAPA (and CAPZ) to go to option 3 will be very difficult and the migration needs to be investigated as to wheter option 3 is realistic.

Also, the constraints that where put on the "managed k8s in CAPI proposal" need to be revisited.

@pydctw
Copy link
Contributor

pydctw commented Oct 28, 2022

Initially, option 2 realistically. For CAPA (and CAPZ) to go to option 3 will be very difficult and the migration needs to be investigated as to wheter option 3 is realistic.

Agreed. Option 3 is a big change and will need a careful planning.

As a reference point, there is a move from Ingress to Gateway API in sig-network. It is not mandatory to migrate but new features will be only developed in Gateway API and the latter provides more functionalities. The team is developing a CLI tool to help migration for end users. Something to think about if we decide to move to option 3 eventually.

Also, the constraints that where put on the "managed k8s in CAPI proposal" need to be revisited.

👍

@richardcase richardcase force-pushed the awsmanaged_cluster_is_back branch 2 times, most recently from bbcffe2 to 21da580 Compare November 9, 2022 14:56
@richardcase richardcase changed the title WIP feat: re-introduce awsmanagedcluster feat: re-introduce awsmanagedcluster Nov 9, 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 Nov 9, 2022
@richardcase richardcase force-pushed the awsmanaged_cluster_is_back branch 3 times, most recently from b2b6889 to 53acf22 Compare November 10, 2022 08:24
@richardcase
Copy link
Member Author

/test ?

@k8s-ci-robot
Copy link
Contributor

@richardcase: The following commands are available to trigger required jobs:

  • /test pull-cluster-api-provider-aws-build
  • /test pull-cluster-api-provider-aws-test
  • /test pull-cluster-api-provider-aws-verify

The following commands are available to trigger optional jobs:

  • /test pull-cluster-api-provider-aws-apidiff-main
  • /test pull-cluster-api-provider-aws-e2e
  • /test pull-cluster-api-provider-aws-e2e-blocking
  • /test pull-cluster-api-provider-aws-e2e-clusterclass
  • /test pull-cluster-api-provider-aws-e2e-conformance
  • /test pull-cluster-api-provider-aws-e2e-conformance-with-ci-artifacts
  • /test pull-cluster-api-provider-aws-e2e-eks

Use /test all to run the following jobs that were automatically triggered:

  • pull-cluster-api-provider-aws-apidiff-main
  • pull-cluster-api-provider-aws-build
  • pull-cluster-api-provider-aws-test
  • pull-cluster-api-provider-aws-verify

In response to this:

/test ?

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.

@richardcase
Copy link
Member Author

/test pull-cluster-api-provider-aws-e2e-eks

@richardcase richardcase force-pushed the awsmanaged_cluster_is_back branch from 53acf22 to cebd443 Compare November 10, 2022 11:04
@richardcase
Copy link
Member Author

/test pull-cluster-api-provider-aws-e2e-eks

@richardcase
Copy link
Member Author

e2e are timing out when provisioning :(

@richardcase
Copy link
Member Author

This is very strange as the e2e are passing locally :(

@richardcase
Copy link
Member Author

/test pull-cluster-api-provider-aws-e2e-blocking

@richardcase richardcase mentioned this pull request Nov 10, 2022
Open
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed 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 Nov 14, 2022
@richardcase
Copy link
Member Author

/test pull-cluster-api-provider-aws-e2e-eks

Ankitasw
Ankitasw reviewed Nov 14, 2022
View reviewed changes
Copy link
Member

@Ankitasw Ankitasw left a comment

Choose a reason for hiding this comment

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

Small nit, otherwise LGTM

@richardcase richardcase force-pushed the awsmanaged_cluster_is_back branch 2 times, most recently from 1997e02 to 068e483 Compare November 14, 2022 18:38
@richardcase
Copy link
Member Author

/test pull-cluster-api-provider-aws-e2e-eks

@richardcase richardcase force-pushed the awsmanaged_cluster_is_back branch from 068e483 to 917da73 Compare November 15, 2022 08:30
@richardcase
Copy link
Member Author

/test pull-cluster-api-provider-aws-e2e-eks

1 similar comment
@richardcase
Copy link
Member Author

/test pull-cluster-api-provider-aws-e2e-eks

@richardcase
Copy link
Member Author

eks e2e have passed after adding the new test: https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api-provider-aws/3797/pull-cluster-api-provider-aws-e2e-eks/1592461302469496832

@Ankitasw
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 Nov 15, 2022
@richardcase
test: add legacy test
6e9e8e3 
This change adds a new EKS e2e test to ensure that we don't break the
original way of using AWSManagedControlPlane for both the
infrastrastructure and control plane. We can considering removing this
test at some point in the future.

Also, a few minor changes as a result of review.

Signed-off-by: Richard Case <[email protected]>
@richardcase richardcase force-pushed the awsmanaged_cluster_is_back branch from 917da73 to 6e9e8e3 Compare November 15, 2022 16:11
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 15, 2022
@richardcase
Copy link
Member Author

Running the e2e again with the addition of the machine pool added to the legacy cluster:

/test pull-cluster-api-provider-aws-e2e-eks

@richardcase
Copy link
Member Author

🎉 the updated eks e2e tests are passing.

/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 Nov 15, 2022
@pydctw
Copy link
Contributor

pydctw commented Nov 16, 2022

I haven't thought about this but great to know EKS works with both 1) AWSMCP and 2) AWSMCP + AWSManagedCluster CRDs. Thanks for adding the legacy test.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 16, 2022
@Ankitasw
Copy link
Member

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Ankitasw

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 16, 2022
@k8s-ci-robot k8s-ci-robot merged commit 8da9555 into kubernetes-sigs:main Nov 16, 2022
This was referenced Nov 22, 2022
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ankitasw Ankitasw Ankitasw left review comments

@pydctw pydctw pydctw left review comments

@AverageMarcus AverageMarcus Awaiting requested review from AverageMarcus

Assignees

@pydctw pydctw

@Ankitasw Ankitasw

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. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
v2.0.0
Development

Successfully merging this pull request may close these issues.

Re-introduce AWSManagedCluster for EKS ClusterClass support
4 participants
@richardcase @pydctw @k8s-ci-robot @Ankitasw