Skip to content
This repository has been archived by the owner on May 6, 2022. It is now read-only.

Add ServiceBroker controller loop #1993

Merged
merged 1 commit into from
May 16, 2018

Conversation

eriknelson
Copy link
Contributor

@eriknelson eriknelson commented Apr 26, 2018

Depends on: #1911 & #1988

This PR adds the namespaced ServiceBroker control loop, which will request a broker's catalog and reconcile namespaced ServiceClasses and ServicePlans. Includes required changes to existing controller funcs and some basic initial unit test support.

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 26, 2018
@@ -66,7 +66,7 @@ We can check the status of the broker:
$ svcat describe broker ups-broker
Name: ups-broker
URL: http://ups-broker-ups-broker.ups-broker.svc.cluster.local
Status: Ready - Successfully fetched catalog entries from broker @ 2018-03-02 16:03:52 +0000 UTC
Status: Ready - Successfully fetched cluster catalog entries from broker @ 2018-03-02 16:03:52 +0000 UTC
Copy link
Contributor

Choose a reason for hiding this comment

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

This appears to have some of the "cluster catalog" stuff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ACK, this is based off an earlier cut of #1911 which should have those fixes. Planning on rebasing this once that PR makes it to master. Will be sure to catch up with you re: the other constant changes and make sure I'm not changing anything that shouldn't.


serviceClasses := []*v1beta1.ServiceClass(nil)
servicePlans := []*v1beta1.ServicePlan(nil)
for _, svc := range in.Services {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is def one area we can simplify in a follow up. I don't necessarily it's worth going to the effort everywhere, but certainly here it is.

errorDeletingClusterServicePlanReason string = "ErrorDeletingServicePlan"
errorDeletingClusterServicePlanMessage string = "Error deleting service plan."
errorListingClusterServiceClassesReason string = "ErrorListingClusterServiceClasses"
errorListingClusterServiceClassesMessage string = "Error listing cluster service classes."
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm really not certain about the constant changes :-/

@@ -0,0 +1,790 @@
/*
Copyright 2017 The Kubernetes Authors.
Copy link
Contributor

Choose a reason for hiding this comment

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

2018

@@ -0,0 +1,1322 @@
/*
Copyright 2017 The Kubernetes Authors.
Copy link
Contributor

Choose a reason for hiding this comment

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

2018

@pmorie
Copy link
Contributor

pmorie commented Apr 27, 2018

Looks like a good start! Very excited to see this progressing.

@eriknelson eriknelson force-pushed the sb-controller branch 3 times, most recently from 0e494d7 to 72a2d57 Compare April 29, 2018 21:46
@eriknelson eriknelson changed the title WIP: Add ServiceBroker controller Add ServiceBroker controller loop Apr 29, 2018
@eriknelson eriknelson force-pushed the sb-controller branch 4 times, most recently from 9483f17 to 5196499 Compare April 30, 2018 19:01
@pmorie
Copy link
Contributor

pmorie commented May 6, 2018

CI failure looks legit:

    --- FAIL: TestCreateServiceInstanceWithProvisionFailure/Status_Created (9.62s)
    	framework.go:111: Server started on port 44690
    	framework.go:142: Test client will use API Server URL of https://localhost:44690
    	controller_test.go:790: controller start
    	controller_test.go:1426: unexpected action type: expected ProvisionInstance, got DeprovisionInstance
    	framework.go:116: Shutting down server on port: 44690

@pmorie
Copy link
Contributor

pmorie commented May 9, 2018

@eriknelson this is still failing CI. Will you take a look? I'd like to get this merged.

@eriknelson
Copy link
Contributor Author

eriknelson commented May 9, 2018 via email

Adds the following:

* Extends controller to support namespaced types
* ServiceBroker control loop
* Initial unit test support
@eriknelson
Copy link
Contributor Author

@pmorie think it just needed a rebase

@n3wscott
Copy link
Contributor

oh man, this is huge...

LGTM, I think we will have to do some refactoring to allow the same logic to apply to the namespaced and non-namespaced resources later. It is going to be a pain in the butt to make sure we keep the loops the same...

@n3wscott n3wscott added the LGTM1 label May 14, 2018
@eriknelson
Copy link
Contributor Author

@n3wscott +1, yeah I've been doing my best to try to keep these PRs as tight as possible. I think this will be one of the bigger ones, apart from updating the ServiceInstance and ServiceBinding loops to be namespaced type aware.

@pmorie
Copy link
Contributor

pmorie commented May 16, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 16, 2018
@pmorie
Copy link
Contributor

pmorie commented May 16, 2018

oops, forgot we don't use those here... sigh

@pmorie pmorie added the LGTM2 label May 16, 2018
@pmorie pmorie merged commit 6a693a7 into kubernetes-retired:master May 16, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm Indicates that a PR is ready to be merged. LGTM1 LGTM2 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.

5 participants