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 support for etcd backups with s3 storage #471

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

Kennybll
Copy link

K3S has built in support for uploading snapshots of the embedded etcd database to S3. This PR adds the settings to enable that to the configuration file.

If the s3 settings are not included it will default to not being enabled, to ensure backward compatibility.

I also added a check to make sure it is only enabled when using the etcd datastore mode.

It may also work with other S3 compatible providers. I cannot give a list, however I tested it with Backblaze B2 and it worked.

An example when uploading to AWS S3 in the us-east-1 region:

datastore:
  mode: etcd
  s3:
    enabled: true
    access_key: EXAMPLE_ACCESS_KEY
    secret_key: EXAMPLE_SECRET_KEY
    bucket: EXAMPLE_BUCKET

Copy link
Owner

@vitobotta vitobotta left a comment

Choose a reason for hiding this comment

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

Wow, you made me VERY happy with this PR! It was on my list, so I appreciate you took the time to implement it.

I requested some changes if you don't mind, but these are mostly minor things and quick changes as you have done a great job really.

Thanks a lot!

docs/Creating_a_cluster.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/configuration/datastore.cr Outdated Show resolved Hide resolved
src/configuration/settings/datastore.cr Outdated Show resolved Hide resolved
src/configuration/settings/s3.cr Outdated Show resolved Hide resolved
src/kubernetes/installer.cr Outdated Show resolved Hide resolved
Kennybll and others added 2 commits October 26, 2024 19:29
also make clear the s3 options are only available for the etcd option
@Kennybll
Copy link
Author

Kennybll commented Oct 26, 2024

I realize it was outside the scope of this PR, but after the request to refactor the structor of the cluster_config.yaml options I figured I would add some of the additional back up options.

The only backup option I could not figure out was schedule_cron. I could not get it to work correctly for the life of me. Maybe it's the way the string was getting parsed with all of the quotes. That can be saved for a future PR. Note to self, I was looking at creating a /etc/rancher/k3s/config.yaml file with the snapshot-cron value, instead of passing it as an arg to the k3s binary.

Copy link
Owner

@vitobotta vitobotta left a comment

Choose a reason for hiding this comment

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

A few more comments, it's looking good!!! 👍

src/configuration/datastore.cr Outdated Show resolved Hide resolved
src/kubernetes/installer.cr Outdated Show resolved Hide resolved
src/kubernetes/installer.cr Outdated Show resolved Hide resolved
docs/etcd S3 backups.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@vitobotta
Copy link
Owner

I realize it was outside the scope of this PR, but after the request to refactor the structor of the cluster_config.yaml options I figured I would add some of the additional back up options.

The only backup option I could not figure out was schedule_cron. I could not get it to work correctly for the life of me. Maybe it's the way the string was getting parsed with all of the quotes. That can be saved for a future PR. Note to self, I was looking at creating a /etc/rancher/k3s/config.yaml file with the snapshot-cron value, instead of passing it as an arg to the k3s binary.

Can you share more details on the issue you had with that setting? It should be a regular string if it's in cron format, isn't that the case?

@Kennybll
Copy link
Author

The only backup option I could not figure out was schedule_cron. I could not get it to work correctly for the life of me. Maybe it's the way the string was getting parsed with all of the quotes. That can be saved for a future PR. Note to self, I was looking at creating a /etc/rancher/k3s/config.yaml file with the snapshot-cron value, instead of passing it as an arg to the k3s binary.

Can you share more details on the issue you had with that setting? It should be a regular string if it's in cron format, isn't that the case?

I tried every variation I could think of for a cli flag. None of them worked. I even tried manually setting it in master_install_script.sh in case it was the YAML parser being weird. If you are able to get it to work that would be awesome as it would be cleaner.
--etcd-snapshot-schedule-cron="* * * * *"
--etcd-snapshot-schedule-cron='* * * * *'
--etcd-snapshot-schedule-cron=* * * * *
--etcd-snapshot-schedule-cron * * * * *

Based on this comment k3s-io/k3s#5983 (comment), I was able to get it to work using a config file. So if snapshot-cron is set, the master_install_script.sh will create a file /etc/rancher/k3s/config.yaml.d/hetzner-k3s.yaml with the flag. If the option is removed, then it will delete that config file if it exists.

Whenever a cli flag changes the k3s service will automatically restart when rerunning the master_install_script.sh. However, if a config file changes, the service will not restart. I created an issue in the k3s repo k3s-io/k3s#11180. But in the meantime we would have to manually restart the k3s service, just in case there are changes in a config file, even if there are none.

since the etcd class isn’t optional in the Datastore Config is will always be set technically, which is fine if they are using an external datastore since the options won’t matter
@vitobotta
Copy link
Owner

I see, good idea to open an issue there. Did you also try normal escaping with \"?

Copy link

@Kennybll
Copy link
Author

Kennybll commented Oct 27, 2024

Yes, I tried escaping with \" and also \\\" just in case

@vitobotta
Copy link
Owner

Yes, I tried escaping with " and also \" just in case

I see. Thanks for the clarification

@Thijmen
Copy link

Thijmen commented Nov 22, 2024

I saw this PR and I am very curious about this. What is the timeline for this PR, when can we expect it in a possible release?

@vitobotta
Copy link
Owner

I saw this PR and I am very curious about this. What is the timeline for this PR, when can we expect it in a possible release?

I can't say exactly when it'll be done because I have some other important things to take care of right now, but it will definitely be part of one of the upcoming releases.

@Thijmen
Copy link

Thijmen commented Nov 22, 2024

@vitobotta thanks for getting back with me, I'll just keep an eye on this PR in that case! :)

@vitobotta
Copy link
Owner

@vitobotta thanks for getting back with me, I'll just keep an eye on this PR in that case! :)

Np :)

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