Repository navigation
fix(containerlab): Fix BMP verify and tenant redeploys - #760
Conversation
verify:bmp failed on any lab whose collector had run long enough for the kubelet to rotate its log. A router sends its Peer Ups once, when its BMP session opens, and the route-monitoring messages that follow push them out of the live file. kubectl logs reads only that file, so every session looked like it had never come up. The check now reads every log file the collector's pod has written on its node, compressed rotations included. It orders them by the kubelet's own file names (restart count, then rotation time, live file last) rather than by modification time, and joins lines the container runtime split into partial records. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
copy_to used docker cp into a destination that might already exist, and docker cp then nests the new copy one level down instead of replacing the old one. Kustomize kept reading the old files, so a changed manifest applied as unchanged. Most deploy scripts worked around it by deleting the destination first, but deploy-ns.sh did not, so an edited tenant never reached a running lab. copy_to now replaces its destination itself and refuses any path outside /galactic/. The per-script deletes it makes redundant are gone, and the comments that described the old behavior say what copy_to does now. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
privateip
changed the title
|
Two reviewers read this PR at head 71c4d91 and agreed it can merge. pr-adversary: "VERDICT: merge. No defect found by reading; the one open item is that the fixes themselves are unrun in the lab." Its one open item is answered by the lab runs below. Its only finding was a nit, a redundant rm -rf in the NAT deploy script's config copy; it is harmless defensive code and was left as is. pr-conventions-reviewer: "VERDICT: merge." Its findings were applied or settled as follows. The title and test-plan wording were applied to the PR body, and both test-plan boxes are now ticked. The two commit subjects (69 and 70 characters) are under the repository's 72-character limit and stay, because rewriting them would need a force-push under the stacked #761. The Co-Authored-By trailers are required attribution. This PR must merge before #761, which edits the same deploy script and then needs a rebase. The RAN: lists were not included in the reports handed to the fixer, so what each reviewer executed is not recorded here. The pr-adversary report says it could not run the BMP check or a tenant redeploy because the lab was unreachable to it. Lab evidence, from runs by the author's session. On a live containerlab lab whose collector log had rotated, the old BMP check failed every session; with this change it passed (44 checks). The rotation path was also exercised against a constructed log directory: a Peer Up present only in a compressed rotated file is found, a split CRI line is rejoined, and a later Peer Down in the live file wins. Re-running a tenant deploy removed the stale nested copy on the node; a test edit then applied as "configured" in a server-side dry run and "unchanged" once reverted. A full clean and deploy from #761, which contains this PR, brought the lab up cleanly, and the BMP check passed on the fresh lab. No code or commits changed in the review pass; only the PR title and body were edited. CI: every check passed on the head. The root unit tests first failed on TestPMTU_VethPodLearnsPathMTU, a known flake tracked in #756 that this PR cannot affect since it changes no Go code; the rerun passed. Auto-merge is enabled with the merge method, the only one the main ruleset allows. The ruleset requires one approving review, a code-owner review, and approval of the last push, so it waits for a human approval. |
2 participants
Summary
On a lab that had run a while, the BMP check reported every BGP session as never up, because it read only the collector's live log and the session announcements had rotated out. It now reads the rotated logs too.
Re-deploying an edited tenant also changed nothing, because the deploy copied the new files beside the old ones. Copies now replace what was there.
Test plan
Related to #699
🤖 Generated with Claude Code