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

📖 Proposal for two new AWS self-managed cluster feature gates #5276

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mzazrivec
Copy link
Contributor

@mzazrivec mzazrivec commented Jan 13, 2025

What type of PR is this?

/kind documentation

What this PR does / why we need it:

Which issue(s) this PR fixes
This proposal should be approved before #5124 can be implemented.

Special notes for your reviewer:

Checklist:

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

Release note:


@k8s-ci-robot
Copy link
Contributor

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added kind/documentation Categorizes issue or PR as related to documentation. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 13, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign dlipovetsky for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found 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 needs-priority needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 13, 2025
@k8s-ci-robot
Copy link
Contributor

Hi @mzazrivec. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow repository.

@mzazrivec mzazrivec changed the title Proposal for two new AWS self-managed cluster feature gates WIP: Proposal for two new AWS self-managed cluster feature gates Jan 13, 2025
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 13, 2025

## Proposal

- Introduce AWSMachine feature gate which would turn on and off the AWSMachineReconciler. The feature gate would be on by default.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Introduce AWSMachine feature gate which would turn on and off the AWSMachineReconciler. The feature gate would be on by default.
- Introduce AWSMachine feature gate which would turn on and off the AWSMachineReconciler. The feature gate would be enabled by default.

## Proposal

- Introduce AWSMachine feature gate which would turn on and off the AWSMachineReconciler. The feature gate would be on by default.
- Introduce AWSCluster feature gate which would turn on and off the AWSClusterReconciler. The feature gate would be on by default.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Introduce AWSCluster feature gate which would turn on and off the AWSClusterReconciler. The feature gate would be on by default.
- Introduce AWSCluster feature gate which would turn on and off the AWSClusterReconciler. The feature gate would be enabled by default.


- change any of the existing feature gates or their semantics

## Proposal
Copy link
Contributor

Choose a reason for hiding this comment

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

Please explain; where is changes will happen in the code , ex ; a new featureGate with name: selfManagedAWSCluster , default: true will be defined here as well as where is the changes will happen in the manager main to enable?disable those features

Please explain the dependency between those 2 new featureGates; you cannot have the AWSMachine while the AWSCluster is disabled.
How do you will handle this case if endUser enabled the AWSMachine without AWSCluster ?

Also explain what possible risks if the endUser disable those FeatureGates while the AWSCluster OR AWSMachine CRs are reconciling. Might be good to mentioned that endUser can enable the AWSCluster with MachinePool while disable the AWSMachine feature.

- Introduce AWSCluster feature gate which would turn on and off the AWSClusterReconciler. The feature gate would be on by default.

### User Stories

Copy link
Contributor

Choose a reason for hiding this comment

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

another user stories is as endUser I want to use EKS only or ROSA only OR EKS & ROSA without enabling AWS self-managed clusters


## Alternatives

The alternative here would be simply not to implement the two new feature gates and rely just on namespace separation.
Copy link
Contributor

Choose a reason for hiding this comment

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

Another alternative is user can use labels to let the controllers only watch certain CRs that have the labels. However, this not completely satisfy the case as the controllers is still enabled.


## Upgrade Strategy

CAPA upgrades should not be affected. Existing deployments will not notice anything different, because the two new feature gates will be on by default (current behavior).
Copy link
Contributor

Choose a reason for hiding this comment

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

Please explain that with upgrade user should expect to see 2 new featureGates has been added to the CAPA deployment with default is true


1. Ability to enable / disable feature gates for AWSCluster and AWSMachine controllers.
2. Both feature gates would be on by default.

Copy link
Contributor

Choose a reason for hiding this comment

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

We need to inform the endUser in case enabling the AWSMachine feature gate without enabling the AWSCluster feature gate.

@mzazrivec mzazrivec force-pushed the proposal_for_granularity_switch branch from b217164 to a6f6503 Compare January 15, 2025 14:06
@serngawy
Copy link
Contributor

please include emojis in the PR title so the verify job can pass

@mzazrivec mzazrivec changed the title WIP: Proposal for two new AWS self-managed cluster feature gates 📖 Proposal for two new AWS self-managed cluster feature gates Jan 15, 2025
@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 Jan 15, 2025
@serngawy
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 15, 2025
title: Feature gates for AWSCluster and AWSMachine
authors:
- "@mzazrivec"
reviewers:
Copy link
Contributor

Choose a reason for hiding this comment

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

Add me as well - @nrb

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nrb Done.

@nrb
Copy link
Contributor

nrb commented Jan 20, 2025

Will give this a proper review, but we agreed in the Jan 20, 2025 community call to set lazy consensus for 1 week. If no major objections to this, we'll merge on Jan 27, 2025.

@mzazrivec mzazrivec force-pushed the proposal_for_granularity_switch branch from a6f6503 to 4c5c448 Compare January 21, 2025 12:21
Copy link
Contributor

@AndiDog AndiDog left a comment

Choose a reason for hiding this comment

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

Fine for me. Just some wording comments.


## Motivation

Motivation for the two new feature gates is the option to turn on and off CAPA's ability to reconcile AWSMachine and AWSCluster resources for AWS self-managed clusters. Currently, controllers for these two resource types are always on.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean EKS clusters? The words "self-managed", "managed" and "unmanaged" are quite ambiguous already and I'd avoid them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough, fixed.


Motivation for the two new feature gates is the option to turn on and off CAPA's ability to reconcile AWSMachine and AWSCluster resources for AWS self-managed clusters. Currently, controllers for these two resource types are always on.

The possibility to turn the respective controllers on and of becomes important in multi-tenant CAPA installations (more than one CAPA installed in a single kubernetes cluster). The two new feature gates in will help to avoid interferences and conflicts between the controllers during resource reconciliations.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The possibility to turn the respective controllers on and of becomes important in multi-tenant CAPA installations (more than one CAPA installed in a single kubernetes cluster). The two new feature gates in will help to avoid interferences and conflicts between the controllers during resource reconciliations.
The possibility to turn the respective controllers on and off becomes important in multi-tenant CAPA installations (more than one CAPA installed in a single kubernetes cluster). The two new feature gates will help avoid interference and conflicts between the controllers during resource reconciliations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.


## Implementation History

- [ ] 01/20/2025: Proposed idea in an issue or [community meeting]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- [ ] 01/20/2025: Proposed idea in an issue or [community meeting]
- [ ] 2025-01-20: Proposed idea in an issue or [community meeting]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed as well, thank you.

@mzazrivec mzazrivec force-pushed the proposal_for_granularity_switch branch from 4c5c448 to bcd1079 Compare January 24, 2025 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. kind/documentation Categorizes issue or PR as related to documentation. needs-priority ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants