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

Jump to bottom

config: change default destination sync type to azure for azure #2306

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

config: change default destination sync type to azure for azure #2306

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e36495a
config: change default destination sync type to azure for azure
viktor-f Oct 14, 2024
477a02b
fixup: misses in migration
viktor-f Oct 14, 2024
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
5 changes: 5 additions & 0 deletions config/providers/azure/sc-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,8 @@ fluentd:
# azcopy is used by az CLI for downloading files
azureCopyBufferGB: 0.3
azureCopyConcurrency: 8

objectStorage:
sync:
## If Harbor or Thanos are using swift then we will automatically use swift for the sync, regardless of the value set for type.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this comment. I'm sure it is obvious once you know why but to someone that does not it's not clear what is going on here.

Does it mean that the value of objectStorage.sync.destinationType is completely ignored if .harbor.persistence.type=swift or .thanos.objectStorage.type=swift or that the sync of those specific buckets are ignoring the value of objectStorage.sync.destinationType? I assume the latter?

To me the object storage configuration throughout the project feels a bit complicated. It really shows in the complex "set-me-if" cases. I wonder if we could simplify it somehow? Worthy of an issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is trying to say that the sync of those specific buckets are ignoring this value. You can check the logic here. I did just copy this line from the base config file, but I could perhaps try to rewrite it to be more clear.

I agree that it is complicated, but it is also supporting a lot of different possibilities. I'm not sure this is something we would spend time on soon, unless it was needed to support something else.

Copy link
Contributor

Choose a reason for hiding this comment

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

Time spent simplifying is time saved dealing with complexity in the future but I agree it's not high priority.

At least having tests for each of the config cases is something I'd like to se. I would be fine just seeing "config -> rendered result" and some "missing dependent config option / set-me-if-x" unit tests.

destinationType: azure
1 change: 0 additions & 1 deletion config/sc-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,6 @@ objectStorage:
enabled: false
dryrun: false

## Only 's3' is currently supported for `.objectStorage.sync.destinationType` as not all default buckets applications have swift support.
## If Harbor or Thanos are using swift then we will automatically use swift for the sync, regardless of the value set for type.
destinationType: s3
# secondaryUrl: set-me if regionEndpoint and or authUrl does not have all the relevant ips and or ports used for rclone-sync networkpolicy.
Expand Down
9 changes: 9 additions & 0 deletions migration/v0.42/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,15 @@ As with all scripts in this repository `CK8S_CONFIG_PATH` is expected to be set.
export CK8S_CLUSTER=<wc|sc|both>
```

1. Update objectStorage.sync.destinationType for azure clusters

Ensure that objectStorage.sync.destinationType keeps current value for azure clusters that have sync enabled.
Default is updated to `azure` for azure clusters.

```bash
./migration/v0.42/prepare/40-azure-sync-destination-type.sh
```

1. Update apps configuration:

This will take a backup into `backups/` before modifying any files.
Expand Down
27 changes: 27 additions & 0 deletions migration/v0.42/prepare/40-azure-sync-destination-type.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
#!/usr/bin/env bash

HERE="$(dirname "$(readlink -f "${0}")")"
ROOT="$(readlink -f "${HERE}/../../../")"

# shellcheck source=scripts/migration/lib.sh
source "${ROOT}/scripts/migration/lib.sh"

log_info "configuring object storage sync type on azure"
if [[ "${CK8S_CLUSTER}" =~ ^(sc|both)$ ]]; then
cloud_provider=$(yq_dig sc .global.ck8sCloudProvider)
if [[ "${cloud_provider}" != "azure" ]]; then
log_info "not running on azure, skipping"
exit 0
fi

if yq_check sc .objectStorage.sync.enabled true; then
if yq_null sc .objectStorage.sync.destinationType; then
log_info "objectStorage sync is enabled, will keep previous default destinationType: s3"
yq_add sc .objectStorage.sync.destinationType "\"s3\""
else
log_info "sync destinationType already has an override, skipping"
fi
else
log_info "object storage sync is not enabled, will use new default destinationType: azure"
fi
fi