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

docker: introduce a new hcl2-friendly mount syntax #9635

Merged
merged 3 commits into from
Dec 15, 2020

Conversation

notnoop
Copy link
Contributor

@notnoop notnoop commented Dec 14, 2020

Introduce a new more-block friendly syntax for specifying mounts with a new mount block type with the target as label:

config {
  image = "..."

  mount {
    type = "..."
    target = "target-path"
    volume_options { ... }
  }
}

The main benefit here is that by mount being a block, it can nest blocks and avoids the compatibility problems noted in https://github.com/hashicorp/nomad/pull/9634/files#diff-2161d829655a3a36ba2d916023e4eec125b9bd22873493c1c2e5e3f7ba92c691R128-R155 .

The intention is for us to promote this mount blocks and quietly deprecate the mounts type, while still honoring to preserve compatibility as much as we could.

This addresses the issue in #9604 .

@notnoop notnoop requested a review from tgross December 14, 2020 21:48
Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

I have a few concerns with this approach.

  • Block labels can't currently be interpolated, which means until we implement Can't use variables or locals on block labels #9522 this would be limited relative to mounts.destination for interpolation.
  • I'm worried that having a very similar alternative is going to be more confusing than having folks migrate.
  • Are there other places we're going to see this problem and does providing an alternative in one place but not others make for an inconsistent UX?

@notnoop
Copy link
Contributor Author

notnoop commented Dec 15, 2020

These are reasonable concerns.

I'm worried that having a very similar alternative is going to be more confusing than having folks migrate.

My concern, is that confusion of when a block syntax is required vs an assignment syntax is going to remain confusing. But I agree that it's better not to rush a new syntax that we may regret and be beholden to for backward compatibility concern.

  • Are there other places we're going to see this problem and does providing an alternative in one place but not others make for an inconsistent UX?

I'd note that a generic solution is to exploit our hcl2 parser special casing of task config, how block encodings match array assignments in the underlying representation. The following two block declarations are actually equivalent (inside a task config:

#### USING ASSIGNMENT SYNTAX: no nested blocks are allowed
mounts = [
  # sample volume mount
  {
    type   = "volume"
    target = "/path/in/container"
    volume_options = {
      no_copy = false
      labels = {
        foo = "bar"
      }
      driver_config = {
        name = "pxd"
        options = {
          foo = "bar"
        }
      }
    }
  },
  # sample tmpfs mount
  {
    type     = "tmpfs"
    target   = "/path/in/container"
    readonly = false
    tmpfs_options = {
      size = 100000 # size in bytes
    }
  }
]

vs

#### USING REPEATED BLOCK DECLARATION SYNTAX: nested blocks are allowed, yay!
mounts {
  type   = "volume"
  target = "/path/in/container"
  volume_options {
    no_copy = false
    labels {
      foo = "bar"
    }
    driver_config {
      name = "pxd"
      options {
        foo = "bar"
      }
    }
  }
}

# sample tmpfs mount
mounts {
  type     = "tmpfs"
  target   = "/path/in/container"
  readonly = false
  tmpfs_options {
    size = 100000 # size in bytes
  }
}

The second syntax is more in line with Nomad's job syntax. The downside is mounts, being plural, is an odd block type choice. If we allow mount alias, and don't require labels, we get the benefit of the consistent design without the block label interpolation oddity. In Nomad, generally repeated blocks typically have labels (e.g. task, group) but we do also have repeated non-labeled blocks (e.g. service, check).

What do you think?

@tgross
Copy link
Member

tgross commented Dec 15, 2020

The second syntax is more in line with Nomad's job syntax. The downside is mounts, being plural, is an odd block type choice. If we allow mount alias, and don't require labels, we get the benefit of the consistent design without the block label interpolation oddity. In Nomad, generally repeated blocks typically have labels (e.g. task, group) but we do also have repeated non-labeled blocks (e.g. service, check).

What do you think?

Oh yeah, that repeated block syntax looks a lot nicer. I'm definitely sold on that if we can avoid having the labels too.

Introduce a new more-block friendly for specifying mounts:

```hcl
config {
  image = "..."

  mount {
    type = "..."
    target = "target-path"
    volume_options { ... }
  }
}
```

We introduce this as the current `mounts` require a list of mounts, that
`hcl2` doesn't allow to have a map syntax.
@vercel vercel bot temporarily deployed to Preview – nomad December 15, 2020 16:33 Inactive
@vercel vercel bot temporarily deployed to Preview – nomad-storybook-and-ui December 15, 2020 16:33 Inactive
@notnoop
Copy link
Contributor Author

notnoop commented Dec 15, 2020

Oh yeah, that repeated block syntax looks a lot nicer. I'm definitely sold on that if we can avoid having the labels too.

Great! I've updated the PR to use the repeated non-labeled block syntax. The test TestConfig_ParseAllHCL shows it in effect. I'll update the docs once the docs re-architecture PR makes it in.

Copy link
Member

@schmichael schmichael left a comment

Choose a reason for hiding this comment

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

New form looks much better. A shame it's not perfectly backward compatible, but we knew this was likely to happen. I'm a bit worried about other places we might be missing. devices seems susceptible but does not have any nested structs (yet?).

Docs to update:

[ci skip]
@vercel vercel bot temporarily deployed to Preview – nomad-storybook-and-ui December 15, 2020 19:12 Inactive
@vercel vercel bot temporarily deployed to Preview – nomad December 15, 2020 19:12 Inactive
@notnoop
Copy link
Contributor Author

notnoop commented Dec 15, 2020

Will update the docs after #9638 merges.

@notnoop notnoop merged commit 8879645 into master Dec 15, 2020
@notnoop notnoop deleted the f-hcl2-docker-mounts branch December 15, 2020 19:13
schmichael pushed a commit that referenced this pull request Dec 16, 2020
Introduce a new more-block friendly syntax for specifying mounts with a new `mount` block type with the target as label:

```hcl
config {
  image = "..."

  mount {
    type = "..."
    target = "target-path"
    volume_options { ... }
  }
}
```

The main benefit here is that by `mount` being a block, it can nest blocks and avoids the compatibility problems noted in https://github.com/hashicorp/nomad/pull/9634/files#diff-2161d829655a3a36ba2d916023e4eec125b9bd22873493c1c2e5e3f7ba92c691R128-R155 .

The intention is for us to promote this `mount` blocks and quietly deprecate the `mounts` type, while still honoring to preserve compatibility as much as we could.

This addresses the issue in #9604 .
backspace pushed a commit that referenced this pull request Jan 22, 2021
Introduce a new more-block friendly syntax for specifying mounts with a new `mount` block type with the target as label:

```hcl
config {
  image = "..."

  mount {
    type = "..."
    target = "target-path"
    volume_options { ... }
  }
}
```

The main benefit here is that by `mount` being a block, it can nest blocks and avoids the compatibility problems noted in https://github.com/hashicorp/nomad/pull/9634/files#diff-2161d829655a3a36ba2d916023e4eec125b9bd22873493c1c2e5e3f7ba92c691R128-R155 .

The intention is for us to promote this `mount` blocks and quietly deprecate the `mounts` type, while still honoring to preserve compatibility as much as we could.

This addresses the issue in #9604 .
@github-actions
Copy link

github-actions bot commented Dec 6, 2022

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants