Repository navigation
config: support for including files & directories (conf.d) - #856
Conversation
**Note: This is a WIP PR that presently only adds the documentation for
the features so that proposal can be discussed.**
Background
==========
While trying to use zrepl in my ansible driven home lab deployment I ran
into an interesting problem. My ZFS based servers subscribe to different
sometimes overlapping roles.
Example role distribution between two servers:
```
serverA:
- common
- web
- file
serverB:
- common
- git
- file
```
Each role wants to create and manage a ZFS dataset with its own
replication / backup policies:
- web: pool/web
- git: pool/git
- file pool/file
At present the creation of a ZFS dataset from each role role is somewhat
very easy, so to is the creation of the basic zrepl configuration file
from the "common" role.
However, when each role tries to register it's job(s) into the singular
zrepl configuration files things get tricky.
I could try adding a role at the end that hardcodes the datasets that
need to be backed up but that seems a bit hacky.
I could also use ansible's `lineinfile` task to try to idempotently add
each dataset's snapshot jobs to the zrepl configuration files but that
causes a problem:
Everytime the "common" role gets run the basic zrepl configuration file
gets re-created causing the web, git, and file roles to all register
changes as they have to re-insert all jobs back into the singular
configuration file.
Proposed Solution
=================
The proposed solution is to allow for the distribution of zrepl job
definition between multiple different YAML files that can be included
from the main zrepl configuration files.
```
global: ...
jobs:
include: jobs.d
```
This directive would be only acceptable in the main configuration file
and is mutually exclusive with any other job definitions in the file.
To keep things lean there will be no conflict resolution provided to
users, job names must be unique across all included job YAML files.
With this feature, the above problem becomes much simpler:
- Common: Sets up the global zrepl configuration and the include
directive
- web/git/file: Each manage their own datasets and create their
jobs.d/web.yml, jobs.d/git.yml, and jobs.d/file.yml.
There was a problem hiding this comment.
Found a typo and suggested some improvements for clarity.
Generally a good proposal IMO.
I think the format could use some discussion.
I've got some questions:
- Why put the includes under
jobs:?
Oh, I see you copied it from the issue. In that case @problame: I suspect it's because you wanted to emphasize the fact that only job definitions should go in other config files, but why not raise an error for now on non-job keys in extension files and allow that possibility later if we find it beneficial? - Why only allow a single include?
- Should we allow the user to specify a single file (
jobs.d/vsjobs.d/specificfile.yml)?
Tl;dr: is there a reason why it's not
includes:
- jobs.d/
- /etc/zrepl/this_one_really_weird_file.yml|
Thanks for the review
Yeah I tried to preserve some of the suggestions in the original issue. I personally agree with you that having a separate key makes more sense. Though I would prefer something more explicit like
No particular reason other than to simplify the potential implementation ( e.g. Zrepl does not need to handle the case where the file
I am not opposed to the idea, It just does not really benefit the proposed use case where the entity writing the configuration file is different than the one writing the jobs file. Aside from my preference for Let me update the request. |
problame
left a comment
There was a problem hiding this comment.
Thanks for going docs first!
I like the proposal with the top-level include + only allowing jobs as a top-level key in the included files.
Keeps things open for extension later on.
Also, it makes it removes ambiguity with regards to how one parses jobs.
Please digest the feedback into an update to this PR (just new commits, no force pushes please) and ask for re-review.
I think after that implementation can start.
As per some of the discussion items I modified the proposal to have a special top level includes key as well as added references to handling of specific file includes.
|
Thanks for the review.
In that case should we still have |
|
@problame I modified the proposal based on your and @InsanePrawn's comments. Please take a look and let me know what you think of the new usage semantics. I also opted to move the documentation into a new file rather than cramming it into |
```yaml include_jobs: "jobs.d/*.yaml" ``` Like `include_keys`, the directory is relative to main configuration file. `include_jobs` can be combined with `jobs`: ```yaml jobs: - name: "zroot-to-zdisk" include_jobs: "jobs.d/*.yaml" ``` See also zrepl/zrepl#856 zrepl/zrepl#856
|
@problame gentle bump on the review |
|
@problame I have finished an initial implementation and added some basic tests. Are you happy with the usage semantics? If so I will push the initial implementation into the PR. |
Very nice! Please do push your implementation, we can always change the code.
IMO no, we're gonna need to detect job name conflicts between drop-in files anyway, no need to treat the main config file differently IMO. (Am i overlooking anything?) |
A relatively straight forward change that adds "include" key support in the main configuration file. The config parser uses the list under this key to open the included configuration files parse them as configs and append their jobs to the main config file.
|
Hey @InsanePrawn / @problame, Sorry this got dropped from my radar for a while since I was struggling with getting the lint job to pass. Finally got back to it. let me know what you think |
|
Gentle bump, are you folks happy with the code. |
- align on typical Go error message style - error wrapping - "path" pacakge in one more place
problame
left a comment
There was a problem hiding this comment.
Made some edits to docs & code.
Will do a follow-up PR that improves test coverage of job names not overlapping, the zrepl daemon command currently panics if that is the case (good) but stuff like zrepl configcheck gives a straight pass, before this PR and after this PR.
conf.d)
Before this PR, config parsing would accept duplicate job names. `zrepl daemon` would later fail to start with a panic. But tools like `zrepl configcheck` would pass. This PR adds a check to ensure job names are unique. Similarly, internal job names were not being rejected by config parsing Move that check to parse-time as well. Last, drive-by change: remove `internal/config/config_include_test.go` introduced in #856 . These aren't necessary because there is already a wildcard test for all valid configs. This PR adds the complimentary "invalid config" wildcard test.
Discard zrepl/zrepl#856. Implemented here long time ago.
**Note: This is a WIP PR that presently only adds the documentation for the features so that proposal can be discussed as per #708 **
Background
While trying to use zrepl in my ansible driven home lab deployment I ran into an interesting problem. My ZFS based servers subscribe to different sometimes overlapping roles.
Example role distribution between two servers:
Each role wants to create and manage a ZFS dataset with its own replication / backup policies:
At present the creation of a ZFS dataset from each role role is somewhat very easy, so to is the creation of the basic zrepl configuration file from the "common" role.
However, when each role tries to register it's job(s) into the singular zrepl configuration files things get tricky.
I could try adding a role at the end that hardcodes the datasets that need to be backed up but that seems a bit hacky.
I could also use ansible's
lineinfiletask to try to idempotently add each dataset's snapshot jobs to the zrepl configuration files but that causes a problem:Everytime the "common" role gets run the basic zrepl configuration file gets re-created causing the web, git, and file roles to all register changes as they have to re-insert all jobs back into the singular configuration file.
Proposed Solution
The proposed solution is to allow for the distribution of zrepl job definition between multiple different YAML files that can be included from the main zrepl configuration files.
This directive would be only acceptable in the main configuration file and is mutually exclusive with any other job definitions in the file.
To keep things lean there will be no conflict resolution provided to users, job names must be unique across all included job YAML files.
With this feature, the above problem becomes much simpler: