Skip to content

Commit bd76330

Browse files
mtr002mtr002
authored
Fix: Improve component path flexibility for Arista (#581)
- Added more mock processes for broader testing. - Changed PID generation to prevent immediate reuse after restart. - Fix reboot context to prevent premature cancellation.
1 parent a214b83 commit bd76330

3 files changed

Lines changed: 115 additions & 97 deletions

File tree

‎gnmi/fakedevice/fakedevice.go‎

Lines changed: 27 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -74,25 +74,20 @@ func Reboot(ctx context.Context, c *ygnmi.Client, rebootTime int64) error {
7474
// RebootComponent updates the component's last reboot time and reason.
7575
func RebootComponent(ctx context.Context, c *ygnmi.Client, componentName string, rebootTime int64) error {
7676
log.Infof("Performing component reboot for %s at time %d", componentName, rebootTime)
77-
78-
// Get current component state to ensure it exists
79-
_, err := ygnmi.Get(ctx, c, ocpath.Root().Component(componentName).State())
80-
if err != nil {
81-
return fmt.Errorf("failed to get component %s state: %v", componentName, err)
82-
}
77+
timestampedCtx := gnmi.AddTimestampMetadata(ctx, rebootTime)
8378

8479
// Set component to inactive temporarily
85-
if _, err := gnmiclient.Replace(ctx, c, ocpath.Root().Component(componentName).OperStatus().State(), oc.PlatformTypes_COMPONENT_OPER_STATUS_INACTIVE); err != nil {
80+
if _, err := gnmiclient.Replace(timestampedCtx, c, ocpath.Root().Component(componentName).OperStatus().State(), oc.PlatformTypes_COMPONENT_OPER_STATUS_INACTIVE); err != nil {
8681
return fmt.Errorf("failed to set component %s inactive: %v", componentName, err)
8782
}
8883

8984
// Update last reboot time
90-
if _, err := gnmiclient.Replace(ctx, c, ocpath.Root().Component(componentName).LastRebootTime().State(), uint64(rebootTime)); err != nil {
85+
if _, err := gnmiclient.Replace(timestampedCtx, c, ocpath.Root().Component(componentName).LastRebootTime().State(), uint64(rebootTime)); err != nil {
9186
return fmt.Errorf("failed to update component %s reboot time: %v", componentName, err)
9287
}
9388

9489
// Update reboot reason
95-
if _, err := gnmiclient.Replace(ctx, c, ocpath.Root().Component(componentName).LastRebootReason().State(), oc.PlatformTypes_COMPONENT_REBOOT_REASON_REBOOT_USER_INITIATED); err != nil {
90+
if _, err := gnmiclient.Replace(timestampedCtx, c, ocpath.Root().Component(componentName).LastRebootReason().State(), oc.PlatformTypes_COMPONENT_REBOOT_REASON_REBOOT_USER_INITIATED); err != nil {
9691
return fmt.Errorf("failed to update component %s reboot reason: %v", componentName, err)
9792
}
9893

@@ -101,7 +96,7 @@ func RebootComponent(ctx context.Context, c *ygnmi.Client, componentName string,
10196

10297
// Now restore the component OperStatus (reboot completed)
10398
finalState := oc.PlatformTypes_COMPONENT_OPER_STATUS_ACTIVE
104-
if _, err := gnmiclient.Replace(ctx, c, ocpath.Root().Component(componentName).OperStatus().State(), finalState); err != nil {
99+
if _, err := gnmiclient.Replace(timestampedCtx, c, ocpath.Root().Component(componentName).OperStatus().State(), finalState); err != nil {
105100
return fmt.Errorf("failed to restore component %s state after reboot: %v", componentName, err)
106101
}
107102

@@ -113,6 +108,7 @@ func RebootComponent(ctx context.Context, c *ygnmi.Client, componentName string,
113108
func SwitchoverSupervisor(ctx context.Context, c *ygnmi.Client, targetSupervisor string, currentActiveSupervisor string, switchoverTime int64) error {
114109
log.Infof("Performing supervisor switchover from %s to %s at time %d", currentActiveSupervisor, targetSupervisor, switchoverTime)
115110

111+
timestampedCtx := gnmi.AddTimestampMetadata(ctx, switchoverTime)
116112
targetPath := ocpath.Root().Component(targetSupervisor)
117113
currentPath := ocpath.Root().Component(currentActiveSupervisor)
118114

@@ -136,7 +132,7 @@ func SwitchoverSupervisor(ctx context.Context, c *ygnmi.Client, targetSupervisor
136132
oc.PlatformTypes_ComponentRedundantRoleSwitchoverReasonTrigger_USER_INITIATED)
137133
gnmiclient.BatchReplace(batch, currentPath.LastSwitchoverReason().Details().State(), "user initiated switchover")
138134

139-
if _, err := batch.Set(ctx, c); err != nil {
135+
if _, err := batch.Set(timestampedCtx, c); err != nil {
140136
return fmt.Errorf("failed to apply switchover updates: %v", err)
141137
}
142138

@@ -149,22 +145,21 @@ func KillProcess(ctx context.Context, c *ygnmi.Client, pid uint32, processName s
149145
log.Infof("KillProcess called with pid=%d, name=%s, signal=%v, restart=%v", pid, processName, signal, restart)
150146

151147
processPath := ocpath.Root().System().Process(uint64(pid))
148+
currentTime := time.Now().UnixNano()
149+
timestampedCtx := gnmi.AddTimestampMetadata(ctx, currentTime)
152150

153151
// HUP signal - reload configuration
154152
if signal == spb.KillProcessRequest_SIGNAL_HUP {
155153
log.Infof("Reloading process %s (PID: %d) configuration", processName, pid)
156154

157-
reloadTime := time.Now().UnixNano()
158-
timestampedCtx := gnmi.AddTimestampMetadata(ctx, reloadTime)
159-
160-
if _, err := gnmiclient.Replace(timestampedCtx, c, processPath.StartTime().State(), uint64(reloadTime)); err != nil {
155+
if _, err := gnmiclient.Replace(timestampedCtx, c, processPath.StartTime().State(), uint64(currentTime)); err != nil {
161156
return fmt.Errorf("failed to update process %s reload time: %v", processName, err)
162157
}
163158
log.Infof("Successfully reloaded process %s (PID: %d)", processName, pid)
164159
return nil
165160
}
166161

167-
if _, err := gnmiclient.Delete(ctx, c, processPath.State()); err != nil {
162+
if _, err := gnmiclient.Delete(timestampedCtx, c, processPath.State()); err != nil {
168163
return fmt.Errorf("failed to delete process %s (PID: %d): %v", processName, pid, err)
169164
}
170165

@@ -176,7 +171,7 @@ func KillProcess(ctx context.Context, c *ygnmi.Client, pid uint32, processName s
176171
time.Sleep(2 * time.Second)
177172

178173
// PID generation for restarted processes
179-
newPID, err := generateNewPID(ctx, c)
174+
newPID, err := generateNewPID(ctx, c, pid)
180175
if err != nil {
181176
log.Errorf("Failed to generate new PID: %v", err)
182177
return
@@ -185,7 +180,7 @@ func KillProcess(ctx context.Context, c *ygnmi.Client, pid uint32, processName s
185180

186181
// Create new process with same name but new PID
187182
restartTime := time.Now().UnixNano()
188-
timestampedCtx := gnmi.AddTimestampMetadata(ctx, restartTime)
183+
restartCtx := gnmi.AddTimestampMetadata(ctx, restartTime)
189184

190185
newProcess := &oc.System_Process{
191186
Name: ygot.String(processName),
@@ -200,7 +195,7 @@ func KillProcess(ctx context.Context, c *ygnmi.Client, pid uint32, processName s
200195
}
201196

202197
newProcessPath := ocpath.Root().System().Process(uint64(newPID))
203-
if _, err := gnmiclient.Replace(timestampedCtx, c, newProcessPath.State(), newProcess); err != nil {
198+
if _, err := gnmiclient.Replace(restartCtx, c, newProcessPath.State(), newProcess); err != nil {
204199
log.Errorf("Failed to restart process %s with new PID %d: %v", processName, newPID, err)
205200
return
206201
}
@@ -264,7 +259,7 @@ func NewChassisComponentsTask() *reconciler.BuiltReconciler {
264259
Details: ygot.String("initial system startup"),
265260
},
266261
}
267-
gnmiclient.BatchUpdate(batch, ocpath.Root().Component(componentName).State(), component)
262+
gnmiclient.BatchReplace(batch, ocpath.Root().Component(componentName).State(), component)
268263
log.Infof("Batching initialization for supervisor component %s", componentName)
269264
}
270265

@@ -280,7 +275,7 @@ func NewChassisComponentsTask() *reconciler.BuiltReconciler {
280275
LastRebootTime: ygot.Uint64(uint64(now)),
281276
LastRebootReason: oc.PlatformTypes_COMPONENT_REBOOT_REASON_UNSET,
282277
}
283-
gnmiclient.BatchUpdate(batch, ocpath.Root().Component(componentName).State(), component)
278+
gnmiclient.BatchReplace(batch, ocpath.Root().Component(componentName).State(), component)
284279
log.Infof("Batching initialization for line card component %s", componentName)
285280
}
286281

@@ -408,10 +403,13 @@ func NewProcessMonitoringTask() *reconciler.BuiltReconciler {
408403

409404
// Mock daemon processes with their PIDs
410405
processes := map[string]uint64{
411-
"bgpd": basePID + 1,
412-
"ospfd": basePID + 2,
413-
"gnmi-server": basePID + 3,
414-
"sysrib": basePID + 4,
406+
"Octa": basePID + 1,
407+
"Gribi": basePID + 2,
408+
"emsd": basePID + 3,
409+
"kim": basePID + 4,
410+
"grpc_server": basePID + 5,
411+
"fibd": basePID + 6,
412+
"rpd": basePID + 7,
415413
}
416414

417415
log.Infof("Initializing %d mock system processes", len(processes))
@@ -446,19 +444,20 @@ func NewProcessMonitoringTask() *reconciler.BuiltReconciler {
446444
}
447445

448446
// generateNewPID generates a new unique PID for restarted processes
449-
func generateNewPID(ctx context.Context, c *ygnmi.Client) (uint32, error) {
447+
func generateNewPID(ctx context.Context, c *ygnmi.Client, excludePID uint32) (uint32, error) {
450448
processes, err := ygnmi.GetAll(ctx, c, ocpath.Root().System().ProcessAny().State())
451449
if err != nil {
452450
return 0, fmt.Errorf("failed to get existing processes: %v", err)
453451
}
454-
// Build set of used PIDs
452+
// Build set of used PIDs with the current (excluded) PID
455453
used := make(map[uint32]bool)
456454
for _, p := range processes {
457455
if p.Pid != nil {
458456
used[uint32(*p.Pid)] = true
459457
}
460458
}
461-
for pid := uint32(1005); pid <= ceilingPID; pid++ {
459+
used[excludePID] = true
460+
for pid := uint32(1008); pid <= ceilingPID; pid++ {
462461
if !used[pid] {
463462
return pid, nil
464463
}

‎gnoi/gnoi.go‎

Lines changed: 55 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,9 @@ import (
4848
const (
4949
supervisor1Name = "Supervisor1"
5050
supervisor2Name = "Supervisor2"
51+
52+
// Kill process default
53+
defaultRestart = true
5154
)
5255

5356
type bgp struct {
@@ -187,6 +190,21 @@ func (s *system) handleComponentReboot(ctx context.Context, r *spb.RebootRequest
187190
return nil, status.Errorf(codes.NotFound, "component %q not found: %v", componentName, err)
188191
}
189192

193+
delay := r.GetDelay()
194+
if delay == 0 {
195+
s.componentRebootsMu.Lock()
196+
if _, exists := s.componentReboots[componentName]; exists {
197+
s.componentRebootsMu.Unlock()
198+
return nil, status.Errorf(codes.AlreadyExists, "reboot already pending for component %q", componentName)
199+
}
200+
s.componentRebootsMu.Unlock()
201+
// Immediate reboot
202+
if err := fakedevice.RebootComponent(context.Background(), s.c, componentName, time.Now().UnixNano()); err != nil {
203+
return nil, status.Errorf(codes.Internal, "failed to reboot component %q: %v", componentName, err)
204+
}
205+
log.Infof("Component %q immediate reboot completed", componentName)
206+
continue
207+
}
190208
// Check if there's already a pending reboot for this component
191209
s.componentRebootsMu.Lock()
192210
if _, exists := s.componentReboots[componentName]; exists {
@@ -196,7 +214,7 @@ func (s *system) handleComponentReboot(ctx context.Context, r *spb.RebootRequest
196214

197215
// Create a cancellation channel for this component
198216
cancelCh := make(chan struct{}, 1)
199-
rebootCtx, cancel := context.WithCancel(ctx)
217+
rebootCtx, cancel := context.WithCancel(context.Background())
200218
s.componentReboots[componentName] = cancelCh
201219
s.componentRebootsMu.Unlock()
202220

@@ -208,39 +226,27 @@ func (s *system) handleComponentReboot(ctx context.Context, r *spb.RebootRequest
208226
s.componentRebootsMu.Unlock()
209227
}
210228

211-
delay := r.GetDelay()
212-
if delay == 0 {
213-
// Immediate reboot
214-
if err := fakedevice.RebootComponent(ctx, s.c, componentName, time.Now().UnixNano()); err != nil {
215-
cleanup()
216-
return nil, status.Errorf(codes.Internal, "failed to reboot component %q: %v", componentName, err)
217-
}
218-
219-
// Remove from pending list after reboot is complete
220-
cleanup()
221-
log.Infof("Component %q immediate reboot completed", componentName)
222-
} else {
223-
// Handle delayed reboot
224-
go func(compName string) {
225-
defer cleanup()
226-
select {
227-
case <-cancelCh:
228-
log.Infof("delayed component reboot for %q cancelled", compName)
229-
case <-rebootCtx.Done():
230-
log.Infof("delayed component reboot for %q cancelled due to context", compName)
231-
case <-time.After(time.Duration(delay) * time.Nanosecond):
232-
now := time.Now().UnixNano()
233-
if err := fakedevice.RebootComponent(rebootCtx, s.c, compName, now); err != nil {
234-
log.Errorf("delayed component reboot for %q failed: %v", compName, err)
235-
return
236-
}
237-
log.Infof("Component %q delayed reboot completed", compName)
229+
// Handle delayed reboot
230+
go func(compName string) {
231+
defer cleanup()
232+
select {
233+
case <-cancelCh:
234+
log.Infof("delayed component reboot for %q cancelled", compName)
235+
case <-rebootCtx.Done():
236+
log.Infof("delayed component reboot for %q cancelled due to context", compName)
237+
case <-time.After(time.Duration(delay) * time.Nanosecond):
238+
now := time.Now().UnixNano()
239+
if err := fakedevice.RebootComponent(rebootCtx, s.c, compName, now); err != nil {
240+
log.Errorf("delayed component reboot for %q failed: %v", compName, err)
241+
return
238242
}
239-
}(componentName)
243+
log.Infof("Component %q delayed reboot completed", compName)
244+
}
245+
}(componentName)
240246

241-
log.Infof("scheduled component reboot for %q with delay %v", componentName, r.GetDelay())
242-
}
247+
log.Infof("scheduled component reboot for %q with delay %v", componentName, r.GetDelay())
243248
}
249+
244250
return &spb.RebootResponse{}, nil
245251
}
246252

@@ -451,7 +457,7 @@ func (s *system) KillProcess(ctx context.Context, r *spb.KillProcessRequest) (*s
451457
}
452458

453459
// HUP is for reload, restart should be false by default
454-
restart := r.GetRestart()
460+
restart := defaultRestart
455461
if signal == spb.KillProcessRequest_SIGNAL_HUP {
456462
restart = false
457463
}
@@ -460,7 +466,7 @@ func (s *system) KillProcess(ctx context.Context, r *spb.KillProcessRequest) (*s
460466
s.processMu.Lock()
461467
defer s.processMu.Unlock()
462468

463-
if err := fakedevice.KillProcess(ctx, s.c, targetPID, processName, signal, restart); err != nil {
469+
if err := fakedevice.KillProcess(context.Background(), s.c, targetPID, processName, signal, restart); err != nil {
464470
return nil, status.Errorf(codes.Internal, "failed to kill process: %v", err)
465471
}
466472

@@ -510,14 +516,22 @@ func (s *system) resolvePIDAndName(ctx context.Context, pid uint32, name string)
510516
// extractComponentNameFromPath extracts the component name from the gNMI path
511517
func extractComponentNameFromPath(path *pb.Path) (string, error) {
512518
elems := path.GetElem()
513-
if len(elems) != 2 ||
514-
elems[0].GetName() != "components" ||
515-
elems[1].GetName() != "component" ||
516-
elems[1].GetKey()["name"] == "" {
517-
return "", status.Errorf(codes.InvalidArgument,
518-
"invalid OpenConfig component path, expected /components/component[name=...], got: %v", path)
519-
}
520-
return elems[1].GetKey()["name"], nil
519+
// Handle Arista format
520+
if len(elems) == 1 {
521+
componentName := elems[0].GetName()
522+
if componentName == "" {
523+
return "", status.Errorf(codes.InvalidArgument, "Invalid component path, element name is empty, got: %v", path)
524+
}
525+
return componentName, nil
526+
}
527+
if len(elems) == 2 &&
528+
elems[0].GetName() == "components" &&
529+
elems[1].GetName() == "component" &&
530+
elems[1].GetKey()["name"] != "" {
531+
return elems[1].GetKey()["name"], nil
532+
}
533+
return "", status.Errorf(codes.InvalidArgument,
534+
"invalid component path, expected either single element or OpenConfig format (/componets/component[name=...]), got: %v", path)
521535
}
522536

523537
// isActiveSupervisor checks for the redundant role of a supervisor

0 commit comments

Comments
 (0)