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

🐛 Fix setting folder S3 on etcd backups #549

Merged
merged 1 commit into from
Jan 10, 2025

Conversation

furkatgofurov7
Copy link
Contributor

@furkatgofurov7 furkatgofurov7 commented Jan 8, 2025

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #499
Reverts: 261395e

Special notes for your reviewer:

Checklist:

  • squashed commits into logical changes
  • includes documentation
  • adds unit tests
  • adds or updates e2e tests

pkg/rke2/config.go Outdated Show resolved Hide resolved
@kkaempf kkaempf added this to the v0.11.0 milestone Jan 9, 2025
@alexander-demicev
Copy link
Member

@furkatgofurov7 will the revert cause a regression? AFAIK #264 addressed a bug we saw when testing etcd restore POC

rke2ServerConfig.EtcdS3SkipSslVerify = !opts.ServerConfig.Etcd.BackupConfig.S3.EnforceSSLVerify
} else {
rke2ServerConfig.EtcdSnapshotDir = opts.ServerConfig.Etcd.BackupConfig.Directory
Copy link
Contributor Author

@furkatgofurov7 furkatgofurov7 Jan 10, 2025

Choose a reason for hiding this comment

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

@alexander-demicev @Danil-Grigorev thanks for reviews, PTAL once again. I updated the code so that EtcdSnapshotDir (local snapshot dir) set to S3.Folder logic is removed and it being set inside else condition is moved out of it, since the condition is redundant here.

Copy link
Contributor

@Danil-Grigorev Danil-Grigorev left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the changes. Was a bit worried about revert bit as well.

@furkatgofurov7 furkatgofurov7 merged commit c2badd9 into rancher:main Jan 10, 2025
7 checks passed
@alexander-demicev alexander-demicev deleted the fix-setting-s3folder branch January 10, 2025 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working
Development

Successfully merging this pull request may close these issues.

[SURE-9588] Issue setting folder S3 on etcd backups
4 participants