Skip to content

Conversation

@martinsander00
Copy link
Contributor

Resolves: #2485

Summary of Changes

  • Added logic to tunnel.tmpl that shuts down user BGP, IBGP sessions, MSDP neighbors, and ISIS when device.status is Drained
  • Added test case and fixture file to verify drained device configuration
  • This implements the device maintenance workflow from RFC-12 (Network Provisioning Framework)

Testing Verification

  • Added unit test render_drained_device_config_successfully that verifies shutdown commands are included in rendered config when device status is Drained

@martinsander00 martinsander00 force-pushed the controller-shut-down-drained-2485 branch from 54c9c78 to 9779824 Compare January 7, 2026 18:26
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR implements device maintenance workflow by adding session shutdown logic when a device status is set to "Drained". It introduces a new DeviceStatusDrained state and modifies the controller template to automatically shut down BGP sessions, MSDP neighbors, and ISIS routing when a device enters this maintenance state.

  • Added DeviceStatusDrained status constant to the device status enumeration
  • Modified tunnel.tmpl to conditionally shutdown routing protocols (BGP, MSDP, ISIS) when device status is "drained"
  • Added comprehensive test coverage with fixture file to verify drained device configuration

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
smartcontract/sdk/go/serviceability/state.go Adds new DeviceStatusDrained constant and string representation
controlplane/controller/internal/controller/templates/tunnel.tmpl Implements conditional shutdown logic for routing protocols based on device status
controlplane/controller/internal/controller/render_test.go Adds test case for drained device configuration rendering
controlplane/controller/internal/controller/fixtures/base.config.drained.txt Provides expected output fixture for drained device test
CHANGELOG.md Documents the new feature

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@martinsander00 martinsander00 force-pushed the controller-shut-down-drained-2485 branch 9 times, most recently from 059db68 to 1a54fec Compare January 9, 2026 03:54
@ben-malbeclabs
Copy link
Contributor

@martinsander00 - what happens when a device becomes active again? From what I can tell from the code, there is no logic to run no neighbor IP_ADDRESS shutdown or to run no shutdown for MSDP peerings when reverting back to activated state. We need to unwind the commands ran when exiting the drained state.

@nikw9944
Copy link
Contributor

nikw9944 commented Jan 9, 2026

From what I can tell from the code, there is no logic to run no neighbor IP_ADDRESS shutdown or to run no shutdown for MSDP peerings when reverting back to activated state.

I believe the "no router msdp" should take care of this for msdp, but the same issue exists for the isis overload bit, we need to add "no router isis" to handle that.

Also it would be good to un-drain the device in the e2e test to prove it works.

no shutdown
!
{{ if eq $.Device.Status.String "hard-drained" }} set-overload-bit
{{ end }}!
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you put the ! on its own line to make it more readable? Also I think there's an extra space before "set-overload-bit".

{{ if eq $.Device.Status.String "hard-drained" }}   set-overload-bit
{{ end }}
!

@ben-malbeclabs
Copy link
Contributor

ben-malbeclabs commented Jan 9, 2026

From what I can tell from the code, there is no logic to run no neighbor IP_ADDRESS shutdown or to run no shutdown for MSDP peerings when reverting back to activated state.

I believe the "no router msdp" should take care of this for msdp, but the same issue exists for the isis overload bit, we need to add "no router isis" to handle that.

@martinsander00 note that we can't do the same 'no router bgp` as we don't fully own the configuration for BGP i.e. the contributor often adds their own configuration.

@martinsander00 martinsander00 force-pushed the controller-shut-down-drained-2485 branch from 60723c1 to fb9ff1e Compare January 9, 2026 15:59
@martinsander00 martinsander00 force-pushed the controller-shut-down-drained-2485 branch from fb9ff1e to 8276e61 Compare January 9, 2026 18:32
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.

controller: add logic that shuts down user BGP, IBGP sessions, MSDP neighbors, and ISIS neighbors when device.status is drained

3 participants