Configure broker relist behavior on a per-broker basis

[pmorie]

Created from #627; we should be able to configure the controller's relist behavior on a per-broker basis. I have requirements for:

1. Relist periodically (can be manually triggered)
2. Do not relist until manually triggered

The user should always be able to trigger a manual relist, see #704

[duglin]

Addition items from #812:

• ability to ask for certain brokers to be excluded from the auto-re-get
• do the re-get and reconcile the data

[pmorie]

I have folks at Red Hat that have volunteered to take this on. We spent some time today talking through a design that satisfies the known requirements for this feature, namely:

• Broker relist behavior should be configurable on a per-broker basis
• Brokers should be relisted on an interval or manually only
• There should be a way to manually trigger a relist for any broker

Here's our proposal:

API changes:

type BrokerRelistBehavior string

const (
  BrokerRelistBehaviorDuration BrokerRelistBehavior = "Duration"
  BrokerRelistBehaviorManual   BrokerRelistBehavior = "Manual"
)

type BrokerSpec struct {
  // other fields omitted
  RelistBehavior BrokerRelistBehavior
  RelistDuration *metav1.Duration
}

Defaults:

• RelistBehavior should default to BrokerRelistBehaviorDuration
• RelistDuration should default to 15m if the broker is refreshed on a duration

Validations:

• RelistBehavior should be a valid value
• RelistDuration must be a valid duration if behavior is to use a duration
• RelistDuration cannot be set unless behavior is to use a duration

Other changes:

• Fuzzer changes to account for the defaulting behavior
• Controller should change to consider the new spec fields to determine whether a broker is ready for relist instead of looking at the existing global config for the controller manager
• Existing controller manager option should become a no-op and should be removed at beta

Additionally, there should be:

• a new subresource (relist) for brokers that causes the broker to be relisted immediately by clearing the status and making the broker appear to require sync.
• a new porcelain command (relist-broker) to invoke the new subresource

I have outlined the basic API changes to provide a good example; @jmrodri will write up his proposal for the subresource and porcelain commands

Note:

This proposal represents an amount of work larger than is appropriate for a single PR. A good factoring of this work into PRs would be:

1. A PR that adds the new API fields, defaults, validations, controller behaviors
2. A PR that adds the new subresource
3. A PR that adds the new porcelain command

Also, all names are merely suggestions; better suggestions are welcome.

[duglin]

couple of questions:

• what's the trigger to do a manual relist?
• why not just have a "duration" field and when zero it means "manual"? Not sure we need two fields to convey what one can do.

[pmorie]

what's the trigger to do a manual relist?

The new relist subresource would result in the broker being relisted

why not just have a "duration" field and when zero it means "manual"? Not sure we need two fields to convey what one can do.

I would strongly prefer not to have magic values for the duration field. Without a magic value to indicate the manual-only behavior, in order to have a default for the duration, we need the behavior enum to disambiguate between 'use the default duration' and 'manual relist only'.

[pmorie]

Actually, even with the magic value, it's impossible to distinguish between 'set the default' and 'manual'

[duglin]

Are we not allow to do... no "duration" property == "use default". value of "0" means "manual"?

[jmrodri]

CLI proposal:

Add a relist-broker subcommand to the kubectl tool. The easiest way to
accomplish this is to piggy back on the plugin support being added by PR #840.
This change will also depend on the relist api proposed in this issue #705.

With PR #840 we could create a new relist-broker executable:

// Some pseudo code and boilerplate stuff
package main

...

const usage = `Usage:
  kubectl plugin relist-broker BROKER_NAME`

func main() {
    svcURL := utils.SCUrlEnv()
    if svcURL == "" {
        svcURL = "192.168.99.100:30080"
    }

    if len(os.Args) < 1 {
        utils.Exit1(usage)
    }
    ...
    restConfig := rest.Config{
        Host:    svcURL,
        APIPath: "/apis/servicecatalog.k8s.io/v1alpha1",
    }

    svcClient, err := clientset.NewForConfig(&restConfig)
    if err != nil {
        utils.Exit1(...)
    }

    // Relist the broker
    resp, err := svcClient.Brokers().Relist()
    if err != nil {
        utils.Exit1(...)
    }

    utils.Ok()
}

We will also need to update the Makefile with the above new utility.

• add a build target for the command

$(BINDIR)/relist-broker/relist-broker: \
		plugin/cmd/kubectl/relist-broker/relist-broker.go \
		plugin/cmd/kubectl/relist-broker/plugin.yaml
	rm -rf $(BINDIR)/relist-broker
	$(DOCKER_CMD) go build -o $@ $<
	$(DOCKER_CMD) cp plugin/cmd/kubectl/relist-broker/*yaml \
		$(BINDIR)/relist-broker/

• add command to the plugins target

plugins: $(BINDIR)/bind-service/bind-service \
			$(BINDIR)/create-service-broker/create-service-broker \
			$(BINDIR)/create-service-instance/create-service-instance \
			$(BINDIR)/relist-broker/relist-broker

The relist-broker utility will also need a plugin.yaml file defined:

name: "relist-broker"
shortDesc: "This command relists the service instances"
command: "./relist-broker"

[jmrodri]

Subresource proposal:

Add a relist subresource for brokers that causes it to be relisted
immediately. It will accomplish this by clearing the status and making
the broker appear to require sync.

First, in pkg/registry/servicecatalog/rest/storage_servicecatalog.go we will add the
relist storage type similar to status.

// other entries omitted
brokerStorage, brokerStatusStorage := broker.NewStorage(*brokerOpts)
brokerRelistStorage := broker.NewStorage(*brokerOpts)

Then add it to the return map.

return map[string]rest.Storage{
// other entries omitted
"brokers/status": brokerStatusStorage,
"brokers/relist": brokerRelistStorage, // NEW ENTRY
...

Since we updated the storage_servicecatalog, we'll want to update the
TestV1Alpha1Storage in storage_servicecatalog_test.go to include
brokerRelistStorage.

The last bit should be to update the broker strategy located in
broker/strategy.go. Add a brokerRelistRESTStrategy struct similar to
the existing brokerStatusRESTStrategy struct.

I believe that's all that needs to get done. But there might be some unknowns in
this section that might result in more changes.

[nilebox]

That might sound a bit hacky, but isn't just removing the status from Broker resource enough to manually force relisting?
Sounds much simpler than adding specific subresources.

[pmorie]

That might sound a bit hacky, but isn't just removing the status from Broker resource enough to manually force relisting?

Users normally don't have the ability to clear status themselves.

I have a solution which doesn't rely on destroying status or creating a subresource which I described in the meeting last thursday, but haven't had a chance to write up yet. TLDR: add a field to BrokerSpec called RelistRequests that is an incrementing counter. When the user needs to do a manual relist, they can bump the value of this field.

No subresource is required to do this, and it fits entirely within kube precedent.

[duglin]

Is there precedence in kube for defining a URL/route/sub-resource??? that represents a function? I'm asking because:

PUT .../myBroker/relist

is a lot easier than:

GET .../myBroker
myBroker.RelistRequests++
PUT .../myBroker

[MHBauer]

Broker has a field that defaults and a manual relist endpoint.