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

man5: newline not allowed in hosts bootstrap table #5249

Closed
1 of 3 tasks
vsoch opened this issue Jun 11, 2023 · 8 comments · Fixed by #5252
Closed
1 of 3 tasks

man5: newline not allowed in hosts bootstrap table #5249

vsoch opened this issue Jun 11, 2023 · 8 comments · Fixed by #5252

Comments

@vsoch
Copy link
Member

vsoch commented Jun 11, 2023

Hola! I'm testing out custom flux broker bootstrap configs, and reading from here:

And when I create an example as follows:

imp = "/usr/libexec/flux/flux-imp"

[access]
allow-guest-user = true
allow-root-owner = true

[resource]
path = "/etc/flux/system/R"

[bootstrap]
curve_cert = "/etc/curve/curve.cert"
default_port = 8050
default_bind = "tcp://eth0:%p"
default_connect = "tcp://%h.flux-service.flux-operator.svc.cluster.local:%p"
hosts = [
    {   
        host="flux-sample-0",
        bind="tcp://eth0:8050",
        connect="tcp://flux-sample-0.flux-service.flux-operator.svc.cluster.local:8050"
    },
    { host="flux-sample-[1-3]"},
]

[archive]
dbpath = "/var/lib/flux/job-archive.sqlite"
period = "1m"
busytimeout = "50s"

I get this bug:

flux-broker: Config file error: /etc/flux/config/broker.toml:17: newline not allowed in inline table

I tried a few variations (with newlines) and the only one that worked required all the attributes in the curly braces to be on the same line, e.g.:

[exec]
imp = "/usr/libexec/flux/flux-imp"

[access]
allow-guest-user = true
allow-root-owner = true

[resource]
path = "/etc/flux/system/R"

[bootstrap]
curve_cert = "/etc/curve/curve.cert"
default_port = 8050
default_bind = "tcp://eth0:%p"
default_connect = "tcp://%h.flux-service.flux-operator.svc.cluster.local:%p"
hosts = [{host="flux-sample-0", bind="tcp://eth0:8050",connect="tcp://flux-sample-0.flux-service.flux-operator.svc.cluster.local:8050"},
         {host="flux-sample-[1-3]"}]
[archive]
dbpath = "/var/lib/flux/job-archive.sqlite"
period = "1m"
busytimeout = "50s"

My versions are slightly old so this might be a bug that was fixed?

# flux version
commands:               0.49.0-225-g53e087510
libflux-core:           0.49.0-225-g53e087510
libflux-security:       0.7.0
build-options:          +caliper+systemd+hwloc==2.1.0+zmq==4.3.2

I'm thinking to address this, either it should be allowed to span multiple lines (it looks nicer, matches the examples in the docs) OR the example here https://flux-framework.readthedocs.io/projects/flux-core/en/latest/man5/flux-config-bootstrap.html#example might be updated to not include the newlines. For the time being I-will-pack-them-close-together-like-sardines-so-close-they-can-smell-eachother Thank you!

Tasks

@vsoch
Copy link
Member Author

vsoch commented Jun 11, 2023

oooh do you see that? We can add a tasklist!
image

@garlick
Copy link
Member

garlick commented Jun 11, 2023

Good catch!

Looks like the TOML spec doesn't allow newlines here so I guess we should fix the examples.

@grondo
Copy link
Contributor

grondo commented Jun 11, 2023

Note that support for newlines has been merged in the TOML spec, but hasn't been released in TOML 1.1 yet.

@vsoch
Copy link
Member Author

vsoch commented Jun 12, 2023

Ah interesting - if it's going to be live (and we can use it) in the next month or so, I'd say skip changing it, because we'd have to go back and change it back (and it's likely not widely used / rarely used). If TOML 1.1 is expected to be out in a much longer time (maybe over 6 months) then we probably should make the fix.

It's hard to tell when that will be - they opened an issue here: toml-lang/toml#928 and the milestone is here: https://github.com/toml-lang/toml/milestone/1

Please note the first issue there is "Not all emojis work as bare keys" so we know they have their priorities straight! 😆

Maybe we could leave as it is anticipating the release, but if even one more developer produces the bug (and shows up to report it) we should do the fix.

@garlick
Copy link
Member

garlick commented Jun 12, 2023

Ah, hmm. There's also the matter of when tomlc99 (which we vendor) will support the newer spec.

@grondo
Copy link
Contributor

grondo commented Jun 12, 2023

Yeah, and while some other toml implementations have already picked this up, there's no evidence tomlc99 is working on support for it.

@vsoch
Copy link
Member Author

vsoch commented Jun 12, 2023

The plot thickens! 🥘

garlick added a commit to garlick/flux-core that referenced this issue Jun 12, 2023
Problem: two of the examples split inline tables across lines,
but TOML 1.0.0 forbits newlines bewteen the curly braces unless
they are valid within a value.

Inline tables are described here:
  https://toml.io/en/v1.0.0#inline-table

Change the formatting of the examples to be valid TOML 1.0.0.

Note that the last example demonstrates using the "arrays of tables"
syntax which may be more readable when inline tables become too long.
Arrays of tables are described here:
  https://toml.io/en/v1.0.0#array-of-tables

All the bootstrap examples were successfully run through
https://www.toml-lint.com/.

Fixes flux-framework#5249
garlick added a commit to garlick/flux-core that referenced this issue Jun 12, 2023
Problem: two of the examples in flux-config-bootstrap(5) split
inline tables across lines, but TOML 1.0.0 forbids newlines
bewteen the curly braces unless they are valid within a value.

Inline tables are described here:
  https://toml.io/en/v1.0.0#inline-table

Change the formatting of the examples to be valid TOML 1.0.0.

Note that the last example demonstrates using the "arrays of tables"
syntax which may be more readable when inline tables become too long.
Arrays of tables are described here:
  https://toml.io/en/v1.0.0#array-of-tables

All the bootstrap examples were successfully run through
https://www.toml-lint.com/.

Fixes flux-framework#5249
@mergify mergify bot closed this as completed in #5252 Jun 12, 2023
@vsoch
Copy link
Member Author

vsoch commented Jun 12, 2023

Wooo thank you!! Very quick fix 👍

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 a pull request may close this issue.

3 participants