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

feat: append nomad task mounts #152

Closed
wants to merge 1 commit into from

Conversation

ttys3
Copy link
Contributor

@ttys3 ttys3 commented Jan 23, 2022

something like this: https://github.com/hashicorp/waypoint/blob/566c65afb74fcfe64f3f0e50ef7eddbd03d9a3c8/internal/serverinstall/nomad.go#L914

will always fail when using the podman driver, because the driver does not handle this mounts now.

this PR fixup it

@hashicorp-cla
Copy link

hashicorp-cla commented Jan 23, 2022

CLA assistant check
All committers have signed the CLA.

@ttys3
Copy link
Contributor Author

ttys3 commented Feb 6, 2022

@lgfa29 can you review this ?

Copy link
Contributor

@lgfa29 lgfa29 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @ttys3, and sorry for the delay and getting it reviewed.

I did a quick test using host volumes and it all seems to work 🙂

Would you mind adding some tests and a CHANGELOG entry?

@ttys3 ttys3 force-pushed the append-nomad-task-mounts branch 2 times, most recently from 54a7beb to 2036fc5 Compare February 18, 2022 16:20
@ttys3
Copy link
Contributor Author

ttys3 commented Feb 18, 2022

@lgfa29 updated. PTAL

tested, this does support https://www.nomadproject.io/docs/job-specification/volume_mount

resolve #146

nomad client host volume config:

client {
  enabled = true

  host_volume "waypoint-server" {
    path = "/var/lib/waypoint"
    read_only = false
  }
}

the job file: volume-mount-demo.nomad

job "docs" {
  datacenters = ["dc1"]

  group "example" {
    volume "my-vol" {
      type      = "host"
      read_only = true
      source    = "waypoint-server"
    }

    task "example" {
      driver = "podman"

      config {
        image = "alpine"

        command = "sleep"
        args    = ["3600"]
      }

      volume_mount {
        volume      = "my-vol"
        destination = "/demo/certs"
      }
    }
  }
}

@ttys3 ttys3 requested a review from lgfa29 February 18, 2022 16:31
@lgfa29
Copy link
Contributor

lgfa29 commented Feb 23, 2022

Ah sorry, I mean like automated testing 😅

Something like this one, to verify that the configuration is properly set in the driver:
https://github.com/hashicorp/nomad-driver-podman/blob/v0.3.0/driver_test.go#L1225-L1232

You may need to update the client.hcl example config file to add a host volume:
https://github.com/hashicorp/nomad-driver-podman/blob/main/examples/nomad/client.hcl

@ttys3
Copy link
Contributor Author

ttys3 commented Feb 26, 2022

Ok, I'll figure a way to write some test.

@lgfa29
Copy link
Contributor

lgfa29 commented Mar 1, 2022

Ok, I'll figure a way to write some test.

Thanks! I linked to an example, but let me know if you need more help.

@lgfa29
Copy link
Contributor

lgfa29 commented May 31, 2022

Oh shoot, I forgot about this PR and ended up merging #169, which does the same thing. I'm going to close this one but credit you in the CHANGELOG entry as well.

Sorry for that @ttys3 😞

@lgfa29 lgfa29 closed this May 31, 2022
lgfa29 added a commit that referenced this pull request May 31, 2022
PR #152 was created before #169, but was not merged even though it has
pretty much the same code.
@ttys3
Copy link
Contributor Author

ttys3 commented Jun 1, 2022

@lgfa29 sorry for the huge delayed unit testing.

I checked the code, almost the same.

but what wondering me is that, I remember what block this PR is missing unit testing.

but I did not see any tests code in #169

@lgfa29
Copy link
Contributor

lgfa29 commented Jun 1, 2022

Yeah, I wasn't paying enough attention when reviewing #169 😕

Did you happen to find a way to test this?

lgfa29 added a commit that referenced this pull request Jun 1, 2022
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

3 participants