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.

Sign up for GitHub

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] Use new zone network config service #1096

Merged
merged 7 commits into from
Feb 20, 2024
+13 −85
Merged

[smf] Use new zone network config service #1096

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
72b2e24
[smf] Use new zone network config service
karencfv Jan 11, 2024
a0d2310
fix test?
karencfv Jan 12, 2024
93897eb
fix test?
karencfv Jan 12, 2024
07bb366
Merge branch 'main' into use-nw-service
karencfv Jan 30, 2024
9a863f9
Extract composite packages to omicron
karencfv Jan 30, 2024
3e2bd7c
tars aren't built if intermediate-only is set
karencfv Feb 7, 2024
9f74bb5
Merge branch 'main' into use-nw-service
karencfv Feb 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 0 additions & 43 deletions agent/agent_method_script.sh
View file
Open in desktop

This file was deleted.

10 changes: 7 additions & 3 deletions agent/smf/agent.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,13 @@
<service_fmri value='svc:/milestone/multi-user' />
</dependency>

<dependency name='zone_network_setup' grouping='require_all' restart_on='none'
type='service'>
<service_fmri value='svc:/oxide/zone-network-setup:default' />
</dependency>

<exec_method type='method' name='start'
exec='/opt/oxide/lib/svc/manifest/crucible/agent.sh'
exec='/opt/oxide/crucible/bin/crucible-agent run -D /opt/oxide/crucible/bin/crucible-downstairs --dataset %{config/dataset} -l [%{config/listen_addr}]:%{config/listen_port} -P %{config/portbase} -p %{config/downstairs_prefix} -s %{config/snapshot_prefix}'
timeout_seconds='30'
/>

Expand All @@ -25,11 +30,10 @@
</property_group>

<property_group name='config' type='application'>
<propval name='datalink' type='astring' value='unknown' />
<propval name='gateway' type='astring' value='unknown' />
<propval name='dataset' type='astring' value='' />
<propval name='listen_addr' type='astring' value='127.0.0.1' />
<propval name='listen_port' type='astring' value='17000' />
<!-- TODO: Are 'uuid' and 'nexus' being used? -->
<propval name='uuid' type='astring' value='' />
<propval name='nexus' type='astring' value='127.0.0.1:12221' />
Comment on lines 37 to 38
Copy link
Contributor

Choose a reason for hiding this comment

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

They might not be used -- I think nexus might have been a relic from before using DNS to find the Nexus address

<propval name='portbase' type='astring' value='19000' />
Expand Down
26 changes: 20 additions & 6 deletions package-manifest.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,36 @@
[package.crucible]
service_name = "crucible"
[package.crucible-svc]
service_name = "crucible-svc"
source.type = "local"
source.rust.binary_names = ["crucible-agent", "crucible-downstairs"]
source.rust.release = true
source.paths = [
{ from = "agent/smf", to = "/var/svc/manifest/site/crucible" },
{ from = "agent/downstairs_method_script.sh", to = "/opt/oxide/lib/svc/manifest/crucible/downstairs.sh" },
{ from = "agent/agent_method_script.sh", to = "/opt/oxide/lib/svc/manifest/crucible/agent.sh" }
]
output.type = "zone"
output.intermediate_only = true

[package.crucible-pantry]
service_name = "pantry"
[package.crucible]
service_name = "crucible"
only_for_targets.image = "standard"
source.type = "composite"
source.packages = [ "crucible-svc.tar.gz", "zone-network-setup.tar.gz" ]
output.type = "zone"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Heya @smklein, I have a couple of questions around packages in this repo

  1. The zone-network-setup.tar.gz package will be built in omicron https://github.com/oxidecomputer/omicron/pull/4677/files#diff-3ef35f168f90144ed9f4e1c80d4e7b95e4584652bf7f22886046a11c7ef630a6R617-R627 . From my understanding, what I have here will work once deployed on the rack (and the led-agent/src/services.rs file is updated of course), but will forever make the rbuild tests fail here?
    If all of that is wrong, will I have to recreate the zone-network-setup service here? :/
  2. I'm not sure what's going on with the packages. I have defined the composite packages here, just like I did in omicron, but for some reason it doesn't create the new composite packages when I run cargo run --bin crucible-package 🤷‍♀️

Copy link
Contributor

Choose a reason for hiding this comment

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

Heya @smklein, I have a couple of questions around packages in this repo

  1. The zone-network-setup.tar.gz package will be built in omicron https://github.com/oxidecomputer/omicron/pull/4677/files#diff-3ef35f168f90144ed9f4e1c80d4e7b95e4584652bf7f22886046a11c7ef630a6R617-R627 . From my understanding, what I have here will work once deployed on the rack (and the led-agent/src/services.rs file is updated of course), but will forever make the rbuild tests fail here?
    If all of that is wrong, will I have to recreate the zone-network-setup service here? :/

I'm not really familiar with what's going on in the rbuild tests in crucible, so I can't really comment on that -- but if this issue is basically a question of dependencies (namely: Omicron generally pulls in Crucible, but here, Crucible wants to pull in something from Omicron, in the form of the network setup), then I think there are two options:

  1. Crucible emits zones that are missing their network config. Omicron adds them in during package assembly.

Pro: I think this would work with minimal setup
Con: Crucible's zones, without this extra step, would be broken out-of-the-box, because they'd be missing network config (seems liket his might be what you're alluding to with the failing tests?)

  1. We could pull the zone-network-setup out of Omicron, and have both crucible and Omicron pull it in?

I don't think we should duplicate the service, but this would be my standard for breaking out of a circular dependency. We did something similar for the Omicron packaging tools, so they could be used on both sides: https://github.com/oxidecomputer/omicron-package

  1. I'm not sure what's going on with the packages. I have defined the composite packages here, just like I did in omicron, but for some reason it doesn't create the new composite packages when I run cargo run --bin crucible-package 🤷‍♀️

This is the entirety of "crucible-package":

crucible/package/src/main.rs

Lines 9 to 23 in e71b10d

#[tokio::main]
async fn main() -> Result<()> {
let cfg = config::parse("package-manifest.toml")?;
let output_dir = Path::new("out");
create_dir_all(output_dir)?;
for (name, package) in cfg.packages {
package
.create_for_target(&Target::default(), &name, output_dir)
.await?;
}
Ok(())
}

What is getting outputted when you run that command?

Copy link
Contributor

Choose a reason for hiding this comment

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

Elaborating on that create_for_target call:

Copy link
Contributor

@smklein smklein Jan 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Also, just to confirm: The "omicron" repo is using the same version of omicron-package as Crucible, so this implementation of building composites should be the same code we're using in Omicron's repo.

EDIT: This is mostly true, except that crucible was missing "sort package creation by dependency order". Added that below.

Copy link
Contributor Author

@karencfv karencfv Jan 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @smklein!

Pro: I think this would work with minimal setup
Con: Crucible's zones, without this extra step, would be broken out-of-the-box, because they'd be missing network config (seems liket his might be what you're alluding to with the failing tests?)

Yeah, that's what I was alluding to with the failing tests. If we choose to go this way we'd have to make significant changes to the rbuild test here.

We could pull the zone-network-setup out of Omicron, and have both crucible and Omicron pull it in?

The more I think about it, this is probably the best way to go. It's extremely likely that we'll have more shared services in the future. For example, clickhouse, ch-keeper and cockroachdb all use the internal-dns service, and a tiny service that disables ssh has just been introduced oxidecomputer/omicron#4716. It's probably best to extract shared services into their own repo it seems 🤔

Do you have any thoughts on this @leftwo ?

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like moving common code to a common location is the way to go, and I don't have any attachment to it being it this repo.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason that the final assembly cannot be done in omicron, keeping the zone network service and all of its illumos dependencies there?

I haven't tried, but I think this is could work if the service names here were changed slightly (crucible-zone, crucible-pantry-zone?). Then omicron could continue downloading those with their new names and combine them with the new zone network service to produce the final crucible and crucible-pantry.

If that's workable, I think it's simpler and keeps final zone assembly in one place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely worth looking into! I'll give it a go


[package.crucible-pantry-svc]
service_name = "pantry-svc"
source.type = "local"
source.rust.binary_names = ["crucible-pantry"]
source.rust.release = true
source.paths = [
{ from = "pantry/smf/pantry.xml", to = "/var/svc/manifest/site/crucible/pantry.xml" },
{ from = "pantry/pantry_method_script.sh", to = "/opt/oxide/lib/svc/manifest/crucible/pantry.sh" }
]
output.type = "zone"
output.intermediate_only = true

[package.crucible-pantry]
service_name = "crucible-pantry"
only_for_targets.image = "standard"
source.type = "composite"
source.packages = [ "crucible-pantry-svc.tar.gz", "zone-network-setup.tar.gz" ]
output.type = "zone"
34 changes: 0 additions & 34 deletions pantry/pantry_method_script.sh
View file
Open in desktop

This file was deleted.

9 changes: 6 additions & 3 deletions pantry/smf/pantry.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,13 @@
<service_fmri value='svc:/milestone/multi-user' />
</dependency>

<dependency name='zone_network_setup' grouping='require_all' restart_on='none'
type='service'>
<service_fmri value='svc:/oxide/zone-network-setup:default' />
</dependency>

<exec_method type='method' name='start'
exec='/opt/oxide/lib/svc/manifest/crucible/pantry.sh'
exec='/opt/oxide/pantry/bin/crucible-pantry run -l [%{config/listen_addr}]:%{config/listen_port}'
timeout_seconds='30'
/>

Expand All @@ -25,8 +30,6 @@
</property_group>

<property_group name='config' type='application'>
<propval name='datalink' type='astring' value='unknown' />
<propval name='gateway' type='astring' value='unknown' />
<propval name='listen_addr' type='astring' value='127.0.0.1' />
<propval name='listen_port' type='astring' value='17000' />
</property_group>
Expand Down
Loading