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

simulate in-tree cloud provider removal with a build tag #80353

Merged
merged 5 commits into from Aug 23, 2019
Merged

simulate in-tree cloud provider removal with a build tag #80353

merged 5 commits into from Aug 23, 2019

Conversation

BenTheElder
Copy link
Member

@BenTheElder BenTheElder commented Jul 19, 2019 •
edited

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test

/kind feature

/kind flake

What this PR does / why we need it: This PR is a first step towards making it possible to build Kubernetes without the in-tree providers, in preparation for their eventual removal.

This can be used to ensure that out of tree providers function without the in-tree provider logic and will be ready to remove the in-tree version. It also produces lighter binaries, which should incentivize the work to move out of tree.

Usage: set GOFLAGS='-tags=nolegacyproviders' when building with make, for example: GOFLAGS='-tags=nolegacyproviders' make quick-release-images.

How this works:

We add build constraints for a synthetic nolegacyproviders build tag to the relevant sources. By default this tag isn't defined, so nothing changes functionally. If it is defined at compile time then the cloud provider files won't build, and some stub methods will build where necessary. Effectively this lets us "delete" the cloud provider code with a build flag, prior to the actual deletion.

To make this work I had to add stub "doc.go" files (without any constraints on them) to various packages so they will build. Go will complain about packages with no matching source files.

Note that the bazel build does not support this. It will still build as before, but you cannot build the "nolegacyproviders" mode as gazelle doesn't support custom build tags / constraints.

Incomplete Work:

This does not touch credential provider logic, which also imports cloud provider code. I'm sure there are other places. Currently it touches:

  • The actual legacy cloud provider packages and their importation in kube-controller-manager
  • IPAM (there is a cloud provider GCE mode)
  • In-Tree Volume Probing (that are cloud related, eventually CSI migration should cover this?)

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

^ possibly this deserves one? I think it's more likely to deserve one if we release anything built in this mode.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

/sig release
/sig cloud-provider

@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/feature Categorizes issue or PR as related to a new feature. sig/release Categorizes an issue or PR as relevant to SIG Release. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jul 19, 2019
@k8s-ci-robot k8s-ci-robot added area/cloudprovider area/kubelet sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/storage Categorizes an issue or PR as relevant to SIG Storage. labels Jul 19, 2019
@BenTheElder
Copy link
Member Author

/hold

Mostly opened this PR to share the prototype at the moment, I could see this requiring a KEP or some form of further discussion first. This idea has been discussed in #sig-cloud-provider and directly with a few interested parties.

cc @cheftako @dims @justaugustus

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 19, 2019
@BenTheElder
Copy link
Member Author

BenTheElder commented Jul 19, 2019 •
edited

kube-controller-manager kube-apiserver both panic in this mode due to the cloud-provider-gce-lb-src-cidrs global flag :/ I thought we'd patched that already. will follow up on that tomorrow my time. the rest of the approach still holds though.

this was fixed in an additional commit

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 19, 2019
@BenTheElder
Copy link
Member Author

discussed offline with @cheftako, opted to ignore credential providers for now and stub / compile out registration of the deprecated gce specific flags.

@BenTheElder
Copy link
Member Author

/priority important-longterm

@k8s-ci-robot k8s-ci-robot added the priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. label Jul 19, 2019
@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 22, 2019
@BenTheElder
Copy link
Member Author

@andrewsykim I switched to providerless 👍
I've also rebased the PR.

@BenTheElder
Copy link
Member Author

If we do drift, the default build should still work so the only risk is developers using this special build might have to fix it, I'm not sure if that should be a blocker to initially implementing the tag. I do think we should have it though.

It's also WAI if this mode builds but produces a non-functioning cluster.

lavalamp
lavalamp reviewed Aug 22, 2019
View reviewed changes
cmd/kube-apiserver/app/options/globalflags_legacyproviders.go Outdated
@@ -0,0 +1,30 @@
// +build !providerless
Copy link
Member

Choose a reason for hiding this comment

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

build tag and file name don't match any more...

Copy link
Member Author

Choose a reason for hiding this comment

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

ACK

lavalamp
lavalamp reviewed Aug 22, 2019
View reviewed changes
cmd/kube-apiserver/app/options/globalflags_nolegacyproviders.go Outdated
@@ -0,0 +1,27 @@
// +build providerless
Copy link
Member

Choose a reason for hiding this comment

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

OTOH "noproviderless" is going to be an unfortunate file name :/

Copy link
Member Author

Choose a reason for hiding this comment

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

providers / providerless?

I can rework the PR, may take a bit 😬

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, sorry, it just kind of jumped out at me, I don't have any good suggestions. "providerlessasinnoproviders" and "providerlessasinserverless"

lavalamp
lavalamp reviewed Aug 22, 2019
View reviewed changes
cmd/kubelet/app/plugins.go
allPlugins = append(allPlugins, configmap.ProbeVolumePlugins()...)
allPlugins = append(allPlugins, vsphere_volume.ProbeVolumePlugins()...)
allPlugins = append(allPlugins, azure_dd.ProbeVolumePlugins()...)
allPlugins = append(allPlugins, projected.ProbeVolumePlugins()...)
allPlugins = append(allPlugins, portworx.ProbeVolumePlugins()...)
Copy link
Member

Choose a reason for hiding this comment

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

There's a lot of items still in this list that I'm not convinced should stay?

@lavalamp
Copy link
Member

ping me when this is ready to go. I have read through it and willing to approve it. 👍

@lavalamp
Copy link
Member

/approve
/lgtm

I think this is pretty safe, we won't be breaking the main build.

As for the packages rotting over time, I'm content that this is low-impact enough and Ben is going to continue to work on this, that I think it's fine to lock in this much progress.

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewsykim, BenTheElder, lavalamp

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 Aug 22, 2019
@BenTheElder
Copy link
Member Author

/hold cancel

@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 Aug 23, 2019
@BenTheElder
Copy link
Member Author

/test all

@dims
Copy link
Member

dims commented Aug 23, 2019

/retest

@k8s-ci-robot k8s-ci-robot merged commit e232921 into kubernetes:master Aug 23, 2019
SIG Release automation moved this from In progress to Done (1.16) Aug 23, 2019
@k8s-ci-robot k8s-ci-robot added this to the v1.16 milestone Aug 23, 2019
@BenTheElder BenTheElder deleted the tags branch August 24, 2019 04:17
This was referenced Aug 26, 2019
Closed
Merged
Merged
@cheftako cheftako mentioned this pull request Apr 27, 2021
Merged
@cici37 cici37 mentioned this pull request Apr 27, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lavalamp lavalamp lavalamp left review comments

@liggitt liggitt liggitt left review comments

@davidz627 davidz627 davidz627 left review comments

@andrewsykim andrewsykim andrewsykim left review comments

@ddebroy ddebroy ddebroy left review comments

@aveshagarwal aveshagarwal Awaiting requested review from aveshagarwal

Assignees

@lavalamp lavalamp

@cheftako cheftako

@liggitt liggitt

@andrewsykim andrewsykim

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/cloudprovider area/kubelet cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. 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. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. release-note-none Denotes a PR that doesn't merit a release note. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/release Categorizes an issue or PR as relevant to SIG Release. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v1.16
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@BenTheElder @fedebongio @dims @andrewsykim @liggitt @lavalamp @k8s-ci-robot @davidz627 @ddebroy @cheftako