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

Implement ListVolumes #144

Merged
merged 9 commits into from
Dec 14, 2023
Merged

Implement ListVolumes #144

merged 9 commits into from
Dec 14, 2023

Conversation

avestuk
Copy link
Contributor

@avestuk avestuk commented Dec 7, 2023

General:

  • Have you removed all sensitive information, including but not limited to access keys and passwords?
  • Have you checked to ensure there aren't other open or closed Pull Requests for the same bug/feature/question?

Pull Request Guidelines:

  1. Does your submission pass tests?
  2. Have you added tests?
  3. Are you addressing a single feature in this PR?
  4. Are your commits atomic, addressing one change per commit?
  5. Are you following the conventions of the language?
  6. Have you saved your large formatting changes for a different PR, so we can focus on your work?
  7. Have you explained your rationale for why this feature is needed?
  8. Have you linked your PR to an open issue

Advertises the capability for Controller ListVolumes which resolves #59 . Allows us to run the external-health-monitor and get volume health stats for our PVCs.

The PR also consolidates the places where our driver capabilities are set as I spent far too much time wondering why the change in ae2665ea54 wasn't being advertised. Furthermore, this meant that we weren't actually advertising the ability to mount

Testing

Testing this feature requires running the external-health-monitor. I chose to run it as part of the linode controller but we can run it as a separate deployment using leader election.

  1. Update the deploy/kubernetes/base/ss-csi-linode-controller.yaml with the following extra container and set the csi-driver to my image. The deploy/kubernetes/base/ds-csi-linode-node.yaml should also have it's plugin image update.
- name: external-health-monitor
    image: registry.k8s.io/sig-storage/csi-external-health-monitor-controller:v0.10.0
    args:
      - "-v=5"
      - "-csi-address=$(ADDRESS)"
      - "-enable-node-watcher=true"
      - "-http-endpoint=:8080"
      - "-metrics-path=/metrics"
    env:
      - name: ADDRESS
        value: /var/lib/csi/sockets/pluginproxy/csi.sock
    volumeMounts:
    - name: socket-dir
      mountPath: /var/lib/csi/sockets/pluginproxy/
    ports:
      - containerPort: 8080
        name: metrics
  - name: linode-csi-plugin
    image: ghcr.io/avestuk/linode-blockstorage-csi-driver:v0.6.0-20-gc06d64a-dirty
  1. Create a PVC and consume it from a Pod - check that the external health monitor logs reflect volume creation
  2. Update the kubelet config to enable the CSIVolumeHealth feature gate
  3. Check the node metrics data and see that volumeHealthStatus is now being reported.
    kubectl get --raw "/api/v1/nodes/lke145862-214328-13bc36d60000/proxy/stats/summary"
    {
     "time": "2023-12-11T14:25:42Z",
     "availableBytes": 10445123584,
     "capacityBytes": 10464022528,
     "usedBytes": 2121728,
     "inodesFree": 655348,
     "inodes": 655360,
     "inodesUsed": 12,
     "name": "csi-example-volume",
     "pvcRef": {
      "name": "csi-example-pvc",
      "namespace": "kube-system"
     },
     "volumeHealthStats": {
      "abnormal": false
     }
    }

@avestuk avestuk force-pushed the controller-get-volume branch 2 times, most recently from c06d64a to 590151e Compare December 11, 2023 13:34
@avestuk avestuk marked this pull request as ready for review December 11, 2023 15:06
@srust
Copy link
Contributor

srust commented Dec 12, 2023

Deal 😀

@avestuk avestuk force-pushed the controller-get-volume branch from 4a625f8 to a4c9e06 Compare December 14, 2023 14:42
@avestuk avestuk requested a review from luthermonson December 14, 2023 14:47
@luthermonson luthermonson self-requested a review December 14, 2023 14:53
@avestuk avestuk force-pushed the controller-get-volume branch from 2e57dcd to e321939 Compare December 14, 2023 14:55
Alex Vest added 9 commits December 14, 2023 14:59
If we get an EIO error from statsfs then its safe to assume the PV is not happy
The previous approach defined driver capabilities in two locations and makes the behaviour confusing
We dont want to print the pointer address
The tests validate most of the returned fields of the Volume part of the
response but not the VolumeContext and ContentSource fields.

All of the fields of the VolumeStatus are validated.

The fake LinodeClient has been added in order to easily mock Linode API
responses.
@avestuk avestuk force-pushed the controller-get-volume branch from e321939 to e367804 Compare December 14, 2023 14:59
@avestuk avestuk merged commit 714b8df into linode:main Dec 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement ControllerGetVolume
3 participants