democratic-csi plugin #113

tgross · 2022-04-12T19:48:12Z

A pack for deploying democratic-csi CSI plugins in their NFS client mode. I've left a few questions too.

Example runs in my Vagrant environment:

plan output

$ nomad-pack plan \
    -var nfs_share_host=192.168.56.69 \
    -var nfs_share_base_path=/var/nfs/general \
    -var nfs_controller_mount_path=/srv/nfs_data \
    ./packs/democratic_csi_nfs
+ Job: "democratic_csi_controller"
+ Task Group: "controller" (2 create)
  + Task: "plugin" (forces create)

» Scheduler dry-run:
- All tasks successfully allocated.
+ Job: "democratic_csi_node"
+ Task Group: "node" (3 create)
  + Task: "plugin" (forces create)

» Scheduler dry-run:
- All tasks successfully allocated.
Plan succeeded

run output

$ nomad-pack run \
    -var nfs_share_host=192.168.56.69 \
    -var nfs_share_base_path=/var/nfs/general \
    -var nfs_controller_mount_path=/srv/nfs_data \
    ./packs/democratic_csi_nfs
  Evaluation ID: d5cfa4fe-802b-89a8-440e-c628b6f9ebae
  Job 'democratic_csi_controller' in pack deployment 'democratic_csi_nfs' registered successfully
  Evaluation ID: eb887e8c-e881-bb10-593a-18af7ec3c5dd
  Job 'democratic_csi_node' in pack deployment 'democratic_csi_nfs' registered successfully
Pack successfully deployed. Use ../nomad-pack-community-registry/packs/democratic_csi_nfs to manage
this this deployed instance with plan, stop, destroy, or info

type         = "csi"
id           = "myvolume"
namespace    = "default"
name         = "myvolume"
plugin_id    = "org.democratic-csi.nfs"

capability {
  access_mode     = "multi-node-multi-writer"
  attachment_mode = "file-system"
}

capability {
  access_mode     = "single-node-writer"
  attachment_mode = "file-system"
}

capability {
  access_mode     = "single-node-reader-only"
  attachment_mode = "file-system"
}

mount_options {
  mount_flags = ["noatime"]
}


$ nomad job status
ID                         Type     Priority  Status   Submit Date
democratic_csi_controller  service  50        running  2022-04-12T16:35:46-04:00
democratic_csi_node        system   50        running  2022-04-12T16:35:46-04:00

$ nomad plugin status org.democratic-csi.nfs
ID                   = org.democratic-csi.nfs
Provider             = org.democratic-csi.nfs
Version              = 1.6.3
Controllers Healthy  = 2
Controllers Expected = 2
Nodes Healthy        = 3
Nodes Expected       = 3

Allocations
ID        Node ID   Task Group  Version  Desired  Status   Created  Modified
eef089f3  0421e35a  controller  0        run      running  43s ago  10s ago
874b3e2d  0421e35a  node        0        run      running  42s ago  10s ago
c6a4e114  63f60fe3  controller  0        run      running  43s ago  11s ago
074e4232  63f60fe3  node        0        run      running  42s ago  11s ago
aae7271e  de82214f  node        0        run      running  42s ago  16s ago

tgross · 2022-04-12T20:06:33Z

packs/democratic_csi_nfs/metadata.hcl

+app {
+  url    = "https://github.com/democratic-csi/democratic-csi"
+  author = "Tim Gross <tgross@hashicorp.com>"
+}


@mikenomitch @angrycub something I noticed here is that we're conflating app.author and a hypothetical pack.maintainer metadata field. Most package managers have a distinction between the two because they're rarely the same person. Ex. redis on Debian Buster is maintained by Chris Lamb, who is not the author of Redis (antirez or Redis Labs).

We also don't include as a requirement any kind of contact info for the pack maintainer. Maybe we don't need it as required metadata for all registries but as a requirement for this registry, so that we have someone to ping as the codeowner. (Also maybe not an email address but a GitHub handle here?)

Yeah, this was originally the author of the underlying app itself (not the pack). But about 40% of PRs get this "wrong" and do pack author. IDK what value its really providing as author of the underlying app.

Git/GitHub will naturally point us to the maintainer (in a roundabout way), so I'm not sure we really even need it for that purpose.

Maybe its something we should just remove?

But about 40% of PRs get this "wrong" and do pack author.

Most likely because that's what Writing Packs recommends. Seems reasonable to me to remove it. I'll fix up the docs as well.

Oh wow! Maybe I'm "wrong" then 🙃

I've opened hashicorp/nomad-pack#240 to update the docs and/or discuss it more generally.

packs/democratic_csi_nfs/variables.hcl

tgross · 2022-04-12T20:09:28Z

packs/democratic_csi_nfs/metadata.hcl

+pack {
+  name        = "democratic_csi_nfs"
+  description = "This pack deploys the democratic-csi plugin, configured for use with NFS"
+  url         = "https://github.com/hashicorp/nomad-pack-community-registry/democratic_csi_nfs"


The docs on this url field are a little vague. It's not really the URL to the directory, right?

It's not really the URL to the directory, right? It is... but @angrycub and I want to remove it and just make this implicit when they grab the registry itself. Because once you fork a registry, all of these are wrong.

I more meant that our docs say to use a URL like https://github.com/hashicorp/nomad-pack-community-registry/nginx but the actual URL to the pack directory is https://github.com/hashicorp/nomad-pack-community-registry/tree/main/packs/nginx

I've opened hashicorp/nomad-pack#240 to update the docs and/or discuss it more generally.

tgross · 2022-04-12T20:40:20Z

packs/democratic_csi_nfs/README.md

+need to have a source NFS volume mounted to any client host that runs
+the controller task.
+
+### Example NFS Server


The democratic-csi plugin supports a lot of different NFS and ZFS setups. I'm not sure if we want to try to support arbitrary configurations in this one pack. It'd be awfully nice, but the client requirements end up being a chore to document (and not feasible for us to test, where this one is pretty simple). Open to suggestion here though for sure.

If we did want to support a broader range of configs, we'd want to have something more like a blob or object for the config file (ref #113 (comment))

I think there are ways we could handle both using the sprig fail function in the template.

Because it's very hard to test without buying an appropriate NAS appliance, I'm going to leave this as-is for now and if someone comes along later and wants to improve upon it they can.

My point in the earlier comment is that you could provide variables to ingest a shapely configuration for something that is easily testable, but allow for a blob in cases where we don't have the ability to know the configuration in advance. Then it would be up to the template to fail if neither way was provided by the user at runtime. If they're bringing a custom config, then they will have to ensure that they are doing the right thing.

tgross · 2022-04-12T20:42:29Z

(I've tagged @davemay99 on this review because I'd like to use packs like this to help document CSI configurations and want to know his thoughts on what's in the README here. I'll have more packs coming for AWS EBS, GCP, etc.)

tgross · 2022-04-12T20:47:44Z

packs/democratic_csi_nfs/README.md

+
+## Variables
+
+The following variables are required:


Is there a way to express this requirement in our variables.hcl? There aren't really reasonable defaults for these values here.

No way to do this :(

If you don't default them, they should end up being required? If they aren't this seems like a bug. (It's inconsistent with my experience with Nomad's HCL2, that is)

No, unfortunately. For example if I render this pack with no variables set I end up with a partially empty template:

template { destination = "${NOMAD_TASK_DIR}/driver-config-file.yaml" data = <<EOH driver: nfs-client instance_id: nfs: shareHost: shareBasePath: "" controllerBasePath: "/storage" dirPermissionsMode: "0777" dirPermissionsUser: root dirPermissionsGroup: root EOH }

For Nomad fields, this usually up throwing an error when we try to submit the job because Nomad will reject it. But for something like the interior of a template, Nomad has no way of knowing whether the field is valid.

angrycub

One suggestion on template. Haven't run locally though yet

packs/democratic_csi_nfs/templates/_constraints.tpl

packs/democratic_csi_nfs/variables.hcl

packs/democratic_csi_nfs/templates/_constraints.tpl

Brandl · 2022-09-26T19:35:26Z

This looks great, why hasn't it been merged so far?

tgross · 2022-09-27T14:09:59Z

Hi @Brandl! While working on and reviewing this pack we hit a couple of "is this really the way we want to do this in Pack?" questions and we just haven't had time to come back.

tgross force-pushed the democratic_csi_nfs branch from 25310c3 to 85035ba Compare April 12, 2022 19:59

tgross commented Apr 12, 2022

View reviewed changes

packs/democratic_csi_nfs/variables.hcl Show resolved Hide resolved

tgross commented Apr 12, 2022

View reviewed changes

tgross force-pushed the democratic_csi_nfs branch 2 times, most recently from 780c33a to 656cbb0 Compare April 12, 2022 20:38

tgross commented Apr 12, 2022

View reviewed changes

tgross marked this pull request as ready for review April 12, 2022 20:41

tgross requested review from angrycub, mikenomitch and davemay99 April 12, 2022 20:41

tgross mentioned this pull request Apr 12, 2022

docs: nested template requires . to pass variables hashicorp/nomad-pack#238

Merged

tgross commented Apr 12, 2022

View reviewed changes

tgross requested a review from DerekStrickland April 12, 2022 20:48

tgross added the pack label Apr 12, 2022

democratic-csi plugin

f5508a5

tgross force-pushed the democratic_csi_nfs branch from 656cbb0 to f5508a5 Compare April 13, 2022 13:45

beauty works better: fix up rendered indentation

5d6f4ec

This was referenced Apr 13, 2022

AWS EBS CSI plugin #114

Merged

Pack: Common CSI Plugins #19

Open

angrycub reviewed Apr 14, 2022

View reviewed changes

packs/democratic_csi_nfs/templates/_constraints.tpl Outdated Show resolved Hide resolved

packs/democratic_csi_nfs/variables.hcl Show resolved Hide resolved

packs/democratic_csi_nfs/templates/_constraints.tpl Outdated Show resolved Hide resolved

tgross mentioned this pull request Apr 14, 2022

docs: remove author and url fields from writing packs hashicorp/nomad-pack#240

Closed

remove if block around range over constraints

2c77235

tgross force-pushed the democratic_csi_nfs branch from 389facc to 2c77235 Compare April 14, 2022 19:23

tgross mentioned this pull request May 11, 2022

Improve CSI Plugin compatibility docs hashicorp/nomad#11250

Open

Add validation; small tweaks

0c4a9db

angrycub merged commit 79e82e7 into main Mar 21, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

democratic-csi plugin #113

democratic-csi plugin #113

tgross commented Apr 12, 2022 •

edited

Loading

tgross Apr 12, 2022 •

edited

Loading

mikenomitch Apr 14, 2022

tgross Apr 14, 2022 •

edited

Loading

mikenomitch Apr 14, 2022

tgross Apr 14, 2022

tgross Apr 12, 2022

mikenomitch Apr 14, 2022

tgross Apr 14, 2022

tgross Apr 14, 2022

tgross Apr 12, 2022

tgross Apr 13, 2022

angrycub Apr 14, 2022

tgross Apr 14, 2022

angrycub Apr 14, 2022 •

edited

Loading

tgross commented Apr 12, 2022

tgross Apr 12, 2022

mikenomitch Apr 14, 2022 •

edited

Loading

angrycub Apr 14, 2022

tgross Apr 14, 2022

angrycub left a comment

Brandl commented Sep 26, 2022

tgross commented Sep 27, 2022

democratic-csi plugin #113

democratic-csi plugin #113

Conversation

tgross commented Apr 12, 2022 • edited Loading

tgross Apr 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgross Apr 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angrycub Apr 14, 2022 • edited Loading

Choose a reason for hiding this comment

tgross commented Apr 12, 2022

Choose a reason for hiding this comment

mikenomitch Apr 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angrycub left a comment

Choose a reason for hiding this comment

Brandl commented Sep 26, 2022

tgross commented Sep 27, 2022

tgross commented Apr 12, 2022 •

edited

Loading

tgross Apr 12, 2022 •

edited

Loading

tgross Apr 14, 2022 •

edited

Loading

angrycub Apr 14, 2022 •

edited

Loading

mikenomitch Apr 14, 2022 •

edited

Loading