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

Add proposal for AzureManagedCluster graduation from experimental #2602

Merged

Conversation

jackfrancis
Copy link
Contributor

@jackfrancis jackfrancis commented Aug 24, 2022

What type of PR is this?

/kind documentation

What this PR does / why we need it:

This PR is a proposal that outlines the set of criteria that must be addressed prior to graduating AzureManagedCluster out of experimental.

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

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

  • squashed commits
  • includes documentation
  • adds unit tests

Release note:

NONE

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/documentation Categorizes issue or PR as related to documentation. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 24, 2022
@jackfrancis jackfrancis added the area/managedclusters Issues related to managed AKS clusters created through the CAPZ ManagedCluster Type label Aug 29, 2022
@marosset
Copy link
Contributor

/test ls

@k8s-ci-robot
Copy link
Contributor

@marosset: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

  • /test pull-cluster-api-provider-azure-build
  • /test pull-cluster-api-provider-azure-ci-entrypoint
  • /test pull-cluster-api-provider-azure-e2e
  • /test pull-cluster-api-provider-azure-test
  • /test pull-cluster-api-provider-azure-verify

The following commands are available to trigger optional jobs:

  • /test pull-cluster-api-provider-azure-apidiff
  • /test pull-cluster-api-provider-azure-apiversion-upgrade
  • /test pull-cluster-api-provider-azure-capi-e2e
  • /test pull-cluster-api-provider-azure-conformance
  • /test pull-cluster-api-provider-azure-conformance-with-ci-artifacts
  • /test pull-cluster-api-provider-azure-coverage
  • /test pull-cluster-api-provider-azure-e2e-csi-migration
  • /test pull-cluster-api-provider-azure-e2e-exp
  • /test pull-cluster-api-provider-azure-e2e-optional
  • /test pull-cluster-api-provider-azure-e2e-workload-upgrade
  • /test pull-cluster-api-provider-azure-windows-containerd-upstream-with-ci-artifacts
  • /test pull-cluster-api-provider-azure-windows-containerd-upstream-with-ci-artifacts-serial-slow

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

  • pull-cluster-api-provider-azure-apidiff
  • pull-cluster-api-provider-azure-build
  • pull-cluster-api-provider-azure-coverage
  • pull-cluster-api-provider-azure-test
  • pull-cluster-api-provider-azure-verify

In response to this:

/test ls

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.

@marosset
Copy link
Contributor

/test pull-cluster-api-provider-azure-ci-entrypoint
/test pull-cluster-api-provider-azure-windows-containerd-upstream-with-ci-artifacts
/test pull-cluster-api-provider-azure-conformance

@k8s-ci-robot
Copy link
Contributor

@jackfrancis: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-cluster-api-provider-azure-windows-containerd-upstream-with-ci-artifacts 242d355 link false /test pull-cluster-api-provider-azure-windows-containerd-upstream-with-ci-artifacts

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@jackfrancis jackfrancis added this to the v1.6 milestone Sep 29, 2022
@jackfrancis
Copy link
Contributor Author

/assign

@jackfrancis jackfrancis force-pushed the AzureManagedCluster-v1-proposal branch from 242d355 to a70c824 Compare October 3, 2022 17:59
@jackfrancis jackfrancis marked this pull request as ready for review October 3, 2022 18:37
@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 Oct 3, 2022
Copy link
Contributor

@nojnhuh nojnhuh left a comment

Choose a reason for hiding this comment

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

Overall LGTM

I remember it being brought up before that there might be an implicit dependency here on graduating MachinePool out of experimental in CAPI before we can call AzureManagedCluster stable. Should we note in this doc that that's something we have yet to think through?

docs/proposals/20220825-azuremanaged-cluster-v1.md Outdated Show resolved Hide resolved
docs/proposals/20220825-azuremanaged-cluster-v1.md Outdated Show resolved Hide resolved
Copy link
Contributor

@CecileRobertMichon CecileRobertMichon left a comment

Choose a reason for hiding this comment

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

Would it make sense for this proposal to be an issue instead? Enhancement proposals are usually defining a new functionality that has associated user stories and implementation details. This is more of a task list/epic for graduating an existing functionality. On the other hand, Integrate Cluster API Proposal Recommendations is something we'd want to have a concrete proposal for, especially when it comes to user-facing API changes.

@jackfrancis jackfrancis force-pushed the AzureManagedCluster-v1-proposal branch from a70c824 to 0114d41 Compare October 5, 2022 22:46
@jackfrancis
Copy link
Contributor Author

@CecileRobertMichon I agree that whatever happens we are going to want to file issues for commited work. Not sure if that can be helped by having a proposal PR that defines things in a markdown in the repo, or if we can simply "publish" the proposal in an issue description. We can discuss in tomorrow's office hours.

@CecileRobertMichon
Copy link
Contributor

We can discuss in tomorrow's office hours.

Did we end up discussing/coming to an agreement on this? personally I'm +1 on just keeping track of tasks as issues with a project board to track progress

@jackfrancis
Copy link
Contributor Author

@CecileRobertMichon time didn't permit yesterday, but yeah I agree that this doc should pair with an "AzureManagedCluster graduation" project board.

I think I'd like to keep this doc open as a (IMO) preferable way to document to all AKS + capz stakeholders the agreed upon definition of "graduation".

@zmalik @sonasingh46 @mtougeron and others, please add any feedback that better reflects your ideas of desired progress, and share this with your CTOs :)

Copy link
Member

@mtougeron mtougeron left a comment

Choose a reason for hiding this comment

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

Just has 2 minor Qs but this looks great from my perspective.

@jackfrancis jackfrancis modified the milestones: v1.6, next Nov 3, 2022
@jackfrancis jackfrancis force-pushed the AzureManagedCluster-v1-proposal branch from 0114d41 to 9600a32 Compare November 3, 2022 22:26

### 2. Integrate Cluster API Proposal Recommendations

Once the proposal is accepted and merged into the Cluster API project as an endorsed set of provider recommendations, capz can audit the existing "`AzureManagedCluster`" experimental implementation for areas of disagreement with said recommendations. For each discovery of disagreement, there should be a defensible reason to matriculate a divergent capz Managed Kubernetes implementation into a post-experimental definition of "`AzureManagedCluster`", and ideally some form of sign-off from other provider maintainers. Where community consensus cannot be reached, capz should strongly consider evolving the experimental "`AzureManagedCluster`" implementation to meet the Cluster API Managed Kubernetes recommendation as a pre-requisite to graduating from experimental.
Copy link
Contributor

Choose a reason for hiding this comment

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

Regarding the possibility of a successor to the CAPI proposal, would we block CAPZ's non-experimental managed cluster implementation on that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. In fact there is no mention of a "Cluster API-native" ManagedCluster definition in this doc. Once a working group starts making progress towards this (assuming this proposal doc hasn't yet merged), it would probably make sense to mention that effort, and to explicitly state that AzureManagedCluster graduation is not dependent upon that proposal.

Off the top of my head I'd estimate that a long-term, capi ManagedCluster solution would be a v1 thing (graduation of AzureManagedCluster will be within the v1beta1 API scope).

@jackfrancis jackfrancis modified the milestones: next, v1.7 Dec 1, 2022
@jackfrancis jackfrancis force-pushed the AzureManagedCluster-v1-proposal branch from 9600a32 to 092dbb9 Compare December 1, 2022 23:07
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 1, 2022
@jackfrancis
Copy link
Contributor Author

@nojnhuh @CecileRobertMichon @mtougeron I've updated this proposal to reflect recent events, added some status to various items, and have started organizing links to issues that reflect work done. PTAL, I wonder if we can merge this soon and then regularly maintain it if things change.

@nojnhuh
Copy link
Contributor

nojnhuh commented Dec 2, 2022

/lgtm
Thanks for putting this all together!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 2, 2022
authors:
- "@jackfrancis"
reviewers:
- @CecileRobertMichon
Copy link
Contributor

Choose a reason for hiding this comment

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

did everyone listed in reviewers actually review? if not let's remove those who didn't prior to merge (or get their review)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zmalik @NovemberZulu @mtougeron could you kindly review this and weigh in?

If we don't hear from folks in ~a week or so let's remove those names from this list and finalize for merge.

Copy link
Member

Choose a reason for hiding this comment

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

I'll take a look by EOD Monday.

Copy link
Contributor

@CecileRobertMichon CecileRobertMichon left a comment

Choose a reason for hiding this comment

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

/lgtm

@nawazkh
Copy link
Member

nawazkh commented Dec 9, 2022

/lgtm
🚀

@mtougeron
Copy link
Member

/lgtm

- https://github.com/kubernetes-sigs/cluster-api/issues/7494


### 3. Pathway Towards Full AKS Feature Support (Status: DONE)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nojnhuh @mtougeron @sonasingh46 @zioproto @LochanRn @luthermonson

All of you (and apologies to others I haven't mentioned!) have made significant feature contributions to the AzureManagedCluster API recently. As a result of that effort, does anyone have any concerns that the existing API surface area may be non-durable w/ respect to graduation from experimental?

The plan is to "move the existing API forward", based on the conclusion that we have done sufficient validation to confidently predict that additional feature work can continue to successfully land on this existing API. In other words, we have no reason to believe that we'll have to break the API in order to accommodate future AKS features.

Can folks weigh in on their confidence level of the above? Especially if you feel otherwise!

Thank you 🙏

Copy link
Contributor

Choose a reason for hiding this comment

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

One of the more potentially disruptive changes I can think of that we haven't addressed yet is exposing the AKS preview APIs (#2625). Even that seems like something we can enable in a backwards-compatible manner though. Overall, nothing else comes to mind that might threaten the integrity of the API based on where I've been focused recently.

@jackfrancis
Copy link
Contributor Author

Lazy consensus timer starting, and expiring ~mid next week (week of Dec 19).

@jackfrancis
Copy link
Contributor Author

Let's call the lazy consensus expired and land this officially before the winter break.

Thanks everyone for your feedback!

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jackfrancis

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 Dec 20, 2022
@k8s-ci-robot k8s-ci-robot merged commit 501a908 into kubernetes-sigs:main Dec 20, 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. area/managedclusters Issues related to managed AKS clusters created through the CAPZ ManagedCluster Type cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/documentation Categorizes issue or PR as related to documentation. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants