RFC: time to remove the "smart" output formatter?

2016-08-09 Thread Tim Penhey 

Hi folks,

While perusing the codebase, I was reminded of our "smart" output formatter.

It is the yaml but not quite yaml one.

I propose we remove it, and default commands that were defaulting to 
that to use yaml instead.


Thoughts?

Tim

--
Juju-dev mailing list
Juju-dev@lists.ubuntu.com
Modify settings or unsubscribe at: 
https://lists.ubuntu.com/mailman/listinfo/juju-dev

Re: Let's talk retries

2016-08-09 Thread Katherine Cox-Buday 
roger peppe  writes:
> There's a fourth way that you haven't mentioned, which fits somewhere
> in between 1 and 2 I think (it was the first explicit general retry
> code in Juju AFAIK), which is utils.Attempt.
>
> I'd suggest that the way it's used matches the pattern you'd like to
> write quite well:
>
>  for attempt := strategy.State(); attempt.Next(); { Do something }

Thanks, Roger, I had missed that one. utils.Attempt would be my next 
preference, but it would have to be taught how to be interruptable via channel. 
I also feel that because so much of Go is oriented towards channels, exposing 
the wait via a channel rather than a function makes sense, but I don't 
immediately have a use-case in mind.

>
> AFAIR this pattern was arrived at after some discussion between myself
> and Gustavo. At the time I was thinking of some kind of callback-based
> scheme like the others schemes you mention, but I think that leaving
> the caller in control and the loop explicit is actually nicer - the
> logic is clear to the reader and more composable with other primitives
> (it is also a good match for the pattern you propose).

I strongly agree with this. In my estimation, favoring composability and inline 
code is much clearer and maintainable than piling one abstractions on another.

> How about making AttemptStrategy a little more flexible?

It looks like we've standardized on juju/retry. I don't have the historical 
context in my cache to know why we've rewritten retries 3 times now o.0

-- 
Katherine

-- 
Juju-dev mailing list
Juju-dev@lists.ubuntu.com
Modify settings or unsubscribe at: 
https://lists.ubuntu.com/mailman/listinfo/juju-dev

Change in provisioning functionality

2016-08-09 Thread Katherine Cox-Buday 
Hey All,

I have just landed a fix for lp:1597170 (juju2 beta10 fails to deploy all 
services in aws if storage is not ready), and it may come with some unexpected 
(desirable?) consequences.

In my fix, I found that -- though there is code in place to do so -- we never 
actually retried to provision machines if an error is encountered. This has 
been corrected.

Please keep an eye out for any unintended consequences to either the UX, or 
base functionality, and raise bugs if necessary.

Thanks very much!

-- 
Katherine

-- 
Juju mailing list
Juju@lists.ubuntu.com
Modify settings or unsubscribe at: 
https://lists.ubuntu.com/mailman/listinfo/juju

Re: Juju as a snap

2016-08-09 Thread Nicholas Skaggs 
On Mon, Aug 8, 2016 at 12:44 AM, Stuart Bishop 
wrote:

>
>
> On 3 August 2016 at 04:00, Nicholas Skaggs 
> wrote:
>
>> The Juju client has been snappified and pushed to the juju store. Note
>> for now it's just an amd64 build.
>>
>> snap install juju --beta --devmode
>>
>> The beta channel contains the latest beta, 2.0-beta13. This is a sneak
>> preview of further builds, including the latest crack available in the edge
>> channel as development happens.
>>
>
> You might want to get the snap added to the normal release process...
> beta14 is the new hotness.
>
> (Launchpad can do automatic builds of multiple architectures from a git or
> bzr branch, and push them to the charm store for you)
>
>
> --
> Stuart Bishop 
>

I've pushed beta14 now, again only an amd64 build however.  As far as
launchpad goes, I do plan on having it build the snaps (with all the arches
we desire), but we are working through a couple kinks in getting it to
build properly in launchpad. I've also opened a PR to upstream the
snapcraft.yaml, and I think it's going to need some updates and ideas about
how to enable developer builds as we go forward. Exciting stuff!
-- 
Juju mailing list
Juju@lists.ubuntu.com
Modify settings or unsubscribe at: 
https://lists.ubuntu.com/mailman/listinfo/juju

Re: Let's talk retries

2016-08-09 Thread Nate Finch 
So, in my opinion, juju/retry is pretty good.  I think with some tweaking
and some intelligent use, it can feel every bit as lightweight as we'd like.

If we have common use cases, we just need to write up an API to encapsulate
them.  If we want a "retry N times or until this channel is closed" then
write an api for that.  Having preconstructed call args with prepopulated
best practices for delays, backoffs, etc. can make it a lot easier.

There's no reason running a complex retry can't look like this:

// closure to encapsulate local variables
f := func() error { return foo.bar(a, b, c) }
err := retry.WithCancel(f, juju.StdRetry, doneCh)

Where StdRetry is the juju standard retry scheme (standard timeout, backoff
strategy, etc), and doneCh is the channel you expect to get closed to
cancel this action.

That's doable with the current API if we add cancellation (though it would
be nice if we extracted the function-to-run from the args struct, since it
makes it more obvious that the retry args can be reused for different
functions).

I think goroutines should be controlled by the caller.  If the caller wants
to put retry.Foo() in a goroutine, it can... but I don't think it's useful
to have that be retry's responsibility, it complicates the package too much.

On Tue, Aug 9, 2016 at 11:26 AM Katherine Cox-Buday <
katherine.cox-bu...@canonical.com> wrote:

> Andrew's queue is certainly nice, but I agree with the points Roger is
> raising:
>
> 1. Go's idiom for coordinating concurrent operations are goroutines and
> channels.
> 2. Sticking to these idioms makes it much easier to compose parts into a
> whole (if only because the language strongly bends code that way).
>
> As a matter of opinion, I also dislike having to instantiate structs to
> encapsulate logic; I would rather write the logic inline, or in a closure
> -- possibly in a goroutine.
>
> William Reade  writes:
>
> > On Tue, Aug 9, 2016 at 10:17 AM, roger peppe
> >  wrote:
> >
> > BTW I'm not saying that a timer queue is never the correct answer.
> > In some
> > circumstances, it can be the exactly the right thing to use.
> >
> >
> > Yeah -- the context here is that katco has been looking at the
> > provisioner, and I think a timer queue is sensible there. Firing off
> > goroutines for every machine is not necessarily bad by any means, but
> > it *is* somewhat harder to get right in every circumstance (e.g. what
> > happens when a machine is removed while that other goroutine is
> > working on provisioning it?).
> >
> > There are certainly many places where a single retryable op *is* the
> > only important thing and there's no call for a control loop. I'm not
> > suggesting we should always use a queue; but I'd tend to encourage it
> > whenever we have a component dealing with many similar retryable
> > tasks. (I definitely should be using it in dependency.Engine, for
> > example.)
> >
> > Cheers
> > William
> >
>
> --
> Katherine
>
> --
> Juju-dev mailing list
> Juju-dev@lists.ubuntu.com
> Modify settings or unsubscribe at:
> https://lists.ubuntu.com/mailman/listinfo/juju-dev
>
-- 
Juju-dev mailing list
Juju-dev@lists.ubuntu.com
Modify settings or unsubscribe at: 
https://lists.ubuntu.com/mailman/listinfo/juju-dev

Re: Let's talk retries

2016-08-09 Thread Katherine Cox-Buday 
Andrew's queue is certainly nice, but I agree with the points Roger is raising:

1. Go's idiom for coordinating concurrent operations are goroutines and 
channels.
2. Sticking to these idioms makes it much easier to compose parts into a whole 
(if only because the language strongly bends code that way).

As a matter of opinion, I also dislike having to instantiate structs to 
encapsulate logic; I would rather write the logic inline, or in a closure -- 
possibly in a goroutine.

William Reade  writes:

> On Tue, Aug 9, 2016 at 10:17 AM, roger peppe
>  wrote:
>
> BTW I'm not saying that a timer queue is never the correct answer.
> In some
> circumstances, it can be the exactly the right thing to use.
> 
>
> Yeah -- the context here is that katco has been looking at the
> provisioner, and I think a timer queue is sensible there. Firing off
> goroutines for every machine is not necessarily bad by any means, but
> it *is* somewhat harder to get right in every circumstance (e.g. what
> happens when a machine is removed while that other goroutine is
> working on provisioning it?).
>
> There are certainly many places where a single retryable op *is* the
> only important thing and there's no call for a control loop. I'm not
> suggesting we should always use a queue; but I'd tend to encourage it
> whenever we have a component dealing with many similar retryable
> tasks. (I definitely should be using it in dependency.Engine, for
> example.)
>
> Cheers
> William
>

-- 
Katherine

-- 
Juju-dev mailing list
Juju-dev@lists.ubuntu.com
Modify settings or unsubscribe at: 
https://lists.ubuntu.com/mailman/listinfo/juju-dev

Re: Question about scripts and blocking

2016-08-09 Thread Stuart Bishop 
On 9 August 2016 at 18:32, Matthew Williams 
wrote:

>
> c) Is there some other solution that I've not considered?
>

Plugins can be used to add the behaviour you need, allowing time for the
core team to catch up. I've already got a blocking action runner that just
needs the juju 1 backport done for example, which we need to make the cli
usable before we use actions in anger on production. Running actions on all
units in a service is a small extension to that. git+ssh://
git.launchpad.net/~stub/+git/juju-act if you are curious.

But yes, I think what you describe is in theory suitable for actions. But
in practice, I think people are not using actions as 'juju run' is more
usable and flexible. https://bugs.launchpad.net/juju-core/+bug/1445066 is
the original bug report on needing blocking actions. I don't know about one
for running on all units in a service, but I have heard it discussed before.

-- 
Stuart Bishop 
-- 
Juju mailing list
Juju@lists.ubuntu.com
Modify settings or unsubscribe at: 
https://lists.ubuntu.com/mailman/listinfo/juju

Re: Juju and snappy implementation spike - feedback please

2016-08-09 Thread John Meinel 
>
> ...
>


>
> The big difference to me is that $SNAP_USER_DATA will roll back if the
> snap is rolled back. I'm not sure what happens if the snap is removed and
> reinstalled. Given end users should no longer need to be messing around
> with the dotfiles, I think the rollback behaviour is what should drive your
> decision. Is it nice behaviour? Or will it mess things up because rollback
> will cause things to get out of sync with the deployments?
>
>
>
So the conceptual "we can make changes to how juju.conf is written and have
it roll forward" sound nice, but in actuality we're quite used to not doing
that, and the problem of "I upgrade Juju, and bootstrapped a new
Controller, and then reverted my Juju version, and can no longer access the
old controller", actually puts versioned config into the bad column.

But snaps have "common" to handle that sort of case, where each version of
a single snap series can use the same data store.

What I haven't heard a specific answer for is, I want to run
"juju-wallyworld" or "juju-menno" or official "juju" and have them all see
the same controllers and accounts. And honestly, if I was running
firefox-custom-build I really don't think I'd want a different set of
bookmarks.

John
=:->
-- 
Juju-dev mailing list
Juju-dev@lists.ubuntu.com
Modify settings or unsubscribe at: 
https://lists.ubuntu.com/mailman/listinfo/juju-dev

Re: Juju and snappy implementation spike - feedback please

2016-08-09 Thread Stuart Bishop 
On 9 August 2016 at 19:08, Ian Booth  wrote:

> I personally like the idea that the snap could use a juju-home interface to
> allow access to the standard ~/.local/share/juju directory; thus allowing
> a snap
> and regular Juju to be used interchangeably (at least initially). This will
> allow thw use case "hey, try my juju snap and you can use your existing
> settings" But, isn't it verboten for snaps to access dot directories in
> user
> home in any way, regardless of what any interface says? We could provide an
> import tool to copy from ~/.local/share/juju to ~/snap/blah...
>
> But in the other case, using a personal snap and sharing settings with the
> official Juju snap - do we know what the official snappy story is around
> this
> scenario? I can't imagine this is the first time it's come up?
>


The big difference to me is that $SNAP_USER_DATA will roll back if the snap
is rolled back. I'm not sure what happens if the snap is removed and
reinstalled. Given end users should no longer need to be messing around
with the dotfiles, I think the rollback behaviour is what should drive your
decision. Is it nice behaviour? Or will it mess things up because rollback
will cause things to get out of sync with the deployments?


-- 
Stuart Bishop 
-- 
Juju-dev mailing list
Juju-dev@lists.ubuntu.com
Modify settings or unsubscribe at: 
https://lists.ubuntu.com/mailman/listinfo/juju-dev

Re: Juju and snappy implementation spike - feedback please

2016-08-09 Thread Ian Booth 
I personally like the idea that the snap could use a juju-home interface to
allow access to the standard ~/.local/share/juju directory; thus allowing a snap
and regular Juju to be used interchangeably (at least initially). This will
allow thw use case "hey, try my juju snap and you can use your existing
settings" But, isn't it verboten for snaps to access dot directories in user
home in any way, regardless of what any interface says? We could provide an
import tool to copy from ~/.local/share/juju to ~/snap/blah...

But in the other case, using a personal snap and sharing settings with the
official Juju snap - do we know what the official snappy story is around this
scenario? I can't imagine this is the first time it's come up?


On 09/08/16 17:27, John Meinel wrote:
> On Aug 9, 2016 1:06 AM, "Nicholas Skaggs" 
> wrote:
>>
>>
>>
>> On Mon, Aug 8, 2016 at 11:49 AM, John Meinel 
> wrote:
>>>
>>> If we are installing in '--devmode' don't we have access to the
> unfiltered $HOME directory if we look for it? And we could ask for an
> interface which is to JUJU_HOME that would give us access to just
> $HOME/.local/share/juju
>>>
>>>
>>> We could then copy that information into the normal 'home' directory of
> the snap. That might give a better experience than having to import your
> credentials anytime you want to just evaluate a dev build of juju.
>>
>> I agree this gets more difficult with the idea of sharing builds -- as
> you say, you have to re-add your credentials for each new developer.In
> regards to your thoughts on --devmode, it does give you greater access, but
> some things are still constrained. The HOME interface doesn't allow access
> to dot files or folders by default. So it's not useful in this instance. If
> juju were to change where it stores it's configuration files (aka, not in a
> dotfolder) this technically becomes not a problem as the current home
> interface would allow this.
> 
> Sure. That's why I mention "We're in dev mode now, and can ask for a
> JUJU_HOME interface vs the existing HOME one." We can also just ask for a
> "give me the root filesystem" interface that doesn't get connected by
> default.
> 
> I think not being able to publish your version of a snap and have it work
> with the "standard" version of a snap is going to be a general issue for
> anyone using snaps for development. So maybe it's a general snap property
> that can give you access to a "named" common directory.
> 
> John
> =:->
> 
>>>
>>>
>>> AIUI, the 'common' folder for a snap is only for different versions of
> the exact same snap, which means that if I publish 'juju-jameinel' it won't
> be able to share anything with 'juju-wallyworld' nor the official 'juju',
> so there isn't any reason to use it.
>>>
>>> I don't know exactly how snap content interfaces work, but it might be
> interesting to be able to share the JUJU_HOME between snaps (like they
> intend to be able to share a "pictures" or "music" directory).
>>>
>>> If we *don't* share them, then we won't easily be able to try a new Juju
> on an existing controller. (If I just want you to see how the new 'juju
> status' is formatted, you'd rather run it on a controller that has a bunch
> of stuff running.)
>>
>> It's worth mentioning / filing a bug about our needs with the snapcraft
> folks to see what options might exist. I've started conversations a few
> weeks ago and solved or got good bugs in on other juju issues. I think they
> understand the application config limitations / issues, so we can push for
> a resolution.
> 

-- 
Juju-dev mailing list
Juju-dev@lists.ubuntu.com
Modify settings or unsubscribe at: 
https://lists.ubuntu.com/mailman/listinfo/juju-dev

Re: Let's talk retries

2016-08-09 Thread William Reade 
On Tue, Aug 9, 2016 at 10:17 AM, roger peppe 
wrote:

> BTW I'm not saying that a timer queue is never the correct answer. In some
> circumstances, it can be the exactly the right thing to use.
>

Yeah -- the context here is that katco has been looking at the provisioner,
and I think a timer queue is sensible there. Firing off goroutines for
every machine is not necessarily bad by any means, but it *is* somewhat
harder to get right in every circumstance (e.g. what happens when a machine
is removed while that other goroutine is working on provisioning it?).

There are certainly many places where a single retryable op *is* the only
important thing and there's no call for a control loop. I'm not suggesting
we should always use a queue; but I'd tend to encourage it whenever we have
a component dealing with many similar retryable tasks. (I definitely should
be using it in dependency.Engine, for example.)

Cheers
William
-- 
Juju-dev mailing list
Juju-dev@lists.ubuntu.com
Modify settings or unsubscribe at: 
https://lists.ubuntu.com/mailman/listinfo/juju-dev

Re: RFC: proposed changes to debug-log API call

2016-08-09 Thread John Meinel 
Sounds like a good idea. Can we get some form of versioning like we do with
Facades so that we can do changes in the future? AIUI we could put it in
the "Accept" header, in the URL or in a Query parameter. Please coordinate
with Ian who has been designing the REST API for CMR so that we use the
same mechanism.

John
=:->

On Aug 9, 2016 12:43 PM, "Tim Penhey"  wrote:

> Hi folks,
>
> This is regarding the streaming /api/:model-uuid/log api server endpoint.
>
> Currently this method sends an initial possible error, then just turns on
> a tap of bytes of preformatted log messages.
>
> What I propose is to change the streaming protocol from just text to a
> stream of structured log messages.
>
> This initial change will have no impact at all on the CLI, but allows for
> future changes like:
>   - showing logs in local time instead of UTC
>   - hiding the filenames by default
>   - not showing the date by default
>   - colors
>
> Is anyone currently using the streaming text aspects of the API right now?
>
> How much warning would you like?  Ideally this change needs to happen
> before we freeze the API for backwards compatibility.
>
> Cheers,
> Tim
>
> --
> Juju-dev mailing list
> Juju-dev@lists.ubuntu.com
> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailm
> an/listinfo/juju-dev
>
-- 
Juju-dev mailing list
Juju-dev@lists.ubuntu.com
Modify settings or unsubscribe at: 
https://lists.ubuntu.com/mailman/listinfo/juju-dev

Re: RFC: proposed changes to debug-log API call

2016-08-09 Thread Adam Collard 
On Tue, 9 Aug 2016 at 09:43 Tim Penhey  wrote:

> Is anyone currently using the streaming text aspects of the API right now?
>

Not us.
-- 
Juju-dev mailing list
Juju-dev@lists.ubuntu.com
Modify settings or unsubscribe at: 
https://lists.ubuntu.com/mailman/listinfo/juju-dev

RFC: proposed changes to debug-log API call

2016-08-09 Thread Tim Penhey 

Hi folks,

This is regarding the streaming /api/:model-uuid/log api server endpoint.

Currently this method sends an initial possible error, then just turns 
on a tap of bytes of preformatted log messages.


What I propose is to change the streaming protocol from just text to a 
stream of structured log messages.


This initial change will have no impact at all on the CLI, but allows 
for future changes like:

  - showing logs in local time instead of UTC
  - hiding the filenames by default
  - not showing the date by default
  - colors

Is anyone currently using the streaming text aspects of the API right now?

How much warning would you like?  Ideally this change needs to happen 
before we freeze the API for backwards compatibility.


Cheers,
Tim

--
Juju-dev mailing list
Juju-dev@lists.ubuntu.com
Modify settings or unsubscribe at: 
https://lists.ubuntu.com/mailman/listinfo/juju-dev

Re: Juju and snappy implementation spike - feedback please

2016-08-09 Thread John Meinel 
On Aug 9, 2016 1:06 AM, "Nicholas Skaggs" 
wrote:
>
>
>
> On Mon, Aug 8, 2016 at 11:49 AM, John Meinel 
wrote:
>>
>> If we are installing in '--devmode' don't we have access to the
unfiltered $HOME directory if we look for it? And we could ask for an
interface which is to JUJU_HOME that would give us access to just
$HOME/.local/share/juju
>>
>>
>> We could then copy that information into the normal 'home' directory of
the snap. That might give a better experience than having to import your
credentials anytime you want to just evaluate a dev build of juju.
>
> I agree this gets more difficult with the idea of sharing builds -- as
you say, you have to re-add your credentials for each new developer.In
regards to your thoughts on --devmode, it does give you greater access, but
some things are still constrained. The HOME interface doesn't allow access
to dot files or folders by default. So it's not useful in this instance. If
juju were to change where it stores it's configuration files (aka, not in a
dotfolder) this technically becomes not a problem as the current home
interface would allow this.

Sure. That's why I mention "We're in dev mode now, and can ask for a
JUJU_HOME interface vs the existing HOME one." We can also just ask for a
"give me the root filesystem" interface that doesn't get connected by
default.

I think not being able to publish your version of a snap and have it work
with the "standard" version of a snap is going to be a general issue for
anyone using snaps for development. So maybe it's a general snap property
that can give you access to a "named" common directory.

John
=:->

>>
>>
>> AIUI, the 'common' folder for a snap is only for different versions of
the exact same snap, which means that if I publish 'juju-jameinel' it won't
be able to share anything with 'juju-wallyworld' nor the official 'juju',
so there isn't any reason to use it.
>>
>> I don't know exactly how snap content interfaces work, but it might be
interesting to be able to share the JUJU_HOME between snaps (like they
intend to be able to share a "pictures" or "music" directory).
>>
>> If we *don't* share them, then we won't easily be able to try a new Juju
on an existing controller. (If I just want you to see how the new 'juju
status' is formatted, you'd rather run it on a controller that has a bunch
of stuff running.)
>
> It's worth mentioning / filing a bug about our needs with the snapcraft
folks to see what options might exist. I've started conversations a few
weeks ago and solved or got good bugs in on other juju issues. I think they
understand the application config limitations / issues, so we can push for
a resolution.
-- 
Juju-dev mailing list
Juju-dev@lists.ubuntu.com
Modify settings or unsubscribe at: 
https://lists.ubuntu.com/mailman/listinfo/juju-dev

Re: Let's talk retries

2016-08-09 Thread roger peppe 
On 9 August 2016 at 07:28, roger peppe  wrote:
> On 9 August 2016 at 01:22, Katherine Cox-Buday
>  wrote:
>> Hey All,
>>
>> We currently have 3 ways we're performing retries in Juju:
>>
>> 1. Archaic, custom, intra-package retry code.
>> 2. github.com/juju/utils::{Countdown,BackoffTimer}
>> 3. github.com/juju/retry (current best practice)
>
> There's a fourth way that you haven't mentioned, which fits
> somewhere in between 1 and 2 I think (it was the first
> explicit general retry code in Juju AFAIK), which is
> utils.Attempt.
>
> I'd suggest that the way it's used matches the pattern
> you'd like to write quite well:
>
>  for attempt := strategy.State(); attempt.Next(); {

Oops; s/State/Start/ of course.

>   Do something
>  }
>
> AFAIR this pattern was arrived at after some discussion between myself
> and Gustavo. At the time I was thinking of some kind of
> callback-based scheme like the others schemes you mention, but
> I think that leaving the caller in control and the loop explicit
> is actually nicer - the logic is clear to the reader and more
> composable with other primitives (it is also a good match
> for the pattern you propose).
>
> How about making AttemptStrategy a little more flexible?
> It was always my intention to admit other kinds of strategy - if
> a BackoffFunc field were added to AttemptStrategy and
> a Stop argument added to AttemptStrategy.Start it would
> become as general as retry.Call, while keeping the nice
> usage pattern and general straightforwardness.
>
>   cheers,
> rog.
>
>> I have just touched some code which fits #1, and when I was attempting to 
>> retrofit it to #3, I found it a little obtuse. Here's why:
>>
>> In my case (and I assume most), I wanted something like this:
>>
>> for range retries { // Loop should break if our parent is cancelled
>> // Do something
>> // Success, retry, or fatal error
>> }
>>
>> To implement this, I do something like this:
>>
>> args := retry.CallArgs{
>> Func: func() error {
>> // Body of my loop
>> },
>> BackoffFunc: func(delay time.Duration, attempt int) time.Duration {
>> if attempt == 1 {
>> return delay
>> }
>> return delay * factor
>> },
>> Stop: dying,
>> // etc...
>> }
>>
>> Call(args)
>>
>> We attempt to encapsulate every scenario for retries in 
>> github.com/juju/retry/::CallArgs. We have variables to determine if errors 
>> are fatal, to notify things, how to back off, etc. This feels very 
>> heavy-weight to me, and a bit like the monolith model rather than the unix 
>> model. I would prefer the logic be left to the caller (i.e. do I break out 
>> of this loop? do I notify something?), and the interface into a retry 
>> strategy be much simpler, say a channel like in time.Tick(time.Duration) 
>> <-chan time.Time.
>>
>> How would folks feel about something like this?
>>
>> func BackoffTick(done <-chan struct{}, clock clock.Cock, delay 
>> time.Duration, factor int64) <-chan time.Time {
>> signalStream := make(chan time.Time)
>> go func() {
>> defer close(signalStream)
>> for {
>> select {
>> case <-done:
>> return
>> case signalStream <- time.After(delay):
>> delay = time.Duration(delay.Nanoseconds() * 
>> factor)
>> }
>> }
>> }()
>> return signalStream
>> }
>>
>> With this, the above becomes:
>>
>> for range BackoffTick(dying, myClock, 1*time.Second, 2) {
>> // Determination of fatality and notifications happen here
>> }
>>
>> If you want a max retry concept, you do this:
>>
>> for range Take(done, 3, BackoffTick(dying, myClock, 1*time.Second, 2)) {
>> // Determination of fatality and notifications happen here
>> }
>>
>> If you want a max duration you do this:
>>
>> done = Timeout(done, 5*time.Minute)
>> for range Take(done, 3, BackoffTick(dying, myClock, 1*time.Second, 2)) {
>> // Determination of fatality and notifications happen here
>> }
>>
>> Functionally, what's in juju/retry is fine; and I can stuff anything I want 
>> into the function. It just feels a little odd to use in that I must put the 
>> body of my loop in a function, and I dislike that the CallArgs struct 
>> attempts to encapsulate a bunch of different scenarios.
>>
>> Thoughts?
>
> I'm not keen on the channel-based approach because if you want to
> terminate early, you need to shut down the goroutine - it's just
> one more resource to clean up. Also, the goroutine that's sending
> the events is one step removed from what's going on in the loop - the
> delay is calculated one step before the current iteration is complete.
>
>   cheers,
> rog.
>
>>
>> Also, is it on the roadmap to address the inconsitant retries throughout 
>> Juju? I almost

Re: Let's talk retries

2016-08-09 Thread roger peppe 
On 9 August 2016 at 01:22, Katherine Cox-Buday
 wrote:
> Hey All,
>
> We currently have 3 ways we're performing retries in Juju:
>
> 1. Archaic, custom, intra-package retry code.
> 2. github.com/juju/utils::{Countdown,BackoffTimer}
> 3. github.com/juju/retry (current best practice)

There's a fourth way that you haven't mentioned, which fits
somewhere in between 1 and 2 I think (it was the first
explicit general retry code in Juju AFAIK), which is
utils.Attempt.

I'd suggest that the way it's used matches the pattern
you'd like to write quite well:

 for attempt := strategy.State(); attempt.Next(); {
  Do something
 }

AFAIR this pattern was arrived at after some discussion between myself
and Gustavo. At the time I was thinking of some kind of
callback-based scheme like the others schemes you mention, but
I think that leaving the caller in control and the loop explicit
is actually nicer - the logic is clear to the reader and more
composable with other primitives (it is also a good match
for the pattern you propose).

How about making AttemptStrategy a little more flexible?
It was always my intention to admit other kinds of strategy - if
a BackoffFunc field were added to AttemptStrategy and
a Stop argument added to AttemptStrategy.Start it would
become as general as retry.Call, while keeping the nice
usage pattern and general straightforwardness.

  cheers,
rog.

> I have just touched some code which fits #1, and when I was attempting to 
> retrofit it to #3, I found it a little obtuse. Here's why:
>
> In my case (and I assume most), I wanted something like this:
>
> for range retries { // Loop should break if our parent is cancelled
> // Do something
> // Success, retry, or fatal error
> }
>
> To implement this, I do something like this:
>
> args := retry.CallArgs{
> Func: func() error {
> // Body of my loop
> },
> BackoffFunc: func(delay time.Duration, attempt int) time.Duration {
> if attempt == 1 {
> return delay
> }
> return delay * factor
> },
> Stop: dying,
> // etc...
> }
>
> Call(args)
>
> We attempt to encapsulate every scenario for retries in 
> github.com/juju/retry/::CallArgs. We have variables to determine if errors 
> are fatal, to notify things, how to back off, etc. This feels very 
> heavy-weight to me, and a bit like the monolith model rather than the unix 
> model. I would prefer the logic be left to the caller (i.e. do I break out of 
> this loop? do I notify something?), and the interface into a retry strategy 
> be much simpler, say a channel like in time.Tick(time.Duration) <-chan 
> time.Time.
>
> How would folks feel about something like this?
>
> func BackoffTick(done <-chan struct{}, clock clock.Cock, delay time.Duration, 
> factor int64) <-chan time.Time {
> signalStream := make(chan time.Time)
> go func() {
> defer close(signalStream)
> for {
> select {
> case <-done:
> return
> case signalStream <- time.After(delay):
> delay = time.Duration(delay.Nanoseconds() * 
> factor)
> }
> }
> }()
> return signalStream
> }
>
> With this, the above becomes:
>
> for range BackoffTick(dying, myClock, 1*time.Second, 2) {
> // Determination of fatality and notifications happen here
> }
>
> If you want a max retry concept, you do this:
>
> for range Take(done, 3, BackoffTick(dying, myClock, 1*time.Second, 2)) {
> // Determination of fatality and notifications happen here
> }
>
> If you want a max duration you do this:
>
> done = Timeout(done, 5*time.Minute)
> for range Take(done, 3, BackoffTick(dying, myClock, 1*time.Second, 2)) {
> // Determination of fatality and notifications happen here
> }
>
> Functionally, what's in juju/retry is fine; and I can stuff anything I want 
> into the function. It just feels a little odd to use in that I must put the 
> body of my loop in a function, and I dislike that the CallArgs struct 
> attempts to encapsulate a bunch of different scenarios.
>
> Thoughts?

I'm not keen on the channel-based approach because if you want to
terminate early, you need to shut down the goroutine - it's just
one more resource to clean up. Also, the goroutine that's sending
the events is one step removed from what's going on in the loop - the
delay is calculated one step before the current iteration is complete.

  cheers,
rog.

>
> Also, is it on the roadmap to address the inconsitant retries throughout 
> Juju? I almost used utils.BackoffTimer until I started looking deeper. I'm 
> going to submit a PR to flag it as deprecated.
>
> --
> Katherine
>
> --
> Juju-dev mailing list
> Juju-dev@lists.ubuntu.com
> Modify settings or unsubscribe at: 
> https://lists.ubuntu.com/mailman/listinfo/juju-dev