brooklyn-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alex Heneveld <alex.henev...@cloudsoftcorp.com>
Subject Re: Proposal: Add appId optional paramater to deploy api
Date Wed, 26 Jul 2017 23:40:43 GMT
The core `id` is a low-level part of `BrooklynObject` used by all adjuncts
and entities and persistence.  It feels wrong and risky making this
something that is user- or client- settable.  I gave one example but there
are others.

What's wrong with a new config key or reusing `camp.id`?  We already use
the latter one if there is a user-specified ID on an entity so it feels
natural to use it, give it special meaning for apps (blocking repeat
deployments), and add support for searching for it.  (Apologies if this was
explained and I missed it.)

--A


On 26 July 2017 at 22:42, Aled Sage <aled.sage@gmail.com> wrote:

> Hi Alex,
>
> Other things get a lot simpler for us if we can just supply the app-id
> (e.g. subsequently searching for the app, or ensuring that a duplicate app
> is not deployed). It also means we're not adding another concept that we
> need to explain to users.
>
> To me, that simplicity is very compelling.
>
> If we want to support conformance to external id requirements, we could
> have a config key for a predicate or regex that the supplied id must
> satisfy. A given user could thus enforce their id standards in the future.
> I'd favour deferring that until there is a need to support it (e.g. we
> could add it at the same time as adding support for a pluggable id
> generator, if we ever do that).
>
> Aled
>
>
>
> On 26/07/2017 15:42, Alex Heneveld wrote:
>
>> 2 feels compelling to me. I want us to have the ability easily to change
>> the ID generation eg to conform with external reqs such as timestamp or
>> source.
>>
>> Go with deploymentUid or similar? Or camp.id?
>>
>> Best
>> Alex
>>
>> On 26 Jul 2017 15:00, "Aled Sage" <aled.sage@gmail.com> wrote:
>>
>> Hi Mark,
>>
>> We removed from EntitySpec the ability to set the id for two reasons:
>>
>> 1. there was no use-case at that time; simplifying the code by deleting it
>> was therefore sensible - we'd deprecated it for several releases.
>>
>> 2. allowing all uids to be generated/managed internally is simpler to
>> reason about, and gives greatest freedom for future refactoring.
>>
>>
>> I don't see (2) as a compelling reason.  And we now have a use-case, so
>> that changes (1).
>>
>> I'd still be tempted to treat this as an internal part of the api, rather
>> than it going on the public `EntitySpec`, but need to look at that more to
>> see how feasible it is.
>>
>> Aled
>>
>>
>>
>> On 26/07/2017 13:36, Mark Mc Kenna wrote:
>>
>> Thanks Geoff for the good summary
>>>
>>> IMO the path of least resistance that provides the best / most
>>> predictable
>>> behaviour is allowing clients to optionally set the app id.
>>>
>>> Off the top of my head I cant see this causing any issue, as long as we
>>> sanitise the supplied id something like [a-zA-Z0-9-]{8,}.
>>>
>>> Was there a particular reason this was removed in the past?
>>>
>>> Cheers
>>> M
>>>
>>> On 26 July 2017 at 13:07, Duncan Grant <duncan.grant@cloudsoftcorp.com>
>>> wrote:
>>>
>>> Thanks all for the advice.
>>>
>>>> I think Geoff's email summarises the issue nicely.  I like Alex's
>>>> suggestion but agree that limiting ourselves to deploy in the first is
>>>> probably significantly easier.
>>>>
>>>> Personally I don't feel comfortable with using a tag for idempotency
>>>> and I
>>>> do like Aled's suggestion of using PUT with a path with /{id} but would
>>>> be
>>>> happy with either as I think they both fit our use case.
>>>>
>>>> thanks
>>>>
>>>> Duncan
>>>>
>>>> On Wed, 26 Jul 2017 at 11:00 Geoff Macartney <
>>>> geoff.macartney@cloudsoft.io
>>>> wrote:
>>>>
>>>> If I understand correctly this isn't quite what Duncan/Aled are asking
>>>> for
>>>>
>>>> though?
>>>>> Which is not a "request id" but an idempotency token for an
>>>>> application.
>>>>>
>>>>> It
>>>>
>>>> would
>>>>> need to work long term, not just cached for a short time, and it would
>>>>>
>>>>> need
>>>>
>>>> to work
>>>>> across e.g. HA failover, so it wouldn't be just a matter of caching it
>>>>> on
>>>>> the server.
>>>>>
>>>>> For what it's worth, I'd have said this is a good use for tags but
>>>>> maybe
>>>>> for ease of reading
>>>>> it's better to have it as a config key as Aled does. As to how to
>>>>> supply
>>>>> the value
>>>>> I agree it should just be on the "deploy" operation.
>>>>>
>>>>> On Tue, 25 Jul 2017 at 19: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