Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Uniter API now has WatchActions #141

Merged
merged 12 commits into from
Jul 9, 2014
4 changes: 4 additions & 0 deletions state/api/uniter/action.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,19 +3,23 @@

package uniter

// Action represents a single instance of an Action call, by name and params.
type Action struct {
name string
params map[string]interface{}
}

// NewAction makes a new Action with specified name and params map.
func NewAction(name string, params map[string]interface{}) (*Action, error) {
return &Action{name: name, params: params}, nil
}

// Name retrieves the name of the Action.
func (a *Action) Name() string {
return a.name
}

// Params retrieves the params map of the Action.
func (a *Action) Params() map[string]interface{} {
return a.params
}
2 changes: 2 additions & 0 deletions state/api/uniter/export_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,5 @@
package uniter

var NewSettings = newSettings

var Call = &call
23 changes: 23 additions & 0 deletions state/api/uniter/unit.go
Original file line number Diff line number Diff line change
Expand Up @@ -437,6 +437,29 @@ func (u *Unit) WatchAddresses() (watcher.NotifyWatcher, error) {
return w, nil
}

// WatchActions returns a StringsWatcher for observing the ids of Actions
// added to the Unit. The initial event will contain the ids of any Actions
// pending at the time the Watcher is made.
func (u *Unit) WatchActions() (watcher.StringsWatcher, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

public method needs a comment

var results params.StringsWatchResults
args := params.Entities{
Entities: []params.Entity{{Tag: u.tag.String()}},
}
err := u.st.call("WatchActions", args, &results)
if err != nil {
return nil, err
}
if len(results.Results) != 1 {
return nil, fmt.Errorf("expected 1 result, got %d", len(results.Results))
}
result := results.Results[0]
if result.Error != nil {
return nil, result.Error
}
w := watcher.NewStringsWatcher(u.st.caller, result)
return w, nil
}

// JoinedRelations returns the tags of the relations the unit has joined.
func (u *Unit) JoinedRelations() ([]string, error) {
var results params.StringsResults
Expand Down
89 changes: 89 additions & 0 deletions state/api/uniter/unit_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,13 @@
package uniter_test

import (
"fmt"
"sort"

"github.com/juju/charm"
"github.com/juju/errors"
"github.com/juju/names"
"github.com/juju/testing"
jc "github.com/juju/testing/checkers"
gc "launchpad.net/gocheck"

Expand Down Expand Up @@ -354,6 +356,93 @@ func (s *unitSuite) TestWatchConfigSettings(c *gc.C) {
wc.AssertClosed()
}

func (s *unitSuite) TestWatchActions(c *gc.C) {
w, err := s.apiUnit.WatchActions()
c.Assert(err, gc.IsNil)

defer statetesting.AssertStop(c, w)
wc := statetesting.NewStringsWatcherC(c, s.BackingState, w)

// Initial event.
wc.AssertChange()

// Add a couple of actions and make sure the changes are detected.
action, err := s.wordpressUnit.AddAction("snapshot", map[string]interface{}{
"outfile": "foo.txt",
})
c.Assert(err, gc.IsNil)
wc.AssertChange(action.Id())

action, err = s.wordpressUnit.AddAction("backup", map[string]interface{}{
"outfile": "foo.bz2",
"compression": map[string]interface{}{
"kind": "bzip",
"quality": float64(5.0),
},
})
c.Assert(err, gc.IsNil)
wc.AssertChange(action.Id())

statetesting.AssertStop(c, w)
wc.AssertClosed()
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test looks good as far as I can tell. It would be good to haves tests which exercise the error returns (by mocking out the API call).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not entirely clear on how to mock up the API call or what pieces need to be assembled for that, versus what pieces are already there. Perhaps we can chat about that tomorrow.

Part of what's confusing me here is that the WatchActions call is a method on Unit, and I don't really see how to put together a Unit that won't have a valid WatchActions call, yet will be a valid Unit.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not suggesting you try to hit the error handling lines during the tests by hitting the real API. With a little restructuring you should be able to have it so you can replace the API call (currently u.st.call()) with a error-returning implementation during tests.

The trick is to introduce a function variable that returns the function to call to make API calls. See state/api/usermanager/client_test.go for an example. Specifically TestUserInfoNoResults and TestUserInfoMoreThanOneResult. They replace call() in client.go in order to simulate various error conditions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can give that a shot, but are you saying we should entirely replace the current u.st.call() approach in state/api/uniter/unit.go? It seems like something which we can logically deduce will only fail in the event of a larger failure of the RPC mechanism.

Right now our ActionsWatcher tests on State are exercising the potential for the duplicate StringsWatcherResponses and handling those errors, so if duplicates are coming down the wire, it's clearly a fault with the RPC mechanism here, which should itself be testing for such conditions.

I'm open to being wrong here, but the reason my tests are somewhat minimal on this method is that the mechanisms are being exercised in their respective packages, and this test is simply ensuring that the RPC calls are working suitably. We're really trying to avoid spinning our wheels in the API; these PRs are backlogging work on filter, which needs to happen in order to push forward on Actions.

Again, if you think this isn't a consistent argument, we can go ahead and push the requested changes through. I just want to be sure we're not expending significant effort here with minimal benefit when we have an obligation to move forward to get an end-to-end Action pulled together.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding these tests.


func (s *unitSuite) TestWatchActionsError(c *gc.C) {
restore := testing.PatchValue(uniter.Call, func(st *uniter.State, method string, params, results interface{}) error {
return fmt.Errorf("Test error")
})

defer restore()

_, err := s.apiUnit.WatchActions()
c.Assert(err.Error(), gc.Equals, "Test error")
}

func (s *unitSuite) TestWatchActionsErrorResults(c *gc.C) {
restore := testing.PatchValue(uniter.Call, func(st *uniter.State, method string, args, results interface{}) error {
if results, ok := results.(*params.StringsWatchResults); ok {
results.Results = make([]params.StringsWatchResult, 1)
results.Results[0] = params.StringsWatchResult{
Error: &params.Error{
Message: "An error in the watch result.",
Code: params.CodeNotAssigned,
},
}
}
return nil
})

defer restore()

_, err := s.apiUnit.WatchActions()
c.Assert(err.Error(), gc.Equals, "An error in the watch result.")
}

func (s *unitSuite) TestWatchActionsNoResults(c *gc.C) {
restore := testing.PatchValue(uniter.Call, func(st *uniter.State, method string, params, results interface{}) error {
return nil
})

defer restore()

_, err := s.apiUnit.WatchActions()
c.Assert(err.Error(), gc.Equals, "expected 1 result, got 0")
}

func (s *unitSuite) TestWatchActionsMoreResults(c *gc.C) {
restore := testing.PatchValue(uniter.Call, func(st *uniter.State, method string, args, results interface{}) error {
if results, ok := results.(*params.StringsWatchResults); ok {
results.Results = make([]params.StringsWatchResult, 2)
}
return nil
})

defer restore()

_, err := s.apiUnit.WatchActions()
c.Assert(err.Error(), gc.Equals, "expected 1 result, got 2")
}

func (s *unitSuite) TestServiceNameAndTag(c *gc.C) {
c.Assert(s.apiUnit.ServiceName(), gc.Equals, "wordpress")
c.Assert(s.apiUnit.ServiceTag(), gc.Equals, "service-wordpress")
Expand Down
8 changes: 6 additions & 2 deletions state/api/uniter/uniter.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,12 @@ func NewState(caller base.Caller, authTag names.UnitTag) *State {
}
}

func (st *State) call(method string, params, results interface{}) error {
return st.caller.Call(uniterFacade, "", method, params, results)
var call = func(st *State, method string, args, results interface{}) error {
return st.caller.Call(uniterFacade, "", method, args, results)
}

func (st *State) call(method string, args, results interface{}) error {
return call(st, method, args, results)
}

// life requests the lifecycle of the given entity from the server.
Expand Down
46 changes: 46 additions & 0 deletions state/apiserver/uniter/uniter.go
Original file line number Diff line number Diff line change
Expand Up @@ -432,6 +432,22 @@ func (u *UniterAPI) watchOneUnitConfigSettings(tag string) (string, error) {
}
return "", watcher.MustErr(watch)
}
func (u *UniterAPI) watchOneUnitActions(tag string) (params.StringsWatchResult, error) {
nothing := params.StringsWatchResult{}
unit, err := u.getUnit(tag)
if err != nil {
return nothing, err
}
watch := unit.WatchActions()

if changes, ok := <-watch.Changes(); ok {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see a new test that exercises this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assumed that since the only error cases here arise from other packages and types, which are suitably tested, that it would be redundant to test them here.

unit, err := u.getUnit(tag)
watch := unit.WatchActions()
return nothing, watcher.MustErr(watch)

In each case, error values will only happen in cases of bad values being passed through this method.

If redundant tests are required, that's fine! I've been assuming that errors in methods outside of mine are exercised in those methods. I'm only wondering about what extent is appropriate.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, sorry, I was referring to the changes result. I didn't see a test that started a watcher with actions already queued, and verified that they came back in the initial return value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fwereade, I've added a test for it, but I believe there's an initial empty event generated when the watcher is added. Take a look:

func (s *uniterSuite) TestWatchPreexistingActions(c *gc.C) {                  
        err := s.wordpressUnit.SetCharmURL(s.wpCharm.URL())                   
        c.Assert(err, gc.IsNil)                                               

        c.Assert(s.resources.Count(), gc.Equals, 0)                           

        firstActionId, err := s.wordpressUnit.AddAction("backup", nil)        
        c.Assert(err, gc.IsNil)                                               
        secondActionId, err := s.wordpressUnit.AddAction("snapshot", nil)     
        c.Assert(err, gc.IsNil)                                               

        args := params.Entities{Entities: []params.Entity{                    
                {Tag: "unit-wordpress-0"},                                    
        }}                                                                    

        result, err := s.uniter.WatchActions(args)                            
        c.Assert(err, gc.IsNil)                                               
        c.Assert(result, gc.DeepEquals, params.StringsWatchResults{           
                Results: []params.StringsWatchResult{                         
                        {StringsWatcherId: "1", Changes: []string{}},         
                },                                                            
        })                                                                    

        // Verify the resource was registered and stop when done              
        c.Assert(s.resources.Count(), gc.Equals, 1)                           
        resource := s.resources.Get("1")                                      
        defer statetesting.AssertStop(c, resource)                            

        // Check that the Watch has consumed the initial event ("returned" in 
        // the Watch call)                                                    
        wc := statetesting.NewStringsWatcherC(c, s.State, resource.(state.StringsWatcher))
        wc.AssertChange(firstActionId, secondActionId)                        
        wc.AssertNoChange()                                                   

        actionId, err := s.wordpressUnit.AddAction("backup", nil)             
        c.Assert(err, gc.IsNil)                                               
        wc.AssertChange(actionId)                                             
        wc.AssertNoChange()                                                   
}

return params.StringsWatchResult{
StringsWatcherId: u.resources.Register(watch),
Changes: changes,
}, nil
}
return nothing, watcher.MustErr(watch)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, that MustErr is a problem. Is it the same everywhere? Even if a single watcher is somehow screwed up, it shouldn't be able to take out the whole API server...

Wouldn't surprise me if we had the damn things everywhere, so don't feel obliged if it's consistent -- but please do add a bug pointing out that MustErr is not safe and should basically never be used except in client code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}

// WatchConfigSettings returns a NotifyWatcher for observing changes
// to each unit's service configuration settings. See also
Expand All @@ -456,6 +472,34 @@ func (u *UniterAPI) WatchConfigSettings(args params.Entities) (params.NotifyWatc
return result, nil
}

// WatchActions returns an ActionWatcher for observing incoming action calls
// to a unit. See also state/watcher.go Unit.WatchActions(). This method
// is called from state/api/uniter/uniter.go WatchActions().
func (u *UniterAPI) WatchActions(args params.Entities) (params.StringsWatchResults, error) {
nothing := params.StringsWatchResults{}

result := params.StringsWatchResults{
Results: make([]params.StringsWatchResult, len(args.Entities)),
}
canAccess, err := u.accessUnit()
if err != nil {
return nothing, err
}
for i, entity := range args.Entities {
_, err := names.ParseUnitTag(entity.Tag)
if err != nil {
return nothing, err
}

err = common.ErrPerm
if canAccess(entity.Tag) {
result.Results[i], err = u.watchOneUnitActions(entity.Tag)
}
result.Results[i].Error = common.ServerError(err)
}
return result, nil
}

// ConfigSettings returns the complete set of service charm config
// settings available to each given unit.
func (u *UniterAPI) ConfigSettings(args params.Entities) (params.ConfigSettingsResults, error) {
Expand Down Expand Up @@ -696,6 +740,8 @@ func (u *UniterAPI) getOneActionByTag(tag names.ActionTag) (params.ActionsQueryR
return result, nil
}

// Actions returns the Actions by Tags passed and ensures that the Unit asking
// for them is the same Unit that has the Actions.
func (u *UniterAPI) Actions(args params.Entities) (params.ActionsQueryResults, error) {
nothing := params.ActionsQueryResults{}

Expand Down
121 changes: 121 additions & 0 deletions state/apiserver/uniter/uniter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -749,6 +749,127 @@ func (s *uniterSuite) TestWatchConfigSettings(c *gc.C) {
wc.AssertNoChange()
}

func (s *uniterSuite) TestWatchActions(c *gc.C) {
err := s.wordpressUnit.SetCharmURL(s.wpCharm.URL())
c.Assert(err, gc.IsNil)

c.Assert(s.resources.Count(), gc.Equals, 0)

args := params.Entities{Entities: []params.Entity{
{Tag: "unit-mysql-0"},
{Tag: "unit-wordpress-0"},
{Tag: "unit-foo-42"},
}}
result, err := s.uniter.WatchActions(args)
c.Assert(err, gc.IsNil)
c.Assert(result, gc.DeepEquals, params.StringsWatchResults{
Results: []params.StringsWatchResult{
{Error: apiservertesting.ErrUnauthorized},
{StringsWatcherId: "1", Changes: []string{}},
{Error: apiservertesting.ErrUnauthorized},
},
})

// Verify the resource was registered and stop when done
c.Assert(s.resources.Count(), gc.Equals, 1)
resource := s.resources.Get("1")
defer statetesting.AssertStop(c, resource)

// Check that the Watch has consumed the initial event ("returned" in
// the Watch call)
wc := statetesting.NewStringsWatcherC(c, s.State, resource.(state.StringsWatcher))
wc.AssertNoChange()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be worthwhile to finish the test by generating an event and seeing it come out of the watcher? I'm concerned that the wc.AssertNoChange could just be passing because the watcher isn't working at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good thought. Added.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's useful seeing the events coming out of the watcher, vs seeing (1) initial events coming out of the initial API return and (2) subsequent events coming out of subsequent Next() calls with that watcher id.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fwereade, not certain whether this was addressed. I'm not clear on what you're thinking of.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As noted further down, I don't think we have a test for initial events. Testing subsequent events by pulling the watcher out of resources and checking it's active -- as you already did -- is fine, so we don't need to muck about with calling Next() methods for further verification of the later events, but initial events do need tests.

Look at it this way: is there a test that would fail if you reinstated the _, ok := watcher.Changes() problem we caught yesterday? If not, we need one.


addedAction, err := s.wordpressUnit.AddAction("snapshot", nil)

wc.AssertChange(addedAction.Id())
wc.AssertNoChange()
}

func (s *uniterSuite) TestWatchPreexistingActions(c *gc.C) {
err := s.wordpressUnit.SetCharmURL(s.wpCharm.URL())
c.Assert(err, gc.IsNil)

c.Assert(s.resources.Count(), gc.Equals, 0)

firstAction, err := s.wordpressUnit.AddAction("backup", nil)
c.Assert(err, gc.IsNil)
secondAction, err := s.wordpressUnit.AddAction("snapshot", nil)
c.Assert(err, gc.IsNil)

args := params.Entities{Entities: []params.Entity{
{Tag: "unit-wordpress-0"},
}}

result, err := s.uniter.WatchActions(args)
c.Assert(err, gc.IsNil)
c.Assert(result, gc.DeepEquals, params.StringsWatchResults{
Results: []params.StringsWatchResult{
{StringsWatcherId: "1", Changes: []string{
firstAction.Id(),
secondAction.Id(),
}},
},
})

// Verify the resource was registered and stop when done
c.Assert(s.resources.Count(), gc.Equals, 1)
resource := s.resources.Get("1")
defer statetesting.AssertStop(c, resource)

// Check that the Watch has consumed the initial event ("returned" in
// the Watch call)
wc := statetesting.NewStringsWatcherC(c, s.State, resource.(state.StringsWatcher))
// TODO: @jcw4 -- this should be:
// wc.AssertNoChange()
wc.AssertChange(firstAction.Id(), secondAction.Id())

addedAction, err := s.wordpressUnit.AddAction("backup", nil)
c.Assert(err, gc.IsNil)
wc.AssertChange(addedAction.Id())
wc.AssertNoChange()
}

func (s *uniterSuite) TestWatchActionsMalformedTag(c *gc.C) {
args := params.Entities{Entities: []params.Entity{
{Tag: "ewenit-mysql-0"},
}}
_, err := s.uniter.WatchActions(args)
c.Assert(err, gc.NotNil)
c.Assert(err.Error(), gc.Equals, `"ewenit-mysql-0" is not a valid tag`)
}

func (s *uniterSuite) TestWatchActionsMalformedUnitName(c *gc.C) {
args := params.Entities{Entities: []params.Entity{
{Tag: "unit-mysql-01"},
}}
_, err := s.uniter.WatchActions(args)
c.Assert(err, gc.NotNil)
c.Assert(err.Error(), gc.Equals, `"unit-mysql-01" is not a valid unit tag`)
}

func (s *uniterSuite) TestWatchActionsNotUnit(c *gc.C) {
args := params.Entities{Entities: []params.Entity{
{Tag: "action-mysql/0_a_0"},
}}
_, err := s.uniter.WatchActions(args)
c.Assert(err, gc.NotNil)
c.Assert(err.Error(), gc.Equals, `"action-mysql/0_a_0" is not a valid unit tag`)
}

func (s *uniterSuite) TestWatchActionsPermissionDenied(c *gc.C) {
args := params.Entities{Entities: []params.Entity{
{Tag: "unit-nonexistentgarbage-0"},
}}
results, err := s.uniter.WatchActions(args)
c.Assert(err, gc.IsNil)
c.Assert(results, gc.NotNil)
c.Assert(len(results.Results), gc.Equals, 1)
result := results.Results[0]
c.Assert(result.Error, gc.NotNil)
c.Assert(result.Error.Message, gc.Equals, "permission denied")
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, this test looks fine but it would be good to have tests for the error returns if possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My assumption here is that methods such as SetCharmURL and Unit.accessUnit are suitably tested in their respective packages; apiservertesting.ErrUnauthorized is the error which should result from an unauthorized Watcher query. Perhaps @johnweldon would have some input here however since we are cooperating on the Watcher code. I'm simply not seeing where to exercise errors here or what sorts of errors I can expect from the Watcher, since this is in reality simply an API call to the method on State. The WatchActions method on State is, I believe, adequately tested.


func (s *uniterSuite) TestConfigSettings(c *gc.C) {
err := s.wordpressUnit.SetCharmURL(s.wpCharm.URL())
c.Assert(err, gc.IsNil)
Expand Down