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

Integrate PVC creation feature using snapshot to replication controller for single cluster #153

Draft
wants to merge 29 commits into
base: main
Choose a base branch
from

Conversation

YichenYa0
Copy link
Contributor

@YichenYa0 YichenYa0 commented Aug 12, 2024

Description

  • This feature extends the existing repctl / replication controller from just creating snapshots to also creating PVCs necessary to start a test copy of the application. For single cluster the test copy of the application must be started in a different namespace than the production copy of the application which is assumed to be running.
  • This uses the replication action to take a snapshot of the target volume(s), and is applied to all volumes in the replication group
  • PVC/PV pairs are created in the namespace of the VolumeSnapshots taken
  • The user can start a new copy of the original application using the replicated objects by specifying the new namespace of the created PVC/PV pairs

GitHub Issues

List the GitHub issues impacted by this PR:

GitHub Issue #
N/A

Checklist:

  • I have performed a self-review of my own code to ensure there are no formatting, vetting, linting, or security issues
  • I have verified that new and existing unit tests pass locally with my changes
  • I have not allowed coverage numbers to degenerate
  • I have maintained at least 90% code coverage
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • I have maintained backward compatibility

How Has This Been Tested?

  • Manual test using replication action
  • Unit testing
  • Integrated testing

Manual Testing Instruction

  • Rebuild repctl
  • The user has the option to specify the snapshot class in the replication action they provide: repctl snapshot --rg rg-id --sn-class vxflexos-snapclass-delete --sn-namespace test-delete --create-pvcs --storage-class vxflexos-217
  • The user also has the option not to specify a snapshot class in the replication action, and a sufficient default class will automatically be supplied or created if it does not already exist: repctl snapshot --rg rg-id --sn-namespace test-default --create-pvcs --storage-class vxflexos-217
  • Deploy an app, but update helm chat to specify namespace of newly created PVC/PV (e.g. test-pg1)
  • Tested on both single and multi cluster.

Test Coverage Change

/controllers/replication-controller/dellcsireplicationgroup_controller.go change:

Original test coverage of main branch Latest test coverage due to added code
58.4% 84.0%

@santhoshatdell
Copy link
Contributor

Is this PR ready for review? It's in draft state.

@falfaroc falfaroc force-pushed the feature/snapshot-failover branch from 939ca96 to 5cad19e Compare August 23, 2024 20:03
@falfaroc falfaroc marked this pull request as ready for review August 26, 2024 21:28
Comment on lines +388 to +393
// example default snapshot class: default-csi-vxflexos
snClass := group.Annotations[controller.SnapshotClass]
driverClass := group.Labels[controller.DriverName]
if snClass == "" {
part := strings.Split(driverClass, ".")[0]
snClass = "default-" + strings.TrimPrefix(part, "csi-") + "-snapshotclass"
Copy link
Contributor

Choose a reason for hiding this comment

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

The example and the implementation do not match.
Not a big deal, but if you're making any other changes, maybe update here as well.

Comment on lines +402 to +403
storageClass := group.Annotations[r.Domain+"/snapshotStorageClass"]
createPVC := group.Annotations[r.Domain+"/snapshotCreatePVC"]
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, in the other PR, there were more concise consts for these array indexes.
Can we update here as well if they exist?

Comment on lines +155 to +159
if createPVCtrue {
rg.Annotations[prefix+"/snapshotCreatePVC"] = "true"
} else {
rg.Annotations[prefix+"/snapshotCreatePVC"] = "false"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if createPVCtrue {
rg.Annotations[prefix+"/snapshotCreatePVC"] = "true"
} else {
rg.Annotations[prefix+"/snapshotCreatePVC"] = "false"
}
rg.Annotations[prefix+"/snapshotCreatePVC"] = createPVCtrue

Additionally, maybe rename createPVCtrue to createPVC to avoid confusion?

@santhoshatdell santhoshatdell marked this pull request as draft October 15, 2024 13:35
@santhoshatdell
Copy link
Contributor

Moving to Draft state, need to check with POs regarding this feature's milestone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants