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

[smf] Allow Crucible Zones to be self-assembling #498

Merged
merged 7 commits into from
Apr 20, 2023
Merged

[smf] Allow Crucible Zones to be self-assembling #498

merged 7 commits into from
Apr 20, 2023

Conversation

smklein
Copy link
Contributor

@smklein smklein commented Nov 1, 2022

Make the "crucible agent" self-assembling.

This intends to replace the following parts of the sled agent in Omicron:

Part of oxidecomputer/omicron#1898

@smklein
Copy link
Contributor Author

smklein commented Nov 1, 2022

oxidecomputer/omicron#1902 shows what the updates look like on the sled agent side

@leftwo
Copy link
Contributor

leftwo commented Nov 7, 2022

Does this change also need oxidecomputer/omicron#1902 to go in before it will work?

@smklein
Copy link
Contributor Author

smklein commented Nov 7, 2022

Does this change also need oxidecomputer/omicron#1902 to go in before it will work?

Yeah, the two need to land together to work. If both are approved, I would merge this, then update the crucible rev while merging 1902 in omicron.

@leftwo leftwo requested a review from ahl January 9, 2023 17:04
Copy link
Contributor

@ahl ahl left a comment

Choose a reason for hiding this comment

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

looks good to me

Copy link
Collaborator

@jclulow jclulow left a comment

Choose a reason for hiding this comment

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

Broadly looks good! Thanks for doing this. A few nits and small changes, but otherwise I think pretty much ready to go! I looked at this in tandem with the other PR: oxidecomputer/omicron#1902

Comment on lines +15 to +17
ipadm show-addr "$DATALINK/ll" || ipadm create-addr -t -T addrconf "$DATALINK/ll"
ipadm show-addr "$DATALINK/omicron6" || ipadm create-addr -t -T static -a "$LISTEN_ADDR" "$DATALINK/omicron6"
route get -inet6 default -inet6 "$GATEWAY" || route add -inet6 default -inet6 "$GATEWAY"
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should really move this network initialisation into a separate service that's uniform in all of the zones, I think. If we're happy with the shape of this (i.e., this sequence of commands with this set of input variables) we could even include it in the brand as effectively a stable interface. You'd be able to just turn it on and specify the datalink/address/gateway in the site profile and that'd be it. We could make svc:/milestone/network depend on it, and all our services could just depend on that.

We can also do that later, of course, if you want to get moving now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm totally on-board with doing this , and agree that it would be better served as a service. Is https://github.com/oxidecomputer/helios-omicron-brand a better spot for this? Where should it live in there?

(I do plan on moving forward with this implemented manually, but I'd be happy to switch over to another service if I could get some guidance on setting up the service within the brand)

pantry/smf/manifest.xml Outdated Show resolved Hide resolved
agent/agent_method_script.sh Show resolved Hide resolved
pantry/pantry_method_script.sh Show resolved Hide resolved
package-manifest.toml Outdated Show resolved Hide resolved
Comment on lines +6 to +7
DATALINK="$(svcprop -c -p config/datalink "${SMF_FMRI}")"
GATEWAY="$(svcprop -c -p config/gateway "${SMF_FMRI}")"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should probably explicitly check that the DATALINK and GATEWAY values here are not set to the unknown default value?

After L3-4, but before L6, we could insert:

. /lib/svc/share/smf_include.sh

Then, if we detect missing configuration, we can exit $SMF_EXIT_ERR_CONFIG to have SMF flag a permanent configuration error; e.g.,

if [[ $DATALINK == unknown ]] || [[ $GATEWAY == unknown ]]; then
        printf 'ERROR: missing datalink or gateway\n' >&2
        exit "$SMF_EXIT_ERR_CONFIG"
fi

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, updated.

Couple things:

  • Is there a standard for checking "empty values" of properties in illumos? I noticed that I could either...
    • (1) totally delete the property, which obfuscates the value by removing it from the manifest.xml file
    • (2) set it to '', but then it appears to be set in the shell to \"\", rather than an actual empty string, which is kinda odd.
    • (3) use this quirky naming scheme, at least for astring values, of setting it to some reserved sentinel value (like default).

I ask because if we generalize some of this stuff to an extra service, I don't think all these values actually need to be set, so some could be only supplied optionally. For example, I think the underlay address we're supplying might not always be available when the zone starts (such as within the switch zone).

agent/agent_method_script.sh Show resolved Hide resolved
@smklein smklein merged commit 12b65eb into main Apr 20, 2023
@smklein smklein deleted the smf-updates branch April 20, 2023 21:04
smklein added a commit to oxidecomputer/omicron that referenced this pull request Apr 21, 2023
…1902)

Part of #1898
Relies on oxidecomputer/crucible#498

Converts Crucible, Cockroach, and Clickhouse to be (mostly)
self-assembling.

I'm happy to proceed and convert the rest of the zones we're launching
using a similar format, if we like how this looks.

Fixes #2886
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 this pull request may close these issues.

4 participants