Open
Conversation
steiler
reviewed
Feb 13, 2026
Collaborator
steiler
left a comment
steiler
left a comment
There was a problem hiding this comment.
just a couple of comments
Member
Author
|
vxlan tests fail, because of #3056 |
Member
Author
steiler
reviewed
Feb 17, 2026
steiler
reviewed
Feb 18, 2026
nodes/default_node.go
Outdated
Comment on lines
+809
to
+814
| parkName := clabutils.ParkingNetnsName(cfg.LongName) | ||
| parkPath, err := ensureNamedNetns(parkName) | ||
| if err != nil { | ||
| return fmt.Errorf("node %q failed creating parking netns: %w", cfg.ShortName, err) | ||
| } | ||
| parkingNode := clablinks.NewGenericLinkNode(parkName, parkPath) |
Collaborator
There was a problem hiding this comment.
Why not create a specific ParkingNode... that does all of this.
nodes/default_node.go
Outdated
Comment on lines
+818
to
+821
| d.GetEndpoints(), | ||
| func(ep clablinks.Endpoint) error { | ||
| return ep.MoveTo(ctx, parkingNode, preMoveSetDownOptions()) | ||
| }, |
Collaborator
There was a problem hiding this comment.
I don't get this... via d.GetEndpoints you already have all endpoints at hand. Iterate over these and execute the MoveTo function.... why this weired indirection?!
nodes/default_node.go
Outdated
Comment on lines
+826
to
+827
| if rbErr := rollbackEndpoints(moved, func(ep clablinks.Endpoint) error { | ||
| return ep.MoveTo(ctx, d, nil) |
Collaborator
There was a problem hiding this comment.
Same here, why not call the MoveTo directly...
nodes/default_node.go
Outdated
Comment on lines
+852
to
+868
| parkName := clabutils.ParkingNetnsName(cfg.LongName) | ||
| parkPath := filepath.Join("/run/netns", parkName) | ||
| if !clabutils.FileOrDirExists(parkPath) { | ||
| return fmt.Errorf( | ||
| "node %q has no parking netns %q; seamless start requires stopping via containerlab", | ||
| cfg.ShortName, | ||
| parkName, | ||
| ) | ||
| } | ||
| parkingNode := clablinks.NewGenericLinkNode(parkName, parkPath) | ||
|
|
||
| // Reparent endpoints to the parking node so MoveTo knows where to find them. | ||
| // Stop and Start are separate process invocations, so the in-memory node | ||
| // reference from Stop does not survive; we restore it here. | ||
| for _, ep := range d.GetEndpoints() { | ||
| ep.SetNode(parkingNode) | ||
| } |
Collaborator
There was a problem hiding this comment.
on the parking node implementation you could have a discovery or so to discover all the existing interfaces and simply move all of them back into the given Node, I envision.
nodes/default_node.go
Outdated
Comment on lines
+1048
to
+1056
| func setEndpointsUp(ctx context.Context, endpoints []clablinks.Endpoint) error { | ||
| for _, ep := range endpoints { | ||
| if err := ep.SetUp(ctx); err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| } |
Collaborator
There was a problem hiding this comment.
This is also a pretty useless use the loop where this is called right now. In my view thats much cleaner.
steiler
reviewed
Feb 20, 2026
steiler
reviewed
Feb 20, 2026
steiler
reviewed
Feb 20, 2026
Member
Author
This reverts commit 83afa45.
kaelemc
reviewed
Mar 5, 2026
Comment on lines
+156
to
+248
| func preStopPrepareVrnetlabQcowAlias(ctx context.Context, d *DefaultNode) { | ||
| aliasName, ok := vrnetlabQcowAliasName(d.Config().Image) | ||
| if !ok { | ||
| log.Debugf( | ||
| "node %q pre-stop vrnetlab qcow alias skipped: unable to infer tag from image %q", | ||
| d.Config().ShortName, | ||
| d.Config().Image, | ||
| ) | ||
| return | ||
| } | ||
|
|
||
| ctx, cancel := context.WithTimeout(ctx, 10*time.Second) | ||
| defer cancel() | ||
|
|
||
| // Some vrnetlab nodes rename the original versioned qcow image after first boot and fail on | ||
| // subsequent starts when they try to rediscover a versioned qcow filename. If there is exactly | ||
| // one non-overlay qcow file in / and our alias is absent, create a hardlink alias based on the | ||
| // image tag. | ||
| cmd := fmt.Sprintf( | ||
| `alias="/%s"; `+ | ||
| `[ -e "$alias" ] && exit 0; `+ | ||
| `src=""; `+ | ||
| `if [ -f /sros.qcow2 ] && [ "/sros.qcow2" != "$alias" ]; then `+ | ||
| `src="/sros.qcow2"; `+ | ||
| `else `+ | ||
| `set -- /*.qcow2; `+ | ||
| `if [ "$1" != "/*.qcow2" ]; then `+ | ||
| `for f in "$@"; do `+ | ||
| `[ "$f" = "$alias" ] && continue; `+ | ||
| `base="${f##*/}"; `+ | ||
| `case "$base" in *overlay*.qcow2) continue ;; esac; `+ | ||
| `if [ -n "$src" ]; then src=""; break; fi; `+ | ||
| `src="$f"; `+ | ||
| `done; `+ | ||
| `fi; `+ | ||
| `fi; `+ | ||
| `[ -n "$src" ] || exit 0; `+ | ||
| `ln "$src" "$alias"`, | ||
| aliasName, | ||
| ) | ||
|
|
||
| execCmd := clabexec.NewExecCmdFromSlice([]string{"sh", "-lc", cmd}) | ||
| res, err := d.RunExec(ctx, execCmd) | ||
| if err != nil { | ||
| log.Warnf( | ||
| "node %q pre-stop vrnetlab qcow alias preparation failed: %v", | ||
| d.Config().ShortName, | ||
| err, | ||
| ) | ||
| return | ||
| } | ||
|
|
||
| if res != nil && res.ReturnCode != 0 { | ||
| log.Warnf( | ||
| "node %q pre-stop vrnetlab qcow alias prep returned code %d (stderr: %s)", | ||
| d.Config().ShortName, | ||
| res.ReturnCode, | ||
| res.Stderr, | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| func vrnetlabQcowAliasName(image string) (string, bool) { | ||
| tag, ok := imageTag(image) | ||
| if !ok { | ||
| return "", false | ||
| } | ||
|
|
||
| return "clab-" + tag + ".qcow2", true | ||
| } | ||
|
|
||
| func imageTag(image string) (string, bool) { | ||
| if at := strings.LastIndex(image, "@"); at != -1 { | ||
| image = image[:at] | ||
| } | ||
|
|
||
| lastSlash := strings.LastIndex(image, "/") | ||
| lastColon := strings.LastIndex(image, ":") | ||
| if lastColon == -1 || lastColon < lastSlash { | ||
| return "", false | ||
| } | ||
|
|
||
| tag := image[lastColon+1:] | ||
| if tag == "" { | ||
| return "", false | ||
| } | ||
|
|
||
| if !imageTagRE.MatchString(tag) { | ||
| return "", false | ||
| } | ||
|
|
||
| return tag, true | ||
| } |
Member
There was a problem hiding this comment.
Not really sure if we want all this in here. I feel maybe it can be something that lives in the vrnetlab side or something, but it would mean people have to build new images.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3055 +/- ##
==========================================
- Coverage 56.92% 56.42% -0.50%
==========================================
Files 216 224 +8
Lines 21469 22019 +550
==========================================
+ Hits 12221 12425 +204
- Misses 8003 8304 +301
- Partials 1245 1290 +45
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The
start/stop/restartcommand starts/stops/restarts one or more nodes in a deployed lab and restores parked dataplaneinterfaces
Tested: