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

Add ability to perform an etcd on-demand snapshot via cli #2819

Merged
merged 19 commits into from
Jan 21, 2021

Conversation

briandowns
Copy link
Contributor

@briandowns briandowns commented Jan 15, 2021

Signed-off-by: Brian Downs brian.downs@gmail.com

Proposed Changes

This PR adds a new sub-command to the K3s command called etcd-snapshot and it takes a number of the standard K3s flags as well as specific flags like name, used to give the snapshot a specific name and dir used to tell K3s where to save the snapshot. The former use applicable in this use case as well as for cron scheduled snapshots and keeps in like with RKE features for snapshotting and will ultimately be pulled into RKE2. An on-demand snapshot will not effect snaptshot retention.

Types of Changes

Verification

  1. Start K3s using Etcd and wait for it to get to steady state.
  2. Run k3s etcd-snapshot to perform an etcd snapshot.
    A. Verify the snapshot has been performed successfully via log output {"level":"info","msg":"saved","path":"/var/lib/rancher/k3s/server/db/snapshots/on-demand-<unix-timestamp>"}
    B. Verify that the newly created snapshot is indeed at the expected location
  3. Run k3s etcd-snapshot --name=my_snapshot to perform and etcd snapshot now.
    A. Perform the same verification steps from above.

Linked Issues

#2758

Further Comments

briandowns and others added 12 commits December 30, 2020 12:50
Signed-off-by: Brian Downs <brian.downs@gmail.com>
Signed-off-by: Brian Downs <brian.downs@gmail.com>
Signed-off-by: Brian Downs <brian.downs@gmail.com>
Signed-off-by: Brian Downs <brian.downs@gmail.com>
Signed-off-by: Brian Downs <brian.downs@gmail.com>
Signed-off-by: Brian Downs <brian.downs@gmail.com>
Co-authored-by: Brad Davidson <brad@oatmail.org>
Co-authored-by: Brad Davidson <brad@oatmail.org>
Signed-off-by: Brian Downs <brian.downs@gmail.com>
Signed-off-by: Brian Downs <brian.downs@gmail.com>
Signed-off-by: Brian Downs <brian.downs@gmail.com>
Signed-off-by: Brian Downs <brian.downs@gmail.com>
@briandowns briandowns requested a review from a team as a code owner January 15, 2021 00:30
@briandowns briandowns self-assigned this Jan 15, 2021
Signed-off-by: Brian Downs <brian.downs@gmail.com>
Signed-off-by: Brian Downs <brian.downs@gmail.com>
@briandowns briandowns requested a review from a team January 20, 2021 17:55
Copy link
Member

@brandond brandond left a comment

Choose a reason for hiding this comment

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

Minor nits to the name/etc-snapshot-name UX.

Note that you don't have to explicitly put the default in the usage text if you specify a value; the CLI library does this for you:

[root@centos01 ~]# k3s etcd-snapshot --help | grep name
   --name value (db) Set the name of the etcd on-demand snapshot. Default: on-demand-<unix-timestamp> (default: "on-demand-1611166541")
[root@centos01 ~]# k3s etcd-snapshot --help | grep name
   --name value (db) Set the name of the etcd on-demand snapshot. Default: on-demand-<unix-timestamp> (default: "on-demand-1611166542")
[root@centos01 ~]# k3s etcd-snapshot --help | grep name
   --name value (db) Set the name of the etcd on-demand snapshot. Default: on-demand-<unix-timestamp> (default: "on-demand-1611166543")

pkg/cli/cmds/server.go Outdated Show resolved Hide resolved
pkg/cli/cmds/etcd_snapshot.go Outdated Show resolved Hide resolved
pkg/cli/cmds/etcd_snapshot.go Outdated Show resolved Hide resolved
Signed-off-by: Brian Downs <brian.downs@gmail.com>
Signed-off-by: Brian Downs <brian.downs@gmail.com>
Signed-off-by: Brian Downs <brian.downs@gmail.com>
pkg/etcd/etcd.go Outdated Show resolved Hide resolved
Co-authored-by: Brad Davidson <brad@oatmail.org>
Copy link
Member

@brandond brandond left a comment

Choose a reason for hiding this comment

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

LGTM except for one docstring typo I just noticed

pkg/etcd/etcd.go Outdated Show resolved Hide resolved
Co-authored-by: Brad Davidson <brad@oatmail.org>
@galal-hussein
Copy link
Contributor

The PR looks good to me, the only comment I have is that I can see users confuse the command with non-embedded etcd k3s, meaning that when user trigger this command with kine it may return etcd error saying that it cant connect to etcd or sometthing similar, it may benefit us to display an error stating that etcd embedded isnt set up.

@briandowns
Copy link
Contributor Author

If this is ran on a system not running etcd, this is the output:

FATA[2021-01-20T20:25:37.727386821Z] managed etcd database has not been initialized 

@galal-hussein
Copy link
Contributor

If this is ran on a system not running etcd, this is the output:

FATA[2021-01-20T20:25:37.727386821Z] managed etcd database has not been initialized 

thats good enough for me, LGTM

Copy link
Contributor

@galal-hussein galal-hussein left a comment

Choose a reason for hiding this comment

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

LGTM

@briandowns briandowns merged commit 1322901 into k3s-io:master Jan 21, 2021
briandowns added a commit to briandowns/k3s that referenced this pull request Mar 15, 2021
* add ability to perform an etcd on-demand snapshot via cli

Signed-off-by: Brian Downs <brian.downs@gmail.com>
(cherry picked from commit 1322901)
briandowns added a commit to briandowns/k3s that referenced this pull request Mar 16, 2021
* add ability to perform an etcd on-demand snapshot via cli

(cherry picked from commit 1322901)
Signed-off-by: Brian Downs <brian.downs@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants