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

Refactor to use legacy cloud provider #61

Merged
merged 1 commit into from
Oct 10, 2019

Conversation

nckturner
Copy link
Contributor

@nckturner nckturner commented Oct 2, 2019

  • This is starting with v1.15.0
  • This is important because we need to start by using legacy-cloud-provider so that we don't fork this repo from in-tree before people migrate. That approach would make migration much more difficult.

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

In order to start using this repo to build valid cloud-controller-manager binaries and begin adding tests, we need to migrate to the legacy-cloud-provider.

NONE

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 2, 2019
@nckturner
Copy link
Contributor Author

/cc @M00nF1sh

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 2, 2019
@nckturner nckturner force-pushed the use-legacy-cloud-provider branch from 436555f to 9c8b8bf Compare October 7, 2019 18:37
@nckturner nckturner changed the title WIP refactor to use legacy cloud provider Refactor to use legacy cloud provider Oct 7, 2019
@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 7, 2019
@nckturner nckturner force-pushed the use-legacy-cloud-provider branch from 9c8b8bf to 767f21d Compare October 7, 2019 18:38
@nckturner
Copy link
Contributor Author

Followed openstack's approach, which involves defining the cobra command, and additionally redefining the initFuncs for each cloud controller.

@nckturner
Copy link
Contributor Author

/cc @andrewsykim

// Use our version instead of the Kubernetes formatted version
versionFlag := cmd.Flags().Lookup("version")
if versionFlag.Value.String() == "true" {
klog.V(1).Infof("%s version: %s", cmd.Name(), version)
Copy link
Member

Choose a reason for hiding this comment

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

exit after printing version?

return controllers
}

func startCloudNodeController(ctx *cloudcontrollerconfig.CompletedConfig, cloud cloudprovider.Interface, stopCh <-chan struct{}) (http.Handler, bool, error) {
Copy link
Member

Choose a reason for hiding this comment

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

If you call NewCloudControllerManagerCommand() in main.go, you should get all of this with it.

Copy link
Contributor Author

@nckturner nckturner Oct 10, 2019

Choose a reason for hiding this comment

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

Yeah I saw that. It would be easier but my plan was to start with this and then eventually replace/remove the dependencies on k/k, which I think will be easier if we start here.

* This is starting with v1.15.0
@nckturner nckturner force-pushed the use-legacy-cloud-provider branch from 767f21d to 5e10840 Compare October 10, 2019 17:40
Copy link
Contributor

@M00nF1sh M00nF1sh left a comment

Choose a reason for hiding this comment

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

I think this is an good starting point where we can iterate from :D

@M00nF1sh
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 10, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: M00nF1sh, nckturner

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 merged commit 8a0b2ca into kubernetes:master Oct 10, 2019
@geckofu
Copy link

geckofu commented Oct 11, 2019

Hi, I'm a newcomer and would like to contribute to this project. Where should I work on after this migration? In kubernetes' main repo staging area as the legacy-cloud-provider says?

@M00nF1sh
Copy link
Contributor

@geckofu
For now, it's still kubernetes's main repo's staging area(which will be synced to legacy-cloud-provider and consumed by this project).
In the future, we'll gradually migrate off legacy-cloud-provider dependency.

@kapilt
Copy link

kapilt commented Oct 16, 2019

i feel like the better approach would have been to go in the opposite direction, ie have the legacy cloud in k8s main repo depend on the code in this repo. as is it just erases most of the progress made in moving the provider out of tree.

@andrewsykim
Copy link
Member

@kapilt that's a good point but there's a few reasons for not importing this repo back from kubernetes/kubernetes:

  • We have less control over the dependencies that are pulled into this repo that would eventually get pulled into all the core Kubernetes components. The existing legacy repo is already contained and we ensure that dep graph doesn't grow.
  • It'll be harder to keep backwards compatibility if we are pulling in new changes here to core Kubernetes
  • It's much easier to iterate this repo from what already exists because the legacy repo passes Kubernetes pre-checks in the main repo and we know have more confidence that what we are supporting out-of-tree should work for most users transitioning from in-tree.

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. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. release-note-none Denotes a PR that doesn't merit a release note. 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.

6 participants