search

kubernetes / cloud-provider-aws

Cloud provider for AWS
https://cloud-provider-aws.sigs.k8s.io/
Apache License 2.0
395 stars 302 forks source link

Removes v2 provider #677

Closed mmerkes closed 1 year ago

mmerkes commented 1 year ago

What type of PR is this?

/kind cleanup

What this PR does / why we need it: This PR is part clean up, part fact-finding mission. On multiple PRs, it's been discussed that v2 is no longer being used. I've been asking around, and I haven't been able to figure out what the history is and what it's used for. It seems like it's abandoned. I know that EKS doesn't use it, and it doesn't look like it's being maintained.

If you have context or if you used v2, please respond with more details! We will keep this PR open at least for a couple of weeks to collect feedback. I will cancel the PR if we find out that it's still be used.

NOTE: I left the v2 mocks package for now as it's used by the ecr-credential-provider. I'll post another PR to move it somewhere more appropriate if this gets merged.

Which issue(s) this PR fixes: N/A

Special notes for your reviewer: Let's give folks time to respond here before we take any action. Does this PR introduce a user-facing change?:

Removes v2 provider.
k8s-ci-robot commented 1 year ago

This issue is currently awaiting triage.

If cloud-provider-aws contributors determine this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
k8s-ci-robot commented 1 year ago

Hi @mmerkes. Thanks for your PR.

I'm waiting for a kubernetes 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](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
hakman commented 1 year ago

/ok-to-test /lgtm /cc @dims @cartermckinnon

cartermckinnon commented 1 year ago

I don't see any usage here: https://grep.app/search?q=k8s.io/cloud-provider-aws/pkg/providers/v2

@james-callahan can you weigh in on this?

mmerkes commented 1 year ago

Initial PR: https://github.com/kubernetes/cloud-provider-aws/pull/114

k8s-ci-robot commented 1 year ago

@mmerkes: The following tests 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-cloud-provider-aws-e2e-kubetest2-quick 3ba9641a5dedecdde9ed494ceca9e7b7e73cd41c link false /test pull-cloud-provider-aws-e2e-kubetest2-quick
pull-cloud-provider-aws-e2e-kubetest2 3ba9641a5dedecdde9ed494ceca9e7b7e73cd41c link false /test pull-cloud-provider-aws-e2e-kubetest2

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](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
olemarkus commented 1 year ago

@nckturner and I have discussed this one at length earlier and concluded it should be removed. Just never got around to this PR.

The reasoning being that a lot of CCM responsibilities have been removed. The service controller is 'deprecated' in favour of LBC and likewise volume controller in favour of CSI. There is a lot to be desired in v1 code quality wise, but we feel the new configuration interface complicates things more than it is worth.

/approve /lgtm /hold

k8s-ci-robot commented 1 year ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: olemarkus

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](https://github.com/kubernetes/cloud-provider-aws/blob/master/OWNERS)~~ [olemarkus] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
nckturner commented 1 year ago

Yes, it was an experiment and the original author is no longer working on it and it never got picked up.

/lgtm

cartermckinnon commented 1 year ago

/retest /lgtm

hakman commented 1 year ago

/hold cancel

mmerkes commented 1 year ago

As promised, final clean up: https://github.com/kubernetes/cloud-provider-aws/pull/678

james-callahan commented 1 year ago

If you have context or if you used v2, please respond with more details! We will keep this PR open at least for a couple of weeks to collect feedback. I will cancel the PR if we find out that it's still be used.

5 hours was far less than a couple of weeks.

I don't see any usage here: https://grep.app/search?q=k8s.io/cloud-provider-aws/pkg/providers/v2

I'm not sure what you expect to see? We use it by passing --v=2 when using the registry.k8s.io/provider-aws/cloud-controller-manager docker image.

@james-callahan can you weigh in on this?

We use it in all our production clusters and I know a few other people using it too. It seemed much better written than the v1 implementation, and worked well.

I'm curious why people want to work on v1 rather than v2?

cartermckinnon commented 1 year ago

5 hours was far less than a couple of weeks.

I agree, the intention was to have this on /hold for a bit. We can always revert, we have plenty of time before 1.29 is cut.

We use it by passing --v=2 when using the registry.k8s.io/provider-aws/cloud-controller-manager docker image.

Oh, that's not what that flag does, that sets the log verbosity. You'd need to use --cloud-provider=aws/v2 and set an env var (ENABLE_ALPHA_V2=true) to use the v2 provider impl. Are you doing that?

I'm curious why people want to work on v1 rather than v2?

From the perspective of EKS: we use v1 exclusively, and have no behavioral/perf reason to move to v2. If we have to fix something, it's always in v1; and making the same change in 2 places is toil. In hindsight, I don't think this implementation option should have been exposed

james-callahan commented 1 year ago

Oh, that's not what that flag does, that sets the log verbosity. You'd need to use --cloud-provider=aws/v2 and set an env var (ENABLE_ALPHA_V2=true) to use the v2 provider impl. Are you doing that?

oh wow! Looks like I removed that a few months ago and completely forgot. I guess when I was debugging e.g. https://github.com/kubernetes/cloud-provider-aws/issues/477#issuecomment-1455251389

In that case I guess I remove my assertion that we're using it in prod. And I'm really confused about https://github.com/kubernetes/cloud-provider-aws/issues/638