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

Migrate instructions #509

Merged
merged 2 commits into from
May 19, 2020

Conversation

mattcary
Copy link
Contributor

/kind documentation

What this PR does / why we need it:
Add guidance on migrating from alpha snapshot version

Does this PR introduce a user-facing change?:

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 18, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @mattcary. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 18, 2020
@mattcary
Copy link
Contributor Author

/cc @msau42

@k8s-ci-robot k8s-ci-robot requested a review from msau42 May 18, 2020 18:03
@mattcary
Copy link
Contributor Author

#457

@msau42
Copy link
Contributor

msau42 commented May 18, 2020

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 18, 2020
docs/kubernetes/user-guides/snapshots.md Outdated Show resolved Hide resolved
docs/kubernetes/user-guides/snapshots.md Outdated Show resolved Hide resolved
docs/kubernetes/user-guides/snapshots.md Outdated Show resolved Hide resolved
cluster with the PD snapshot handle from the alpha snapshot, then bind that to a
new `VolumeSnapshot`.

If you are able to set the `deletionPolicy` of any existing
Copy link
Contributor

Choose a reason for hiding this comment

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

Throughout the doc, it would be good to clarify if you're referring to the alpha version or beta version of the object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

PD CSI driver deployment. This may be a safer option if you are not sure about
the deletion policy of your alpha snapshot.

#### Steps to restore from an existing snapshot.
Copy link
Contributor

Choose a reason for hiding this comment

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

These steps are useful even outside the context of trying to migrate from alpha to beta. Maybe this should be pulled out of the migration section and made more top level. Then in the migration section, you can refer to the "import an existing snapshot" section.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's a good point. I was a little surprised myself that I couldn't find any guide on manually provisioning snapshot contents.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've done this, creating a migration section and referring to that from the migration tips section.

docs/kubernetes/user-guides/snapshots.md Show resolved Hide resolved
@mattcary mattcary force-pushed the migrate-instructions branch 2 times, most recently from 746d42b to 9d8bff0 Compare May 18, 2020 21:15
@@ -122,3 +125,150 @@
lost+found
sample-file.txt
```

### Manually Provisioning Snapshot Contents.
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think of calling it something like "Import a pre-existing snapshot"? I think this description aligns better with what a user would want to accomplish without using Kubernetes terms.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

#### Troubleshooting

If the `VolumeSnapshot`, `VolumeSnapshotContents` and `PersistentVolumeClaim`
are not all mutually syncrhonized, the pod will not start. Try `kubectl describe
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: synchronized

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

snapshotter. In many cases, snapshots created with the alpha will not be able to
be used with a beta driver. In some cases, they can be migrated.

*You may lose all data in your snapshot!*
Copy link
Contributor

Choose a reason for hiding this comment

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

Bold and add "Attention" similar to the other notes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

snapshot-controller and PD CSI driver:

* snapshot-controller:v2.0.1
* csi-provisioner:v1.5.0-gke.0
Copy link
Contributor

Choose a reason for hiding this comment

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

gke.gcr.io?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated


#### Migrating by Restoring from a Manually Provisioned Snapshot

The idea is to provision a `VolumeSnapshotContents` in your new or upgraded beta
Copy link
Contributor

Choose a reason for hiding this comment

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

clarify this is a beta VolumeSnapshotContent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

the deletion policy of your alpha snapshot.

After confirming that the PD snapshot is available, restore it using the beta PD
CSI driver as described above in "Manually Provisioning Snapshot Contents". The
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make this a relative link?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so...


After confirming that the PD snapshot is available, restore it using the beta PD
CSI driver as described above in "Manually Provisioning Snapshot Contents". The
restoration should in a cluster with the beta PD CSI driver installed. At not
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: At no point

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


#### Migrating by Restoring from a Manually Provisioned Snapshot

The idea is to provision a `VolumeSnapshotContents` in your new or upgraded beta
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this actually won't work if you upgrade a cluster right? Because the alpha CRD will be still be installed, preventing the beta CRD from being installed. So creation of the beta objects won't succeed until the alpha CRD is deleted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll clarify that an upgrade means the alpha components are removed.

I don't want to get in to uninstalling everything; I tried it and it's a major PITA due to all the finalizers cross-linked between everything. It's really easy to get into a state where you have to patch out finalizers manually just to get resources to go away. Not to mention the problems of accidentally trying to delete CRDs while there are still resources around (eg, because of the finalizers).

@mattcary mattcary force-pushed the migrate-instructions branch 3 times, most recently from d8fbc41 to 1491a6c Compare May 18, 2020 22:39

After confirming that the PD snapshot is available, restore it using the beta PD
CSI driver as described above in [Import a Pre-Existing
Snapshot](#import-a-pre-existing-snapshot). The restoration should in a cluster
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: should be

then bind that to a new beta `VolumeSnapshot`.

Note that if using the same upgraded cluster where the alpha driver was
installed, the alpha driver must be completely removed, including its CRDs.
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be useful to note somewhere here that uninstalling the CRD will delete all the CR objects, so it's best to save all the snapshot handles before doing that?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess you also should save the information about what workload it was associated with too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How does that sound?

FWIW I wasn't actually able to uninstall the driver & CRDs without wedging things. That's why I added the advice about spinning up a new cluster.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you weren't able to get it to work, then maybe let's just say in the docs that you'll need to migrate to a new cluster. The advice about saving the objects from the old cluster still stand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I've made it slightly stronger.

@mattcary mattcary force-pushed the migrate-instructions branch 2 times, most recently from ff3a466 to fdf6eb8 Compare May 18, 2020 23:13
@msau42
Copy link
Contributor

msau42 commented May 19, 2020

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 19, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattcary, msau42

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 19, 2020
@k8s-ci-robot k8s-ci-robot merged commit 3324f67 into kubernetes-sigs:master May 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants