[PATCH] Implement list-vms command

Roman Kennke rkennke at redhat.com
Thu Apr 12 22:04:03 UTC 2012


Hiya!

> > > > This patch implements the first useful command, list-vms. This
> > > > command
> > > > prints out a list of currently monitored VMs.
> > > > 
> > > > A few implementation notes:
> > > > 
> > > > - I added a generic attribute interface to ApplicationContext to
> > > > store
> > > > and retrieve other objects than we have in the API. Motivation
> > > > for this
> > > > is that some modules will require different things in the
> > > > AppContext
> > > > than others. Access to this is done by an enum, and should go
> > > > through an
> > > > accessor-helper-class that does the casting.
> > > 
> > > Not sure how I feel about making it easier to add more global
> > > state.
> > 
> > Yeah, I was unsure about this as well... I don't like it much. But
> > then
> > I found that the ApplicationContext can be used in a very generic way
> > by
> > the different modules. There are some things (like the DAOFactory)
> > that
> > seems useful in all/most of them, and some (like the CLI specific
> > stuff,
> > CommandRegistry would be a candidate) doesn't make any sense to put
> > in
> > shared code. I see the following solutions:
> > 
> > - Like I did, a generic attribute map, keyed by enums. I chose enums
> > over strings or even objects, exactly to restrict the number of stuff
> > that can be stored there. However, we still need to be very careful
> > with
> > it. Access would usually not go through this, but some accessor
> > utility
> > class, which works much like the ApplicationContext access now, but
> > does
> > the internal mapping to the enums and also the casting to the correct
> > type.
> > - We could have subclasses ApplicationContext for modules that need
> > more
> > specific stuff. For example, we could have:
> > 
> > public class CLIApplicationContext extends ApplicationContext {
> >   public static CLIApplicationContext getInstance() {
> >     return (CLIApplicationContext) ApplicationContext.getInstance();
> >   }
> > 
> >  ...
> > }
> > 
> > I haven't thought of this before, but now that I write it down, it
> > looks
> > much nicer than the enum-accessor-class thingy.
> > 
> > - Of course, those 2 ideas could be mixed up. Have one superclass
> > that
> > is only a map, and subclasses that provide specific accessors to
> > that.
> > The map accessors could be made protected, so that no client code can
> > use it directly. Not sure if I would prefer that or the just-fields
> > storage. I guess it doesn't really matter much.
> > 
> > What do you think?
> > 
> 
> I also share the unlike of adding more global context, would very much
> prefer that for more specific things we subclass ApplicationContext, and
> add specific accessor methods for those things needed.  Is much more
> clear.

Yeah. Although, with my latest changes (to be posted for review soon), I
no longer need it.. for now. (We *could* put the CommandContextFactory
there, instead of making it a singleton of its own though.)

> > > > - The DB connection and ApplicationContext is set up in
> > > > AppContextSetup
> > > > class (this will be reused by other commands).
> > > > - Argument parsing is done by JOpt-Simple.
> > > > 
> > > > For an example of how this looks like, see here:
> > > > http://rkennke.wordpress.com/2012/04/12/thermostat-cli/
> > > > 
> > > 
> > > Thanks for that link which shows the command active. Looking at
> > > this, I
> > > wonder if the adding more commands like this is the right way. The
> > > existing commands do major things (start agent, start db, (TODO:
> > > start
> > > gui client)), while this one is a "small" command. Seems to me like
> > > we
> > > need a similar major command. How about changing it to something
> > > like
> > > this instead:
> > > 
> > > thermostat query list-vms
> > > 
> > > What do you think?
> > 
> > Hmm, not sure, why is starting the db/agent/etc more major than the
> > real
> > usage? To me it's almost the other way around, starting the services
> > is
> > only needed because we implemented it that way, what the user really
> > wants is to drill down into his problem. I.e.:
> > 
> > - Start Thermostat services
> > - Find the VM (using list-vms)
> > - Print some memory usage stats (using (TODO) memory-stats)
> > - Find memory leak (using (TODO) find-memory-leak)
> > .. etc
> > 
> > Prefixing all of them with 'query' doesn't seem to add value or
> > usability IMO. And it would complicate the implementation quite a
> > bit.
> > 
> 
> I don't see the value in 'query' prefix either, but this leads me
> to think beyond.  What about this:
> 
> Rather than implementing separate commands to be entered from the
> command line, implement a command that begins an interpreter mode.
> Then several commands can be issued in a single run (without
> connecting/disconnecting from the database).
> 
> $ thermostat interpreter
> Connecting to database...
> Ready.
> | list-hosts
> {list of hosts}
> 
> | list-vms host1
> {list of vms}
> 
> | gc-stats vm3
> {some summary}
> 
> | config set-value foo bar
> OK
> 
> |
> 
> etc etc etc...
> 
> Of course, done carefully the same command implementations
> could be used for both an interpreter mode and command line
> single-shot use.

You know what? This is *exactly* the plan. We will have our own
interpreter mode which works just like that. Plus: there will be
commands that carry over some state (the ongoing DB connection is one
such thing, but I can also imagine something like
breakpoints/watchpoints etc that are not possible to implement with a
one-shot command). The current CLI architecture already takes that into
account. We basically only need that interpreter thingy and plug in the
existing commands and it should just work.

> > > > There are situations that are not yet handled well:
> > > > - when the DB is not running
> > > > - when the agent is not running (missing host-info collection)
> > > > 
> > > > Both result in not very useful (for ordinary users) stacktraces.
> > > > This
> > > > needs to be addressed in another iteration.
> > > > 
> > > > Ok to go in?
> > > 
> > > More comments below.
> > > 
> > > > diff --git
> > > > a/tools/src/main/java/com/redhat/thermostat/tools/cli/AppContextSetup.java
> > > > b/tools/src/main/java/com/redhat/thermostat/tools/cli/AppContextSetup.java
> > > > new file mode 100644
> > > > --- /dev/null
> > > > +++
> > > > b/tools/src/main/java/com/redhat/thermostat/tools/cli/AppContextSetup.java
> > > 
> > > > +class AppContextSetup {
> > > > +
> > > > +    void setupAppContext(String dbUrl) {
> > > > +        StartupConfiguration config = new
> > > > ConnectionConfiguration(dbUrl);
> > > > +
> > > > +        ConnectionProvider connProv = new
> > > > MongoConnectionProvider(config);
> > > > +        DAOFactory daoFactory = new MongoDAOFactory(connProv);
> > > > +        Connection connection = daoFactory.getConnection();
> > > > +        connection.connect();
> > > > +
> > > >        ApplicationContext.getInstance().setDAOFactory(daoFactory);
> > > > +
> > > 
> > > This bit here looks like a duplicate of/similar to what's in the
> > > gui
> > > client's main. Perhaps this code can be shared?
> > 
> > Yes, there is potential for sharing here for sure. I need to see what
> > exactly we can do.
> > 
> 
> I think that Agent could use this more directly than the guiclient.
> Especially once the promised handling of failed connections is
> implemented ;)
> 
> The gui client has some differences, in that it needs to first set
> the appropriate connection parameters before trying to connect.

Yeah. I moved the AppContextSetup to common, so we can reuse it in other
places. Even the GUI client should only set the dbUrl, no?

> > > > diff --git
> > > > a/tools/src/main/java/com/redhat/thermostat/tools/cli/CLIAppContextUtils.java
> > > > b/tools/src/main/java/com/redhat/thermostat/tools/cli/CLIAppContextUtils.java
> > > > new file mode 100644
> > > > --- /dev/null
> > > > +++
> > > > b/tools/src/main/java/com/redhat/thermostat/tools/cli/CLIAppContextUtils.java
> > > 
> > > > +public class CLIAppContextUtils {
> > > > +
> > > > +    public static AppContextSetup getAppContextSetup() {
> > > > +        ApplicationContext ctx =
> > > > ApplicationContext.getInstance();
> > > > +        AppContextSetup setup = (AppContextSetup)
> > > > ctx.getAttribute(CLIAppContext.SETUP);
> > > > +        if (setup == null) {
> > > > +            setup = new AppContextSetup();
> > > > +            setAppContextSetup(setup);
> > > > +        }
> > > > +        return setup;
> > > > +    }
> > > > +
> > > > +    public static void setAppContextSetup(AppContextSetup setup)
> > > > {
> > > > +        ApplicationContext ctx =
> > > > ApplicationContext.getInstance();
> > > > +        ctx.setAttribute(CLIAppContext.SETUP, setup);
> > > > +    }
> > > > +
> > > 
> > > I don't see why the AppContextSetup has to be stored as an
> > > attribute.
> > > Can't we just construct AppContextSetup once when we need it?
> > 
> > Hmm, yes, that is something I don't really know how to resolve. The
> > problem I had was how to test the command without actually connecting
> > to
> > the DB. The command was clearly doing 2 things: setting up the
> > DAOFactory/Connection/etc, based on the --dbUrl option, and then
> > doing
> > the actual query. My idea was to factor out the setting-up of
> > connection
> > related things into the AppContextSetup class, and then somehow
> > inject
> > it. And 'somehow' turned out to be the ApplicationContext. However,
> > as
> > you correctly pointed out, it doesn't really make much sense there,
> > so I
> > need to figure out a better way to do that. But what? Can I inject it
> > through a setter from the command framework? Maybe. But we don't need
> > it
> > for all the commands (we could ignore it and never call it though).
> > Should the command ask some sort of factory to create an
> > AppContextSetup
> > when it needs one? But then we need to have a way to inject the
> > factory... it only seems to defer the problem and/or create more
> > singletones. Ideas? Maybe I am blind already.
> > 
> 
> Yeah, I like the idea of adding it (maybe even by constructor) to
> Command.  Then for specific command that should not even need it we
> can test that it isn't called :)
> 
> Although, I can't imagine a thermostat cli tool that didn't need
> to connect to the database.

I added it in the CommandContext. This way, no change in the API of
Command is needed, a Command impl can get the thing *when* it needs it
(and as you said, most cmds will need it), and we have a nice way to
inject a mock for testing (even automagically via
TestCommandContextFactory).

Need to finish off the thing tomorrow, will send another review.

Cheers,
Roman





More information about the Thermostat mailing list