Re: FullStatus API when machine or unit is StatusDown
On Sun, May 18, 2014 at 9:55 PM, Menno Smits wrote: > I'm working on a change to make "juju status" report the remote unit when > a relation hook fails (https://bugs.launchpad.net/juju-core/+bug/1194481). > > fwiw, i've updated to reflect that the underlying obscurity is much reduced already via inclusion of relation-name. > During this work I've noticed that the FullStatus client API does some > munging of the agent state info string if the agent appears to be dead but > shouldn't be. It replaces the agent state info string with "(status: status > info)" (note the parens) and then changes the returned status to > StatusDown. You've probably noticed juju status strings with parens around > them before - this is why. > > indeed, i've mostly noticed it because it happens often when it never should (new units/agents coming up). > This code is at state/apiserver/client/status.go:540. > > This is problematic for two reasons: > >1. the client API code is making formatting decisions that should be >left up to the client. In the case of the ticket I'm working on, the juju >status command handler ends up having to detect and unpick the parens in >status string while generating the status output so that it can correctly >append the remote unit name. This is easy enough but ugly and feels wrong. > > agreed. > >1. because the munging is done within the FullStatus API call, clients >that are consuming events via a watcher will not see the munged status and >info. If a client was to use both a watcher and FullStatus, it could see >different status values for a unit or machine between the two APIs. > > + ugh > > > What I'd like to do is not have the client API munge the status info > string and instead use the StatusData map to report the record the original > status. When a unit or machine status is StatusDown a client can look in > StatusData for the pre-agent-down status if it wants it. > sounds good. > > It would also be good to move the "if agent down set the status to > StatusDown" logic to somewhere deeper so that all clients get a consistent > view of the status. I can live with it if this isn't feasible however - > this part isn't necessary to get the ticket done. > > I'm concerned that this change may impact the GUI. As far as I can tell > the GUI doesn't call FullStatus so any changes to that API shouldn't affect > it (please correct me if I'm wrong). Changing how StatusDown is set however > will probably impact the GUI. > the gui doesn't call fullstatus afaik, but it is not the only api consumer, gui is primarily on the watch api and then fetching/mutating individual entities (services, units). -k > > Thoughts? > > - Menno > > > > > > > > > -- > 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: Ensuring tests pass on gccgo
I wasn't suggesting that slices should be sorted in tests, but rather that
the real APIs define the ordering for elements stored and returned. This is
useful for humans too.
Either way, just a suggestion.
gustavo @ http://niemeyer.net
On May 22, 2014 5:49 PM, "Nate Finch" wrote:
> Although using DeepEquals on sorted lists does make for
> easier-to-understand test failure messages, the burden on the developer to
> sort the slices when they write the test doesn't seem worth it to me. We
> write tests a lot more often than we need to debug failing tests. If it
> was just slices of strings, then sorting is easy enough, but for other
> random types, you now have to figure out how to sort the types in a fully
> deterministic way, which can be difficult and error prone for large
> structs, especially as those structs change.
>
> It's much easier and less error prone to say "just use jc.SameContents".
> On May 22, 2014 10:42 AM, "Gustavo Niemeyer" <
> gustavo.nieme...@canonical.com> wrote:
>
>> I apologize. After reading one of the review entries using
>> SameContents, I see I've misunderstood what the problem is about. The
>> v1 and v2 in your example is a slice, not a map, which means the test
>> is comparing slices of undefined order. This is certainly bogus and
>> needs addressing in the code itself, irrespective of any map-related
>> issues.
>>
>> What I generally do in those cases is to define the ordering of the
>> slice. Besides enabling DeepEquals, it also makes the visual
>> comparison more pleasant.
>>
>>
>>
>> On Thu, May 22, 2014 at 11:34 AM, Gustavo Niemeyer
>> wrote:
>> > On Wed, May 21, 2014 at 10:43 PM, Ian Booth
>> wrote:
>> >> We are working to make all juju-core unit tests pass using gccgo. In
>> case you
>> >> didn't already know, there's a common issue which has caused a lot of
>> the
>> >> failures to date. Here's a quick heads up on how to deal with it.
>> >>
>> >> golang-go and gcc-go have different map implementations which results
>> in
>> >> ordering differences, affecting things like range etc (simplistically
>> put,
>> >> gcc-go's map ordering is random whereas currently golang-go is somewhat
>> >> deterministic).
>> >
>> > This is changing in the main compiler as well, in Go 1.3:
>> >
>> > http://tip.golang.org/doc/go1.3#map
>> >
>> > So it'll become even less deterministic there as well.
>> >
>> >> Now of course, maps are unordered but what we sometimes do in
>> >> the code is to use a map to hold some data (maybe to eliminate
>> duplicates) and
>> >> then expose that data via a slice or array. If we then do a
>> c.Assert(v1,
>> >> gc.DeepEquals, v2), it will fail on gcc-go, since the order of items
>> in the 2
>> >> slices is different, even though the values are the same.
>> >
>> > If that's really the case, it's definitely a bug in gccgo. gocheck's
>> > DeepEquals is implemented in terms of reflect.DeepEqual, which should
>> > not care about the map order. In the standard library of the main
>> > compiler, it clearly does not:
>> >
>> > for _, k := range v1.MapKeys() {
>> > if !deepValueEqual(v1.MapIndex(k), v2.MapIndex(k),
>> > visited, depth+1) {
>> > return false
>> > }
>> > }
>> >
>> > So gocheck's DeepEquals is fine for such map tests, assuming no bugs
>> > in the underlying implementation.
>> >
>> >
>> > gustavo @ http://niemeyer.net
>>
>>
>>
>> --
>> gustavo @ http://niemeyer.net
>>
>> --
>> 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: Proposed new dependency: github.com/juju/errors (and github.com/juju/errgo)
I agree with Roger, I don't think a wrapper around Errgo is the right way to go. If we don't like the way errgo behaves, we should change it. It's our package, let's have it work the way we want it to work. On Thu, May 22, 2014 at 10:47 AM, roger peppe wrote: > On 14 May 2014 10:24, Tim Penhey wrote: > > Hi all, > > > > I took it upon myself to get Rog's errgo library used inside juju-core. > > Thanks for doing this. > > > Dimiter recently did a hunk of work in the juju-core/errors package to > > have functions to add context to some core error types while keeping > > their type. > > > > What I did was to move this errors package out of juju-core and into its > > own package, and update it to use the errgo as a base. For those that > > are interested the diff for the errors_test (to show that everything > > works as before) is as shown at the bottom of this email. > > > > I also added in simple methods based on my earlier error handling > > package, but changed them to be based on errgo. This adds in the > > functions: New, Errorf, Check, Trace, Annotate, Annotatef, Wrap, > ErrorStack. > > Any particular reason to wrap the errgo functions rather than using errgo > directly? Personally, I see the role of the errors package to be about > specific error classifications (something that errgo tries hard to be > entirely > agnostic about), but errgo could, and I believe should, be used directly > for all > the more general calls. > > Since it's likely that other packages will start to use errgo, and they > don't > necessarily want to be importing juju/errors, there are advantages > to using the same error generation and classification style that any > other errgo-using package would use - in particular, it makes the code > easier > to manipulate programmatically. > > There are also advantages in not having two subtly different APIs > around, even if one is implemented in terms of the other. I believe > we should pick one set of calls and use them consistently throughout > the code base. > > BTW, I have a gofix clone which automates 95+% of the changes > to use errgo - it's at code.google.com/p/rog-go/exp/cmd/errfix > It splits the changes into several modules so that the > changes can be made in more-easily verifiable steps. > > > I have tested juju/errors with both gc and gccgo and the tests pass with > > both. Interestingly juju/errgo has a few test failures with gccgo, but > > Dave is looking at them and assures me they are simple fixes. > > As an aside, I do think that any new dependencies we add should pass all > > tests with both gc and gccgo. > > Thanks a lot for doing this. I really should install and try gccgo! > > cheers, > rog. > > -- > 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: Ensuring tests pass on gccgo
Although using DeepEquals on sorted lists does make for
easier-to-understand test failure messages, the burden on the developer to
sort the slices when they write the test doesn't seem worth it to me. We
write tests a lot more often than we need to debug failing tests. If it
was just slices of strings, then sorting is easy enough, but for other
random types, you now have to figure out how to sort the types in a fully
deterministic way, which can be difficult and error prone for large
structs, especially as those structs change.
It's much easier and less error prone to say "just use jc.SameContents".
On May 22, 2014 10:42 AM, "Gustavo Niemeyer"
wrote:
> I apologize. After reading one of the review entries using
> SameContents, I see I've misunderstood what the problem is about. The
> v1 and v2 in your example is a slice, not a map, which means the test
> is comparing slices of undefined order. This is certainly bogus and
> needs addressing in the code itself, irrespective of any map-related
> issues.
>
> What I generally do in those cases is to define the ordering of the
> slice. Besides enabling DeepEquals, it also makes the visual
> comparison more pleasant.
>
>
>
> On Thu, May 22, 2014 at 11:34 AM, Gustavo Niemeyer
> wrote:
> > On Wed, May 21, 2014 at 10:43 PM, Ian Booth
> wrote:
> >> We are working to make all juju-core unit tests pass using gccgo. In
> case you
> >> didn't already know, there's a common issue which has caused a lot of
> the
> >> failures to date. Here's a quick heads up on how to deal with it.
> >>
> >> golang-go and gcc-go have different map implementations which results in
> >> ordering differences, affecting things like range etc (simplistically
> put,
> >> gcc-go's map ordering is random whereas currently golang-go is somewhat
> >> deterministic).
> >
> > This is changing in the main compiler as well, in Go 1.3:
> >
> > http://tip.golang.org/doc/go1.3#map
> >
> > So it'll become even less deterministic there as well.
> >
> >> Now of course, maps are unordered but what we sometimes do in
> >> the code is to use a map to hold some data (maybe to eliminate
> duplicates) and
> >> then expose that data via a slice or array. If we then do a c.Assert(v1,
> >> gc.DeepEquals, v2), it will fail on gcc-go, since the order of items in
> the 2
> >> slices is different, even though the values are the same.
> >
> > If that's really the case, it's definitely a bug in gccgo. gocheck's
> > DeepEquals is implemented in terms of reflect.DeepEqual, which should
> > not care about the map order. In the standard library of the main
> > compiler, it clearly does not:
> >
> > for _, k := range v1.MapKeys() {
> > if !deepValueEqual(v1.MapIndex(k), v2.MapIndex(k),
> > visited, depth+1) {
> > return false
> > }
> > }
> >
> > So gocheck's DeepEquals is fine for such map tests, assuming no bugs
> > in the underlying implementation.
> >
> >
> > gustavo @ http://niemeyer.net
>
>
>
> --
> gustavo @ http://niemeyer.net
>
> --
> 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: Proposed new dependency: github.com/juju/errors (and github.com/juju/errgo)
On 14 May 2014 10:24, Tim Penhey wrote: > Hi all, > > I took it upon myself to get Rog's errgo library used inside juju-core. Thanks for doing this. > Dimiter recently did a hunk of work in the juju-core/errors package to > have functions to add context to some core error types while keeping > their type. > > What I did was to move this errors package out of juju-core and into its > own package, and update it to use the errgo as a base. For those that > are interested the diff for the errors_test (to show that everything > works as before) is as shown at the bottom of this email. > > I also added in simple methods based on my earlier error handling > package, but changed them to be based on errgo. This adds in the > functions: New, Errorf, Check, Trace, Annotate, Annotatef, Wrap, ErrorStack. Any particular reason to wrap the errgo functions rather than using errgo directly? Personally, I see the role of the errors package to be about specific error classifications (something that errgo tries hard to be entirely agnostic about), but errgo could, and I believe should, be used directly for all the more general calls. Since it's likely that other packages will start to use errgo, and they don't necessarily want to be importing juju/errors, there are advantages to using the same error generation and classification style that any other errgo-using package would use - in particular, it makes the code easier to manipulate programmatically. There are also advantages in not having two subtly different APIs around, even if one is implemented in terms of the other. I believe we should pick one set of calls and use them consistently throughout the code base. BTW, I have a gofix clone which automates 95+% of the changes to use errgo - it's at code.google.com/p/rog-go/exp/cmd/errfix It splits the changes into several modules so that the changes can be made in more-easily verifiable steps. > I have tested juju/errors with both gc and gccgo and the tests pass with > both. Interestingly juju/errgo has a few test failures with gccgo, but > Dave is looking at them and assures me they are simple fixes. > As an aside, I do think that any new dependencies we add should pass all > tests with both gc and gccgo. Thanks a lot for doing this. I really should install and try gccgo! cheers, rog. -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
Re: Ensuring tests pass on gccgo
I apologize. After reading one of the review entries using
SameContents, I see I've misunderstood what the problem is about. The
v1 and v2 in your example is a slice, not a map, which means the test
is comparing slices of undefined order. This is certainly bogus and
needs addressing in the code itself, irrespective of any map-related
issues.
What I generally do in those cases is to define the ordering of the
slice. Besides enabling DeepEquals, it also makes the visual
comparison more pleasant.
On Thu, May 22, 2014 at 11:34 AM, Gustavo Niemeyer
wrote:
> On Wed, May 21, 2014 at 10:43 PM, Ian Booth wrote:
>> We are working to make all juju-core unit tests pass using gccgo. In case you
>> didn't already know, there's a common issue which has caused a lot of the
>> failures to date. Here's a quick heads up on how to deal with it.
>>
>> golang-go and gcc-go have different map implementations which results in
>> ordering differences, affecting things like range etc (simplistically put,
>> gcc-go's map ordering is random whereas currently golang-go is somewhat
>> deterministic).
>
> This is changing in the main compiler as well, in Go 1.3:
>
> http://tip.golang.org/doc/go1.3#map
>
> So it'll become even less deterministic there as well.
>
>> Now of course, maps are unordered but what we sometimes do in
>> the code is to use a map to hold some data (maybe to eliminate duplicates)
>> and
>> then expose that data via a slice or array. If we then do a c.Assert(v1,
>> gc.DeepEquals, v2), it will fail on gcc-go, since the order of items in the 2
>> slices is different, even though the values are the same.
>
> If that's really the case, it's definitely a bug in gccgo. gocheck's
> DeepEquals is implemented in terms of reflect.DeepEqual, which should
> not care about the map order. In the standard library of the main
> compiler, it clearly does not:
>
> for _, k := range v1.MapKeys() {
> if !deepValueEqual(v1.MapIndex(k), v2.MapIndex(k),
> visited, depth+1) {
> return false
> }
> }
>
> So gocheck's DeepEquals is fine for such map tests, assuming no bugs
> in the underlying implementation.
>
>
> gustavo @ http://niemeyer.net
--
gustavo @ http://niemeyer.net
--
Juju-dev mailing list
Juju-dev@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/juju-dev
Re: Ensuring tests pass on gccgo
On Wed, May 21, 2014 at 10:43 PM, Ian Booth wrote:
> We are working to make all juju-core unit tests pass using gccgo. In case you
> didn't already know, there's a common issue which has caused a lot of the
> failures to date. Here's a quick heads up on how to deal with it.
>
> golang-go and gcc-go have different map implementations which results in
> ordering differences, affecting things like range etc (simplistically put,
> gcc-go's map ordering is random whereas currently golang-go is somewhat
> deterministic).
This is changing in the main compiler as well, in Go 1.3:
http://tip.golang.org/doc/go1.3#map
So it'll become even less deterministic there as well.
> Now of course, maps are unordered but what we sometimes do in
> the code is to use a map to hold some data (maybe to eliminate duplicates) and
> then expose that data via a slice or array. If we then do a c.Assert(v1,
> gc.DeepEquals, v2), it will fail on gcc-go, since the order of items in the 2
> slices is different, even though the values are the same.
If that's really the case, it's definitely a bug in gccgo. gocheck's
DeepEquals is implemented in terms of reflect.DeepEqual, which should
not care about the map order. In the standard library of the main
compiler, it clearly does not:
for _, k := range v1.MapKeys() {
if !deepValueEqual(v1.MapIndex(k), v2.MapIndex(k),
visited, depth+1) {
return false
}
}
So gocheck's DeepEquals is fine for such map tests, assuming no bugs
in the underlying implementation.
gustavo @ http://niemeyer.net
--
Juju-dev mailing list
Juju-dev@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/juju-dev
Re: Ensuring tests pass on gccgo
Sorry, I should have made it clearer - we are going to investigate how
best to do it without significantly slowing down landing time, and
running in parallel is an option. We'd really like to do it, but if we
can't then we can make it CI.
On Thu 22 May 2014 18:11:14 EST, John Meinel wrote:
> Please don't actually slow down the time it takes to land code to trunk by
> actually running the tests 2x in a row (and IME gccgo test suite is
> actually more than 1x the time for the golang test suite to run).
> I suppose if you can put them in parallel, but I'd really like to see it
> just be a CI test.
>
> John
> =:->
>
>
>
> On Thu, May 22, 2014 at 5:43 AM, Ian Booth wrote:
>
>> Hi folks
>>
>> We are working to make all juju-core unit tests pass using gccgo. In case
>> you
>> didn't already know, there's a common issue which has caused a lot of the
>> failures to date. Here's a quick heads up on how to deal with it.
>>
>> golang-go and gcc-go have different map implementations which results in
>> ordering differences, affecting things like range etc (simplistically put,
>> gcc-go's map ordering is random whereas currently golang-go is somewhat
>> deterministic). Now of course, maps are unordered but what we sometimes do
>> in
>> the code is to use a map to hold some data (maybe to eliminate duplicates)
>> and
>> then expose that data via a slice or array. If we then do a c.Assert(v1,
>> gc.DeepEquals, v2), it will fail on gcc-go, since the order of items in
>> the 2
>> slices is different, even though the values are the same.
>>
>> In such cases, we need to ensure that we use jc.SameContents rather than
>> DeepEquals. By "such cases", that means where we truly only care about the
>> contents of the data and not the order.
>>
>> Here's a brief example, a snippet of code taken from a constraints test.
>>
>> cons := constraints.MustParse(t.cons)
>> unsupported, err := validator.Validate(cons)
>> if t.err == "" {
>> c.Assert(err, gc.IsNil)
>> c.Assert(unsupported, jc.SameContents, t.unsupported)
>> } else {
>> c.Assert(err, gc.ErrorMatches, t.err)
>> }
>>
>> Above, "unsupported" is a []string of attribute names. We don't care what
>> order
>> they are returned in from Validate(), just that the correct values are all
>> there. So instead of:
>>
>> c.Assert(unsupported, jc.DeepEquals, t.unsupported)
>>
>> we needed to change it to:
>>
>> c.Assert(unsupported, jc.SameContents, t.unsupported)
>>
>> The need to use SameContents depends on the internal implementation of the
>> code
>> under test. But as a general rule, if we use SameContents where we are
>> saying
>> that order of results is not relevant, then we can be sure tests will pass
>> on
>> gcc-go as well as golang-go.
>>
>> If unsure, please test on gcc-go using something like:
>>
>> go test --compiler=gccgo
>>
>> We will soon be gating trunk landings based on *both* golang-go and gcc-go
>> having successful test runs and we'd like it if we didn't introduce more
>> test
>> failures to fix in the interim.
>>
>>
>>
>>
>>
>> --
>> 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: Ensuring tests pass on gccgo
Please don't actually slow down the time it takes to land code to trunk by
actually running the tests 2x in a row (and IME gccgo test suite is
actually more than 1x the time for the golang test suite to run).
I suppose if you can put them in parallel, but I'd really like to see it
just be a CI test.
John
=:->
On Thu, May 22, 2014 at 5:43 AM, Ian Booth wrote:
> Hi folks
>
> We are working to make all juju-core unit tests pass using gccgo. In case
> you
> didn't already know, there's a common issue which has caused a lot of the
> failures to date. Here's a quick heads up on how to deal with it.
>
> golang-go and gcc-go have different map implementations which results in
> ordering differences, affecting things like range etc (simplistically put,
> gcc-go's map ordering is random whereas currently golang-go is somewhat
> deterministic). Now of course, maps are unordered but what we sometimes do
> in
> the code is to use a map to hold some data (maybe to eliminate duplicates)
> and
> then expose that data via a slice or array. If we then do a c.Assert(v1,
> gc.DeepEquals, v2), it will fail on gcc-go, since the order of items in
> the 2
> slices is different, even though the values are the same.
>
> In such cases, we need to ensure that we use jc.SameContents rather than
> DeepEquals. By "such cases", that means where we truly only care about the
> contents of the data and not the order.
>
> Here's a brief example, a snippet of code taken from a constraints test.
>
> cons := constraints.MustParse(t.cons)
> unsupported, err := validator.Validate(cons)
> if t.err == "" {
> c.Assert(err, gc.IsNil)
> c.Assert(unsupported, jc.SameContents, t.unsupported)
> } else {
> c.Assert(err, gc.ErrorMatches, t.err)
> }
>
> Above, "unsupported" is a []string of attribute names. We don't care what
> order
> they are returned in from Validate(), just that the correct values are all
> there. So instead of:
>
> c.Assert(unsupported, jc.DeepEquals, t.unsupported)
>
> we needed to change it to:
>
> c.Assert(unsupported, jc.SameContents, t.unsupported)
>
> The need to use SameContents depends on the internal implementation of the
> code
> under test. But as a general rule, if we use SameContents where we are
> saying
> that order of results is not relevant, then we can be sure tests will pass
> on
> gcc-go as well as golang-go.
>
> If unsure, please test on gcc-go using something like:
>
> go test --compiler=gccgo
>
> We will soon be gating trunk landings based on *both* golang-go and gcc-go
> having successful test runs and we'd like it if we didn't introduce more
> test
> failures to fix in the interim.
>
>
>
>
>
> --
> 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
