CSI: allow updates to volumes on re-registration

[tgross]

Fixes #11786 (as described in #11786 (comment))

The CSI CreateVolume RPC is idempotent given that the topology,
capabilities, and parameters are unchanged. CSI volumes have many
user-defined fields that are immutable once set, and many fields that
are not user-settable.

Update the Register RPC so that updating a volume via the API merges
onto any existing volume without touching Nomad-controlled fields,
while validating it with the same strict requirements expected for
idempotent CreateVolume RPCs.

---

Note to reviewers: I've broken out two small refactoring commits, so this is likely best reviewed commit-by-commit.

[lgfa29]

LGMT.

I added a couple of comments, but I don't know if they are actually problems.

[lgfa29]

Does passing a Copy to CSIVolumeDenormalize means that old is not denormalized at this point?

[tgross]

Yes! We denormalize-on-read rather than having to push volume updates on every allocation status update. The volume only gets written with those denormalized updates on claim transitions (which happens in either csi_hook.go on the client or in volumewatcher on the leader).

[lgfa29]

If topologies are immutable, do we need to check in the other direction as well? Meaning, can an update request reduce the scope of the topologies?

I wrote a test case like this and it fails because it doesn't return an error"

		{
			name: "invalid topology update - missing",
			v: &CSIVolume{
				Topologies: []*CSITopology{
					{Segments: map[string]string{"rack": "R1"}},
					{Segments: map[string]string{"rack": "R3"}},
				},
			},
			update: &CSIVolume{
				RequestedTopologies: &CSITopologyRequest{
					Required: []*CSITopology{
						{Segments: map[string]string{"rack": "R1"}},
					},
				},
			},
			expected: "volume topology request update was not compatible with existing topology",
			expectFn: func(t *testing.T, v *CSIVolume) {
				require.Len(t, v.Topologies, 2)
			},
		},

[tgross]

I think we're ok, but I should definitely add a test covering this case that asserts the expected behavior so that it's not only "accidentally ok".

The TopologyRequirement message in the CreateVolume spec has examples, but the high-level description is this:

If requisite is specified, the provisioned volume MUST be accessible from at least one of the requisite topologies.

The easiest example is to assume we're creating an AWS EBS volume. These are accessible within single availability zone. So suppose I provide the following topology request:

topology_request {
  required {
    topology { segments { "topology.ebs.csi.aws.com/zone" = "us-east-1a" } }
    topology { segments { "topology.ebs.csi.aws.com/zone" = "us-east-1b" } }
  }
}

The plugin can create the volume in either us-east-1a or us-east-1b, and the resulting volume object topology will have topology like this:

&CSIVolume{
	Topologies: []*CSITopology{
		{Segments: map[string]string{"topology.ebs.csi.aws.com/zone": "us-east-1a"},
		},
	},
}

So what we're trying to do here is to allow user to update their request to be:

topology_request {
  required {
    topology { segments { "topology.ebs.csi.aws.com/zone" = "us-east-1a" } }
  }
}

Because that request could have been sent and resulted in the same topology.

All that being said, having described all that I'm wondering whether it really makes sense to let users change the topology_request at all if it can't result in a changed topology. This feels like something we'll write in the docs and then folks will open issues about anyways because it's confusing behavior.

[tgross]

Hm, that behavior is even more confusing because you can't reduce your request to:

topology_request {}

I think I'm going to change this so that once Topologies is set by the plugin, you can't change the TopologyRequest at all. That's going to have less weird edge cases.

[tgross]

@lgfa29 I've done that in e6146f6

[shoenig]

LGTM

[lgfa29]

Nice, thanks for the explanation.

[github-actions]

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.