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

Adapt csi driver #1

Merged
merged 12 commits into from Jan 5, 2024
Merged

Adapt csi driver #1

merged 12 commits into from Jan 5, 2024

Conversation

Panaetius
Copy link
Member

removes the dynamic volume provisioning (the driver was creating subfolders in buckets dynamically to use as volumes, which we don't want)
updates everything to k8s 1.24
updates csi-test sanity tests to newest version and makes them pass
various nix fixes to get stuff actually running

adds support to read config from a secret based on pvcs, not from pvs.

Copy link
Member

@olevski olevski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this on GCP. The installation of the helm chart failed but I made a few edits. Notably:

  • we should not template a namespace (helm will do this for you if you need it)
  • we need a dockerfile (I created one, I will pass it on via slack)
  • the args in the values file should be changed to exclude /bin/csi-rclone-plugin because the Docker image should have this as the entrypoint in the Dockerfile

Once I installed things, I applied the pvc-only example. Here I also removed the namespace and if we keep this example we should do the same in the code. People should be able to apply the manifests in whatever namespace they want. And we should update the storage class in the examples to match whatever storage class we have set in the values.

After the installation the mounting failed with this:

failed to provision volume with StorageClass "rclone-csi-csi-rclone": error getting secret rclone-provisioner-secret in namespace csi-rclone: secrets "rclone-provisioner-secret" not found

I am not sure why this secret would be needed if I am mounting a public bucket.

Lastly because I have lots of comments and some of them are questions just let me know if you want to just talk about this over slack tomorrow at 14:00 your time Ralf. Or anytime after that.

These are the secrets I have in my test helm deployment:

default      csi-rclone-example-2               Opaque               3      19h
default      rclone-provisioner-secret          Opaque               6      21h
default      rclone-secret                      Opaque               7      21h

@@ -17,6 +17,7 @@ csiControllerRclone:
args:
- --csi-address=$(ADDRESS)
- --capacity-ownerref-level=0
- "--extra-create-metadata" # do not remove this, it is required for correct functioning
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this should not be removed it should be hardcoded in the templates and not be added in the values file. Then the stuff in the values file should be merged with the array in the template.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be done in another PR.

devenv/nix/goModule.nix Show resolved Hide resolved
examples/pvc-only/pvc-only.yaml Outdated Show resolved Hide resolved
pkg/rclone/controllerserver.go Show resolved Hide resolved
pkg/rclone/controllerserver.go Show resolved Hide resolved
pkg/rclone/controllerserver.go Show resolved Hide resolved
@@ -60,7 +61,7 @@ csiNodepluginRclone:
imagePullPolicy: IfNotPresent
rclone:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reminder we need a toleration field in the values file so that the daemonset with the node plugin can run on nodes that are tainted. Without this we cannot mount stuff in sessions because the user sessions on renku are running on dedicated/tainted nodes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another thing on the helm chart do not template a namespace.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO in a followup PR.

@@ -6,9 +6,10 @@ metadata:
{{- include "chart.labels" . | nindent 4 }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name of the storage class should be hardcoded or even better set in a values file with a default value. It should not be templated from the helm chart name.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO in a followup PR.

@olevski
Copy link
Member

olevski commented Jan 4, 2024

Here is the Dockerfile I used to test:

FROM golang:alpine3.19 as builder
WORKDIR /app
COPY go.mod go.sum ./
RUN go mod download 
COPY cmd cmd
COPY pkg pkg
RUN go build -o csi_rclone cmd/csi-rclone-plugin/main.go 

FROM alpine:3.19
COPY --from=builder /app/csi_rclone /
ADD https://downloads.rclone.org/rclone-current-linux-amd64.zip ./
RUN unzip rclone-current-linux-amd64.zip && \
    cd rclone-*-linux-amd64 && \
    cp rclone /usr/bin/ && \
    chown root:root /usr/bin/rclone && \
    chmod 755 /usr/bin/rclone && \
    rm -rf /rclone-*-linux-amd64
ENTRYPOINT ["/csi_rclone"]

@olevski olevski self-requested a review January 5, 2024 15:43
Copy link
Member

@olevski olevski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets do followup reviews for the outstanding things around the helm charts.

@Panaetius Panaetius merged commit c786514 into master Jan 5, 2024
@Panaetius Panaetius deleted the adapt-csi-driver branch January 5, 2024 15:46
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.

None yet

2 participants