Skip to content

[containers]: add system-start flag for auto-start of containers#1201

Draft
saehejkang wants to merge 2 commits intoapple:mainfrom
saehejkang:add-system-start-to-containers
Draft

[containers]: add system-start flag for auto-start of containers#1201
saehejkang wants to merge 2 commits intoapple:mainfrom
saehejkang:add-system-start-to-containers

Conversation

@saehejkang
Copy link
Contributor

@saehejkang saehejkang commented Feb 12, 2026

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update

Motivation and Context

Relates to #158

Adds a simple flag that can be set on container create and container run to auto-start any container that is stopped after container system stop.

Testing

  1. Create a container or run a container with the --system-start flag set
  2. Run container ls to see that the flag is enabled
  3. Run container system stop
  4. Run container system start and the logs should output the containers that were started up again
  5. Run container ls to see that the container is running
  • Tested locally
  • Added/updated tests
  • Added/updated docs

name: .customLong("systemstart"),
help: "Automatically start the container when the container system starts"
)
public var systemStart: Bool = false
Copy link
Contributor Author

@saehejkang saehejkang Feb 12, 2026

Choose a reason for hiding this comment

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

I neglected to use autoStart because containers are automatically spun up with run and create. It will cause confusion, as that is a totally different process from what this flag should inherently be doing.

// is also a very simple check and faster than doing an rpc to get the same result.
if state.client != nil {
// However, if it marked for system-start, continue with bootstrap.
if state.client != nil && state.snapshot.createOptions?.systemStart != true {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you explain rationale behind this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops. That was a check I added just in case to test something and I forgot to remove it.


// Start all the containers that have system-start enabled
log.info("starting containers", metadata: ["startTimeoutSeconds": "\(Self.startTimeoutSeconds)"])
try await startContainers()
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be invoked before the API server finishes bootstrapping.

Copy link
Contributor Author

@saehejkang saehejkang Feb 13, 2026

Choose a reason for hiding this comment

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

Hmmm. Interestingly enough, I tested it by waiting until the API server was bootstrapped/kernel install and the function never got invoked. The logic will need to be updated a tiny bit so this function is invoked correctly.

private func startContainers() async throws {
let client = ContainerClient()
let containers = try await client.list()
let systemStartContainers = containers.filter { $0.createOptions?.systemStart == true && $0.status != .running }
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be $0.status == .stopped?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

either or in this case


private func createHeader() -> [[String]] {
[["ID", "IMAGE", "OS", "ARCH", "STATE", "ADDR", "CPUS", "MEMORY", "STARTED"]]
[["ID", "IMAGE", "OS", "ARCH", "STATE", "ADDR", "CPUS", "MEMORY", "STARTED", "SYSTEM-START"]]
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure it's the best to print "SYSTEM-START" here..

Copy link
Contributor Author

@saehejkang saehejkang Feb 13, 2026

Choose a reason for hiding this comment

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

I was mulling on the idea of adding this to the print output. I feel users should have the ability to know if a container will be started automatically on system startup. It might be worthwhile adding a --verbose option (like image list), where this field will be added to the output (default output will not included it).

/// Starts all containers that are configured to automatically start on system start.
///
/// - Throws: `AggregateError` containing all errors encountered during container startup.
private func startContainers() async throws {
Copy link
Contributor

Choose a reason for hiding this comment

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

We might be able to move starting containers to the API server, near loadAtBoot? Then, we can avoid changing ContainerSnapshot.

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 can take a look at this but ContainerSnapshot was changed just so the SYSTEM-START field could be outputted.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh got it. We need to change ContainerSnapshot anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

NVM this comment, I was testing AI based PR review yesterday and it corrupted my brain :)

@saehejkang
Copy link
Contributor Author

saehejkang commented Feb 13, 2026

Because these changes need to sort of "simulate" a system stop/start, I am thinking of making the following changes to the Makefile for tests. Thought about this for a while and tried different things, but below is the only solution that I makes sense.

exit_code=0; \
# Run setup tests (create containers with --system-start)
echo "Running setup tests..." ; \
$(SWIFT) test -c $(BUILD_CONFIGURATION) $(SWIFT_CONFIGURATION) --filter TestCLICreateCommand || exit_code=1 ; \

# Stop the system
echo "Stopping container system..." ; \
bin/container system stop && sleep 3 && scripts/ensure-container-stopped.sh ; \

# Start the system again
echo "Starting container system..." ; \
bin/container system start $(SYSTEM_START_OPTS) ; \

# Run verification tests (check containers are running)
echo "Running verification tests..." ; \
$(SWIFT) test -c $(BUILD_CONFIGURATION) $(SWIFT_CONFIGURATION) --filter TestSystemStart || exit_code=1 ; \

# Rest of the tests below (omitted for readability)

@@ -16,11 +16,13 @@

public struct ContainerCreateOptions: Codable, Sendable {
Copy link
Contributor

Choose a reason for hiding this comment

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

@jglogan @dcantah How do you think merging this into ContainerConfiguration.label?

For now, there's no way to inform the users of this create options.
label might be a good choice to expose it without drastic changing?

* `-v, --volume <volume>`: Bind mount a volume into the container
* `--virtualization`: Expose virtualization capabilities to the container (requires host and guest support)
* `--runtime`: Set the runtime handler for the container (default: container-runtime-linux)
* `--systemstart`: Automatically start the container when the container system starts
Copy link
Contributor

Choose a reason for hiding this comment

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

--system-start would be better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. Will make that change before this gets merged.

/// Starts all containers that are configured to automatically start on system start.
///
/// - Throws: `AggregateError` containing all errors encountered during container startup.
private func startContainers() async throws {
Copy link
Contributor

Choose a reason for hiding this comment

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

NVM this comment, I was testing AI based PR review yesterday and it corrupted my brain :)

@saehejkang
Copy link
Contributor Author

@JaewonHur Take a look at the comment above and let me know if you see a better way to handle this. I would love to get your input!

@JaewonHur
Copy link
Contributor

Sorry for the delay! We are still thinking about how ContainerConfiguration.labels should be used versus ContainerCreateOptions.

Regarding the test, it feels somewhat unusual to me if a single test case is performed by multiple subsequent commands.

How about adding a new (self-contained) test case under Tests/CLITests/Subcommands/System?
There, we can start API server, create system-start container, stop API server, and restart again.

@saehejkang
Copy link
Contributor Author

Sorry for the delay! We are still thinking about how ContainerConfiguration.labels should be used versus ContainerCreateOptions.

All good and no worries.

How about adding a new (self-contained) test case under Tests/CLITests/Subcommands/System?
There, we can start API server, create system-start container, stop API server, and restart again.

That was my initial thought, but I think I was having issues with executing container system start and container system stop there. I don't think the test suites are configured to properly start/stop the system, as that seems to be handled by the install/uninstall scripts (used in the makefile when running integration tests).

I was thinking that since this is the first "feature" that relies on the system, we build out a single new test suite. This test suite will be ran after an intentional system stop/start (using the scripts again in the makefile). Maybe I am not being clear enough, but it is also hard to explain my thought process in a text box lol.

@JaewonHur
Copy link
Contributor

I agree with your last sentence :)
A new test suite for "features" that rely on system operations, which can run after all integration tests are done (and system is stopped as usual).

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.

2 participants

Comments