Run cEOS postdeploy through container runtime - #3280
Conversation
|
Addressed the requested follow-up on this PR. Changes pushed:
Validation:
|
|
@ankudinov Following up on your July 28 review: the requested docs and tests are now added on this branch, along with the cEOS Robot coverage described in the latest update. Please take another look when you have a moment. |
34fe3e4 to
896e7e1
Compare
|
@ankudinov I rebased this branch onto current |
ankudinov
left a comment
ankudinov
left a comment
There was a problem hiding this comment.
@orrious Thank you for adding the tests! This PR deserves a better description explaining the problem and what it's changing. And this description must be correctly reflected in docs. Even after reading the recent doc update it is still not clear what this PR solves.
Here is the AI generated summary at a cost of few tokens burned. If you agree - please rewrite and add this to the doc in a shortened from:
Existing lifecycle:
deploy → cEOS PostDeploy() → configure interfaces/IPs
Before:
Scrapligo → spawn `docker/podman exec -it ... Cli` subprocess
→ interactive EOS CLI session
After:
cEOS PostDeploy() → containerlab Runtime.Exec()
→ run EOS `Cli -c ...` as one batch command
Why do it?
Use containerlab’s native Docker/Podman runtime connection instead of spawning a host-side CLI binary.
Avoid interactive TTY/Scrapligo behavior.
Capture return code, stdout and stderr properly.
Retry CLI startup up to 60 times, rather than the old unbounded retry.
Make execution more consistent with runtime configuration, particularly Podman.
It also changes the EOS command sequence slightly:
Adds configure terminal.
Uses --abort-on-error.
Ends with end and write memory.
Adds unit and integration tests confirming management/data-interface addresses are applied.
My take: the title is misleading. This is a cEOS-specific transport refactor/robustness change, not a new post-deploy feature. Furthermore, the old implementation already selected either the docker or podman executable, so the PR’s practical motivation is not explained especially well. There is no PR description or linked bug.
I'd also explain a bit more about post-deploy in the doc. However this isn't directly related to this PR and can be skipped for now.
| cliCmd := "Cli -p 15 --abort-on-error -c $'" + strings.Join(cfgs, "\n") + "'" | ||
|
|
||
| for range 60 { | ||
| execCmd := clabexec.NewExecCmdFromSlice([]string{"/bin/bash", "-lc", cliCmd}) |
There was a problem hiding this comment.
Should we really involve bash here? Is there a specific use case for that? Is direct Cli call an option?
clabexec.NewExecCmdFromSlice([]string{
"Cli",
"-p", "15",
"--abort-on-error",
"-c", strings.Join(cfgs, "\n"),
})| var lastResp *clabexec.ExecResult | ||
| cliCmd := "Cli -p 15 --abort-on-error -c $'" + strings.Join(cfgs, "\n") + "'" | ||
|
|
||
| for range 60 { |
There was a problem hiding this comment.
If understand this correctly, this simply throws the whatever post deploy command at cEOS 60 times without any possibility to detect syntax errors, unsupported commands, etc. While I'd expect this line to be healthy in general and not have all of that, can we consider an alternative approach?
- loop 60 times to detect readiness with some simple CLI
- once CLI is ready - stop looping and send post deploy once
- Return any configuration error immediately with stderr
| lastErr = err | ||
| lastResp = resp | ||
| log.Debugf("%s - Cli not ready (%v, %v) - waiting.", nodeCfg.LongName, err, resp) | ||
| ceosPostDeploySleep(2 * time.Second) |
There was a problem hiding this comment.
have to test that in a live lab, but I suspect cancellation will not stop this loop. Please check and fix if it's the case. I'll run this fork in my lab during the final review.
2 participants
No description provided.