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

Replace Config CRD #538

Closed
unguiculus opened this issue Jun 11, 2018 · 8 comments
Closed

Replace Config CRD #538

unguiculus opened this issue Jun 11, 2018 · 8 comments
Assignees
Labels
Breaking change Impacts backwards compatibility Enhancement/User End-User Enhancement to Velero
Milestone

Comments

@unguiculus
Copy link

unguiculus commented Jun 11, 2018

I'm trying to help get the PR for a Ark Helm chart merged.

helm/charts#3795

Over at the chats repo we have a CI that lints and install charts. I figured this is not possible with Ark. I'll have to wrap the deployment into an if statement that makes sure it is not created by the CI job because Ark won't come up if it doesn't have a cloud provider and credentials set.

Now, since Ark has its config separate in the Config CRD, I find that kind of strange. Actually, this mean the config is spread over two places. I understand that this separation is probably necessary because the secret needs to be mounted. But then why separate it at all instead of having a bunch of CLI args and everything in the deployment?

On the other hand, I like the separate Config resource. I wonder if there is a way to also reference secrets in there. I guess the initial Ark deployment would then have to be turned into a little daemon whose sole purpose is to boostrap real Ark server deployments based on the complete config mounting the configured secret. In theory this would allow boostrapping multiple Ark servers with different configs. I think, this is when a separate config really would make sense. Has such a setup been discussed? Would that make sense?

@bhack
Copy link

bhack commented Jun 11, 2018

Any other specific requirement for the imminent 0.9 release?

@ncdc
Copy link
Contributor

ncdc commented Jun 11, 2018

@unguiculus thanks for your feedback! We are eventually going to remove the Config resource and replace it with multiple types that we'll use to configure backup storage and volume storage. Anything that's left over will become command line flags. This is planned for our v0.10.0 release, which is a couple of months away.

@bhack we'll be releasing v0.9.0.alpha.1 this week and we'll have more information available once it's out.

@ncdc ncdc changed the title Ark Config Discussion Replace Config CRD Jun 11, 2018
@ncdc ncdc self-assigned this Jun 11, 2018
@ncdc ncdc added Enhancement Breaking change Impacts backwards compatibility labels Jun 11, 2018
@rosskukulinski
Copy link
Contributor

Related to #103 and the Backup/SnapShot Target APIs.

@rosskukulinski rosskukulinski added Enhancement/User End-User Enhancement to Velero and removed Enhancement labels Jun 24, 2018
@ncdc ncdc added this to the v1.0.0 milestone Jun 26, 2018
@skriss skriss modified the milestones: v1.0.0, v0.10.0 Aug 6, 2018
@skriss skriss assigned carlisia and unassigned ncdc Aug 6, 2018
@skriss
Copy link
Contributor

skriss commented Aug 6, 2018

@carlisia:

Our Config CRD has a bunch of fields that we want to either move into other CRDs or switch to server flags, so we can get rid of the Config type.

PersistentVolumeProvider and BackupStorageProvider will be replaced with the backup locations work.

BackupSyncPeriod, GCSyncPeriod, ScheduleSyncPeriod, PodVolumeOperationTimeout are all polling intervals/timeouts. I think the first and last probably need to become server flags, while the middle two (gcSyncPeriod and scheduleSyncPeriod) should be converted to constants where they're used and not exposed as configurable values.

ResourcePriorities should remain configurable - I think this can be a server flag (but let me know before you start on this, want to think about it a little bit).

RestoreOnlyMode will eventually be replaced by the accessMode field on backup locations. We need to decide what to do with it in the interim. I think moving it to a server flag in the short-term would be fine, but let me think on it too.

@carlisia
Copy link
Contributor

carlisia commented Aug 8, 2018

In talking to @skriss , we are going to make ResourcePriorities take a value that is a comma-separated list, and RestoreOnlyMode will be a server flag for now (it’ll be temporary).

@ncdc
Copy link
Contributor

ncdc commented Aug 8, 2018

I think we'll need to have separate resource priorities for backups and restores. #586

@skriss
Copy link
Contributor

skriss commented Aug 8, 2018

sure, so let's just add the --restore-resource-priorities for now since that's what we currently offer, and then we can do the other one later.

@carlisia
Copy link
Contributor

carlisia commented Aug 8, 2018

Copy that 👍.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking change Impacts backwards compatibility Enhancement/User End-User Enhancement to Velero
Projects
None yet
Development

No branches or pull requests

6 participants