[EnMasse] REST API changes continued...

[Ulf Lilleengen]

Hi,

After some discussions with Rob and Gordon yesterday, I think there is 
an agreement that we can go with (2) and leave (1) on the drawing table.

For completeness, however, I'd like to hear your opinions on an 
alternative approach for (1), which I think would give a reasonable 
compromise in the strimzi integration, and remove at least 2447 lines of 
java code (and possible even more since the api server does not support 
watches etc. at present).

If we define a custom resource definition for the address space 
definition with inlined addresses, we could remove the api-server 
alltogether. The reason I think this is more attractive is that there 
are a lot of features provided out-of-the-box like watches, json-patch, 
merge-patch, builtin authn and authz, finalizers etc...(see "Common 
Features" in [1])

For the strimzi case, the address-space-controller would have to 
create/delete the strimzi configmaps for the topics based on the address 
space definition.

For brokered and standard, it could temporarily write the address 
configmap until agent/console/standard-controller is using the address 
space crd.

Some questions:

a) how well does a CRD perform? Is it ok to have 100k addresses in a 
single CRD?
b) it is ok to loose the ability to choose the underlying persistence 
for the config?
c) can we live without the ability to delete single addresses using json 
patch?
d) for strimzi, is it important that we reflect changes to the strimzi 
configmaps back into the address space definition?

The reason I like this approach is that we can remove a component from 
our system in addition to the reduced complexity in agent. So to me, the 
disadvantages of not being able to delete a single address using json 
patch is outweighed by the huge reduction in complexity.

[1] 
https://kubernetes.io/docs/concepts/api-extension/custom-resources/#customresourcedefinitions

Best regards,

Ulf

On 05/15/2018 12:03 PM, Ulf Lilleengen wrote:
> Hi all,
> 
> The initial work on the REST API to enable CRD support for address 
> spaces will soon be merged.
> 
> I'd like to continue the discussion around addresses and CRDs. The goals 
> (I think) are to "Allow users to create/get/update/delete addresses 
> using oc or kubectl".
> 
> So far, there have been 2 proposals from the community, not mutually 
> exclusive. I have most comments around the first proposal though.
> 
> 1) Add address definitions to the address space definition. I'll call 
> this 'inlined addresses'. One example of that is:
> 
> apiVersion: enmasse.io/v1
> kind: AddressSpace
> metadata:
>     name: myspace
> spec:
>     type: standard
>     plan: small-standard
>     addresses:
>       - address: myqueue
>         type: queue
>         plan: small-queue
>       - ...
> status:
>     isReady: true
>     addresses:
>       - address: myqueue
>         phase: Configuring
>         isReady: false
>         messages:
>           - "Autolink not present on router"
>           - "Address not present on router"
>       - ...
> 
> I have a few concerns with our current implementations ability to 
> support both per-address APIs and inlined addresses.
> 
> Today, concurrent modifications to addresses and address spaces are 
> handled by using a resourceVersion (which reflects the resource version 
> in the configmap) in the metadata of the resource (in the same way as 
> other k8s resources). If 2 clients update a resource at the same time, 
> one of them will fail and should retry.
> 
> If we expose both the per-address REST API and alternative (2) to modify 
> individual addresses, _and_ allowing the same addresses to be changed 
> through the address space definition, how do we prevent one client to 
> overwrite the changes made by another?
> 
> Moreover, if a client wants to watch an address space for changes, if we 
> continue to use 1 configmap per address, then the api server will have 
> to handle 1 watch for the address space and 1 for the addresses which 
> complicates the implementation (although should be doable).
> 
> To me, the easiest solution to the above questions is changing the 
> implementation to store everything in the same configmap, and then use 
> the resourceVersion of the address space configmap in the per-address 
> resource definition exposed in the REST API and in (2). I think this 
> would work for the standard address space type (and brokered address 
> space type as long as we create addresses 'via' the configmap and not 
> directly in the broker).
> 
> However, integrating with strimzi will be harder the way strimzi works 
> today. For instance, the logical way to integrate with strimzi would be 
> to have the REST API proxy requests to the Strimzi API. I.e. when 
> creating an address for the 'kafka' address space, it would (until 
> strimzi supports custom resources) create a configmap per address that 
> the strimzi topic-controller would use to configure kafka. The same 
> issues as raised above with the combination of 'inline' addresses with 
> per-address configmaps would be the same.
> 
> Inlined addresses seems to set some limitations on the granularity of 
> how we persist the data and also requires the controllers for different 
> address spaces to use the enmasse API for reading address information. 
> Storing all address information in a single object feels wrong to me, 
> and I'm a little worried about how this will scale.
> 
> If the goals is that the user should be able to create addresses 
> declaratively, I think that is satisfied with (2). However, if the 
> majority still thinks the address inlining is the way forward, and we 
> can find a good way to integrate with strimzi, I won't stand in the way 
> for it.
> 
> 
> 
> 2) Introduce a top-level API for modifying addresses (for CRDs)
> 
> The idea is to support the following path for 
> creating/reading/updating/deleting addresses: 
> /apis/enmasse.io/v1/namespaces/[:namespace]/addresses
> 
> To ensure that addresses from different address spaces do not conflict, 
> the proposal is to prefix the address name with the address space name 
> and introduce a separator character which cannot be part of the address 
> space name.
> 
> One proposal is to use the '.' as the separator. This would cause the 
> address definition that is created to be:
> 
> apiVersion: enmasse.io/v1
> kind: Address
> metadata:
>      name: myspace.addr1
> spec:
>      type: queue
>      plan: small-queue
>      address: addr_1
> 
> When using the command line tool, the address would show up as:
> 
> $ oc get addresses
> NAME               AGE
> myspace.addr1      12s
> myspace.addr2      11s
> otherspace.addr1   6s
> 
> Should we remove the currently required addressSpace field from metadata 
> as in the above example?
> 
> Other concerns? I think this proposal can be quickly implemented as is.
> 
> 
> Best regards,
> 
> Ulf
> 

-- 
Ulf