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

pass in stopCh to cloud provider Initialize method for custom controllers #70038

Merged
merged 2 commits into from
Oct 20, 2018
Merged

pass in stopCh to cloud provider Initialize method for custom controllers #70038

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2908174
pass in stopCh to cloud provider Initialize method for custom control…
andrewsykim Oct 19, 2018
3521ebd
pass in stopCh for GCE cluster ID controller
andrewsykim Oct 19, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/cloud-controller-manager/app/controllermanager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ func startControllers(c *cloudcontrollerconfig.CompletedConfig, stop <-chan stru
}
if cloud != nil {
// Initialize the cloud provider with a reference to the clientBuilder
cloud.Initialize(c.ClientBuilder)
cloud.Initialize(c.ClientBuilder, stop)
}
// Start the CloudNodeController
nodeController := cloudcontrollers.NewCloudNodeController(
Expand Down
2 changes: 1 addition & 1 deletion cmd/kube-controller-manager/app/controllermanager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -468,7 +468,7 @@ func StartControllers(ctx ControllerContext, startSATokenController InitFunc, co
// Initialize the cloud provider with a reference to the clientBuilder only after token controller
// has started in case the cloud provider uses the client builder.
if ctx.Cloud != nil {
ctx.Cloud.Initialize(ctx.ClientBuilder)
ctx.Cloud.Initialize(ctx.ClientBuilder, ctx.Stop)
}

for controllerName, initFn := range controllers {
Expand Down
2 changes: 1 addition & 1 deletion pkg/cloudprovider/providers/aws/aws.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1159,7 +1159,7 @@ func newAWSCloud(cfg CloudConfig, awsServices Services) (*Cloud, error) {
}

// Initialize passes a Kubernetes clientBuilder interface to the cloud provider
func (c *Cloud) Initialize(clientBuilder cloudprovider.ControllerClientBuilder) {
func (c *Cloud) Initialize(clientBuilder cloudprovider.ControllerClientBuilder, stop <-chan struct{}) {
c.clientBuilder = clientBuilder
c.kubeClient = clientBuilder.ClientOrDie("aws-cloud-provider")
c.eventBroadcaster = record.NewBroadcaster()
Expand Down
2 changes: 1 addition & 1 deletion pkg/cloudprovider/providers/azure/azure.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,7 +390,7 @@ func parseConfig(configReader io.Reader) (*Config, error) {
}

// Initialize passes a Kubernetes clientBuilder interface to the cloud provider
func (az *Cloud) Initialize(clientBuilder cloudprovider.ControllerClientBuilder) {
func (az *Cloud) Initialize(clientBuilder cloudprovider.ControllerClientBuilder, stop <-chan struct{}) {
az.kubeClient = clientBuilder.ClientOrDie("azure-cloud-provider")
az.eventBroadcaster = record.NewBroadcaster()
az.eventBroadcaster.StartRecordingToSink(&v1core.EventSinkImpl{Interface: az.kubeClient.CoreV1().Events("")})
Expand Down
3 changes: 2 additions & 1 deletion pkg/cloudprovider/providers/cloudstack/cloudstack.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,8 @@ func newCSCloud(cfg *CSConfig) (*CSCloud, error) {
}

// Initialize passes a Kubernetes clientBuilder interface to the cloud provider
func (cs *CSCloud) Initialize(clientBuilder cloudprovider.ControllerClientBuilder) {}
func (cs *CSCloud) Initialize(clientBuilder cloudprovider.ControllerClientBuilder, stop <-chan struct{}) {
}

// LoadBalancer returns an implementation of LoadBalancer for CloudStack.
func (cs *CSCloud) LoadBalancer() (cloudprovider.LoadBalancer, bool) {
Expand Down
3 changes: 2 additions & 1 deletion pkg/cloudprovider/providers/fake/fake.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,8 @@ func (f *FakeCloud) ClearCalls() {
}

// Initialize passes a Kubernetes clientBuilder interface to the cloud provider
func (f *FakeCloud) Initialize(clientBuilder cloudprovider.ControllerClientBuilder) {}
func (f *FakeCloud) Initialize(clientBuilder cloudprovider.ControllerClientBuilder, stop <-chan struct{}) {
}

func (f *FakeCloud) ListClusters(ctx context.Context) ([]string, error) {
return f.ClusterList, f.Err
Expand Down
4 changes: 2 additions & 2 deletions pkg/cloudprovider/providers/gce/gce.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -610,7 +610,7 @@ func tryConvertToProjectNames(configProject, configNetworkProject string, servic

// Initialize takes in a clientBuilder and spawns a goroutine for watching the clusterid configmap.
// This must be called before utilizing the funcs of gce.ClusterID
func (gce *GCECloud) Initialize(clientBuilder cloudprovider.ControllerClientBuilder) {
func (gce *GCECloud) Initialize(clientBuilder cloudprovider.ControllerClientBuilder, stop <-chan struct{}) {
gce.clientBuilder = clientBuilder
gce.client = clientBuilder.ClientOrDie("cloud-provider")

Expand All @@ -620,7 +620,7 @@ func (gce *GCECloud) Initialize(clientBuilder cloudprovider.ControllerClientBuil
gce.eventRecorder = gce.eventBroadcaster.NewRecorder(scheme.Scheme, v1.EventSource{Component: "gce-cloudprovider"})
}

go gce.watchClusterID()
go gce.watchClusterID(stop)
}

// LoadBalancer returns an implementation of LoadBalancer for Google Compute Engine.
Expand Down
4 changes: 2 additions & 2 deletions pkg/cloudprovider/providers/gce/gce_clusterid.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ type ClusterID struct {
}

// Continually watches for changes to the cluster id config map
func (gce *GCECloud) watchClusterID() {
func (gce *GCECloud) watchClusterID(stop <-chan struct{}) {
gce.ClusterID = ClusterID{
cfgMapKey: fmt.Sprintf("%v/%v", UIDNamespace, UIDConfigMapName),
client: gce.client,
Expand Down Expand Up @@ -105,7 +105,7 @@ func (gce *GCECloud) watchClusterID() {
var controller cache.Controller
gce.ClusterID.store, controller = cache.NewInformer(newSingleObjectListerWatcher(listerWatcher, UIDConfigMapName), &v1.ConfigMap{}, updateFuncFrequency, mapEventHandler)

controller.Run(nil)
controller.Run(stop)
Copy link
Member

@cheftako cheftako Oct 19, 2018 •
edited

Choose a reason for hiding this comment

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

Wondering if it would be good to make this change in a separate PR. I believe we have other such fixes which need to be made. Might be better to not conflate the Interface change with actual fixes which may have unexpected side effects.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can at least do separate commit

Copy link
Member Author

Choose a reason for hiding this comment

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

done, PTAL :)

Copy link
Member

Choose a reason for hiding this comment

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

Looks good. I think there is no way the other change will need to be rolled back. This change is unlikely but possible. Was thinking it might be good (optional) to make them separate PRs on the off change this change needed to be rolled back.

}

// GetID returns the id which is unique to this cluster
Expand Down
3 changes: 2 additions & 1 deletion pkg/cloudprovider/providers/openstack/openstack.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -367,7 +367,8 @@ func newOpenStack(cfg Config) (*OpenStack, error) {
}

// Initialize passes a Kubernetes clientBuilder interface to the cloud provider
func (os *OpenStack) Initialize(clientBuilder cloudprovider.ControllerClientBuilder) {}
func (os *OpenStack) Initialize(clientBuilder cloudprovider.ControllerClientBuilder, stop <-chan struct{}) {
}

// mapNodeNameToServerName maps a k8s NodeName to an OpenStack Server Name
// This is a simple string cast.
Expand Down
3 changes: 2 additions & 1 deletion pkg/cloudprovider/providers/ovirt/ovirt.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,8 @@ func newOVirtCloud(config io.Reader) (*OVirtCloud, error) {
}

// Initialize passes a Kubernetes clientBuilder interface to the cloud provider
func (v *OVirtCloud) Initialize(clientBuilder cloudprovider.ControllerClientBuilder) {}
func (v *OVirtCloud) Initialize(clientBuilder cloudprovider.ControllerClientBuilder, stop <-chan struct{}) {
}

func (v *OVirtCloud) Clusters() (cloudprovider.Clusters, bool) {
return nil, false
Expand Down
3 changes: 2 additions & 1 deletion pkg/cloudprovider/providers/photon/photon.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,8 @@ func newPCCloud(cfg PCConfig) (*PCCloud, error) {
}

// Initialize passes a Kubernetes clientBuilder interface to the cloud provider
func (pc *PCCloud) Initialize(clientBuilder cloudprovider.ControllerClientBuilder) {}
func (pc *PCCloud) Initialize(clientBuilder cloudprovider.ControllerClientBuilder, stop <-chan struct{}) {
}

// Instances returns an implementation of Instances for Photon Controller.
func (pc *PCCloud) Instances() (cloudprovider.Instances, bool) {
Expand Down
2 changes: 1 addition & 1 deletion pkg/cloudprovider/providers/vsphere/vsphere.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ func init() {
}

// Initialize passes a Kubernetes clientBuilder interface to the cloud provider
func (vs *VSphere) Initialize(clientBuilder cloudprovider.ControllerClientBuilder) {
func (vs *VSphere) Initialize(clientBuilder cloudprovider.ControllerClientBuilder, stop <-chan struct{}) {
}

// Initialize Node Informers
Expand Down
5 changes: 3 additions & 2 deletions staging/src/k8s.io/cloud-provider/cloud.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,9 @@ type ControllerClientBuilder interface {
// Interface is an abstract, pluggable interface for cloud providers.
type Interface interface {
// Initialize provides the cloud with a kubernetes client builder and may spawn goroutines
// to perform housekeeping activities within the cloud provider.
Initialize(clientBuilder ControllerClientBuilder)
// to perform housekeeping or run custom controllers specific to the cloud provider.
// Any tasks started here should be cleaned up when the stop channel closes.
Initialize(clientBuilder ControllerClientBuilder, stop <-chan struct{})
Copy link
Member

Choose a reason for hiding this comment

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

This breaks go compatibility, is that OK? I think it won't be OK once there's out of tree implementors?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a discussion we (SIG Cloud Provider) are having right now. There are some differing opinions around if breaking the interface is okay as long as the behaviour is not changed. In my opinion at least, this is fine because the SIG that defines the interface works closely with the implementors and we can form consensus on breaking changes like this one before moving forward. But you are correct in that we should have a definitive answer for this when we have stable out-of-tree providers. For now we are incrementally trying to put the interface at a better spot while we are still "beta" so breaking changes like this are OK in my opinion (but feel free to disagree).

// LoadBalancer returns a balancer interface. Also returns true if the interface is supported, false otherwise.
LoadBalancer() (LoadBalancer, bool)
// Instances returns an instances interface. Also returns true if the interface is supported, false otherwise.
Expand Down