openconfig / featureprofiles

Feature Profiles are groups of OpenConfig paths and tests which verify their behavior
Apache License 2.0
52 stars 149 forks source link

osinstall_test.go script need to update tc.dualSup checks "For Junos Evolved image transfer and activate standby are not needed"" #424

Closed avaneesh90 closed 1 year ago

avaneesh90 commented 2 years ago

Hi ,

Need to update the script for vender specific:

Below code changes needed at https://github.com/openconfig/featureprofiles/blob/main/feature/gnoi/os/tests/osinstall/osinstall_test.go#L91:

if tc.dualSup { switch dut.Vendor() { case ondatra.JUNIPER: fmt.Println("For Junos Evolved image transfer and activate standby are not needed") case ondatra.CISCO: tc.transferOS(ctx, t, true) tc.activateOS(ctx, t, true) case ondatra.ARISTA: tc.transferOS(ctx, t, true) tc.activateOS(ctx, t, true) } }

bstoll commented 2 years ago

We generally do not want to introduce changes like this as the goal of the tests is to not have any mentions of any specific vendors. The process for software upgrades is documented in the GNOI protobuf and requires supporting these steps in order to upgrade a standby routing engine. I'm not familiar with the background on why transfer/activate are not needed. Is this a temporary bug workaround?

aaronmillisor commented 2 years ago

I can't speak for the Juniper case, but in IOS XR there is no concept of manually transferring images between RP's. You can transfer files manually, but the OS management has its own special handling. The state is automatically synchronized between RP's in the system as part of the install process for the software. Some other platforms require you to manually transfer/sync images between controller cards as files but this is not universal, even though the gNOI protobuf assumes that it is.

dplore commented 2 years ago

Thanks for reporting the issue. To be compliant to the gNOI specification for OS installs and osinstall_test the device must be able to have an OS install be completed as specified without vendor specific deviations.

xw-g commented 2 years ago

@aaronmillisor and @avaneesh90, I think there might be some confusion here. (maybe because the name of the wrapper function?).

My opinions are:

Therefore, I think Juniper and Cisco's gNOI implementation maybe can have some idempotent behavior here? i.e. upon receiving the tc.transferOS(ctx, t, true), the device should just return InstallResponse->SyncProgress + InstallResponse->Validated message right away, to indicate that the OS is already on the backup supervisor.

Thoughts?

aaronmillisor commented 2 years ago

We have investigated this in the past, however I believe that this would break the semantics that ordering "MUST" not be enforced. https://github.com/openconfig/gnoi/blob/c3391aacaa1b33c4dfc4052d70938fb083798e3a/os/os.proto#L71

In practice, a system which performs automatic synchronization across RP's acts like a single-RP system from a software-management perspective. My preference would be to formalize the treatment of such systems as though they are single-RP for the purposes of software management (even if things like card restarts can still occur).

*Edit: If we took this approach of treating systems that auto-sync as single-RP, we could have a test runtime flag to indicate 'single rp' or 'dual rp', rather than a per-vendor flag.

mojiiba commented 2 years ago

Please review https://github.com/openconfig/featureprofiles/pull/675 and let us know if you any concerns. Also, the current test uses no-reboot=true, but then it reboots the device. I think the test flow needs to be fixed. If a device supports no-reboot activation, then we should be able to verify the os without the reboot.

avaneesh90 commented 1 year ago

Refering to https://github.com/openconfig/featureprofiles/pull/675