[cinder-csi-plugin] Rationalise all things topology #2865
Conversation
k8s-ci-robot
added
the
release-note-none
k8s-ci-robot
added
cncf-cla: yes
stephenfin
force-pushed
the
cinder-topology-cleanup
branch
from
a5d3216 to
ae701b6
Compare
k8s-ci-robot
added
size/L
stephenfin
force-pushed
the
cinder-topology-cleanup
branch
from
ae701b6 to
1ba7175
Compare
mdbooth
left a comment
mdbooth
left a comment
There was a problem hiding this comment.
Thanks for this series. Thanks also for splitting it into separate commits with excellent commit messages. That made it much easier to review.
I had some nits which you're welcome to ignore, but I think an issue in the fifth commit might be worth fixing.
I wonder if it's worth splitting the last commit, which actually changes the behaviour of --with-topology into a separate PR. Whether it's worth it might depend on the next point.
I also wonder if we need to guard this behaviour change somehow: has --with-topology been included in a release yet? If not I think we're good, but if it has I don't think we can make a breaking change like this without guardrails.
stephenfin
force-pushed
the
cinder-topology-cleanup
branch
from
1ba7175 to
9bc4857
Compare
Good point. I've dropped this. We would want a release note for the change. |
k8s-ci-robot
added
the
lgtm
k8s-ci-robot
added
the
needs-rebase
This is actually handled by csi-provisioner, but it's arguably incorrect. Signed-off-by: Stephen Finucane <stephenfin@redhat.com>
Calling '.On' with the same expected arguments multiple times will only result in a single mock: the first one. We should avoid doing this. We also remove some useless comments that simply duplicate what the calls already say. Signed-off-by: Stephen Finucane <stephenfin@redhat.com>
Signed-off-by: Stephen Finucane <stephenfin@redhat.com>
This highlights an issue (IMO) is how we handle parameter generation. Signed-off-by: Stephen Finucane <stephenfin@redhat.com>
Group everything together rather than having it spread out as it is
currently. A decoder of the logic, to ensure nothing has changed:
1. Determine what the OpenStack Cinder volume's AZ will be:
a. If the `availability` parameter is set on the Storage Class, the
cinder volume will use this as the AZ.
b. Otherwise, the cinder volume will use: an AZ extracted from one of
the preferred CSI Topologies; an AZ extracted from one of the
requisite (available) Topologies; or no AZ.
2. Determine what the Kubernetes CSI Volume's topology will be:
a. If the `ignore-volume-az` option is set, the CSI Volume will use
the preferred CSI Volume Topologies, if any, else None. [*]
b. Otherwise, the CSI Volume will generate a Volume Topology from the
Cinder Volume's AZ.
[*] This practically means that `ignore-volume-az` is only useful when
the availability parameter is set on the Storage Class. This in turn
means the CSI Volume Topology is junk as it has no bearing on the
"real" topology constraint on the volume.
Signed-off-by: Stephen Finucane <stephenfin@redhat.com>
stephenfin
force-pushed
the
cinder-topology-cleanup
branch
from
9bc4857 to
03149e5
Compare
k8s-ci-robot
removed
lgtm
|
/assign |
|
/unassign @mdbooth |
k8s-ci-robot
added
the
lgtm
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kayrus The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
5 participants
What this PR does / why we need it:
This is a patch that came out of my work on #2861. Currently, generation of the topology request for the Cinder Volume is logically separated from the generation of the topology request for the CSI Volume. This makes things harder to understand. Move some things around to resolve this and make everything a little easier to grok. This is kept separate from #2862 since I don't expect we'll want to backport this.
Which issue this PR fixes(if applicable):
n/a
Special notes for reviewers:
I have a decoder of the logic embedded in the commit message. Hopefully this helps with review, but please don't assume the decoder itself is correct! 😅 I have also added additional unit tests that should capture any changes in logic.
Release note: