-
Notifications
You must be signed in to change notification settings - Fork 401
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
Feature/299 PIT backups #301
base: master
Are you sure you want to change the base?
Conversation
@@ -70,6 +70,7 @@ def choose_backup(output, recovery_target_time): | |||
match_timestamp = match = None | |||
for backup in backup_list: | |||
last_modified = parse(backup['last_modified']) | |||
last_modified = last_modified.replace(tzinfo=recovery_target_time.tzinfo) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First of all I don't really understand how you managed to get such exception.
- Spilo is using UTC timezone.
- wal-e backup-list produces last_modified in the format
2019-02-09T01:01:36.000Z
- wal-g backup-list produces last_modified in the format
2019-02-09T01:01:36Z
All aforementioned facts will produce datetime objects with tzinfo either tzlocal
or tzutc
(for AWS S3).
You are changing the timezone in the last_modified timestamp from wal-e/wal-g backup-list output to a timezone from --recovery-target-time. This is undesirable.
Can it be that you passed the CLONE_TARGET_TIME
without specifying a timezone?
If you are using GCS could you verify the timestamp format in the backup-list output? Does it have a timezone? If it doesn't, how it relates to UTC? Is it possible to convert it to UTC?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are changing the timezone in the last_modified timestamp from wal-e/wal-g backup-list output to a timezone from --recovery-target-time.
That is one of putting it. Another way of putting it is that I am matching the last modified timestamp to match the timezone of the recovery target timestamp for the PIT recovery to work.
Can it be that you passed the
CLONE_TARGET_TIME
without specifying a timezone?
Refer to issue #299 where I do specify the command I run and particularly the --recovery-target-time
flag is set with the timezone specified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is one of putting it.
The problem is that for us the last_modified
already contains UTC timezone! Therefore it is really important to answer following questions:
- If you are using GCS could you verify the timestamp format in the backup-list output?
- Does it (timestamp) have a timezone?
- If it doesn't, how it relates to UTC?
- Is it possible to convert it to UTC?`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will attempt to perform the same operation with the CLONE_TARGET_TIME set. Then get back to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am using GCS and am having the same problem as @kidynamit. The last_modified
is a datatime
object without tzinfo
and thus it seems to be a bug in wal-e as they mention in their docs that last_modified
should have tzinfo
associated with it.
As for your questions @CyberDem0n
- Format is
yyyy-mm-dd hh:mm:ss
- No, it does not have a timezone
- Currently, there seems to be no way to relate it to UTC
- See above, so no.
Maybe we could detect whether last_modified
has a tzinfo
and on None
replace it with UTC? Currently this blocks me from using the Kubernetes Operator in combination with GCS for PITR restores using WAL-E and GCS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nielsmeima I've posted a fix. Turns out there was an expanded size bytes we could use.
@CyberDem0n I think I fixed it appending the expanded_size_bytes field. |
Hi, I hitted this bug today.
And in below code,
I think that commits in this PR looks like CORRECT! So Could you review and merge this? |
Is there any update on this? This still causes an issue on restores from GCS 3+ years after the PR was opened. Is there anything I can do to get some movement on this? |
fixed the aware vs unaware compare
#299