make mock driver support PV healthy monitor #268

fengzixu · 2020-05-31T09:27:40Z

What I did in this PR

Mock ControllerGetVolume RPC interface
Extend ListVolumes for supporting VolumeCondition support
Extend the NodeVolumeStats functions

This PR is for supporting test of PV health monitor
Fixes: kubernetes-csi/external-health-monitor#9

Updates CSI Spec to v1.3.0.  
Updates the mock driver to support volume health monitoring by implementing ControllerGetVolume and updating ListVolumes and NodeVolumeStats functions.

k8s-ci-robot · 2020-05-31T09:27:47Z

Welcome @fengzixu!

It looks like this is your first PR to kubernetes-csi/csi-test 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-csi/csi-test has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2020-05-31T09:27:48Z

Hi @fengzixu. Thanks for your PR.

I'm waiting for a kubernetes-csi 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.

fengzixu · 2020-05-31T09:34:51Z

/ok-to-test

k8s-ci-robot · 2020-05-31T09:35:04Z

@fengzixu: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/ok-to-test

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.

xing-yang · 2020-05-31T12:23:04Z

/ok-to-test

xing-yang · 2020-05-31T12:23:48Z

/assign @xing-yang

xing-yang · 2020-06-03T14:32:32Z

go.mod

@@ -3,7 +3,7 @@ module github.com/kubernetes-csi/csi-test/v3
 go 1.12

 require (
-	github.com/container-storage-interface/spec v1.2.0
+	github.com/container-storage-interface/spec v1.2.0-rc1.0.20200530102742-4c5274ab4e5e


You can use the latest v1.3.0 release:
https://github.com/container-storage-interface/spec/releases/tag/v1.3.0

xing-yang · 2020-06-03T14:33:35Z

mock/service/controller.go

+		Status: &csi.ControllerGetVolumeResponse_VolumeStatus{},
+	}
+
+	volumeStatus, err := s.NodeGetVolumeStats(ctx, &csi.NodeGetVolumeStatsRequest{})


Why is NodeGetVolumeStats called within ControllerGetVolume?

I misunderstand the working mechanism between Controller and Agent. Revised

xing-yang · 2020-06-03T14:34:30Z

mock/service/controller.go

@@ -388,8 +420,19 @@ func (s *service) ListVolumes(
 	)

 	for i = 0; i < len(entries); i++ {
+		volumeStatus, err := s.NodeGetVolumeStats(ctx, &csi.NodeGetVolumeStatsRequest{


ListVolumes is controller PRC. We should not call a Node PRC within a controller RPC.

Sure. Thanks for your comment.

xing-yang · 2020-06-03T14:35:32Z

/assign @NickrenREN

NickrenREN · 2020-06-04T06:47:14Z

What I did in this PR
Mock ControllerGetVolume RPC interface
Extend ListVolumes for supporting VolumeCondition support

There are two newly added RPC interfaces, one is controller side: ControllerGetVolume and the other one is node side: NodeGetVolumeStats. Both of them should be mocked here

fengzixu · 2020-06-07T07:22:58Z

@xing-yang @NickrenREN

Please review it again.

driver/driver.mock.go

NickrenREN · 2020-06-07T09:03:17Z

mock/service/controller.go

+
+	i, v := s.findVolByID(ctx, req.VolumeId)
+	if i < 0 {
+		return nil, status.Error(codes.NotFound, req.VolumeId)


Could we return VolumeCondition and set its Abnormal and Message fields

NickrenREN · 2020-06-07T09:04:52Z

mock/service/controller.go

+
+	resp := &csi.ControllerGetVolumeResponse{
+		Volume: &v,
+		Status: &csi.ControllerGetVolumeResponse_VolumeStatus{},


Could we generate the VolumeCondition field instead of returning an empty one (nil)?

Please refer to the comments

type ControllerGetVolumeResponse_VolumeStatus struct { // A list of all the `node_id` of nodes that this volume is // controller published on. // This field is OPTIONAL. // This field MUST be specified if the PUBLISH_UNPUBLISH_VOLUME // controller capability is supported. // published_node_ids MAY include nodes not published to or // reported by the SP. The CO MUST be resilient to that. PublishedNodeIds []string `protobuf:"bytes,1,rep,name=published_node_ids,json=publishedNodeIds,proto3" json:"published_node_ids,omitempty"` // Information about the current condition of the volume. // This field is OPTIONAL. // This field MUST be specified if the // VOLUME_CONDITION controller capability is supported. VolumeCondition *VolumeCondition `protobuf:"bytes,2,opt,name=volume_condition,json=volumeCondition,proto3" json:"volume_condition,omitempty"` XXX_NoUnkeyedLiteral struct{} `json:"-"` XXX_unrecognized []byte `json:"-"` XXX_sizecache int32 `json:"-"` }

For error, I mock the VolumeCondition filed. For normal response, I mocked the PublishedNodeIds

// Information about the current condition of the volume. // This field is OPTIONAL. // This field MUST be specified if the // VOLUME_CONDITION controller capability is supported.

NickrenREN · 2020-06-07T09:07:30Z

mock/service/node.go

@@ -427,6 +434,7 @@ func (s *service) NodeGetVolumeStats(ctx context.Context,
 				Unit:  csi.VolumeUsage_BYTES,
 			},
 		},
+		VolumeCondition: &csi.VolumeCondition{},


Could we also construct some abnormal conditions ?

fengzixu · 2020-06-17T16:00:53Z

@xing-yang @NickrenREN Request review

xing-yang · 2020-06-23T13:20:39Z

mock/service/controller.go

+		resp.Status.VolumeCondition.Abnormal = true
+		resp.Status.VolumeCondition.Message = "volume not found"
+		return resp, status.Error(codes.NotFound, req.VolumeId)
+	}


Can you add a case where volume is found but condition is Abnormal=true?

For the mock test, I think we can divide it into 2 parts to do

Add basic mock test functions for testing if volume-healthy-monitor can work(Due to the initial implementation PR wasn’t merged, it is hard to do further works)

Add more unit, integration, e2e test for testing volume-healthy-monitor can work correctly on positive and negative cases

The reasons I want to do a testing task like above are below

The initial PR blocks many works and it isn’t perfect. We can ensure it works normally and improves it

After initial PR was merged. It is better for us to let more contributors contribute this project.

xing-yang · 2020-06-23T15:24:36Z

/lgtm

xing-yang · 2020-06-23T15:24:51Z

/assign @pohly

pohly

/approve

k8s-ci-robot · 2020-06-23T17:51:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fengzixu, pohly

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:

~~OWNERS~~ [pohly]

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 added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 31, 2020

k8s-ci-robot requested review from jsafrane and msau42 May 31, 2020 09:27

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 31, 2020

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label May 31, 2020

fengzixu mentioned this pull request May 31, 2020

Mock ControllerGetVolume/NodeGetVolumeStats/ListVolumes RPCs to test controller/agent here kubernetes-csi/external-health-monitor#9

Closed

fengzixu force-pushed the master branch from dc9f47f to 80e8f1b Compare May 31, 2020 09:34

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 31, 2020

k8s-ci-robot assigned xing-yang May 31, 2020

xing-yang reviewed Jun 3, 2020

View reviewed changes

k8s-ci-robot assigned NickrenREN Jun 3, 2020

fengzixu force-pushed the master branch from 80e8f1b to ba27592 Compare June 7, 2020 06:47

fengzixu force-pushed the master branch from 09f8fcd to b4d6db5 Compare June 7, 2020 07:07

NickrenREN reviewed Jun 7, 2020

View reviewed changes

driver/driver.mock.go Show resolved Hide resolved

NickrenREN reviewed Jun 7, 2020

View reviewed changes

fengzixu force-pushed the master branch from b4d6db5 to 4c42647 Compare June 17, 2020 15:58

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 17, 2020

fengzixu requested review from xing-yang and NickrenREN June 17, 2020 16:00

fengzixu force-pushed the master branch from 379bddf to f29b9bf Compare June 17, 2020 23:34

make mock driver support pv healthy monitor

c93fe8b

fengzixu force-pushed the master branch from f29b9bf to c93fe8b Compare June 18, 2020 08:29

xing-yang reviewed Jun 23, 2020

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 23, 2020

k8s-ci-robot assigned pohly Jun 23, 2020

xing-yang approved these changes Jun 23, 2020

View reviewed changes

pohly approved these changes Jun 23, 2020

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 23, 2020

k8s-ci-robot merged commit 1bfc435 into kubernetes-csi:master Jun 23, 2020

fengzixu mentioned this pull request Jun 27, 2020

Initial implementation to add external health monitor controller and agent kubernetes-csi/external-health-monitor#3

Merged

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Aug 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make mock driver support PV healthy monitor #268

make mock driver support PV healthy monitor #268

fengzixu commented May 31, 2020 •

edited by xing-yang

Loading

k8s-ci-robot commented May 31, 2020

k8s-ci-robot commented May 31, 2020

fengzixu commented May 31, 2020

k8s-ci-robot commented May 31, 2020

xing-yang commented May 31, 2020

xing-yang commented May 31, 2020

xing-yang Jun 3, 2020 •

edited

Loading

fengzixu Jun 7, 2020

xing-yang Jun 3, 2020

fengzixu Jun 7, 2020

xing-yang Jun 3, 2020

fengzixu Jun 7, 2020

xing-yang commented Jun 3, 2020

NickrenREN commented Jun 4, 2020 •

edited

Loading

fengzixu commented Jun 7, 2020

NickrenREN Jun 7, 2020

NickrenREN Jun 7, 2020

NickrenREN Jun 7, 2020 •

edited

Loading

fengzixu Jun 17, 2020

NickrenREN Jun 18, 2020

NickrenREN Jun 7, 2020

fengzixu commented Jun 17, 2020

xing-yang Jun 23, 2020

fengzixu Jun 23, 2020

xing-yang Jun 23, 2020

xing-yang commented Jun 23, 2020

xing-yang commented Jun 23, 2020

pohly left a comment

k8s-ci-robot commented Jun 23, 2020

make mock driver support PV healthy monitor #268

make mock driver support PV healthy monitor #268

Conversation

fengzixu commented May 31, 2020 • edited by xing-yang Loading

What I did in this PR

k8s-ci-robot commented May 31, 2020

k8s-ci-robot commented May 31, 2020

fengzixu commented May 31, 2020

k8s-ci-robot commented May 31, 2020

xing-yang commented May 31, 2020

xing-yang commented May 31, 2020

xing-yang Jun 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xing-yang commented Jun 3, 2020

NickrenREN commented Jun 4, 2020 • edited Loading

fengzixu commented Jun 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickrenREN Jun 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fengzixu commented Jun 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xing-yang commented Jun 23, 2020

xing-yang commented Jun 23, 2020

pohly left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 23, 2020

fengzixu commented May 31, 2020 •

edited by xing-yang

Loading

xing-yang Jun 3, 2020 •

edited

Loading

NickrenREN commented Jun 4, 2020 •

edited

Loading

NickrenREN Jun 7, 2020 •

edited

Loading