CSI Node info registration in kubelet #67684

verult · 2018-08-22T01:30:57Z

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #67683

Special notes for your reviewer:
Feature issue: kubernetes/enhancements#557
Design doc: kubernetes/community#2034

Missing pieces:

CSI client retry and exponential backoff logic.
CSINodeInfo object validation
e2e test with all the CSI machinery.

An RBAC rule is also added to support external-provisioner topology updates.

Release note:

Registers volume topology information reported by a node-level Container Storage Interface (CSI) driver. This enables Kubernetes support of CSI topology mechanisms.

verult · 2018-08-22T01:32:24Z

/sig storage
/assign @liggitt @saad-ali @vladimirvivien @sbezverk @msau42

verult · 2018-08-22T22:13:16Z

pkg/volume/csi/nodeinfomanager/nodeinfomanager.go

+
+// updateNode repeatedly attempts to update the corresponding node object
+// which is modified by applying the given update functions in order.
+func (lm *nodeInfoManager) updateNode(updateFuncs []nodeUpdateFunc) error {


Will rename all the receivers to nim.

verult · 2018-08-22T22:14:11Z

pkg/volume/csi/nodeinfomanager/nodeinfomanager.go

+		}
+
+		if val, ok := existingDriverMap[csiDriverName]; ok {
+			if reflect.DeepEqual(val, topologyKeys) {


Will check set equality instead in the new version that uses CSINodeInfo.

verult · 2018-08-24T00:09:02Z

Feel free to hold off on reviewing this PR for now - I'm almost done with wiring up CSINodeInfo and a good amount of logic will become obsolete

verult · 2018-08-25T02:53:48Z

Added integration with CSINodeInfo. Please only review the last commit - everything before it are rebased from #67803.

Missing functionality is updated in the top post, but adding those won't cause any major structural changes. The PR is ready for review now.

@liggitt @saad-ali @vladimirvivien @sbezverk @msau42 @gnufied

verult · 2018-08-27T22:39:02Z

@saad-ali and I discussed offline, and we are going to leave the CSI label prefix as is in Kubernetes instead of reversing it, for the time being.

msau42 · 2018-08-27T21:43:47Z

pkg/volume/csi/nodeinfomanager/nodeinfomanager.go

+
+var nodeKind = v1.SchemeGroupVersion.WithKind("Node")
+
+// nodeInfoManager is struct of channels used for communication between the driver registration


This comment needs to be updated. I don't see any channels being used

msau42 · 2018-08-27T21:45:13Z

pkg/volume/csi/nodeinfomanager/nodeinfomanager.go

+		return fmt.Errorf("error adding CSI driver node info: driverNodeID must not be empty")
+	}
+
+	if topology == nil {


If a plugin doesn't support topology will this be nil?

Yeah, thanks for the catch!

msau42 · 2018-08-27T21:46:38Z

pkg/volume/csi/nodeinfomanager/nodeinfomanager.go

+	}
+}
+
+func (nim *nodeInfoManager) AddNodeInfo(driverName string, driverNodeID string, topology *csipb.Topology) error {


Can you add a comment here describing exactly all the objects/fields that are being modified/added?

msau42 · 2018-08-27T22:03:19Z

pkg/volume/csi/nodeinfomanager/nodeinfomanager.go

+		// exponential backoff to avoid exhausting the apiserver.
+
+		nodeClient := nim.k8s.CoreV1().Nodes()
+		node, err := nodeClient.Get(string(nim.nodeName), metav1.GetOptions{})


are there plans to use a shared informer? If so, we should clone the node object first before modifying it so it doesn't impact other uses that may be sharing the same pointer.

No I wasn't planning to. Driver registration is fairly infrequent so watching the object doesn't seem worth it

msau42 · 2018-08-27T22:04:09Z

pkg/volume/csi/nodeinfomanager/nodeinfomanager.go

+
+		needUpdate := false
+		for _, update := range updateFuncs {
+			newNode, updated, err := update(node)


This works only if the multiple updateFuncs are not conflicting with each other.

I'll make it more explicit in the comments for this function that because updateFuncs are called in order, each updateFunc should consider the effects of previous updateFuncs

each updateFunc should consider the effects of previous updateFuncs

what does that mean?

I added a comment here - does that help or should it be more clear?
https://github.com/kubernetes/kubernetes/blob/9c855fb9e62de46f511d9dc0446425c1b5a76499/pkg/volume/csi/nodeinfomanager/nodeinfomanager.go#L119

You mean, if two update functions both update same field then last one will win, but caller of this function should try and ensure that - it does not happen in first place?

Well said! Will update the comment

msau42 · 2018-08-27T22:21:50Z

pkg/volume/csi/nodeinfomanager/nodeinfomanager.go

+			driverInfo.NodeID = driverNodeID
+			driverInfo.TopologyKeys = topologyKeys
+		}
+		driverInfos = append(driverInfos, driverInfo)


Should we be adding all drivers in this call to just update a single driver? Do all the CSI drivers register again when we upgrade the node?

Should we be looking at the annotations at all? IIUC, this is supposed to be the replacement for the annotations

Yeah CSI drivers should register again upon node upgrade, and this is here as a safeguard to ensure annotations and CSINodeInfo are consistent while we support both. There is always a chance that kubelet is upgraded while the previous Node object stays, and annotations and CSINodeInfo could be momentarily out of sync.

Is it necessary to keep the annotations and CSIDriverInfo in sync on the first driver's registration, if the other driver will register again later? In an upgrade scenario, then this means we're on the first boot after upgrade and we're processing the first driver registration. We're adding an entry for all the other drivers too even though they have not gone through registration yet. Could that cause other issues?

I think this method should get whole list of registered CSI drivers so it creates/updates complete CSINodeInfo with all drivers. The result should be the same in the end, however it feels more robust. I don't line CSINodeInfo with empty topology for some drivers just because we translated it from an annotation.

This method gets called for a single driver registration though, and only on the first one since it's creating the object for the first time.

Discussed with @msau42 offline. The current implementation doesn't guarantee the node info is always up-to-date. In particular, with the current kubelet plugin registration design, there is no way to remove a non-existent driver unless it detects that a driver socket is removed (after unregistration is implemented). If a driver is removed while kubelet is down, the node information will remain. So whatever issue comes up when CSINodeInfo is kept in sync with annotation is unavoidable today.

But I'll remove the logic to keep the two sources in sync, because it complicates the implementation without adding too much benefit. One minor advantage of keeping them in sync is to avoid debugging confusion, but I think it'll be OK.

msau42 · 2018-08-27T22:54:44Z

pkg/volume/csi/csi_plugin.go

+		// all other CSI components will be able to get the actual socket of CSI drivers by its name.
+		csiDrivers.Lock()
+		defer csiDrivers.Unlock()
+		csiDrivers.driversMap[pluginName] = csiDriver{driverName: pluginName, driverEndpoint: endpoint}


Previously the lock was being held across the entire function instead of just the map update. Could there be any issues if this map has an entry but we haven't finished registering the plugin yet? Should we update the map at the very end?

In both the previous and the current version, the lock is only held on write, all within this RegistrationCallback() function. The map has to be updated before the csi.NodeGetInfo() call unfortunately because it relies on the driver info to call out to gRPC.

However, when plugin unregistration is implemented in the future, depending on how it's implemented, there could be a race between calls of RegistrationCallback(). Will update to lock the entire function once.

Discussed with @msau42 offline. For now we'll keep the map locked as short as possible. When unregistration is implemented we'll reconsider again.

Will add comments explaining why it's OK to keep locking short.

…ing to CSINodeInfo; added unit tests for node updates; updated RBAC, NodeAuthorizer, NodeRestriction.

saad-ali · 2018-09-07T04:29:22Z

/lgtm
/approve

liggitt · 2018-09-07T16:12:49Z

plugin/pkg/auth/authorizer/rbac/bootstrappolicy/policy.go

@@ -164,6 +164,10 @@ func NodeRules() []rbacv1.PolicyRule {
 			nodePolicyRules = append(nodePolicyRules, csiDriverRule)
 		}
 	}
+	if utilfeature.DefaultFeatureGate.Enabled(features.KubeletPluginsWatcher) {
+		csiNodeInfoRule := rbacv1helpers.NewRule("get", "create", "update", "patch", "delete").Groups("csi.storage.k8s.io").Resources("csinodeinfos").RuleOrDie()


Looks like the unit test is failing because this feature flag is enabled by default now. Is this still the right flag that determines if the node needs these permissions, or is it fenced by the CSINodeInfo flag as well now?

If this is correct as is, the fixture for the unit test just needs regenerating (instructions are in the unit test failure message)

That's one spot I missed. Thanks for the catch!

liggitt · 2018-09-07T16:27:09Z

one comment on the gate being checked by the RBAC policy change and the unit test failure, authz and admission changes lgtm otherwise

it's worth noting that the removal of driver info from the node depends on observing the removal of the driver (rather than reconciling to current observed state), and doesn't handle errors in removing driver info by retrying. That doesn't necessarily block this PR, but seems important to resolve before this graduates.

verult · 2018-09-07T18:45:39Z

To clarify, in this PR driver is only unregistered when there's an error adding the driver. Driver unregistration will be added in the next release. The rest of the CSI system should always assume node info is somewhat out of date, since kubelet can't provide the guarantee that updates are reflected immediately anyway, in the best case.

Does the same logic apply to driver addition as well?

verult · 2018-09-07T20:21:33Z

Updated RBAC test

verult · 2018-09-07T20:38:26Z

Keeping a laundry list of considerations for CSI kubelet driver registration moving forward:
#67972

liggitt · 2018-09-07T20:43:16Z

/approve

RBAC, node authz and admission changes lgtm

verult · 2018-09-07T21:39:47Z

/retest

verult · 2018-09-07T23:42:00Z

The GCE e2e failure is likely caused by this PR. Taking a look

msau42 · 2018-09-08T00:58:16Z

/lgtm

k8s-ci-robot · 2018-09-08T00:58:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, msau42, saad-ali, verult

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:

~~hack/OWNERS~~ [liggitt]
~~pkg/controller/volume/attachdetach/OWNERS~~ [saad-ali]
~~pkg/features/OWNERS~~ [liggitt,saad-ali]
~~pkg/volume/csi/OWNERS~~ [saad-ali]
~~plugin/pkg/admission/noderestriction/OWNERS~~ [liggitt]
~~plugin/pkg/auth/OWNERS~~ [liggitt]
~~staging/src/k8s.io/csi-api/OWNERS~~ [saad-ali]
~~test/OWNERS~~ [liggitt,saad-ali]

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 · 2018-09-08T02:54:37Z

@verult: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-local-e2e-containerized	7ab3a2fb33939d37d78475d20eb68f646c7eb115	link	`/test pull-kubernetes-local-e2e-containerized`

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.

fejta-bot · 2018-09-08T05:13:38Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

k8s-github-robot · 2018-09-08T07:16:51Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here: https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md.

verult · 2018-09-12T17:55:16Z

Per #68519 (comment): installing CRD outside k8s core brings back issue #2:

What should be the AddNodeInfo() behavior if CRD doesn’t exist? Continue with registration? Fail and unregister?

I side with @msau42 that if the CSINodeInfo feature flag is enabled, one would expect topology to work, and continuing with registration by falling back to node annotation means topology breaks. So it should fail and unregister the driver. This is the current behavior so no actions required.

k8s-ci-robot requested review from ericchiang and gnufied August 22, 2018 01:31

k8s-ci-robot assigned liggitt, msau42, saad-ali, sbezverk and vladimirvivien Aug 22, 2018

verult force-pushed the top-csi-driver-registration branch from f87962d to c71f9cb Compare August 22, 2018 19:24

verult commented Aug 22, 2018

View reviewed changes

verult force-pushed the top-csi-driver-registration branch from c71f9cb to b2ff926 Compare August 22, 2018 22:17

verult force-pushed the top-csi-driver-registration branch from b2ff926 to ab5b04e Compare August 25, 2018 02:43

verult force-pushed the top-csi-driver-registration branch from ab5b04e to 0128473 Compare August 25, 2018 03:06

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

msau42 reviewed Aug 27, 2018

View reviewed changes

verult force-pushed the top-csi-driver-registration branch from 50db873 to db19252 Compare September 7, 2018 01:57

Implemented logic in kubelet for registering node info, including wir…

becc6a9

…ing to CSINodeInfo; added unit tests for node updates; updated RBAC, NodeAuthorizer, NodeRestriction.

verult force-pushed the top-csi-driver-registration branch from db19252 to e966eb1 Compare September 7, 2018 03:13

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 7, 2018

liggitt reviewed Sep 7, 2018

View reviewed changes

saad-ali mentioned this pull request Sep 7, 2018

CSI informer is spamming logs in non-alpha clusters #68378

Closed

verult force-pushed the top-csi-driver-registration branch from e966eb1 to a4f339a Compare September 7, 2018 20:20

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 7, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 7, 2018

verult mentioned this pull request Sep 7, 2018

Consolidated CSIDriver logic under CSIDriverRegistry flag #68426

Merged

Rearranged feature flags

94d649b

verult force-pushed the top-csi-driver-registration branch from a4f339a to 94d649b Compare September 8, 2018 00:45

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 8, 2018

k8s-github-robot merged commit f26556c into kubernetes:master Sep 8, 2018

msau42 mentioned this pull request Sep 10, 2018

E2E failure: CSI hostpath tests are failing in alpha clusters since 9/8 #68490

Closed

marpaia mentioned this pull request Sep 20, 2018

Include SIG headers in the duplicate notes section marpaia/release-notes#6

Merged


		var nodeKind = v1.SchemeGroupVersion.WithKind("Node")

		// nodeInfoManager is struct of channels used for communication between the driver registration

CSI Node info registration in kubelet #67684

CSI Node info registration in kubelet #67684

Conversation

verult commented Aug 22, 2018 • edited Loading

verult commented Aug 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

verult commented Aug 24, 2018

verult commented Aug 25, 2018

verult commented Aug 27, 2018

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saad-ali commented Sep 7, 2018

liggitt Sep 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Sep 7, 2018 • edited Loading

verult commented Sep 7, 2018

verult commented Sep 7, 2018

verult commented Sep 7, 2018

liggitt commented Sep 7, 2018 • edited Loading

verult commented Sep 7, 2018

verult commented Sep 7, 2018

msau42 commented Sep 8, 2018

k8s-ci-robot commented Sep 8, 2018

k8s-ci-robot commented Sep 8, 2018 • edited Loading

fejta-bot commented Sep 8, 2018

k8s-github-robot commented Sep 8, 2018

verult commented Sep 12, 2018

verult commented Aug 22, 2018 •

edited

Loading

liggitt Sep 7, 2018 •

edited

Loading

liggitt commented Sep 7, 2018 •

edited

Loading

liggitt commented Sep 7, 2018 •

edited

Loading

k8s-ci-robot commented Sep 8, 2018 •

edited

Loading