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

support volume_mount in task #51

Merged
merged 2 commits into from
Jan 7, 2021

Conversation

lisongmin
Copy link
Contributor

support volume_mount which defined in https://www.nomadproject.io/docs/job-specification/volume_mount

inspired from docker dirver.

@shishir-a412ed
Copy link
Collaborator

@lisongmin We already support volume, bind, and tmpfs mounts.
Code reference: buildMountpoint

Can you elaborate on what this PR is adding, which is currently missing in the driver?

@shishir-a412ed shishir-a412ed self-requested a review January 4, 2021 18:32
@github-actions
Copy link

github-actions bot commented Jan 4, 2021

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@lisongmin
Copy link
Contributor Author

@lisongmin We already support volume, bind, and tmpfs mounts.
Code reference: buildMountpoint

Can you elaborate on what this PR is adding, which is currently missing in the driver?

There are two ways to describe mount in nomad:

  1. specified the mounts via driver's config:

    task "mysql-server" {
      driver = "containerd-driver"
    
      env = {
        "MYSQL_ROOT_PASSWORD" = "password"
      }
    
      config {
        image = "hashicorp/mysql-portworx-demo:latest"
    
      mounts = [ {
        type = "bind"
        source      = "/mysql"
        target = "/var/lib/mysql"
        options   = ["ro"]
      }]
    
        port_map {
          db = 3306
        }
      }
    }
    
  2. specified the mounts via task's volume_mount:

    task "mysql-server" {
      driver = "containerd-driver"
    
      volume_mount {
        volume      = "mysql"
        destination = "/var/lib/mysql"
        read_only   = false
      }
    
      env = {
        "MYSQL_ROOT_PASSWORD" = "password"
      }
    
      config {
        image = "hashicorp/mysql-portworx-demo:latest"
    
        port_map {
          db = 3306
        }
      }
    }
    

and the later one is not supported by this plugin now.

@lisongmin
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@shishir-a412ed
Copy link
Collaborator

@lisongmin Why do you need (2) when both (1) and (2) are doing the same thing?

@lisongmin
Copy link
Contributor Author

No, it's not necessary.

But it's better to support it since there are many(some?) tutorials use the volume_mount option. and reduce people to debug the problem why not mounted.

Maybe keep compatible to docker is also a good reason?

@shishir-a412ed
Copy link
Collaborator

@lisongmin okay, Can you add an integration test with a task volume_mount (2) example?

The integration tests use the jobs defined in the example dir

You can add a new job in the example dir and use that to add an integration test.

The PR might need a rebase as well.

Thank you for the contributions 🙂

@lisongmin lisongmin force-pushed the support-volume_mount branch 2 times, most recently from f9bf4b3 to ed1114a Compare January 6, 2021 06:32
@shishir-a412ed
Copy link
Collaborator

@lisongmin Okay I think we have a bigger problem 🙂.

The agent.hcl is used by both the vagrant setup and tests setup

Currently, this will break the vagrant setup since there is no path /tmp/host_volume/s1 on the host. The path is only setup as part of the tests. Vagrant setup is mostly for users who want to quickly bring a vagrant VM up (with all the necessary dependencies installed e.g. containerd, containerd-driver) and manually try out a few jobs.

I think we should separate the example/agent.hcl for the two use-cases.

Can we:

  1. Create a separate example/agent_tests.hcl which is a copy of example/agent.hcl +
client {
   host_volume "s1" {
     path = "/tmp/host_volume/s1"
     read_only = false
   }
 }
  1. Change run_tests.sh to use the new example/agent_tests.hcl

tests/run_tests.sh Outdated Show resolved Hide resolved
example/agent.hcl Outdated Show resolved Hide resolved
tests/005-test-volume_mount.sh Outdated Show resolved Hide resolved
Copy link
Collaborator

@robloxrob robloxrob left a comment

Choose a reason for hiding this comment

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

@lisongmin Awesome work on this PR! Added a few feedback items. Thank you so much for your efforts and contribution.

example/volume_mount.nomad Outdated Show resolved Hide resolved
example/agent.hcl Outdated Show resolved Hide resolved
@lisongmin lisongmin force-pushed the support-volume_mount branch 2 times, most recently from cdfb9c1 to f512019 Compare January 7, 2021 03:19
tests/005-test-volume_mount.sh Outdated Show resolved Hide resolved
tests/005-test-volume_mount.sh Show resolved Hide resolved
Copy link
Collaborator

@shishir-a412ed shishir-a412ed left a comment

Choose a reason for hiding this comment

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

LGTM

@shishir-a412ed shishir-a412ed merged commit c8520c6 into Roblox:master Jan 7, 2021
@lisongmin lisongmin deleted the support-volume_mount branch February 11, 2021 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants