Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug v2.0.0] Volume Topology Requirements do not conform to CSI spec #333

Closed
6 tasks done
apricote opened this issue Nov 24, 2022 · 6 comments
Closed
6 tasks done
Assignees
Labels
bug Something isn't working

Comments

@apricote
Copy link
Member

apricote commented Nov 24, 2022

Summary

Topology labels added to new volumes in v2.0.0 does not conform to the CSI spec.

We plan to fix this ASAP, but the change will require manual fixing of affected PersistentVolumes.

See section recommended actions to learn how to proceed.

This issue will be updated as the situation progresses. I recommend subscribing to the issue to receive notifications.

Recommended Actions

  • If you have not installed hcloud-csi-driver v2.0.0 or you are a new user
    • Upgrade to v2.1.0+ at a time convenient to you.
  • If you have installed hcloud-csi-driver v2.0.0

Background

The CSI Spec supports topology requirements for volumes, this happens in two steps:

  • On registration, every node returns its topology in the NodeGetInfo call
  • On volume creation, the Container Orchestrator (k8s/nomad) specifies a list of required topologies for the new volume (CreateVolume call), the driver then chooses which of the supplied topologies the volume will serve and returns those in the response.
  • The CO then tries to schedule the workload to a node that can use the volume (based on the volumes accessible_topology and the node accessible_topology)

For version 2.0.0 of the CSI driver we made a change to the topologies, to fix an issue with cluster-autoscaler and to use a standardized label, see #302 for details of this change:

 node:
   csi.hetzner.cloud/location: hel1
+  topology.kubernetes.io/region: hel1

 volume:
-  csi.hetzner.cloud/location: hel1
+  topology.kubernetes.io/region: hel1

The change was validated with Kubernetes and everything worked as we expected it. It turns out that this does not strictly adher to the CSI spec, because the created volume has a accessible_topology that does not match the nodes accessible_topology. The csi.hetzner.cloud/location label is missing.

The accessible_topology segments are combined using AND in the spec, so theoretically this should not be schedulable in Kubernetes. In practice the Kubernetes scheduler allows this, and the volume can still be attached. Because of this we did not find the bug prior to release.

The Nomad scheduler on the other hand is strict, and only allows scheduling to happen if all topology constraints from the volume match the ones of the node.

While we could implement a dirty hack to make this work in Nomad, we want to do the right thing and conform to the CSI spec. For this, we will revert the changes from #302 and go back to only using the topology constraint csi.hetzner.cloud/location.

All PersistentVolumes created with v2.0.0 of the csi-driver reference the topology.kubernetes.io/region label, which will not be reported by the CSI driver in v2.1.0+. Because of the way Kubernetes handles scheduling, the volumes will still be useable in v2.1.0+ of the driver. We will provide a guide to fix the topology labels for these volumes ASAP.

Plan

This plan tracks the progress of individual steps to deal with the issue. The details of this plan might change.

@apricote apricote added the bug Something isn't working label Nov 24, 2022
@apricote apricote self-assigned this Nov 24, 2022
@apricote apricote pinned this issue Nov 24, 2022
@apricote apricote changed the title [Bug] Volume Topology Requirements do not conform to CSI spec in v2.0.0 [Bug v2.0.0] Volume Topology Requirements do not conform to CSI spec Nov 24, 2022
apricote added a commit that referenced this issue Nov 24, 2022
apricote added a commit that referenced this issue Nov 25, 2022
Revert the topology segment label used for new volumes to
`csi.hetzner.cloud/location`. Adding the new label was a mistake and leads
to incompatibility with the CSI spec (and Nomad).

We plan to fully revert the changes, but that will require user intervention
to fix all volumes created with the new label. By changing the label used
on new volumes, we can limit the amount of volumes that need to be fixed
for users that already upgraded to v2.0.0.

See issue #333 for details.
apricote added a commit that referenced this issue Nov 25, 2022
Revert the topology segment label used for new volumes to
`csi.hetzner.cloud/location`. Adding the new label was a mistake and leads
to incompatibility with the CSI spec (and Nomad).

We plan to fully revert the changes, but that will require user intervention
to fix all volumes created with the new label. By changing the label used
on new volumes, we can limit the amount of volumes that need to be fixed
for users that already upgraded to v2.0.0.

See issue #333 for details.
apricote added a commit that referenced this issue Nov 25, 2022
Revert the topology segment label used for new volumes to
`csi.hetzner.cloud/location`. Adding the new label was a mistake and leads
to incompatibility with the CSI spec (and Nomad).

We plan to fully revert the changes, but that will require user intervention
to fix all volumes created with the new label. By changing the label used
on new volumes, we can limit the amount of volumes that need to be fixed
for users that already upgraded to v2.0.0.

See issue #333 for details.
@apricote
Copy link
Member Author

Version v2.0.1 has been released. Users of v2.0.0 should upgrade to this version as soon as possible, as new volumes created with this version will not need to be fixed to work with v2.1.0 later.

Users of v1.x should not upgrade to v2.0.1, please see the Recommended Actions in the note above.

apricote added a commit that referenced this issue Nov 25, 2022
Revert all changes we made to our reported topology for nodes and
volumes.

Because we report two segments on the `NodeGetInfo` call, but
only one of them on `CreateVolume` we are not compliant with the CSI
spec. This did not matter for Kubernetes, because the scheduler still
worked, but it breaks Nomad.

As we are not compliant with the CSI spec, we decided to revert these
changes, even though it will require user intervention to fix volumes
created in the meantime.

For details see issue #333.
@madalinignisca
Copy link

I'm confused with the recommendations. If 2.0.1 should be the first good and fixed version of 2.x, on a new cluster installing for first time, would 2.0.1 be good to use?

If I want to start today a new cluster, what should block me in installing 2.0.1?

@apricote
Copy link
Member Author

I'm confused with the recommendations. If 2.0.1 should be the first good and fixed version of 2.x, on a new cluster installing for first time, would 2.0.1 be good to use?

If I want to start today a new cluster, what should block me in installing 2.0.1?

Nothing blocks you, but that version still does not follow the CSI spec, so we do not recommend using that version for a new cluster, only if you are using v2.0.0 at the moment.

Version v2.1.0 will be released in the next 2 hours, so you might want to wait for this instead of installing v1.6.0 and later migrating to v2.x.

@madalinignisca
Copy link

2h is great. Many thanks. I thought it might take another few days or more. For me this are great news, I intended to have it ready for next week.

apricote added a commit that referenced this issue Nov 25, 2022
Revert all changes we made to our reported topology for nodes and
volumes.

Because we report two segments on the `NodeGetInfo` call, but
only one of them on `CreateVolume` we are not compliant with the CSI
spec. This did not matter for Kubernetes, because the scheduler still
worked, but it breaks Nomad.

As we are not compliant with the CSI spec, we decided to revert these
changes, even though it will require user intervention to fix volumes
created in the meantime.

For details see issue #333.
apricote added a commit that referenced this issue Nov 25, 2022
Also removed section about cluster-autoscaler fix, as we reverted that in v2.1.0, see #333 for details.
@apricote
Copy link
Member Author

Version v2.1.0 has been released.

New users and users of v1.x should upgrade/use this version.

Users of v2.0.x may upgrade to this version. Volumes created with v2.0.0 still reference invalid topology labels and we are working on a migration guide for these volumes.

@apricote
Copy link
Member Author

apricote commented Dec 8, 2022

The guideline to fix PersistentVolumes created with the v2.0.0 release is now published: https://github.com/hetznercloud/csi-driver/blob/main/docs/v2.0.0-fix-volume-topology/fix-volume-topology.md

If you have any further questions, feel free to comment here or contact the Hetzner Cloud support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants