Update Reboot proto #99

xw-g · 2022-09-30T16:39:07Z

Propose to:

Clarify the behavior when the list of subcomponents is not set.
Because RebootStatusRequest supports a list of subcomponents, the RebootStatusResponse should also have a way to identify status per subcomponent.

Another option is to deprecate the list of subcomponents in all reboot related proto, and only allow the request to target a single subcomponent. But then it raises more questions, like:

Can the single subcomponent be an optional field?
If it can be an optional field, what the default behavior if it's not set?
If the default behavior is to apply to the whole device (including all subcomponents), then any status reports should also be per component anyway.

xw-g · 2022-09-30T16:39:58Z

@marcushines to review.

coveralls · 2022-09-30T16:40:43Z

Pull Request Test Coverage Report for Build 3160674502

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 0.0%

Totals
Change from base Build 3093533408:	0.0%
Covered Lines:	0
Relevant Lines:	0

💛 - Coveralls

xw-g · 2022-09-30T17:02:52Z

This will probably impact the following merged PRs:

marcushines · 2022-09-30T17:39:49Z

system/system.proto

+    string reason = 4;  // Reason for reboot.
+    uint32 count = 5;   // Number of reboots since active.
+  }
+  repeated Status statuses = 6;


#not sure the logics correctly - if send a statusrequest that is empty what is the expected return? a statuses with a len of 1?

what if there is no reboot in process?
does the system return a single status message saying no reboot or empty statuses

should the Status have the path that is been queried as part of the response?

ah, good catch. I copied and pasted the fields too fast and forgot to add the subcomponent paths. I just added it.

Reading the existing fields (e.g. count, active), seems like the original idea is to return reboot status (i.e. active=false) even no active reboot or planned reboot on the sub-component? The original idea makes sense to me. RebootStatusRequest reports all reboots activities which covers the history and planning. Therefore, to align with the original idea I think we can clarify here saying that if sub-component not set, return status on all sub-components. Thoughts?

@xw-g The last entry of Reboot Status is covered in the openconfig-platform: last-reboot-reason.

leaf last-reboot-reason { type identityref { base oc-platform-types:COMPONENT_REBOOT_REASON; } description "This reports the reason of the last reboot of the component."; }

Should the RebootStatusRequest be applicable to only current/planned reboot requests?

i would return error here if unset - as the total number of components may be large

marcushines · 2022-09-30T17:42:17Z

system/system.proto

@@ -108,7 +108,8 @@ message RebootRequest {
  uint64 delay = 2;
  // Informational reason for the reboot.
  string message = 3;
-  // Optional sub-components to reboot.
+  // Optional, sub-components to reboot.
+  // If it's not set, reboot the whole network device.


hmm i feel like we should require the component to reboot since rebooting a chassis as the default seems like a rather large hammer to "default" to

This PR does require the user/client to be careful with the big hammer.

I had the similar concern, but I also debated with myself that if the field is required, then we probably want to have a very standard way (across all the vendors) to say this Foo component/sub-component means the whole device for all vendors? Is it safe to use /components/component/name (identity=CHASSIS) here? @dplore

Should the subcomponent be renamed to component? What is the logic behind naming it a subcomponent? In openconfig-platform we have component -> subcomponent relationship. If we assume that reboot works on paths defined in openconfig-platform, then maybe using a subcomponent name is not entirely logical.

yes component makes sense - and if not set return not found or invalid parameter

then I suppose to reboot the chassis users should use the /components/component[name=chassis] or something similar to indicate the the control plane modules should reboot?

We should require a single component. If the component is not specified, there should be an error.

Also agreed that we should explicitly clarify that reboot of /components/component[name=chassis] indicates a full system reboot.

hellt · 2022-10-05T11:36:14Z

system/system.proto

+    types.Path subcomponent =
+        1;              // sub-component that the reboot status is for.


Unnecessary newline?

hellt · 2022-10-29T08:47:23Z

Hi all,
Since this PR is targeting the Reboot RPC, I would like to clarify what is the common thinking for the RebootResponse when you reboot the active CPM or the whole chassis.

If gNOI server runs on CPM_A, then what is expected of a system when you reboot this CPM or the whole chassis? Should the response be sent back before the component goes for a reboot? or should it just go for a reboot which results in a TCP session to break?

/cc @marcushines

rgwilton · 2023-03-15T15:14:07Z

If the concern is about accidentally rebooting a chassis, as opposed to a linecard, then perhaps the simplest thing to do would be to split these into two separate reboot RPC operations?

E.g., RebootEntireDevice and RebootSubComponent?

I'm also not sure that I see a great reason to allow multiple components to be rebooted in a single RPC, generally rebooting stuff should be pretty rare and not performance critical.

dplore · 2023-04-21T18:19:52Z

system/system.proto

+  // Optional, sub-components to reboot.
+  // If it's not set, reboot the whole network device.


Suggested change

// Optional, sub-components to reboot.

// If it's not set, reboot the whole network device.

// This string must contain the /components/component/name to reboot

// If not set, INVALID_ARGUMENT should be returned.

// If the request is accepted by the device, a gRPC OK should be returned.

// Reboot of /components/component[name=chassis] indicates a full system reboot.

// In the case of a full device reboot, a gRPC OK should still be returned and the gRPC

// session closed. This indicates to the client that the request was accepted and the

// reboot will proceed.

//

@dplore

// In the case of a full device reboot without delay, a gRPC OK should still be returned and the gRPC // session closed. This indicates to the client that the request was accepted and the // reboot will proceed. With delay, gRPC OK will be returned

dplore · 2023-04-21T18:24:28Z

system/system.proto

+  uint32 count = 5 [deprecated = true];   // Number of reboots since active.
+
+  message Status {
+    types.Path subcomponent =


Can we use component instead of sub-component? A flat list of components should do the job. Any use cases for hierarchy of components can be determined via gNMI.

Component should be mandatory. The component tree is very large on most devices.

xw-g · 2023-04-21T22:10:59Z

Internal bug b/245550570.

LimeHat · 2024-02-21T19:34:51Z

@dplore I'm curious, is there a plan to move this PR forward? Been stale for almost a year.

dplore · 2024-02-22T23:01:03Z

It's been pushed out for some time. I have queued it in my sprint plan, but currently is about 3 weeks out for review/refactoring.

update commit

3de3ece

xw-g mentioned this pull request Sep 30, 2022

RebootStatus RPC ambiguity #54

Open

marcushines reviewed Sep 30, 2022

View reviewed changes

add sub-component in Status message

ad773e0

hellt reviewed Oct 5, 2022

View reviewed changes

mojiiba mentioned this pull request Oct 29, 2022

gNOI-3.4: fix gnoi path for component and reboot status with empty subcomponent openconfig/featureprofiles#717

Merged

dplore requested changes Apr 21, 2023

View reviewed changes

dplore linked an issue Apr 21, 2023 that may be closed by this pull request

RebootStatus RPC ambiguity #54

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Reboot proto #99

Update Reboot proto #99

xw-g commented Sep 30, 2022

xw-g commented Sep 30, 2022 •

edited

Loading

coveralls commented Sep 30, 2022 •

edited

Loading

xw-g commented Sep 30, 2022

marcushines Sep 30, 2022

marcushines Sep 30, 2022

xw-g Sep 30, 2022

AnandJyrm Oct 4, 2022

marcushines Oct 6, 2022

marcushines Sep 30, 2022

xw-g Sep 30, 2022

hellt Oct 5, 2022

marcushines Oct 6, 2022

hellt Oct 7, 2022

marcushines Oct 25, 2022

dplore Mar 15, 2023

hellt Oct 5, 2022

hellt commented Oct 29, 2022

rgwilton commented Mar 15, 2023

dplore Apr 21, 2023

AnandJyrm Apr 21, 2023

dplore Apr 21, 2023

dplore Apr 21, 2023

xw-g commented Apr 21, 2023

LimeHat commented Feb 21, 2024

dplore commented Feb 22, 2024

		types.Path subcomponent =
		1; // sub-component that the reboot status is for.

		// Optional, sub-components to reboot.
		// If it's not set, reboot the whole network device.

-  // Optional, sub-components to reboot.
-  // If it's not set, reboot the whole network device.
+  // This string must contain the /components/component/name to reboot
+  // If not set, INVALID_ARGUMENT should be returned.
+  // If the request is accepted by the device, a gRPC OK should be returned.
+  // Reboot of /components/component[name=chassis] indicates a full system reboot.
+  // In the case of a full device reboot, a gRPC OK should still be returned and the gRPC
+  // session closed.  This indicates to the client that the request was accepted and the
+  // reboot will proceed.
+  //

Update Reboot proto #99

Are you sure you want to change the base?

Update Reboot proto #99

Conversation

xw-g commented Sep 30, 2022

xw-g commented Sep 30, 2022 • edited Loading

coveralls commented Sep 30, 2022 • edited Loading

Pull Request Test Coverage Report for Build 3160674502

💛 - Coveralls

xw-g commented Sep 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hellt commented Oct 29, 2022

rgwilton commented Mar 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xw-g commented Apr 21, 2023

LimeHat commented Feb 21, 2024

dplore commented Feb 22, 2024

xw-g commented Sep 30, 2022 •

edited

Loading

coveralls commented Sep 30, 2022 •

edited

Loading