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.

Sign up for GitHub

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

Glacier support #19

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Glacier support #19

Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b156b1f
Add support for glacier transitions. Also try to create the specified
kithrup Jun 10, 2017
4c80351
Fix a comment.
kithrup Jun 10, 2017
0c92dcd
Hack fix for an attribute error.
kithrup Jun 10, 2017
6794c2e
Feedback:
kithrup Jun 16, 2017
6190c5c
Fix run-time issues.
kithrup Jun 16, 2017
0c91eca
Thanks to review, and experiment, found out that storage class stays …
kithrup Jun 16, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 81 additions & 6 deletions z3/snap.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,13 +62,13 @@ class S3Snapshot(object):
MISSING_PARENT = 'missing parent'
PARENT_BROKEN = 'parent broken'

def __init__(self, name, metadata, manager, size):
def __init__(self, name, metadata, manager, size, key=None):
self.name = name
self._metadata = metadata
self._mgr = manager
self._reason_broken = None
self.size = size

self.key = key
def __repr__(self):
if self.is_full:
return "<Snapshot {} [full]>".format(self.name)
Expand Down Expand Up @@ -140,7 +140,7 @@ def _snapshots(self):
for key in self.bucket.list(prefix):
key = self.bucket.get_key(key.key)
name = key.key[strip_chars:]
snapshots[name] = S3Snapshot(name, metadata=key.metadata, manager=self, size=key.size)
snapshots[name] = S3Snapshot(name, metadata=key.metadata, manager=self, size=key.size, key=key)
return snapshots

def list(self):
Expand Down Expand Up @@ -405,6 +405,17 @@ def restore(self, snap_name, dry_run=False, force=False):
current_snap = self.s3_manager.get(snap_name)
if current_snap is None:
raise Exception('no such snapshot "{}"'.format(snap_name))
try:
if current_snap.key.ongoing_restore == True:
raise Exception('snapshot {} is currently being restore from glocier; try again later'.format(snap_name))
if current_snap.key.storage_class == "GLACIER":
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm... their documentation states the storage class should remain GLACIER. Is that not the case? http://docs.aws.amazon.com/AmazonS3/latest/dev/restoring-objects.html

Copy link
Author

Choose a reason for hiding this comment

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

Experimenting now. It'll take a few hours for it to happen. I was going by the boto documentation, which seemed to imply it.

Copy link
Author

Choose a reason for hiding this comment

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

Confirmed via experimentation. The expiration count is pretty arbitrary, but I am not sure the best way to make that configurable.

current_snap.key.restore(days=5)
Copy link
Contributor

Choose a reason for hiding this comment

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

I might be getting it wrong but: it seems this only handles glacier-restore for the most recent snapshot. Shouldn't all 'to-be-downloaded' snapshots be glacier-restored in one run?

Would it not make sense to move the glacier-restore logic in to the loop that also figures out which snapshots need downloading, so it can ask for them all to be restored?

Copy link
Author

Choose a reason for hiding this comment

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

I haven't done a restore at all, so it wasn't clear to me what I needed to do. That would be loop that adds to to_restore?

raise Exception('snapshot {} is currently in glacier storage, requesting transfer now'.format(snap_name))
except AttributeError:
# This seems to be if the FakeKey object doesn't have the ongoing_restore attribute
Copy link
Contributor

Choose a reason for hiding this comment

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

Ignoring the AttributeError to get the tests to pass is not very nice. Maybe now it's just because of FakeKey.ongoing_restore, but in the future it could mask a legitimate bug.

A better course of action would be to change FakeKey to this:


class FakeKey(object):
    def __init__(self, name, metadata=None, storage_class="STANDARD_IA"):
        self.name = name
        self.key = name
        self.metadata = metadata
        self.size = 1234
        self.ongoing_restore = None
        self.storage_class = storage_class

Then drop the exception handling all-together.

Copy link
Author

Choose a reason for hiding this comment

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

Okay, making that change in my branch.

pass
except:
Copy link
Contributor

Choose a reason for hiding this comment

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

There's no need to do a blanket except and re-raise, that's Python's default behaviour.

Copy link
Author

Choose a reason for hiding this comment

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

My original code had some other there, I changed it to a pass simply as a placeholder. Fixed.

raise
to_restore = []
while True:
z_snap = self.zfs_manager.get(current_snap.name)
Expand Down Expand Up @@ -519,10 +530,16 @@ def restore(bucket, s3_prefix, filesystem, snapshot_prefix, snapshot, dry, force


def parse_args():
def to_bool(s):
if s.lower() in ("yes", "1", "true", "t", "y"):
return True
return False

cfg = get_config()
parser = argparse.ArgumentParser(
description='list z3 snapshots',
)
parser.register('type', 'bool', to_bool)
parser.add_argument('--s3-prefix',
dest='s3_prefix',
default=cfg.get('S3_PREFIX', 'z3-backup/'),
Expand All @@ -536,6 +553,12 @@ def parse_args():
default=None,
help=('Only operate on snapshots that start with this prefix. '
'Defaults to zfs-auto-snap:daily.'))
parser.add_argument("--use-glacier",
dest='use_glacier',
type=bool,
default=to_bool(cfg.get("USE_GLACIER", "False")),
help='Use glacier for storage')

subparsers = parser.add_subparsers(help='sub-command help', dest='subcommand')

backup_parser = subparsers.add_parser(
Expand Down Expand Up @@ -574,7 +597,7 @@ def main():
args = parse_args()

try:
s3_key_id, s3_secret, bucket = cfg['S3_KEY_ID'], cfg['S3_SECRET'], cfg['BUCKET']
s3_key_id, s3_secret, bucket_name = cfg['S3_KEY_ID'], cfg['S3_SECRET'], cfg['BUCKET']

extra_config = {}
if 'HOST' in cfg:
Expand All @@ -583,8 +606,60 @@ def main():
sys.stderr.write("Configuration error! {} is not set.\n".format(err))
sys.exit(1)

bucket = boto.connect_s3(s3_key_id, s3_secret, **extra_config).get_bucket(bucket)

s3 = boto.connect_s3(s3_key_id, s3_secret, **extra_config)
try:
bucket = s3.get_bucket(bucket_name)
except boto.exception.S3ResponseError as e:
if e.error_code == 'NoSuchBucket':
# Let's try creating it
bucket = s3.create_bucket(bucket_name)
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

print("Created bucket {}: {}".format(bucket_name, bucket), file=sys.stderr)
else:
raise

try:
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please extract the whole bucket life-cycle handling section to a separate function?

Copy link
Author

Choose a reason for hiding this comment

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

Done.

lifecycle = bucket.get_lifecycle_config()
except boto.exception.S3ResponseError:
lifecycle = None

# See if we have a lifecycle rule for glacier.
# The rule name will depend on the S3_PREFIX -- if
# that's empty, then the rule is just "z3 transition";
# otherise, it is "z3 transition ${S3_PREFIX}" (but with
# '/' converted to ' ').
lifecycle_rule_name = "z3 transition"
if args.s3_prefix:
lifecycle_rule_name += " " + args.s3_prefix.replace("/", " ")
while lifecycle_rule_name.endswith(" "):
Copy link
Contributor

Choose a reason for hiding this comment

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

You want to use string.rstrip(" ")

Copy link
Author

Choose a reason for hiding this comment

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

Another holdover, I was originally changing / to - :). Changed.

lifecycle_rule_name = lifecycle_rule_name[:-1]

rule_index = None
for indx, rule in enumerate(lifecycle or []):
if rule.id == lifecycle_rule_name:
rule_index = indx
break

if not args.use_glacier:
# If we don't use glacier, we want to remove the lifecycle policy
# if it exists
if rule_index is not None:
lifecycle.pop(rule_index)
else:
if rule_index is None:
# Okay, we need to add a lifecycle
if lifecycle is None:
lifecycle = boto.s3.lifecycle.Lifecycle()
transition=boto.s3.lifecycle.Transition(days=1, storage_class="GLACIER")
print("trasition rule = {}".format(transition), file=sys.stderr)
print("prefix = {}".format(args.s3_prefix or None), file=sys.stderr)
lifecycle.add_rule(id=lifecycle_rule_name,
status="Enabled",
prefix=args.s3_prefix or None,
transition=transition)
print("lifecycle = {}".format(lifecycle.to_xml()), file=sys.stderr)
if lifecycle is not None:
bucket.configure_lifecycle(lifecycle)
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't the transition to GLACIER happen only on upload?
I'm especially concerned by the fact this alters the storage class even if ran with --dry-run. A dry run should have no side effects.

Copy link
Author

Choose a reason for hiding this comment

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

One of the reasons I did that was that I have it set up to remove the lifecycle rule if use_glacier is no longer in the configuration.

Changed it.


fs_section = "fs:{}".format(args.filesystem)
if args.snapshot_prefix is None:
snapshot_prefix = cfg.get("SNAPSHOT_PREFIX", section=fs_section)
Expand Down