initial implementation of refactor host-path-driver #276

fengzixu · 2021-04-18T04:19:52Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

For fixing bugs that are mentioned in the issues below

Data lost after reboot #251 (comment)

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Fixing bug of host-path-csi-driver which may causes it's failed to start up.

fengzixu · 2021-04-18T04:27:39Z

/assign @fengzixu @pohly @xing-yang

k8s-ci-robot · 2021-04-18T04:49:00Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: fengzixu
To complete the pull request process, please ask for approval from pohly after the PR has been reviewed.

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

Needs approval from an approver in each of these files:

OWNERS

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

pohly · 2021-04-19T07:45:51Z

pkg/state/snapshot.go

+	return filepath.Join(dataRoot, fmt.Sprintf("%s%s", snapshotID, snapshotExt))
+}
+
+func (hps *HostPathDriverState) loadSnapshotsFromFile(snapshotFilePath string) error {


Instead of loadSnapshotsFromFile and loadVolumesFromFile and many different files, a single loadFromFile and saveToFile with one state file that contains multiple volumes and snapshots is enough.

loadFromFile then only needs to be called once during startup when constructing HostPathDriverState. saveToFile needs to be called after each change to the in-memory struct.

Don't worry about failure scenarios when writing that file fails, just check and return the error.

This is not a high-quality implementation and should come with a warning that a real driver may have to be more careful about not loosing state, but for this driver it is sufficient.

Instead of loadSnapshotsFromFile and loadVolumesFromFile a single loadFromFile and saveToFile

I agree this point. Let me improve it. But I don't think aggregate all of logic into the single state.go file is good idea. I want this refractory change can give a better experience for the new contributor of this project. It is easy for them to understand" What state package did".

loadFromFile then only needs to be called once during startup when constructing HostPathDriverState. saveToFile needs to be called after each change to the in-memory struct.

Sure

"The state package manages the internal state of the driver on a node. It ensures that the driver can be stopped and restarted".

It should be that simple. All additional logic for managing snapshots and volumes should be where it resides at the moment.

pohly · 2021-04-19T07:46:14Z

pkg/state/snapshot.go

+	// Lock before acting on global state. A production-quality
+	// driver might use more fine-grained locking.
+	hps.SnapshotsFileRWLock.Lock()
+	defer hps.SnapshotsFileRWLock.Unlock()


Locking should be entirely in the gRPC layer, not here.

pohly · 2021-04-19T07:47:44Z

pkg/state/types.go

+	// backed by a Pod volume.
+	dataRoot            = "/csi-data-dir"
+	volumesDataDir      = "/csi-volumes-data"
+	volumesDataFilePath = path.Join(volumesDataDir, "volumes.json")


These paths must be configurable for unit testing and when running the driver manually as non-root outside of a container.

k8s-ci-robot · 2021-04-19T16:11:55Z

@fengzixu: PR needs rebase.

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 · 2021-04-19T16:12:04Z

@fengzixu: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-kubernetes-csi-csi-driver-host-path-1-19-on-kubernetes-1-19	`e0b0fd9`	link	`/test pull-kubernetes-csi-csi-driver-host-path-1-19-on-kubernetes-1-19`
pull-kubernetes-csi-csi-driver-host-path-1-20-on-kubernetes-1-20	`e0b0fd9`	link	`/test pull-kubernetes-csi-csi-driver-host-path-1-20-on-kubernetes-1-20`
pull-kubernetes-csi-csi-driver-host-path-unit	`e0b0fd9`	link	`/test pull-kubernetes-csi-csi-driver-host-path-unit`
pull-kubernetes-csi-csi-driver-host-path-1-18-on-kubernetes-1-18	`e0b0fd9`	link	`/test pull-kubernetes-csi-csi-driver-host-path-1-18-on-kubernetes-1-18`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

fengzixu · 2021-04-19T16:23:19Z

pkg/state/state.go

+	return snapshot, nil
+}
+
+func (hps *HostPathDriverState) DeleteSnapshot(snapshotId string) error {


Create/Delete snapshot method doesn't implement "file operation"

fengzixu · 2021-04-19T16:23:57Z

pkg/state/state_test.go

+	assert.EqualValues(t, testSnapshots, snapshots)
+}
+
+// func TestLoadStateFromFile(t *testing.T) {


Test should be uncommented after https://github.com/kubernetes-csi/csi-driver-host-path/pull/276/files#r615995654

pohly · 2021-04-19T17:10:52Z

pkg/state/state.go

+// adds the volume to the list.
+//
+// It returns the volume path or err if one occurs. That error is suitable as result of a gRPC call.
+func (hps *HostPathDriverState) CreateVolume(volID, name string, cap int64, volAccessType AccessType, ephemeral bool, kind string, maxVolumeSize int64, capacity Capacity) (hpv *HostPathVolume, finalErr error) {


The "state" package should not implement the complete logic for creating and manipulating volumes and snapshots. That mixes state management with CSI driver logic.

HostPathDriverState.AddVolume should just take a Volume struct and then have a few lines of code which update the update the map and store the entire struct in a state file.

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 18, 2021

k8s-ci-robot requested review from msau42 and pohly April 18, 2021 04:19

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Apr 18, 2021

fengzixu force-pushed the fix-bug-listing-volume branch from 51639b6 to a93049e Compare April 18, 2021 04:25

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 18, 2021

fengzixu changed the title ~~WIPL initial implementation of refactor host-path-driver~~ [WIP]:initial implementation of refactor host-path-driver Apr 18, 2021

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 18, 2021

k8s-ci-robot assigned fengzixu, pohly and xing-yang Apr 18, 2021

pohly reviewed Apr 19, 2021

View reviewed changes

refact hostpath driver

e0b0fd9

fengzixu force-pushed the fix-bug-listing-volume branch from ba026be to e0b0fd9 Compare April 19, 2021 16:11

fengzixu changed the title ~~[WIP]:initial implementation of refactor host-path-driver~~ initial implementation of refactor host-path-driver Apr 19, 2021

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 19, 2021

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 19, 2021

fengzixu requested a review from pohly April 19, 2021 16:18

fengzixu commented Apr 19, 2021

View reviewed changes

pohly reviewed Apr 19, 2021

View reviewed changes

pohly mentioned this pull request Apr 20, 2021

dump and restore internal state #277

Merged

fengzixu closed this Apr 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

initial implementation of refactor host-path-driver #276

initial implementation of refactor host-path-driver #276

fengzixu commented Apr 18, 2021

fengzixu commented Apr 18, 2021

k8s-ci-robot commented Apr 18, 2021

pohly Apr 19, 2021

fengzixu Apr 19, 2021 •

edited

Loading

pohly Apr 19, 2021

pohly Apr 19, 2021

pohly Apr 19, 2021

k8s-ci-robot commented Apr 19, 2021

k8s-ci-robot commented Apr 19, 2021

fengzixu Apr 19, 2021

fengzixu Apr 19, 2021

pohly Apr 19, 2021

initial implementation of refactor host-path-driver #276

initial implementation of refactor host-path-driver #276

Conversation

fengzixu commented Apr 18, 2021

fengzixu commented Apr 18, 2021

k8s-ci-robot commented Apr 18, 2021

pohly Apr 19, 2021

Choose a reason for hiding this comment

fengzixu Apr 19, 2021 • edited Loading

Choose a reason for hiding this comment

pohly Apr 19, 2021

Choose a reason for hiding this comment

pohly Apr 19, 2021

Choose a reason for hiding this comment

pohly Apr 19, 2021

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 19, 2021

k8s-ci-robot commented Apr 19, 2021

fengzixu Apr 19, 2021

Choose a reason for hiding this comment

fengzixu Apr 19, 2021

Choose a reason for hiding this comment

pohly Apr 19, 2021

Choose a reason for hiding this comment

fengzixu Apr 19, 2021 •

edited

Loading