Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update Reboot proto #99

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Update Reboot proto #99

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3de3ece
update commit
xw-g Sep 30, 2022
ad773e0
add sub-component in Status message
xw-g Sep 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 23 additions & 7 deletions system/system.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes

Copy link
Member

Choose a reason for hiding this comment

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

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.

Comment on lines +111 to +112
Copy link
Member

Choose a reason for hiding this comment

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

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.
//

Copy link

Choose a reason for hiding this comment

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

@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

repeated types.Path subcomponents = 4;
// Force reboot if sanity checks fail. (ex. uncommited configuration)
bool force = 5;
Expand Down Expand Up @@ -138,7 +139,9 @@ enum RebootMethod {
// request.
message CancelRebootRequest {
string message = 1; // informational reason for the cancel
repeated types.Path subcomponents = 2; // optional sub-components.
// Optional, sub-components to cancel reboot.
// If it's not set, cancel all outstanding reboot requests.
repeated types.Path subcomponents = 2;
}

message CancelRebootResponse {
Expand All @@ -149,11 +152,24 @@ message RebootStatusRequest {
}

message RebootStatusResponse {
bool active = 1; // If reboot is active.
uint64 wait = 2; // Time left until reboot.
uint64 when = 3; // Time to reboot in nanoseconds since the epoch.
string reason = 4; // Reason for reboot.
uint32 count = 5; // Number of reboots since active.
bool active = 1 [deprecated = true]; // If reboot is active.
uint64 wait = 2 [deprecated = true]; // Time left until reboot.
uint64 when = 3
[deprecated = true]; // Time to reboot in nanoseconds since the epoch.
string reason = 4 [deprecated = true]; // Reason for reboot.
uint32 count = 5 [deprecated = true]; // Number of reboots since active.

message Status {
types.Path subcomponent =
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

1; // sub-component that the reboot status is for.
Comment on lines +163 to +164
Copy link
Contributor

Choose a reason for hiding this comment

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

Unnecessary newline?

bool active = 2; // If reboot is active.
uint64 wait = 3; // Time left until reboot.
uint64 when = 4; // Time to reboot in nanoseconds since the epoch.
string reason = 5; // Reason for reboot.
uint32 count = 6; // Number of reboots since active.
}

repeated Status statuses = 6;
Copy link
Contributor

Choose a reason for hiding this comment

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

#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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • 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?

Copy link

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

}

// A TimeRequest requests the current time accodring to the target.
Expand Down