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

Fix error propagation in checkandAddSnapshotFinalizers and add tests #263

Merged
merged 1 commit into from
Mar 4, 2020
Merged

Fix error propagation in checkandAddSnapshotFinalizers and add tests #263

merged 1 commit into from
Mar 4, 2020

Conversation

ggriffiths
Copy link
Member

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
Adds more tests, and fixes a few bugs I found.

Which issue(s) this PR fixes:
Fixes #196

Special notes for your reviewer:

  • I uncommented all commented tests, fixed them, and added additional ones where needed.
  • code cov for snapshot_controller.go is now 82.9%, up from 74% previously.
  • broke up the tests & bug fixes into separate commits

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. labels Mar 3, 2020
@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Mar 3, 2020
ggriffiths
ggriffiths commented Mar 3, 2020
View reviewed changes
pkg/common-controller/snapshot_controller.go
@@ -665,26 +671,6 @@ func (ctrl *csiSnapshotCommonController) updateSnapshotErrorStatusWithEvent(snap
return nil
}

// isSnapshotConentBeingUsed checks if snapshot content is bound to snapshot.
Copy link
Member Author

Choose a reason for hiding this comment

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

this isn't used anywhere

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

@ggriffiths
Copy link
Member Author

/assign @xing-yang

@ggriffiths ggriffiths force-pushed the common_controller_uts_2 branch from 33419b0 to 15ac765 Compare March 3, 2020 04:58
ggriffiths
ggriffiths commented Mar 3, 2020
View reviewed changes
pkg/common-controller/snapshot_controller.go
ctrl.checkandAddSnapshotFinalizers(snapshot)
if err := ctrl.checkandAddSnapshotFinalizers(snapshot); err != nil {
klog.Errorf("error check and add Snapshot finalizers for snapshot [%s]: %v", snapshot.Name, errFinalizer)
ctrl.eventRecorder.Event(snapshot, v1.EventTypeWarning, "SnapshotCheckandUpdateFailed", fmt.Sprintf("Failed to check and update snapshot: %s", err.Error()))
Copy link
Member Author

Choose a reason for hiding this comment

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

I used this warning name from a test that used to test this behavior (before the controller split).

ggriffiths
ggriffiths commented Mar 3, 2020
View reviewed changes
pkg/common-controller/snapshot_delete_test.go
@@ -130,159 +130,44 @@ var snapshotClasses = []*crdv1.VolumeSnapshotClass{
func TestDeleteSync(t *testing.T) {
tests := []controllerTest{
{
name: "1-1 - content with empty snapshot class is deleted if it is bound to a non-exist snapshot and also has a snapshot uid specified",
Copy link
Member Author

Choose a reason for hiding this comment

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

Many of these tests are no longer required in this package, or are duplicates of the new ones I wrote below (3-1, 3-2, etc)

yuxiangqian
yuxiangqian reviewed Mar 4, 2020
View reviewed changes
Copy link
Contributor

@yuxiangqian yuxiangqian left a comment

Choose a reason for hiding this comment

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

some nits otherwise LGTM. thanks

pkg/common-controller/snapshot_controller.go Outdated
@@ -301,7 +305,9 @@ func (ctrl *csiSnapshotCommonController) checkandAddSnapshotFinalizers(snapshot
if addSourceFinalizer || addBoundFinalizer {
// Snapshot is not being deleted -> it should have the finalizer.
klog.V(5).Infof("checkandAddSnapshotFinalizers: Add Finalizer for VolumeSnapshot[%s]", utils.SnapshotKey(snapshot))
ctrl.addSnapshotFinalizer(snapshot, addSourceFinalizer, addBoundFinalizer)
if err := ctrl.addSnapshotFinalizer(snapshot, addSourceFinalizer, addBoundFinalizer); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: how about simply? "return ctrl.addSnapshot....."

pkg/common-controller/snapshot_controller.go
@@ -665,26 +671,6 @@ func (ctrl *csiSnapshotCommonController) updateSnapshotErrorStatusWithEvent(snap
return nil
}

// isSnapshotConentBeingUsed checks if snapshot content is bound to snapshot.
Copy link
Contributor

Choose a reason for hiding this comment

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

+1

pkg/common-controller/framework_test.go Outdated
@@ -1494,3 +1501,5 @@ func emptyDataSecretAnnotations() map[string]string {
utils.AnnDeletionSecretRefNamespace: "default",
}
}

func toStrPointer(str string) *string { return &str }
Copy link
Contributor

Choose a reason for hiding this comment

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

where is this function used? seems not in this PR

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yeah, I was using it for a test originally but removed it's usage. Will remove this

@ggriffiths ggriffiths force-pushed the common_controller_uts_2 branch 2 times, most recently from 65ae98f to a5c9c35 Compare March 4, 2020 18:17
@ggriffiths
Copy link
Member Author

Addressed your suggestions @yuxiangqian. Thanks!

@yuxiangqian
Copy link
Contributor

/lgtm /approve

@yuxiangqian
Copy link
Contributor

/assign @yuxiangqian

@yuxiangqian
Copy link
Contributor

@ggriffiths please squash the commits. thanks

@ggriffiths ggriffiths force-pushed the common_controller_uts_2 branch from a5c9c35 to 9830b6e Compare March 4, 2020 18:48
@yuxiangqian
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ggriffiths, yuxiangqian

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 Mar 4, 2020
@yuxiangqian
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 4, 2020
@k8s-ci-robot k8s-ci-robot merged commit 3dd4e07 into kubernetes-csi:master Mar 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yuxiangqian yuxiangqian yuxiangqian left review comments

@lpabon lpabon Awaiting requested review from lpabon

Assignees

@xing-yang xing-yang

@yuxiangqian yuxiangqian

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. release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve code coverage for pkg/common-controller
4 participants
@ggriffiths @yuxiangqian @k8s-ci-robot @xing-yang