-
Notifications
You must be signed in to change notification settings - Fork 152
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
Design for integrating kopia with kanister #1482
Conversation
Thanks for submitting this pull request 🎉. The team will review it soon and get back to you. If you haven't already, please take a moment to review our project contributing guideline and Code of Conduct document. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good overall. Minor nits.
…oc-kanister-kopia-integration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is getting close. Let's work on getting it merged.
Here are some info that I think should be included:
Problem Statement - what problems are we trying to solve? why Kopia? what's wrong with the existing approach? slower, stale dependencies, unencrypted in-flight data, not scalable etc.
UX - how would this affect existing users? what would their upgrade path looks like? E.g., if they just upgrade Kanister and make no changes to their blueprints, will things still work as-is? How would they opt-in to the new Kopia-based functions? Do we have example blueprint, actionset? Do we need to feature flag this?
Versioning - how are we going to manage the different versions of the Kanister Functions to ensure backward compatibility? what version scheme are we using?
Server setup and failure domain - what steps would a user take to set up and tear down their Kopia server? Will the logs of this server be persisted for debugging purposes? What happens if the server failed while a backup is in progress? Will we retry, fallback to current approach etc.?
…oc-kanister-kopia-integration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Part 1 of my review. Yet to go through the newly created parts. Let me know what you think
…oc-kanister-kopia-integration
design/kanister-kopia-integration/kanister-kopia-integration.md
Outdated
Show resolved
Hide resolved
design/kanister-kopia-integration/kanister-kopia-integration.md
Outdated
Show resolved
Hide resolved
design/kanister-kopia-integration/kanister-kopia-integration.md
Outdated
Show resolved
Hide resolved
…oc-kanister-kopia-integration
design/kanister-kopia-integration/kanister-kopia-integration.md
Outdated
Show resolved
Hide resolved
Signed-off-by: Ivan Sim <ivan.sim@kasten.io>
afc8034
to
16ebbea
Compare
design/kopia.md
Outdated
|
||
The content of the configuration file, server certificate fingerprint, and | ||
and access password are exported to the remote data mover clients via the | ||
`KubeExec` functionality over TLS. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if I completely understand this. Would we use outputArtifacts
to pass this information to all other BackupData
, RestoreData
, etc. actions defined in BP?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this part still needs to be fleshed out. With KubeTask
, we have more control over what goes into the container before it's started. With KubeExec
, current thought is to POST
these info to the exec
subresource of the data mover pod via its stdin
.
Signed-off-by: Ivan Sim <ivan.sim@kasten.io>
Also remove service account-based access approach, due to RBAC concerns Signed-off-by: Ivan Sim <ivan.sim@kasten.io>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added some more questions and thoughts. Also, in general, I find that wrapping the lines to about 80 chars width (in text, markdown docs) makes it easier to add review comments, and improve readability. Might wanna consider doing that.
design/kanister-kopia-integration.md
Outdated
# required: supported values are s3, gcs, and azure | ||
type: s3 | ||
# required | ||
bucket: my-bucket | ||
# optional: specified in case of S3-compatible stores | ||
endpoint: https://foo.example.com | ||
# optional: used as a sub path in the bucket for all backups | ||
path: kanister/backups | ||
# required, if supported by the provider | ||
region: us-west-1 | ||
# optional: if set to true, do not verify SSL cert. | ||
# Default, when omitted, is false | ||
skipSSLVerify: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should document all the supported keys, to help the users to put these secrets together. AFAICT, the only place where this is documented now is in the kopia
CLI code.
matchLabels: | ||
pod: kopia-client | ||
app: monitoring | ||
status: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pavannd1 We also discussed about adding a new string field Progress
in the status subresource that would convey the users if RepositoryServer is ready to use or not
- Value would either be "ServerReady" if ready
- "ServerStopped" if an error occurred in the controller
- Or "ServerPending" when the Reconcile callback is in progress
- Kanister allows mutliple versions of Functions to be registered with the | ||
controller. | ||
- Existing Functions are registered with the default `v0.0.0` version. Find | ||
more information | ||
[here](https://docs.kanister.io/functions.html#existing-functions). | ||
- The following Data Functions will be registered with a second | ||
version `v1.0.0-alpha`: | ||
|
||
1. BackupData | ||
2. BackupDataAll | ||
3. BackupDataStats | ||
4. CopyVolumeData | ||
5. DeleteData | ||
6. DeleteDataAll | ||
7. RestoreData | ||
8. RestoreDataAll |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed, instead of adding versions, we would add new functions with the names having*WithKopia
suffix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shlokc9 can you add a note about this is yet to decide or TBD.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I'll add a note about this. 👍🏼
Maybe a separate design would be needed for handling kopia clients inside Kanister Functions....
…oc-kanister-kopia-integration
…nisterio/kanister into design-doc-kanister-kopia-integration
Change Overview
This PR consist of a design document that discusses ways to integrate Kopia with Kanister
Pull request type
Please check the type of change your PR introduces:
Issues
Test Plan