Skip to content

Add PIM logNeighborChanges field - #552

Open
rgildein wants to merge 1 commit into
mainfrom
feat/pim-log-neighbor-changes
Open

Add PIM logNeighborChanges field#552
rgildein wants to merge 1 commit into
mainfrom
feat/pim-log-neighbor-changes

Conversation

@rgildein

@rgildein rgildein commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Add PIM log-neighbor-changes to CRD

Description

Add PIM logNeighborChanges field

  • Added logNeighborChanges *bool field to PIMSpec in api/core/v1alpha1/pim_types.go
  • Added LogNbhChng *bool (json:"logNbhChng,omitempty") to the NX-OS PIMDom struct in internal/provider/cisco/nxos/pim.go — field is omitted when nil (user did not configure it), avoiding spurious gNMI Sets
  • Wired the field in EnsurePIM (internal/provider/cisco/nxos/provider.go): dom.LogNbhChng = req.PIM.Spec.LogNeighborChanges
  • Added pim_dom_log test fixture + golden file in internal/provider/cisco/nxos/pim_test.go and testdata/pim_dom_log.json
  • Updated test/gnmi/testdata/cisco-nxos-gnmi/pim.txtar with logNeighborChanges: true in spec and "logNbhChng": true in expected gNMI state

Test results

Phase 1 — local checks

Check Result
Lint ✓ 0 issues
Unit tests ✓ 21/21 packages passed
gNMI integration tests ✓ 20/20 passed

Phase 2 — real device (Cisco NX-OS, 10.1.1.1:57403)

Applied PIM CR with logNeighborChanges: true referencing loopback lo99.

Operator reconcile log (first apply):

Updating  path=System/pim-items/inst-items/dom-items/Dom-list[name=default]
          payload={"name":"default","adminSt":"enabled","logNbhChng":true}

gnmic GET response (System/pim-items/inst-items/dom-items/Dom-list[name=default]):

[
  {
    "source": "10.1.1.1:57403",
    "timestamp": 1788878844530406667,
    "time": "2026-09-08T16:47:24.530406667+02:00",
    "updates": [
      {
        "Path": "System/pim-items/inst-items/dom-items/Dom-list[name=default]",
        "values": {
          "System/pim-items/inst-items/dom-items/Dom-list": [
            {
              "adminSt": "enabled",
              "if-items": {
                "If-list": [
                  {
                    "id": "lo99",
                    "ipAddr": "10.99.99.99/32",
                    "pimSparseMode": true
                  }
                ]
              },
              "jpDelay": 100,
              "logNbhChng": true,
              "mtu": 1500,
              "name": "default"
            }
          ]
        }
      }
    ]
  }
]

Idempotency: On subsequent reconciles (after logNbhChng was already set), the operator emitted:

Configuration is already up-to-date  path=System/pim-items/inst-items/dom-items/Dom-list[name=default]

No gNMI Set was issued — idempotency confirmed.

Notes

The NX-OS YANG field name is logNbhChng (bool), not a string enum. The platform default is false.
Since the field is a pointer with omitempty, omitting logNeighborChanges from the CR spec sends no value to the device, leaving the platform default intact.

- Added `logNeighborChanges *bool` field to `PIMSpec` in `api/core/v1alpha1/pim_types.go`
- Added `LogNbhChng *bool` (`json:"logNbhChng,omitempty"`) to the NX-OS `PIMDom` struct in
  `internal/provider/cisco/nxos/pim.go` — field is omitted when nil (user did not configure it), avoiding spurious gNMI
  Sets
- Wired the field in `EnsurePIM` (`internal/provider/cisco/nxos/provider.go`): `dom.LogNbhChng =
  req.PIM.Spec.LogNeighborChanges`
- Added `pim_dom_log` test fixture + golden file in `internal/provider/cisco/nxos/pim_test.go` and
  `testdata/pim_dom_log.json`
- Updated `test/gnmi/testdata/cisco-nxos-gnmi/pim.txtar` with `logNeighborChanges: true` in spec and `"logNbhChng":
  true` in expected gNMI state

Co-authored-by: Claude <claude@anthropic.com>
Signed-off-by: Robert Gildein <rgildein@users.noreply.github.com>
@rgildein
rgildein force-pushed the feat/pim-log-neighbor-changes branch from 7a70279 to 1bd7310 Compare September 8, 2026 15:17
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown

Merging this branch changes the coverage (1 decrease, 1 increase)

Impacted Packages Coverage Δ 🤖
github.com/ironcore-dev/network-operator/api/core/v1alpha1 2.51% (-0.00%) 👎
github.com/ironcore-dev/network-operator/internal/provider/cisco/nxos 9.49% (+0.03%) 👍

Coverage by file

Changed files (no unit tests)

Changed File Coverage Δ Total Covered Missed 🤖
github.com/ironcore-dev/network-operator/api/core/v1alpha1/pim_types.go 12.50% (ø) 8 1 7
github.com/ironcore-dev/network-operator/api/core/v1alpha1/zz_generated.deepcopy.go 0.00% (ø) 2697 (+4) 0 2697 (+4)
github.com/ironcore-dev/network-operator/internal/provider/cisco/nxos/pim.go 53.85% (+7.69%) 13 7 (+1) 6 (-1) 👍
github.com/ironcore-dev/network-operator/internal/provider/cisco/nxos/provider.go 0.35% (-0.00%) 2287 (+1) 8 2279 (+1) 👎

Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code.

Changed unit test files

  • github.com/ironcore-dev/network-operator/internal/provider/cisco/nxos/pim_test.go

@felix-kaestner

felix-kaestner commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

@rgildein did you check whether this field is some vendor-agnostic standard that we want to add to the "core" api types or something Cisco-specific hence should rather go to a provider config? ref/

### Vendor-Specific Extensions (providerConfigRef)
Core API resources define a vendor-neutral configuration surface. When
a vendor requires additional settings, they are expressed as a separate
vendor-specific CRD referenced via `spec.providerConfigRef`.
The controller fetches the referenced object as unstructured data and
passes it through to the provider, which deserializes it into the
appropriate type. This keeps vendor awareness entirely within the
provider — core controllers never interpret vendor-specific content.
Vendor-specific CRDs live under dedicated API groups, separate from
the core API group.

@hardikdr hardikdr added the area/switch-automation Automation processes for network switch management and operations. label Sep 9, 2026
@hardikdr hardikdr added this to Roadmap Sep 9, 2026
@rgildein

rgildein commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

@rgildein did you check whether this field is some vendor-agnostic standard that we want to add to the "core" api types or something Cisco-specific hence should rather go to a provider config? ref/

### Vendor-Specific Extensions (providerConfigRef)
Core API resources define a vendor-neutral configuration surface. When
a vendor requires additional settings, they are expressed as a separate
vendor-specific CRD referenced via `spec.providerConfigRef`.
The controller fetches the referenced object as unstructured data and
passes it through to the provider, which deserializes it into the
appropriate type. This keeps vendor awareness entirely within the
provider — core controllers never interpret vendor-specific content.
Vendor-specific CRDs live under dedicated API groups, separate from
the core API group.

Good call, you are right. I check it and there is no alternative for this in OpenConfig. What would be an option here? Going with custom resource for Cisco or refer Cisco specific resource in PIM?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/switch-automation Automation processes for network switch management and operations. size/M

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

3 participants