stored: add AccessMode SD->Device directive to reserve devices exclusively for reading or writing by SamuelBoerlin · Pull Request #1464 · bareos/bareos

SamuelBoerlin · 2023-05-10T16:28:24Z

Thank you for contributing to the Bareos Project!

Hello, I'm proposing to add a new config directive to SD->Device: AccessMode. The purpose of this directives is to make it possible to reserve devices exclusively for read or write jobs.
My use case is the following: I would like to have a pool which can both be written to (e.g. via virtualfull) and migrated/copied from to another pool at the same time. By reserving at least one device for write jobs only and others for read jobs only, I can ensure that the virtualfull jobs and migration/copy jobs can always run concurrently and don't block each other (unless they want to use same volume).
As far as I can tell, this is currently not possible. Simply adding more devices to the pool would not ensure that there is always a device free for the migration or copy jobs.

Is there interest for such a feature? If so I'd also look into adding tests, updating the relevant docs and diagrams if necessary.

Please check

Short description and the purpose of this PR is present above this paragraph
Your name is present in the AUTHORS file (optional)

If you have any questions or problems, please give a comment in the PR.

Helpful documentation and best practices

Checklist for the reviewer of the PR (will be processed by the Bareos team)

Make sure you check/merge the PR using devtools/pr-tool to have some simple automated checks run and a proper changelog record added.

General

Is the PR title usable as CHANGELOG entry?
Purpose of the PR is understood
Commit descriptions are understandable and well formatted
Check backport line
Required backport PRs have been created

Source code quality

Source code changes are understandable
Variable and function names are meaningful
Code comments are correct (logically and spelling)
Required documentation changes are present and part of the PR

Tests

Decision taken that a test is required (if not, then remove this paragraph)
The choice of the type of test (unit test or systemtest) is reasonable
Testname matches exactly what is being tested
On a fail, output of the test leads quickly to the origin of the fault

arogge · 2023-05-11T08:47:51Z

Hi!
Thank you very much for taking the time to work on Bareos.
Your approach looks reasonable, and the feature would really help a lot in the situations where reservation doesn't do the right thing.
We would really like to move forward here.

I guess we'll have some more-or-less picky change requests though :)

SamuelBoerlin · 2023-05-18T13:24:19Z

Hi! Thank you very much for taking the time to work on Bareos. Your approach looks reasonable, and the feature would really help a lot in the situations where reservation doesn't do the right thing. We would really like to move forward here.

I guess we'll have some more-or-less picky change requests though :)

Hi Arogge! That's great to hear. In that case I'll continue working on this some more!
Please, go ahead and let me know about those change requests 👍

arogge · 2023-05-25T13:38:47Z

core/src/stored/stored_conf.cc

+  {"ReadOnly", CFG_TYPE_BOOL, ITEM(res_dev, read_only), 0, CFG_ITEM_DEFAULT, "false", NULL, NULL},
+  {"WriteOnly", CFG_TYPE_BOOL, ITEM(res_dev, write_only), 0, CFG_ITEM_DEFAULT, "false", NULL, NULL},


I totally understand that you didn't want to add a new type to the configuration parser, but right now with the two booleans we can have four states:

(ReadOnly == false && WriteOnly == false) the old default behaviour

(ReadOnly == true && WriteOnly == false) only use this device for reading

(ReadOnly == false && WriteOnly == true) only use this device for writing

(ReadOnly == true && WriteOnly == true) what does this mean?

So I would argue that we should use one parameter of type IO_DIRECTION to configure this behaviour.
However, I have no thorough idea how to actually name that parameter.

In the process it would probably a good idea to refactor IO_DIRECTION to use "read" and "write" instead of "in" and "out" while retaining backwards compatibility.

You're right, that makes much more sense. I'll look into it.
How about calling it "Access Mode"?

That sounds promising! Maybe then IO_DIRECTION should accept "readonly", "writeonly" and "readwrite", so we would end up with something like:

Access Mode = readonly

in the configuration.

I've made it use IO_DIRECTION now and changed IO_DIRECTION to also accept read(only)/write(only) and readwrite. Next I'd rename AutoXflateMode and its values since it's no longer specific to the AutoXflate plugin. Am I on the right track with this?

Yes, it's definitely a good idea to rename that.

Also, please note that you should rebase your branch onto the latest master.

SamuelBoerlin · 2023-07-04T10:07:07Z

I've tried updating the docs but I'm unable to get it to include the new AccessMode directive. Building the docs works fine, it just doesn't include AccessMode for some reason.

arogge · 2023-07-13T08:32:53Z

Again, sorry for keeping you waiting so long.
To fix the problem with the docs, you have to build with -Ddocs-build-json=yes and -Dtraymonitor=yes once so the autogenerated configuration schema files get updated. The files will then show up as changed and you can commit them.

Concerning the PR as a whole, I really like the changes. However, I probably won't be able to review/discuss/merge it before my holiday, so it will be waiting till beginning of August. Sorry for that.

SamuelBoerlin · 2023-07-13T10:12:51Z

No problem. My Bareos setup is currently running fine as is, so it's not urgent. But anyways, do you think this can be backported to 22 too, because I'm assuming version 23 would not release until around the end of this/beginning of next year?

Ah I see. I had tried docs-build-json=yes before but couldn't get it to work because I tried running it again a second time with yes, which then removed all those schema files again and errored out:

[100%] Built target clean-docs
Running Sphinx v4.3.2
/mount/bareos/docs/manuals
Traceback (most recent call last):
  File "/mount/bareos/docs/manuals/scripts/generate-resource-descriptions.py", line 497, in <module>
    with open(args.filename) as data_file:
FileNotFoundError: [Errno 2] No such file or directory: 'source/include/autogenerated/bareos-dir-config-schema.json'

Configuration error:
There is a programmable error in your configuration file:

Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/sphinx/config.py", line 329, in eval_config_file
    exec(code, namespace)
  File "/mount/bareos/docs/manuals/source/conf.py", line 415, in <module>
    subprocess.check_call(
  File "/usr/lib/python3.10/subprocess.py", line 369, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['scripts/generate-resource-descriptions.py', '--sphinx', 'source/include/autogenerated/bareos-dir-config-schema.json']' returned non-zero exit status 1.

make[3]: *** [docs/manuals/CMakeFiles/docs.dir/build.make:70: docs/manuals/CMakeFiles/docs] Error 2
make[2]: *** [CMakeFiles/Makefile2:6529: docs/manuals/CMakeFiles/docs.dir/all] Error 2
make[1]: *** [CMakeFiles/Makefile2:6536: docs/manuals/CMakeFiles/docs.dir/rule] Error 2
make: *** [Makefile:2176: docs] Error 2

Now it works, but I'm getting the following diffs which seems a bit odd. Should those paths really change? Or should I just commit the AccessMode entry?

[100%] Comparing json files in "/mount/bareos/docs/manuals/source/include/autogenerated" with git repo
 .../include/autogenerated/bareos-dir-config-schema.json       |  2 +-
 .../source/include/autogenerated/bareos-fd-config-schema.json |  4 ++--
 .../source/include/autogenerated/bareos-sd-config-schema.json | 11 +++++++++--
 docs/manuals/source/include/autogenerated/usage/bconsole.txt  |  6 ------
 4 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/docs/manuals/source/include/autogenerated/bareos-dir-config-schema.json b/docs/manuals/source/include/autogenerated/bareos-dir-config-schema.json
index 526ed6cd4..cf45d8fc3 100644
--- a/docs/manuals/source/include/autogenerated/bareos-dir-config-schema.json
+++ b/docs/manuals/source/include/autogenerated/bareos-dir-config-schema.json
@@ -54,7 +54,7 @@
         "WorkingDirectory": {
           "datatype": "DIRECTORY",
           "code": 0,
-          "default_value": "/var/lib/bareos",
+          "default_value": "/usr/local/var/lib/bareos",
           "platform_specific": true,
           "equals": true
         },
diff --git a/docs/manuals/source/include/autogenerated/bareos-fd-config-schema.json b/docs/manuals/source/include/autogenerated/bareos-fd-config-schema.json
index 596e0f573..172f3df03 100644
--- a/docs/manuals/source/include/autogenerated/bareos-fd-config-schema.json
+++ b/docs/manuals/source/include/autogenerated/bareos-fd-config-schema.json
@@ -198,7 +198,7 @@
         "WorkingDirectory": {
           "datatype": "DIRECTORY",
           "code": 0,
-          "default_value": "/var/lib/bareos",
+          "default_value": "/usr/local/var/lib/bareos",
           "platform_specific": true,
           "equals": true
         },
@@ -465,7 +465,7 @@
         "WorkingDirectory": {
           "datatype": "DIRECTORY",
           "code": 0,
-          "default_value": "/var/lib/bareos",
+          "default_value": "/usr/local/var/lib/bareos",
           "platform_specific": true,
           "equals": true
         },
diff --git a/docs/manuals/source/include/autogenerated/bareos-sd-config-schema.json b/docs/manuals/source/include/autogenerated/bareos-sd-config-schema.json
index b5aae5ebb..473c0d71b 100644
--- a/docs/manuals/source/include/autogenerated/bareos-sd-config-schema.json
+++ b/docs/manuals/source/include/autogenerated/bareos-sd-config-schema.json
@@ -198,14 +198,14 @@
         "WorkingDirectory": {
           "datatype": "DIRECTORY",
           "code": 0,
-          "default_value": "/var/lib/bareos",
+          "default_value": "/usr/local/var/lib/bareos",
           "platform_specific": true,
           "equals": true
         },
         "BackendDirectory": {
           "datatype": "DIRECTORY_LIST",
           "code": 0,
-          "default_value": "/usr/lib/bareos/backends",
+          "default_value": "/usr/local/lib/bareos/backends",
           "platform_specific": true,
           "equals": true
         },
@@ -641,6 +641,13 @@
           "default_value": "on",
           "equals": true
         },
+        "AccessMode": {
+          "datatype": "IO_DIRECTION",
+          "code": 0,
+          "default_value": "readwrite",
+          "equals": true,
+          "description": "Access mode specifies whetherthis device can be used for reading, writing or for both modes."
+        },
         "AutoSelect": {
           "datatype": "BOOLEAN",
           "code": 0,
diff --git a/docs/manuals/source/include/autogenerated/usage/bconsole.txt b/docs/manuals/source/include/autogenerated/usage/bconsole.txt
index 53a19ed0c..04aadfb72 100644
--- a/docs/manuals/source/include/autogenerated/usage/bconsole.txt
+++ b/docs/manuals/source/include/autogenerated/usage/bconsole.txt
@@ -22,12 +22,6 @@ Options:
     -l,--list-directors
         List defined Directors.

-    -p,--pam-credentials-filename <path>:FILE
-        PAM Credentials file.
-
-    -o
-        Force sending pam credentials unencrypted.
-
     -s,--no-signals
         No signals (for debugging)

make[3]: *** [docs/manuals/CMakeFiles/check-git.dir/build.make:89: docs/manuals/CMakeFiles/check-git] Error 1
make[2]: *** [CMakeFiles/Makefile2:6476: docs/manuals/CMakeFiles/check-git.dir/all] Error 2
make[1]: *** [CMakeFiles/Makefile2:6536: docs/manuals/CMakeFiles/docs.dir/rule] Error 2
make: *** [Makefile:2176: docs] Error 2

arogge · 2023-07-13T10:16:03Z

It may be possible to Backport this to 22. However, our plan is to release 23.0.0 earlier than at the end of the year.

Concerning the schema files: now you know why recreating them is not enabled by default. I think you can just git add -p and select the hunk that's relevant for you.

SamuelBoerlin · 2023-07-13T10:48:49Z

Okay, thanks!

arogge · 2023-07-13T14:31:04Z

core/src/stored/dev.h

  int oflags{};               /**< Read/write flags */
  DeviceMode open_mode{DeviceMode::kUndefined};
  std::string device_type{};
+  IODirection access_mode{};  /**< Allowed access mode(s) for reservation */


this should probably be inizialized to READ_WRITE

That makes sense.
Done.

core/src/stored/reserve.cc

arogge · 2023-08-09T15:10:01Z

We discussed the backporting internally. Sadly we won't backport this feature to Bareos 22. You'll have to stick with a patched version or run Bareos Next until the release of Bareos 23 (which shouldn't take too long from now)

SamuelBoerlin · 2023-08-10T11:12:23Z

We discussed the backporting internally. Sadly we won't backport this feature to Bareos 22. You'll have to stick with a patched version or run Bareos Next until the release of Bareos 23 (which shouldn't take too long from now)

Thanks - that's ok, I understand. Looking forward to Bareos 23.

This directive allows to reserve devices for either read or write jobs.

SamuelBoerlin force-pushed the read-write-only-devices branch from e458820 to c275f70 Compare May 18, 2023 13:28

arogge requested changes May 25, 2023

View reviewed changes

bruno-at-bareos assigned arogge Jun 22, 2023

SamuelBoerlin force-pushed the read-write-only-devices branch from a8ca576 to 9c414f6 Compare June 30, 2023 15:50

SamuelBoerlin changed the title ~~stored: add ReadOnly and WriteOnly SD->Device directives to reserve devices for read or write jobs~~ stored: add AccessMode SD->Device directive to reserve devices exclusively for reading or writing Jun 30, 2023

SamuelBoerlin requested a review from arogge July 4, 2023 10:07

SamuelBoerlin force-pushed the read-write-only-devices branch from d9835df to f9377b6 Compare July 13, 2023 10:50

arogge reviewed Jul 13, 2023

View reviewed changes

core/src/stored/reserve.cc Show resolved Hide resolved

arogge force-pushed the read-write-only-devices branch from a6d5b0a to b01e08f Compare August 9, 2023 10:30

arogge added the enhancement label Aug 9, 2023

arogge approved these changes Aug 9, 2023

View reviewed changes

SamuelBoerlin added 9 commits August 10, 2023 13:07

stored: add ReadOnly/WriteOnly SD Device directive

8303b9d

This directive allows to reserve devices for either read or write jobs.

tests: add test for readonly/writeonly reservation

dbaa302

refactor: use AccessMode instead of ReadOnly/WriteOnly

cceb42d

refactor: rename AutoXflateMode

e73db91

stored: remove unnecessary access mode checks

2d87906

docs: add docs for SD->Device->AccessMode

e3fb178

fix whitespace

1978ed5

improve docs

7d45e09

update docs schema

3bb6642

SamuelBoerlin and others added 3 commits August 10, 2023 13:07

format

9305db1

stored: initialize device access_mode to READ_WRITE

3599400

Update CHANGELOG.md

7fe96ea

BareosBot force-pushed the read-write-only-devices branch from 8b8047d to 7fe96ea Compare August 10, 2023 13:07

BareosBot merged commit 44db38c into bareos:master Aug 10, 2023

		{"ReadOnly", CFG_TYPE_BOOL, ITEM(res_dev, read_only), 0, CFG_ITEM_DEFAULT, "false", NULL, NULL},
		{"WriteOnly", CFG_TYPE_BOOL, ITEM(res_dev, write_only), 0, CFG_ITEM_DEFAULT, "false", NULL, NULL},

Conversation

SamuelBoerlin commented May 10, 2023 • edited by arogge Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Thank you for contributing to the Bareos Project!

Please check

Helpful documentation and best practices

Checklist for the reviewer of the PR (will be processed by the Bareos team)

General

Source code quality

Tests

Uh oh!

arogge commented May 11, 2023

Uh oh!

SamuelBoerlin commented May 18, 2023

Uh oh!

arogge May 25, 2023

Choose a reason for hiding this comment

Uh oh!

SamuelBoerlin Jun 30, 2023

Choose a reason for hiding this comment

Uh oh!

arogge Jun 30, 2023

Choose a reason for hiding this comment

Uh oh!

SamuelBoerlin Jun 30, 2023

Choose a reason for hiding this comment

Uh oh!

arogge Jun 30, 2023

Choose a reason for hiding this comment

Uh oh!

SamuelBoerlin commented Jul 4, 2023

Uh oh!

arogge commented Jul 13, 2023

Uh oh!

SamuelBoerlin commented Jul 13, 2023

Uh oh!

arogge commented Jul 13, 2023

Uh oh!

SamuelBoerlin commented Jul 13, 2023

Uh oh!

arogge Jul 13, 2023

Choose a reason for hiding this comment

Uh oh!

SamuelBoerlin Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

arogge commented Aug 9, 2023

Uh oh!

SamuelBoerlin commented Aug 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

SamuelBoerlin commented May 10, 2023 •

edited by arogge

Loading

SamuelBoerlin Jul 13, 2023 •

edited

Loading