brooklyn-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Aled Sage <aled.s...@gmail.com>
Subject Re: Proposal: Add appId optional paramater to deploy api
Date Wed, 26 Jul 2017 09:46:33 GMT
// apologies this ended up so long!

Hi Alex, Svet, all,

Interesting suggestion by Alex - sounds useful. However, even with that 
it would require implementing a big chunk of logic in the upstream system.

I keep coming back to thinking that `appId=...` gives the simplest 
semantics for such systems.

---
The caching of `X-Request-ID` approach doesn't work in the scenario when 
the Brooklyn process has restarted, or has failed over in HA mode (I'm 
presuming you don't intend to write them to persisted state).

We'd therefore still end up with an upstream implementation as though 
`X-Request-ID` wasn't supported.

We'd still want either `appId=...` or `deploymentUid=...` so that we can 
subsequently query to find if there is a given orphaned application.

It would be nice if the query was simple/efficient. That's easy if using 
appId, but fiddly to search for an app with a given `deploymentUid` 
config value or tag (I think currently it would require listing all 
apps, then querying each app for this config key; or changing brooklyn 
to expand the rest api; or running an additional custom web-app for such 
bespoke queries).

---
I think it's fine/sensible to focus on the specific problem of 
deploy-app, because that is the current use-case.

Separately, having a general solution that works in some situations is a 
nice addition, but feels like a lower priority for our use-case.

---
I presume if implementing the `X-Request-ID` cache:

  * The cache would be transient (i.e. not persisted for restart/HA).
  * It would record everything with side-effects (POST/PUT/DELETE, but
    not GET/HEAD).
  * We'd record it as soon as the request was received (rather than
    waiting for the request to complete).
  * We'd return a 409 (conflict) response if we received a duplicate.
  * It would be configurable to turn this feature on/off (defaulting to on).
  * The time for keeping these request-ids would be configurable,
    defaulting to something like an hour.

Assuming folk agree it's separate, then let's treat that as a separate 
proposal / email thread.

---

Mark has pointed out that strictly speaking we shouldn't be using query 
parameters with POST requests.

The RESTful way of doing it if supplying a custom app id would be:

     `PUT /applications/<custom-app-id>`

It's not so obvious how best to supply a deploymentUid in the url.

---

To Svet's point of using a tag (if going for the `deploymentUid` 
approach), good point - agree that would probably be better. We'd 
probably add it as the string `org.apache.brooklyn.deploymentUid=<val>`.

It would have been nicer (in this use-case and various others) if tags 
was a map rather than a list, but we could live with adding the uid to 
the list.

---

p.s. To again summarise the requirements/thinking: we want to make it 
easier for upstream systems to use Brooklyn robustly. We want to more 
easily/safely handle retries, and to be able to easily query for and/or 
discover orphaned apps (where a request got through, but response was 
lost). Our primary use-case in the upsteram system is deploying apps. We 
don't want to modify the user's yaml files.

Aled



On 26/07/2017 10:16, Svetoslav Neykov wrote:
> I think both proposals make sense. Even if implemented separately.
>
> Wondering what's the right place to put the ID though. Isn't the tags a better place
for that?  I suppose it depends on whether we want YAML blueprints to have access to it.
>
> Svet.
>
>
>> On 25.07.2017 г., at 21:56, Alex Heneveld <alex.heneveld@cloudsoftcorp.com>
wrote:
>>
>> Aled-
>>
>> Should this be applicable to all POST/DELETE calls?  Imagine an
>> `X-caller-request-uid` and a filter which caches them server side for a
>> short period of time, blocking duplicates.
>>
>> Solves an overlapping set of problems.  Your way deals with a
>> "deploy-if-not-present" much later in time.
>>
>> --A
>>
>> On 25 July 2017 at 17:44, Aled Sage <aled.sage@gmail.com> wrote:
>>
>>> Hi all,
>>>
>>> I've been exploring adding support for `&deploymentUid=...` - please see
>>> my work-in-progress PR [1].
>>>
>>> Do people think that is a better or worse direction than supporting
>>> `&appId=...` (which would likely be simpler code, but exposes the Brooklyn
>>> internals more).
>>>
>>> For `&appId=...`, we could either revert [2] (so we could set the id in
>>> the EntitySpec), or we could inject it via a different (i.e. add a new)
>>> internal way so that it isn't exposedon our Java api classes.
>>>
>>> Thoughts?
>>>
>>> Aled
>>>
>>> [1] https://github.com/apache/brooklyn-server/pull/778
>>>
>>> [2] https://github.com/apache/brooklyn-server/pull/687/commits/5
>>> 5eb11fa91e9091447d56bb45116ccc3dc6009df
>>>
>>>
>>>
>>> On 07/07/2017 18:28, Aled Sage wrote:
>>>
>>>> Hi,
>>>>
>>>> Taking a step back to justify why this kind of thing is really
>>>> important...
>>>>
>>>> This has come up because we want to call Brooklyn in a robust way from
>>>> another system, and to handle a whole load of failure scenarios (e.g. that
>>>> Brooklyn is temporarily down, connection fails at some point during the
>>>> communication, the client in the other system goes down and another
>>>> instance tries to pick up where it left off, etc).
>>>>
>>>> Those kind of thing becomes much easier if you can make certain
>>>> assumptions such as an API call being idempotent, or it guaranteeing to
>>>> fail with a given error if that exact request has already been accepted.
>>>>
>>>> ---
>>>>
>>>> I much prefer the semantics of the call failing (with a meaningful error)
>>>> if the app already exists - that will make retry a lot easier to do safely.
>>>>
>>>> As for config keys on the app, in Duncan's use-case he'd much prefer to
>>>> not mess with the user's YAML (e.g. to inject another config key before
>>>> passing it to Brooklyn). It would be simpler in his case to supply in the
>>>> url `?appId=...` or `?deploymentId=...`.
>>>>
>>>> For using `deploymentId`, we could but that feels like more work. We'd
>>>> want create a lookup of applications indexed by `deploymentId` as well as
>>>> `appId`, and to fail if it already exists. Also, what if someone also
>>>> defines a config key called `deploymentId` - would that be forbidden? Or
>>>> would we name-space the config key with `org.apache.brooklyn.deploymentId`?
>>>> Even with those concerns, I could be persuaded of the
>>>> `org.apache.brooklyn.deploymentId` approach.
>>>>
>>>> For "/application's ID is not meant to be user-supplied/", that has
>>>> historically been the case but why should we stick to that? What matters
is
>>>> that the appId is definitely unique. That will be checked when creating the
>>>> application entity. We could also include a regex check on the supplied id
>>>> to make sure it looks reasonable (in case someone is already relying on app
>>>> ids in weird ways like for filename generations, which would lead to a risk
>>>> of script injection).
>>>>
>>>> Aled
>>>>
>>>>
>>>> On 07/07/2017 17:38, Svetoslav Neykov wrote:
>>>>
>>>>> Hi Duncan,
>>>>>
>>>>> I've solved this problem before by adding a caller generated config key
>>>>> on the app (now it's also possible to tag them), then iterating over
the
>>>>> deployed apps, looking for the key.
>>>>>
>>>>> An alternative which I'd like to mention is creating an async deploy
>>>>> operation which immediately returns an ID generated by Brooklyn. There's
>>>>> still a window where the client connection could fail though, however
small
>>>>> it is, so it doesn't fully solve your use case.
>>>>>
>>>>> Your use case sounds reasonable so agree a solution to it would be nice
>>>>> to have.
>>>>>
>>>>> Svet.
>>>>>
>>>>>
>>>>> On 7.07.2017 г., at 18:33, Duncan Grant <duncan.grant@cloudsoftcorp.com>
>>>>>> wrote:
>>>>>>
>>>>>> I'd like to propose adding an appId parameter to the deploy endpoint.
>>>>>> This
>>>>>> would be optional and would presumably reject any attempt to start
a
>>>>>> second
>>>>>> app with the same id.  If set the appId would obviously be used in
>>>>>> place of
>>>>>> the generated id.
>>>>>>
>>>>>> This proposal would be of use in scripting deployments in a distributed
>>>>>> environment where deployment is not the first step in a number of
>>>>>> asynchronous jobs and would give us a way of "connecting" those jobs
up.
>>>>>> Hopefully it will help a lot in making things more robust for end-users.
>>>>>> Currently, if the client’s connection to the Brooklyn server fails
while
>>>>>> waiting for a response, it’s impossible to tell if the app was
>>>>>> provisioned
>>>>>> (e.g. how can you tell the difference between a likely-looking app,
and
>>>>>> another one deployed with an identical blueprint?). This would make
it
>>>>>> safe
>>>>>> to either retry the deploy request, or to query for the app with
the
>>>>>> expected id to see if it exists.
>>>>>>
>>>>>> Initially I'm hoping to use this in a downstream project but I think
>>>>>> this
>>>>>> would be useful to others.
>>>>>>
>>>>>> If no one has objections I'll aim to implement this over the next
>>>>>> couple of
>>>>>> weeks.  On the other hand I'm totally open to suggestions of a better
>>>>>> approach.
>>>>>>
>>>>>> Thanks
>>>>>>
>>>>>> Duncan Grant
>>>>>>
>>>>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message