Skip to content

Commit

Permalink
Removed dependency on node annotation in favor of CSINode resource
Browse files Browse the repository at this point in the history
  • Loading branch information
@Danil-Grigorev
Danil-Grigorev committed Jan 31, 2020
1 parent 57193d7 commit 1c61d6a
Show file tree
Hide file tree
Showing 4 changed files with 39 additions and 151 deletions.
15 changes: 5 additions & 10 deletions pkg/controller/csi_handler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -692,19 +692,14 @@ func (h *csiHandler) getNodeID(driver string, nodeName string, va *storage.Volum
klog.V(4).Infof("Found NodeID %s in CSINode %s", nodeID, nodeName)
return nodeID, nil
}
klog.V(4).Infof("CSINode %s does not contain driver %s", nodeName, driver)
// CSINode exists, but does not have the requested driver.
// Fall through to Node annotation.
} else {
// Can't get CSINode, fall through to Node annotation.
klog.V(4).Infof("Can't get CSINode %s: %s", nodeName, err)
errMessage := fmt.Sprintf("CSINode %s does not contain driver %s", nodeName, driver)
klog.V(4).Info(errMessage)
return "", errors.New(errMessage)
}

// Check Node annotation.
node, err := h.nodeLister.Get(nodeName)
if err == nil {
return GetNodeIDFromNode(driver, node)
}
// Can't get CSINode, check Volume Attachment.
klog.V(4).Infof("Can't get CSINode %s: %s", nodeName, err)

// Check VolumeAttachment annotation as the last resort if caller wants so (i.e. has provided one).
if va == nil {
Expand Down
84 changes: 31 additions & 53 deletions pkg/controller/csi_handler_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -194,19 +194,10 @@ func node() *v1.Node {
return &v1.Node{
ObjectMeta: metav1.ObjectMeta{
Name: testNodeName,
Annotations: map[string]string{
"csi.volume.kubernetes.io/nodeid": fmt.Sprintf("{ %q: %q }", testAttacherName, testNodeID),
},
},
}
}

func nodeWithoutAnnotations() *v1.Node {
n := node()
n.Annotations = nil
return n
}

func csiNode() *storage.CSINode {
return &storage.CSINode{
ObjectMeta: metav1.ObjectMeta{
Expand Down Expand Up @@ -295,7 +286,7 @@ func TestCSIHandler(t *testing.T) {
//
{
name: "VolumeAttachment added -> successful attachment",
initialObjects: []runtime.Object{pvWithFinalizer(), node()},
initialObjects: []runtime.Object{pvWithFinalizer(), node(), csiNode()},
addedVA: va(false /*attached*/, "" /*finalizer*/, nil /* annotations */),
expectedActions: []core.Action{
// Finalizer is saved first
Expand All @@ -312,7 +303,7 @@ func TestCSIHandler(t *testing.T) {
},
{
name: "VolumeAttachment with InlineVolumeSpec -> successful attachment",
initialObjects: []runtime.Object{node()},
initialObjects: []runtime.Object{node(), csiNode()},
addedVA: vaWithInlineSpec(va(false /*attached*/, "" /*finalizer*/, nil /* annotations */)),
expectedActions: []core.Action{
core.NewPatchAction(vaGroupResourceVersion, metav1.NamespaceNone, testPVName+"-"+testNodeName,
Expand All @@ -328,7 +319,7 @@ func TestCSIHandler(t *testing.T) {
},
{
name: "readOnly VolumeAttachment added -> successful attachment",
initialObjects: []runtime.Object{pvReadOnly(pvWithFinalizer()), node()},
initialObjects: []runtime.Object{pvReadOnly(pvWithFinalizer()), node(), csiNode()},
addedVA: va(false /*attached*/, "" /*finalizer*/, nil /* annotations */),
expectedActions: []core.Action{
// Finalizer is saved first
Expand All @@ -345,7 +336,7 @@ func TestCSIHandler(t *testing.T) {
},
{
name: "readOnly VolumeAttachment with InlineVolumeSpec -> successful attachment",
initialObjects: []runtime.Object{node()},
initialObjects: []runtime.Object{node(), csiNode()},
addedVA: vaInlineSpecReadOnly(vaWithInlineSpec(va(false /*attached*/, "" /*finalizer*/, nil /* annotations */))),
expectedActions: []core.Action{
core.NewPatchAction(vaGroupResourceVersion, metav1.NamespaceNone, testPVName+"-"+testNodeName,
Expand All @@ -361,7 +352,7 @@ func TestCSIHandler(t *testing.T) {
},
{
name: "VolumeAttachment updated -> successful attachment",
initialObjects: []runtime.Object{pvWithFinalizer(), node()},
initialObjects: []runtime.Object{pvWithFinalizer(), node(), csiNode()},
updatedVA: va(false, "", nil),
expectedActions: []core.Action{
// Finalizer is saved first
Expand All @@ -378,7 +369,7 @@ func TestCSIHandler(t *testing.T) {
},
{
name: "VolumeAttachment with InlineVolumeSpec updated -> successful attachment",
initialObjects: []runtime.Object{node()},
initialObjects: []runtime.Object{node(), csiNode()},
updatedVA: vaWithInlineSpec(va(false /*attached*/, "" /*finalizer*/, nil /* annotations */)),
expectedActions: []core.Action{
// Finalizer is saved first
Expand All @@ -395,7 +386,7 @@ func TestCSIHandler(t *testing.T) {
},
{
name: "VolumeAttachment with attributes -> successful attachment",
initialObjects: []runtime.Object{pvWithAttributes(pvWithFinalizer(), map[string]string{"foo": "bar"}), node()},
initialObjects: []runtime.Object{pvWithAttributes(pvWithFinalizer(), map[string]string{"foo": "bar"}), node(), csiNode()},
updatedVA: va(false, "", nil),
expectedActions: []core.Action{
// Finalizer is saved first
Expand All @@ -412,7 +403,7 @@ func TestCSIHandler(t *testing.T) {
},
{
name: "VolumeAttachment with InlineVolumeSpec and attributes -> successful attachment",
initialObjects: []runtime.Object{node()},
initialObjects: []runtime.Object{node(), csiNode()},
updatedVA: vaInlineSpecWithAttributes(vaWithInlineSpec(va(false, "", nil)) /*va*/, map[string]string{"foo": "bar"} /*attributes*/),
expectedActions: []core.Action{
// Finalizer is saved first
Expand All @@ -429,7 +420,7 @@ func TestCSIHandler(t *testing.T) {
},
{
name: "VolumeAttachment with secrets -> successful attachment",
initialObjects: []runtime.Object{pvWithSecret(pvWithFinalizer(), "secret"), node(), secret()},
initialObjects: []runtime.Object{pvWithSecret(pvWithFinalizer(), "secret"), node(), secret(), csiNode()},
updatedVA: va(false, "", nil),
expectedActions: []core.Action{
core.NewGetAction(secretGroupResourceVersion, "default", "secret"),
Expand All @@ -447,7 +438,7 @@ func TestCSIHandler(t *testing.T) {
},
{
name: "VolumeAttachment with InlineVolumeSpec and secrets -> successful attachment",
initialObjects: []runtime.Object{node(), secret()},
initialObjects: []runtime.Object{node(), secret(), csiNode()},
updatedVA: vaInlineSpecWithSecret(vaWithInlineSpec(va(false, "", nil)) /*va*/, "secret" /*secret*/),
expectedActions: []core.Action{
core.NewGetAction(secretGroupResourceVersion, "default", "secret"),
Expand All @@ -465,7 +456,7 @@ func TestCSIHandler(t *testing.T) {
},
{
name: "VolumeAttachment with empty secrets -> successful attachment",
initialObjects: []runtime.Object{pvWithSecret(pvWithFinalizer(), "emptySecret"), node(), emptySecret()},
initialObjects: []runtime.Object{pvWithSecret(pvWithFinalizer(), "emptySecret"), node(), emptySecret(), csiNode()},
updatedVA: va(false, "", nil),
expectedActions: []core.Action{
core.NewGetAction(secretGroupResourceVersion, "default", "emptySecret"),
Expand All @@ -483,7 +474,7 @@ func TestCSIHandler(t *testing.T) {
},
{
name: "VolumeAttachment with InlineVolumeSpec and empty secrets -> successful attachment",
initialObjects: []runtime.Object{node(), emptySecret()},
initialObjects: []runtime.Object{node(), emptySecret(), csiNode()},
updatedVA: vaInlineSpecWithSecret(vaWithInlineSpec(va(false, "", nil)) /*va*/, "emptySecret" /*secret*/),
expectedActions: []core.Action{
core.NewGetAction(secretGroupResourceVersion, "default", "emptySecret"),
Expand Down Expand Up @@ -513,7 +504,7 @@ func TestCSIHandler(t *testing.T) {
},
{
name: "VolumeAttachment updated -> PV finalizer is added",
initialObjects: []runtime.Object{pv(), node()},
initialObjects: []runtime.Object{pv(), node(), csiNode()},
updatedVA: va(false, "", nil),
expectedActions: []core.Action{
// PV Finalizer after VA
Expand All @@ -534,7 +525,7 @@ func TestCSIHandler(t *testing.T) {
},
{
name: "error saving PV finalizer -> controller retries",
initialObjects: []runtime.Object{pv(), node()},
initialObjects: []runtime.Object{pv(), node(), csiNode()},
updatedVA: va(false, "", nil),
reactors: []reaction{
{
Expand Down Expand Up @@ -604,7 +595,7 @@ func TestCSIHandler(t *testing.T) {
},
{
name: "VolumeAttachment added -> successful attachment incl. metadata",
initialObjects: []runtime.Object{pvWithFinalizer(), node()},
initialObjects: []runtime.Object{pvWithFinalizer(), node(), csiNode()},
addedVA: va(false, "", nil),
expectedActions: []core.Action{
// Finalizer is saved first
Expand Down Expand Up @@ -695,12 +686,12 @@ func TestCSIHandler(t *testing.T) {
expectedActions: []core.Action{
core.NewPatchAction(vaGroupResourceVersion, metav1.NamespaceNone, testPVName+"-"+testNodeName,
types.MergePatchType, patch(va(false, fin, ann), vaWithAttachError(va(false, fin, ann),
"node \"node1\" not found"))),
"csinode.storage.k8s.io \"node1\" not found"))),
},
},
{
name: "failed write with VA finializers -> controller retries",
initialObjects: []runtime.Object{pvWithFinalizer(), node()},
initialObjects: []runtime.Object{pvWithFinalizer(), node(), csiNode()},
addedVA: va(false, "", nil),
reactors: []reaction{
{
Expand Down Expand Up @@ -744,7 +735,7 @@ func TestCSIHandler(t *testing.T) {
},
{
name: "failed write with attached=true -> controller retries",
initialObjects: []runtime.Object{pvWithFinalizer(), node()},
initialObjects: []runtime.Object{pvWithFinalizer(), node(), csiNode()},
addedVA: va(false, "", nil),
reactors: []reaction{
{
Expand Down Expand Up @@ -788,7 +779,7 @@ func TestCSIHandler(t *testing.T) {
},
{
name: "CSI attach fails -> controller retries",
initialObjects: []runtime.Object{pvWithFinalizer(), node()},
initialObjects: []runtime.Object{pvWithFinalizer(), node(), csiNode()},
addedVA: va(false, "", nil),
expectedActions: []core.Action{
// Finalizer is saved first
Expand All @@ -815,7 +806,7 @@ func TestCSIHandler(t *testing.T) {
},
{
name: "CSI attach times out -> controller retries",
initialObjects: []runtime.Object{pvWithFinalizer(), node()},
initialObjects: []runtime.Object{pvWithFinalizer(), node(), csiNode()},
addedVA: va(false, "", nil),
expectedActions: []core.Action{
// Finalizer is saved first
Expand All @@ -832,28 +823,28 @@ func TestCSIHandler(t *testing.T) {
},
},
{
name: "Node without annotations -> error",
initialObjects: []runtime.Object{pvWithFinalizer(), nodeWithoutAnnotations()},
name: "Node without CSINode -> error",
initialObjects: []runtime.Object{pvWithFinalizer(), node()},
addedVA: va(false, fin, ann),
expectedActions: []core.Action{
core.NewPatchAction(vaGroupResourceVersion, metav1.NamespaceNone, testPVName+"-"+testNodeName,
types.MergePatchType, patch(va(false /*attached*/, fin, ann),
vaWithAttachError(va(false, fin, ann), "node \"node1\" has no NodeID annotation"))),
vaWithAttachError(va(false, fin, ann), "csinode.storage.k8s.io \"node1\" not found"))),
},
},
{
name: "CSINode exists without the driver, Node without annotations -> error",
initialObjects: []runtime.Object{pvWithFinalizer(), nodeWithoutAnnotations(), csiNodeEmpty()},
name: "CSINode exists without the driver -> error",
initialObjects: []runtime.Object{pvWithFinalizer(), node(), csiNodeEmpty()},
addedVA: va(false, fin, ann),
expectedActions: []core.Action{
core.NewPatchAction(vaGroupResourceVersion, metav1.NamespaceNone, testPVName+"-"+testNodeName,
types.MergePatchType, patch(va(false /*attached*/, fin, ann),
vaWithAttachError(va(false, fin, ann), "node \"node1\" has no NodeID annotation"))),
vaWithAttachError(va(false, fin, ann), "CSINode node1 does not contain driver csi/test"))),
},
},
{
name: "CSINode exists with the driver, Node without annotations -> success",
initialObjects: []runtime.Object{pvWithFinalizer(), nodeWithoutAnnotations(), csiNode()},
initialObjects: []runtime.Object{pvWithFinalizer(), node(), csiNode()},
addedVA: va(false /*attached*/, "" /*finalizer*/, nil),
expectedActions: []core.Action{
// Finalizer is saved first
Expand All @@ -870,7 +861,7 @@ func TestCSIHandler(t *testing.T) {
},
{
name: "VolumeAttachment with GCEPersistentDiskVolumeSource -> successful attachment",
initialObjects: []runtime.Object{gcePDPVWithFinalizer(), node()},
initialObjects: []runtime.Object{gcePDPVWithFinalizer(), node(), csiNode()},
addedVA: va(false /*attached*/, "" /*finalizer*/, nil),
expectedActions: []core.Action{
// Finalizer is saved first
Expand Down Expand Up @@ -1095,7 +1086,7 @@ func TestCSIHandler(t *testing.T) {
expectedActions: []core.Action{
core.NewPatchAction(vaGroupResourceVersion, metav1.NamespaceNone, testPVName+"-"+testNodeName,
types.MergePatchType, patch(deleted(va(true, fin, nil)),
deleted(vaWithDetachError(va(true, fin, nil), "node \"node1\" not found")))),
deleted(vaWithDetachError(va(true, fin, nil), "csinode.storage.k8s.io \"node1\" not found")))),
},
},
{
Expand All @@ -1112,19 +1103,6 @@ func TestCSIHandler(t *testing.T) {
{"detach", testVolumeHandle, "annotatedNodeID", noAttrs, noSecrets, readWrite, success, detached, noMetadata, 0},
},
},
{
name: "VolumeAttachment marked for deletion -> node is preferred over VA annotation for NodeID",
initialObjects: []runtime.Object{pvWithFinalizer(), node()},
addedVA: deleted(va(true, fin, map[string]string{vaNodeIDAnnotation: "annotatedNodeID"})),
expectedActions: []core.Action{
core.NewPatchAction(vaGroupResourceVersion, metav1.NamespaceNone, testPVName+"-"+testNodeName,
types.MergePatchType, patch(deleted(va(true, fin, map[string]string{vaNodeIDAnnotation: "annotatedNodeID"})),
deleted(va(false /*attached*/, "", map[string]string{vaNodeIDAnnotation: "annotatedNodeID"})))),
},
expectedCSICalls: []csiCall{
{"detach", testVolumeHandle, testNodeID, noAttrs, noSecrets, readWrite, success, detached, noMetadata, 0},
},
},
{
name: "failed write with attached=false -> controller retries",
initialObjects: []runtime.Object{pvWithFinalizer(), node()},
Expand Down Expand Up @@ -1354,7 +1332,7 @@ func TestCSIHandlerReadOnly(t *testing.T) {
//
{
name: "read-write PV -> attached as read-write",
initialObjects: []runtime.Object{pvWithFinalizer(), node()},
initialObjects: []runtime.Object{pvWithFinalizer(), node(), csiNode()},
addedVA: va(false /*attached*/, "" /*finalizer*/, nil /* annotations */),
expectedActions: []core.Action{
// Finalizer is saved first
Expand All @@ -1371,7 +1349,7 @@ func TestCSIHandlerReadOnly(t *testing.T) {
},
{
name: "read-only PV -> attached as read-write",
initialObjects: []runtime.Object{pvReadOnly(pvWithFinalizer()), node()},
initialObjects: []runtime.Object{pvReadOnly(pvWithFinalizer()), node(), csiNode()},
addedVA: va(false /*attached*/, "" /*finalizer*/, nil /* annotations */),
expectedActions: []core.Action{
// Finalizer is saved first
Expand Down
24 changes: 2 additions & 22 deletions pkg/controller/util.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@ import (
"regexp"

"github.com/container-storage-interface/spec/lib/go/csi"
"github.com/evanphx/json-patch"
"k8s.io/api/core/v1"
jsonpatch "github.com/evanphx/json-patch"
v1 "k8s.io/api/core/v1"
storage "k8s.io/api/storage/v1beta1"
"k8s.io/apimachinery/pkg/types"
"k8s.io/client-go/kubernetes"
Expand Down Expand Up @@ -93,7 +93,6 @@ func markAsDetached(client kubernetes.Interface, va *storage.VolumeAttachment) (

const (
defaultFSType = "ext4"
nodeIDAnnotation = "csi.volume.kubernetes.io/nodeid"
csiVolAttribsAnnotationKey = "csi.volume.kubernetes.io/volume-attributes"
vaNodeIDAnnotation = "csi.alpha.kubernetes.io/node-id"
)
Expand All @@ -114,25 +113,6 @@ func GetFinalizerName(driver string) string {
return "external-attacher/" + SanitizeDriverName(driver)
}

// GetNodeIDFromNode returns nodeID string from node annotations.
func GetNodeIDFromNode(driver string, node *v1.Node) (string, error) {
nodeIDJSON, ok := node.Annotations[nodeIDAnnotation]
if !ok {
return "", fmt.Errorf("node %q has no NodeID annotation", node.Name)
}

var nodeIDs map[string]string
if err := json.Unmarshal([]byte(nodeIDJSON), &nodeIDs); err != nil {
return "", fmt.Errorf("cannot parse NodeID annotation on node %q: %s", node.Name, err)
}
nodeID, ok := nodeIDs[driver]
if !ok {
return "", fmt.Errorf("cannot find NodeID for driver %q for node %q", driver, node.Name)
}

return nodeID, nil
}

// GetNodeIDFromCSINode returns nodeID from CSIDriverInfoSpec
func GetNodeIDFromCSINode(driver string, csiNode *storage.CSINode) (string, bool) {
for _, d := range csiNode.Spec.Drivers {
Expand Down
Loading

0 comments on commit 1c61d6a

Please sign in to comment.