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

Add sidecar-controller unit tests #191

Conversation

@ggriffiths
Copy link
Member

ggriffiths commented Nov 8, 2019

Signed-off-by: Grant Griffiths grant@portworx.com

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
Adds unit tests to sidecar-controller

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE
Signed-off-by: Grant Griffiths <grant@portworx.com>
// - Latest version of snapshots contents saved by the controller.
// - Queue of all saves (to simulate "content/snapshot updated" events). This queue
// contains all intermediate state of an object - e.g. a snapshot.VolumeName
// is updated first and snapshot.Phase second. This queue will then contain both

This comment has been minimized.

Copy link
@xing-yang

xing-yang Nov 8, 2019

Collaborator

This section needs to be updated. It talks about snapshot.VolumeName, snapshot.Phase, etc.

volumes map[string]*v1.PersistentVolume
claims map[string]*v1.PersistentVolumeClaim
contents map[string]*crdv1.VolumeSnapshotContent
snapshots map[string]*crdv1.VolumeSnapshot

This comment has been minimized.

Copy link
@xing-yang

xing-yang Nov 8, 2019

Collaborator

We should not need StorageClasses, volumes, claims, snapshots.

// etcd+/API server) when an action performed by the reactor matches given verb
// ("get", "update", "create", "delete" or "*"") on given resource
// ("volumesnapshotcontents", "volumesnapshots" or "*").
type reactorError struct {

This comment has been minimized.

Copy link
@xing-yang

xing-yang Nov 8, 2019

Collaborator

remove volumesnapshots

}

// React is a callback called by fake kubeClient from the controller.
// In other words, every snapshot/content change performed by the controller ends

This comment has been minimized.

Copy link
@xing-yang

xing-yang Nov 8, 2019

Collaborator

remove snapshot


// checkSnapshots compares all expectedSnapshots with set of snapshots at the end of the
// test and reports differences.
func (r *snapshotReactor) checkSnapshots(expectedSnapshots []*crdv1.VolumeSnapshot) error {

This comment has been minimized.

Copy link
@xing-yang

xing-yang Nov 8, 2019

Collaborator

Do we need snapshots?

This comment has been minimized.

Copy link
@xing-yang

xing-yang Nov 8, 2019

Collaborator

Probably just need VolumeSnapshotRefs?

case *crdv1.VolumeSnapshotContent:
vol, _ := obj.(*crdv1.VolumeSnapshotContent)
klog.V(4).Infof("reactor queue: %s", vol.Name)
case *crdv1.VolumeSnapshot:

This comment has been minimized.

Copy link
@xing-yang

xing-yang Nov 8, 2019

Collaborator

Need this?

return obj
}

// syncAll simulates the controller periodic sync of contents and snapshot. It

This comment has been minimized.

Copy link
@xing-yang

xing-yang Nov 8, 2019

Collaborator

Remove snapshot


// deleteSnapshotEvent simulates that a snapshot has been deleted in etcd and the
// controller receives 'snapshot deleted' event.
func (r *snapshotReactor) deleteSnapshotEvent(snapshot *crdv1.VolumeSnapshot) {

This comment has been minimized.

Copy link
@xing-yang

xing-yang Nov 8, 2019

Collaborator

Don't need this


// addSnapshotEvent simulates that a snapshot has been deleted in etcd and the
// controller receives 'snapshot added' event.
func (r *snapshotReactor) addSnapshotEvent(snapshot *crdv1.VolumeSnapshot) {

This comment has been minimized.

Copy link
@xing-yang

xing-yang Nov 8, 2019

Collaborator

Don't need this

@ggriffiths ggriffiths force-pushed the ggriffiths:new_beta_split_controller_unittests branch 2 times, most recently from 16392a3 to 4a282f7 Nov 9, 2019
Signed-off-by: Grant Griffiths <grant@portworx.com>
@ggriffiths ggriffiths force-pushed the ggriffiths:new_beta_split_controller_unittests branch from 4a282f7 to e4e2f3b Nov 9, 2019
@xing-yang

This comment has been minimized.

Copy link
Collaborator

xing-yang commented Nov 9, 2019

I'm going to merge this so that we have some unit test coverage for the sidecar. We can add more tests later.

@xing-yang

This comment has been minimized.

Copy link
Collaborator

xing-yang commented Nov 9, 2019

/lgtm
/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Nov 9, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ggriffiths, xing-yang

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

@xing-yang xing-yang changed the title WIP: Add sidecar-controller unit tests Add sidecar-controller unit tests Nov 9, 2019
@k8s-ci-robot k8s-ci-robot merged commit 105b84d into kubernetes-csi:master Nov 9, 2019
6 of 7 checks passed
6 of 7 checks passed
tide Not mergeable.
Details
cla/linuxfoundation ggriffiths authorized
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
pull-kubernetes-csi-external-snapshotter-1-14-on-kubernetes-1-14 Job succeeded.
Details
pull-kubernetes-csi-external-snapshotter-1-15-on-kubernetes-1-15 Job succeeded.
Details
pull-kubernetes-csi-external-snapshotter-1-16-on-kubernetes-1-16 Job succeeded.
Details
pull-kubernetes-csi-external-snapshotter-unit Job succeeded.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.