Add explicit "Installed" field to CSINodeInfo and change update semantics #70515

davidz627 · 2018-11-01T01:22:19Z

/assign @verult @saad-ali
/cc @bertinatto @jsafrane @vladimirvivien

/kind feature
/sig storage

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 #70504

This PR Introduces the Installed field and MigratedOnNode field to the CSINodeInfo object. The semantics of the object have also been changed so that on driver installation we create/update the driver info with Installed=true and on uninstallation of the driver the driver info object persists but Installed=false. This enables detection of version/configuration skew between master and node for CSI Migration (kubernetes/enhancements#625).

I have also added hooks for un-registering/uninstalling the driver

Breaking change: CSINodeInfo split into Spec and Status. New fields Available and VolumePluginMechanism added to CSINodeInfo csi-api object. CSIDriverInfo no longer deleted on Driver uninstallation, instead Available flag is set to false.

davidz627 · 2018-11-01T01:23:09Z

/assign @liggitt
for API review/approval

staging/src/k8s.io/csi-api/pkg/crd/testdata/csinodeinfo.yaml

pkg/volume/csi/nodeinfomanager/nodeinfomanager.go

verult · 2018-11-01T18:55:19Z

pkg/volume/csi/nodeinfomanager/nodeinfomanager.go

@@ -452,31 +434,20 @@ func (nim *nodeInfoManager) removeCSINodeInfo(csiDriverName string) error {

 		nodeInfoClient := csiKubeClient.CsiV1alpha1().CSINodeInfos()
 		nodeInfo, err := nodeInfoClient.Get(string(nim.nodeName), metav1.GetOptions{})
-		if nodeInfo == nil || errors.IsNotFound(err) {


Why is this block removed?

nodeInfo should never be nil here, it's an error if it is and we want to fail fast

pkg/volume/csi/nodeinfomanager/nodeinfomanager.go

davidz627 · 2018-11-01T21:05:04Z

/assign @thockin @lavalamp @smarterclayton
the original API reviewers for this API

staging/src/k8s.io/csi-api/pkg/apis/csi/v1alpha1/types.go

davidz627 · 2018-11-08T22:25:52Z

/retest

staging/src/k8s.io/csi-api/pkg/apis/csi/v1alpha1/types.go

pkg/volume/csi/nodeinfomanager/nodeinfomanager.go

staging/src/k8s.io/csi-api/pkg/apis/csi/v1alpha1/README.md

verult

Logic largely LGTM, minus a couple of nits.

Will leave to API reviewers to give the final LGTM

staging/src/k8s.io/csi-api/pkg/apis/csi/v1alpha1/README.md

davidz627 · 2018-11-09T01:15:56Z

/retest

davidz627 · 2018-11-09T02:10:26Z

/retest

staging/src/k8s.io/csi-api/pkg/apis/csi/v1alpha1/types.go

… Object. Split CSINodeInfo into Spec and Status.

…e new fields.

lavalamp · 2018-11-09T05:05:35Z

staging/src/k8s.io/csi-api/pkg/crd/manifests/csinodeinfo.yaml

+                    description: Whether the CSI driver is installed.
+                    type: boolean
+                  volumePluginMechanism:
+                    description: Indicates to external components the required mechanism


I think I'd expect these descriptions to match the comments on the go type? We can fix that in a followup since it's probably a tooling problem...

lavalamp · 2018-11-09T05:06:00Z

/approve
/lgtm

k8s-ci-robot · 2018-11-09T05:06:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidz627, lavalamp, mikedanese, saad-ali

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:

~~cluster/addons/storage-crds/OWNERS~~ [lavalamp,mikedanese,saad-ali]
~~pkg/volume/csi/OWNERS~~ [lavalamp,saad-ali]
~~plugin/pkg/admission/noderestriction/OWNERS~~ [lavalamp,mikedanese]
~~staging/src/k8s.io/csi-api/OWNERS~~ [lavalamp,saad-ali]
~~staging/src/k8s.io/csi-api/pkg/apis/OWNERS~~ [lavalamp]
~~staging/src/k8s.io/csi-api/pkg/crd/OWNERS~~ [lavalamp]
~~test/OWNERS~~ [lavalamp,mikedanese,saad-ali]

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

fejta-bot · 2018-11-09T09:56:22Z

/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.

fejta-bot · 2018-11-09T12:44:21Z

/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.

msau42

Please work with @verult to update and cut a new external provisioner. This impacts his e2e work for csi topology.

msau42 · 2018-11-11T14:10:21Z

staging/src/k8s.io/csi-api/pkg/apis/csi/v1alpha1/README.md

+For CSI Migration Alpha we expect any user who turns on the feature has both
+Kubelet and ADC at a version where the CSINodeInfo's are being created on
+Kubelet startup. We will not promote the feature to Beta (on by default) until
+the CSINodeInfo's are being created on Kubelet startup for a 2 version skew to


I don't think this is going to work. CSINodeInfo is under an alpha feature gate which means they won't be created by default. I think you might need two feature gates, one for controller and one for nodes, then move the node feature gate to beta first.

Or ADC should fallback to intree if migration is on but CSINodeInfo doesn't exist

Nm I think I got it now. The CSIMigration feature gate cannot move to beta until 2 releases after CSINodeInfo is beta. It might be good to explicitly detail this here.

Actually no I'm confused again. The logic that auto creates CSINodeInfo objects for migrated drivers will be under the specific plugin's CSIMigration feature gate?

#70909

The plan is that CSINodeInfo objects are auto created iff CSINodeInfo feature gate is on.
Then the gates for each CSIMigration plugin can only go Beta (default) 2 releases after the CSINodeInfos have been added.

would be happy to discuss in-person

msau42 · 2018-11-11T14:15:57Z

staging/src/k8s.io/csi-api/pkg/apis/csi/v1alpha1/types.go

 	// This MUST be the same name returned by the CSI GetPluginName() call for
 	// that driver.
-	Driver string `json:"driver"`
+	Name string `json:"name"`


What happens if the driver is upgraded and changes values stored in the spec? Are we allowing spec to be mutable for this reason?

I think Spec should be mutable as topology keys or node ID may change for the driver

k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Nov 1, 2018

k8s-ci-robot assigned saad-ali and verult Nov 1, 2018

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. kind/feature Categorizes issue or PR as related to a new feature. sig/storage Categorizes an issue or PR as relevant to SIG Storage. labels Nov 1, 2018

k8s-ci-robot requested review from bertinatto, jsafrane and vladimirvivien November 1, 2018 01:22

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API labels Nov 1, 2018

k8s-ci-robot assigned liggitt Nov 1, 2018

liggitt reviewed Nov 1, 2018

View reviewed changes

staging/src/k8s.io/csi-api/pkg/crd/testdata/csinodeinfo.yaml Outdated Show resolved Hide resolved

davidz627 force-pushed the feature/csiNodeInfo branch from e0c1a09 to bf36bb0 Compare November 1, 2018 01:46

liggitt mentioned this pull request Nov 1, 2018

[WIP] Move CSIDriver and CSINodeInfo objects to beta #70367

Closed

verult suggested changes Nov 1, 2018

View reviewed changes

k8s-ci-robot assigned lavalamp, smarterclayton and thockin Nov 1, 2018

davidz627 force-pushed the feature/csiNodeInfo branch from 750c423 to 1ee61f6 Compare November 1, 2018 21:18

k8s-ci-robot added the sig/testing Categorizes an issue or PR as relevant to SIG Testing. label Nov 1, 2018

davidz627 force-pushed the feature/csiNodeInfo branch 3 times, most recently from 0fe427d to 02b677b Compare November 1, 2018 22:00

davidz627 mentioned this pull request Nov 1, 2018

CSI Kubelet Plugin Watcher DeRegistration #70548

Merged

lavalamp reviewed Nov 1, 2018

View reviewed changes

staging/src/k8s.io/csi-api/pkg/apis/csi/v1alpha1/types.go Outdated Show resolved Hide resolved

lavalamp reviewed Nov 1, 2018

View reviewed changes

staging/src/k8s.io/csi-api/pkg/apis/csi/v1alpha1/types.go Outdated Show resolved Hide resolved

lavalamp reviewed Nov 8, 2018

View reviewed changes

staging/src/k8s.io/csi-api/pkg/apis/csi/v1alpha1/types.go Outdated Show resolved Hide resolved

davidz627 force-pushed the feature/csiNodeInfo branch from 7c96be0 to ca72b25 Compare November 8, 2018 22:39

verult reviewed Nov 8, 2018

View reviewed changes

pkg/volume/csi/nodeinfomanager/nodeinfomanager.go Outdated Show resolved Hide resolved

verult reviewed Nov 8, 2018

View reviewed changes

staging/src/k8s.io/csi-api/pkg/apis/csi/v1alpha1/README.md Outdated Show resolved Hide resolved

verult reviewed Nov 8, 2018

View reviewed changes

staging/src/k8s.io/csi-api/pkg/apis/csi/v1alpha1/README.md Outdated Show resolved Hide resolved

davidz627 force-pushed the feature/csiNodeInfo branch from ca72b25 to c7e7c90 Compare November 9, 2018 00:44

lavalamp reviewed Nov 9, 2018

View reviewed changes

staging/src/k8s.io/csi-api/pkg/apis/csi/v1alpha1/types.go Outdated Show resolved Hide resolved

lavalamp reviewed Nov 9, 2018

View reviewed changes

staging/src/k8s.io/csi-api/pkg/apis/csi/v1alpha1/types.go Outdated Show resolved Hide resolved

davidz627 added 3 commits November 8, 2018 19:44

Add fields available and volumePluginMechanism to CSINodeInfo CRD API…

3595dee

… Object. Split CSINodeInfo into Spec and Status.

Updated test files with new fields

4621887

Change semantics of driver install and uninstall in CSINodeInfo to us…

06f3b26

…e new fields.

davidz627 force-pushed the feature/csiNodeInfo branch from c7e7c90 to 06f3b26 Compare November 9, 2018 03:45

lavalamp reviewed Nov 9, 2018

View reviewed changes

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

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

k8s-ci-robot merged commit e133ab2 into kubernetes:master Nov 9, 2018

davidz627 deleted the feature/csiNodeInfo branch November 9, 2018 18:05

verult mentioned this pull request Nov 9, 2018

Add validation of CSINodeInfo fields before Create/Update actions #70742

Merged

msau42 reviewed Nov 11, 2018

View reviewed changes

davidz627 mentioned this pull request Feb 14, 2019

Make CSINodeInfo and CSIDriver Core APIs #73883

Merged

james-stocks mentioned this pull request Jun 17, 2019

[kubectl] Upgrade to k8s 1.14.3 habitat-sh/core-plans#2657

Closed

liggitt removed the sig/auth Categorizes an issue or PR as relevant to SIG Auth. label Feb 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add explicit "Installed" field to CSINodeInfo and change update semantics #70515

Add explicit "Installed" field to CSINodeInfo and change update semantics #70515

davidz627 commented Nov 1, 2018 •

edited

Loading

davidz627 commented Nov 1, 2018

verult Nov 1, 2018

davidz627 Nov 1, 2018

davidz627 commented Nov 1, 2018

davidz627 commented Nov 8, 2018

verult left a comment

davidz627 commented Nov 9, 2018

davidz627 commented Nov 9, 2018

lavalamp Nov 9, 2018

lavalamp commented Nov 9, 2018

k8s-ci-robot commented Nov 9, 2018

fejta-bot commented Nov 9, 2018

fejta-bot commented Nov 9, 2018

msau42 left a comment

msau42 Nov 11, 2018

msau42 Nov 11, 2018

msau42 Nov 11, 2018

davidz627 Nov 12, 2018

davidz627 Nov 12, 2018

msau42 Nov 11, 2018

davidz627 Nov 12, 2018

Add explicit "Installed" field to CSINodeInfo and change update semantics #70515

Add explicit "Installed" field to CSINodeInfo and change update semantics #70515

Conversation

davidz627 commented Nov 1, 2018 • edited Loading

davidz627 commented Nov 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidz627 commented Nov 1, 2018

davidz627 commented Nov 8, 2018

verult left a comment

Choose a reason for hiding this comment

davidz627 commented Nov 9, 2018

davidz627 commented Nov 9, 2018

Choose a reason for hiding this comment

lavalamp commented Nov 9, 2018

k8s-ci-robot commented Nov 9, 2018

fejta-bot commented Nov 9, 2018

fejta-bot commented Nov 9, 2018

msau42 left a 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

davidz627 commented Nov 1, 2018 •

edited

Loading