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

WIP: Add a juju subcommand for backups. #97

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
72 changes: 72 additions & 0 deletions cmd/juju/backup.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
// Copyright 2014 Canonical Ltd.
// Licensed under the AGPLv3, see LICENCE file for details.

package main

import (
"fmt"

"github.com/juju/cmd"
"github.com/juju/juju/cmd/envcmd"
"github.com/juju/juju/juju"
"github.com/juju/juju/state/api"
)

type backupClient interface {
Backup(backupFilePath string) (string, error)
}

type BackupCommand struct {
/* XXX Shuld this really be an env-specific command? */
Copy link
Member

Choose a reason for hiding this comment

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

Yes. You want to backup the data for a specific environment. Almost everything that "juju" does is in the context of 1 environment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. I'm not sure what I was thinking. :)

envcmd.EnvCommandBase
Filename string
}

var backupDoc = fmt.Sprintf(`
This command will generate a backup of juju's state as a local gzipped tar
file (.tar.gz). If no filename is provided, one is generated with a name
like this:

%s

where <timestamp> is the timestamp of when the backup is generated.

The filename of the generated archive will be printed upon success.
`, fmt.Sprintf(api.BACKUP_FILENAME, "<timestamp>"))
Copy link
Member

Choose a reason for hiding this comment

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

Given there is a "%s" in BACKUP_FILENAME, should this actually be called something like BACKUP_FILENAME_TEMPLATE or BACKUP_FILENAME_FORMAT?
Also, AIUI our general practice in Go is to not use ALL_CAPS_STYLE for constants.
we would just use "api.BackupFilenameTemplate"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. And please continue to let me know when I'm doing something non-idiomatic.


func (c *BackupCommand) Info() *cmd.Info {
return &cmd.Info{
Name: "backup",
Args: "[filename]",
Purpose: "create a backup of juju's state",
Doc: backupDoc,
}
}

func (c *BackupCommand) Init(args []string) error {
filename, err := cmd.ZeroOrOneArgs(args)
if err == nil {
c.Filename = filename
}
return err
}

func (c *BackupCommand) run(ctx *cmd.Context, client backupClient) error {
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason we want to split out run() from Run() here?

I'm sure we had a few commands that had this structure because of compatibility code paths (run one way for API only code, and have an alternate runDirect sort of code path when we don't have an API server.)

If we do want it split out, then I would probably call it something more significant, like "runBackup".

Note also, we'll want to have code that handles when the API doesn't provide Backup support.
It probably just needs to detect that backup isn't supported and let the user know that their server just doesn't support backup as an operation.

If my API Versioning code lands, then it would be a version check on the Backup api. If it hasn't landed yet, then it would be trying to call client.Backup, and checking if the error return from the call is CodeNotImplemented.

I can see that this did let you split out the call to run() for testing purposes, which is neat, so probably we just want to name it something a bit more descriptive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I did this so I could use a fake client in the test code. I'll fix the name. I'm still adjusting to Go's namespace model (a.k.a. packages). :)

As to handling lack of backup support, this changeset would definitely require that the client already Backup() implemented. Sorry. I wasn't clear that this PR wasn't meant to be stand-alone. In the meantime users can continue to use the plugin.

update: See later note about API compatibility.

filename, err := client.Backup(c.Filename)
if err != nil {
return err
}

fmt.Fprintln(ctx.Stdout, filename)
return nil
}

func (c *BackupCommand) Run(ctx *cmd.Context) error {
client, err := juju.NewAPIClientFromName(c.EnvName)
if err != nil {
return err
}
defer client.Close()

return c.run(ctx, client)
}
105 changes: 105 additions & 0 deletions cmd/juju/backup_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
// Copyright 2014 Canonical Ltd.
// Licensed under the AGPLv3, see LICENCE file for details.

package main

import (
"errors"
"fmt"

"github.com/juju/cmd"
gc "launchpad.net/gocheck"

"github.com/juju/juju/state/api"
"github.com/juju/juju/testing"
)

type BackupCommandSuite struct {
testing.BaseSuite
}

var _ = gc.Suite(&BackupCommandSuite{})

// a fake client and command

var fakeBackupFilename = fmt.Sprintf(api.BACKUP_FILENAME, 1402686077)

type fakeBackupClient struct {
err error
}

func (c *fakeBackupClient) Backup(backupFilePath string) (string, error) {
if c.err != nil {
return "", c.err
}

if backupFilePath == "" {
backupFilePath = fakeBackupFilename
}
/* Don't actually do anything! */
return backupFilePath, c.err
}

type fakeBackupCommand struct {
BackupCommand
client fakeBackupClient
}

func (c *fakeBackupCommand) Run(ctx *cmd.Context) error {
return c.run(ctx, &c.client)
}

// help tests

func (s *BackupCommandSuite) TestBackupHelp(c *gc.C) {
// Check the help output.

info := (&BackupCommand{}).Info()

expected := fmt.Sprintf(`
usage: juju %s [options] %s
purpose: %s

options:
...

%s
`, info.Args, info.Purpose, info.Doc)

ctx, err := testing.RunCommand(c, &BackupCommand{}, "--help")

c.Assert(err, gc.IsNil)
c.Assert(testing.Stdout(ctx), gc.Matches, expected)
}

// options tests

func (s *BackupCommandSuite) TestBackupDefaults(c *gc.C) {
client := fakeBackupClient{}
command := fakeBackupCommand{client: client}

ctx, err := testing.RunCommand(c, &command)

c.Assert(err, gc.IsNil)
c.Assert(testing.Stdout(ctx), gc.Matches, fakeBackupFilename+"\n")
}

func (s *BackupCommandSuite) TestBackupFilename(c *gc.C) {
filename := "foo.tgz"
client := fakeBackupClient{}
command := fakeBackupCommand{client: client}

ctx, err := testing.RunCommand(c, &command, filename)

c.Assert(err, gc.IsNil)
c.Assert(testing.Stdout(ctx), gc.Matches, filename+"\n")
}

func (s *BackupCommandSuite) TestBackupError(c *gc.C) {
client := fakeBackupClient{err: errors.New("something went wrong")}
command := fakeBackupCommand{client: client}

_, err := testing.RunCommand(c, &command)

c.Assert(err, gc.NotNil)
}
Copy link
Member

Choose a reason for hiding this comment

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

Given that you have the "err" functionality, we could test the compatibility code. I do really like how you've ended up structuring this, as it makes it easy to test the various edge cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Blame my experience with unit tests in Python. :)

3 changes: 3 additions & 0 deletions cmd/juju/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,9 @@ func registerCommands(r commandRegistry, ctx *cmd.Context) {
// Manage users and access
r.Register(NewUserCommand())

// Manage backups.
r.Register(wrapEnvCommand(&BackupCommand{}))

// Manage state server availability.
Copy link
Member

Choose a reason for hiding this comment

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

So to test that you've correctly Register'd the command, I've generally added a test that "juju backup -h" prints out the expected backupDoc.
If you look it "main_test.go" there are quite a few tests like ""sync-tools", "--help"".

The general idea is that if I can comment out that line, and a test doesn't fail, then we're probably missing something. :)

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 have a test that does that, but more vaguely using testing.RunCommand() rather than badrun(). However, I only did that because I had trouble getting badrun to work right. I'll revisit that. Thanks for pointing it out.

r.Register(wrapEnvCommand(&EnsureAvailabilityCommand{}))
}
Expand Down
1 change: 1 addition & 0 deletions cmd/juju/main_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,7 @@ var commandNames = []string{
"api-endpoints",
"authorised-keys", // alias for authorized-keys
"authorized-keys",
"backup",
"bootstrap",
"debug-hooks",
"debug-log",
Expand Down
7 changes: 7 additions & 0 deletions state/api/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -593,6 +593,13 @@ func (c *Client) DestroyEnvironment() error {
return c.call("DestroyEnvironment", nil, nil)
}

const BACKUP_FILENAME = "jujubackup-%d.tar.gz"
Copy link
Member

Choose a reason for hiding this comment

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

Offhand, this really doesn't feel like it should be in the state/api file.
This seems very specific to how the CLI is going to interact with the API. And as such, it is entirely appropriate to have the constant in the CLI code.
It might be that there would be a constant somewhere inside the apiserver that would be saying something like (psuedocode):
if filename == "" {
filename = fmt.Sprintf(FilenameTemplate, time.Now())
}
sort of thing. However, it would be inappropriate (IMO) to have the CLI code peeking at constants in "apiserver" code.
So from that point of view, it might just be more appropriate to not have a template at all that we try to share, and just write it as explicit text in the help doc.
Thoughts?

Choose a reason for hiding this comment

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

Actually that code is effectively already in the state/api file in a PR of mine. Eric has just factored it out here to reuse it. (It's the default filename used by the client Backup method.) An alternative would be for that code to not use a default and have the CLI always pass in a filename. The danger of just hardcoding it in the help message is that we could change the format and then forget to change the help message too. (There isn't a constant in the apiserver code as it doesn't send a filename.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Like Michael said, this just factors out the default filename from his changeset. I figured we could add the template into his backup API code. One advantage is that I was able to use the template in the help message, so it should always stay in sync. I was just imagining what would make things easier if at some point we switched to bz2, etc.


func (c *Client) Backup(backupFilePath string) (string, error) {
/* XXX stub */
return backupFilePath, nil
}

// AddLocalCharm prepares the given charm with a local: schema in its
// URL, and uploads it via the API server, returning the assigned
// charm URL. If the API server does not support charm uploads, an
Expand Down