[containers]: add system-start flag for auto-start of containers#1201
[containers]: add system-start flag for auto-start of containers#1201saehejkang wants to merge 2 commits intoapple:mainfrom
Conversation
| name: .customLong("systemstart"), | ||
| help: "Automatically start the container when the container system starts" | ||
| ) | ||
| public var systemStart: Bool = false |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
Could you explain rationale behind this?
There was a problem hiding this comment.
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() |
There was a problem hiding this comment.
This might be invoked before the API server finishes bootstrapping.
There was a problem hiding this comment.
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 } |
There was a problem hiding this comment.
Should be $0.status == .stopped?
There was a problem hiding this comment.
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"]] |
There was a problem hiding this comment.
I'm not sure it's the best to print "SYSTEM-START" here..
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
We might be able to move starting containers to the API server, near loadAtBoot? Then, we can avoid changing ContainerSnapshot.
There was a problem hiding this comment.
I can take a look at this but ContainerSnapshot was changed just so the SYSTEM-START field could be outputted.
There was a problem hiding this comment.
Oh got it. We need to change ContainerSnapshot anyway.
There was a problem hiding this comment.
NVM this comment, I was testing AI based PR review yesterday and it corrupted my brain :)
|
Because these changes need to sort of "simulate" a system stop/start, I am thinking of making the following changes to the 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 { | |||
| * `-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 |
There was a problem hiding this comment.
--system-start would be better.
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
NVM this comment, I was testing AI based PR review yesterday and it corrupted my brain :)
|
@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! |
|
Sorry for the delay! We are still thinking about how 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 |
All good and no worries.
That was my initial thought, but I think I was having issues with executing 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. |
Type of Change
Motivation and Context
Relates to #158
Adds a simple flag that can be set on
container createandcontainer runto auto-start any container that is stopped aftercontainer system stop.Testing
--system-startflag setcontainer lsto see that the flag isenabledcontainer system stopcontainer system startand the logs should output the containers that were started up againcontainer lsto see that the container isrunning