Enable defining a network as redundant during restart through the UI

[GaOrtiga]

Description

By utilizing the restartNetwork API and setting the makeredundant parameter to true, ACS allows the user to make a guest network redundant during its restart process; however, this feature was only available through the API.
An adjustment was made to add this functionality to the UI.

Types of changes

• Breaking change (fix or feature that would cause existing functionality to change)
• New feature (non-breaking change which adds functionality)
• Bug fix (non-breaking change which fixes an issue)
• Enhancement (improves an existing feature and functionality)
• Cleanup (Code refactoring and cleanup, that may add test cases)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

• Major
• Minor

Screenshots (if appropriate):

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[codecov]

Codecov Report

Merging #7405 (bb53dbf) into main (3dfbb40) will increase coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##               main    #7405    +/-   ##
==========================================
  Coverage     12.68%   12.69%            
- Complexity     8656     8669    +13     
==========================================
  Files          2718     2718            
  Lines        256173   256359   +186     
  Branches      39926    39967    +41     
==========================================
+ Hits          32504    32553    +49     
- Misses       219535   219669   +134     
- Partials       4134     4137     +3     

see 14 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[DaanHoogland]

@GaOrtiga i think this was removed from the UI on purpose. I don´t mind either way but it seems to me operators want to have control on whether users are allowed to make this mistake (as i understood it).

[DaanHoogland]

code lgtm btw

[borisstoyanov]

exactly, tbh I'm against this api option to exist at all since whether redundant or not should be defined in the offering not as a single action

[GaOrtiga]

@GaOrtiga i think this was removed from the UI on purpose. I don´t mind either way but it seems to me operators want to have control on whether users are allowed to make this mistake (as i understood it).

I understand, however, users already can do this through the API; therefore, operators don't fully control it. Also, for VPCs this option exists in the UI while restarting them.
I see @borisstoyanov's point on redundant being a feature defined on the offering, however, the discussion would be outside of the PR's scope; could we create an issue or a thread on the mail-list to discuss the make redundant feature (for both VPC and guest networks)?

[weizhouapache]

As far as I know, some users wanted to disable this on UI, but some others want this.
As a compromise, can we enable it only for root admin ? if users want it to be visible for domain admins or regular users, create a new network offering and do it through network update with the new network offering.

[GaOrtiga]

As far as I know, some users wanted to disable this on UI, but some others want this. As a compromise, can we enable it only for root admin ? if users want it to be visible for domain admins or regular users, create a new network offering and do it through network update with the new network offering.

We could also apply these changes to the UI to maintain consistence with the VPCs section and provide an option in the backend that lets admins determine whether or not to allow users to have access to this. This would standardize the code and address both issues. What are your thoughts on this? If you guys agree on that, we can move with this PR as is, and then create a new one to introduce this other featureset into ACS.

[DaanHoogland]

As far as I know, some users wanted to disable this on UI, but some others want this. As a compromise, can we enable it only for root admin ? if users want it to be visible for domain admins or regular users, create a new network offering and do it through network update with the new network offering.

We could also apply these changes to the UI to maintain consistence with the VPCs section and provide an option in the backend that lets admins determine whether or not to allow users to have access to this. This would standardize the code and address both issues. What are your thoughts on this? If you guys agree on that, we can move with this PR as is, and then create a new one to introduce this other featureset into ACS.

I agree with the addition that disabling it in the backend should also remove the option on the UI.

[weizhouapache]

what's your opinions ?
cc @rohityadavcloud @alexandremattioli @andrijapanicsb @NuxRo

[andrijapanicsb]

There were a lot of cases where people have "accidentally" switched this on and didn't know the consequences, so... not sure what is the best way forward.

[rohityadavcloud]

Sorry -1, we had a conversation in the past. In fact this feature never had existed, until I had put it in. After discussions and feedback from users esp service providers, we decided to remove the option from the UI though the API param may still be available via UI. This is because service providers may have an explicit network offering with redundant routers, users without using/paying for one could this form to circumvent to get redundant routers for a network not intended for them.

However, there is a middle ground but probably not necessary - it could be via adding a global setting for operators to decide if they want to enable such a feature for end users. (listCapabilities or other means can export this option to the UI to show/hide such a setting).

[weizhouapache]

@GaOrtiga
it seems difficult to get a consensus.

will you consider a new global setting to manage it ? (requires UI, API and service layer changes)

[GaOrtiga]

There were a lot of cases where people have "accidentally" switched this on and didn't know the consequences, so... not sure what is the best way forward.

We could add a warning message explaining the consequences when that option is selected

Sorry -1, we had a conversation in the past. In fact this feature never had existed, until I had put it in. After discussions and feedback from users esp service providers, we decided to remove the option from the UI though the API param may still be available via UI. This is because service providers may have an explicit network offering with redundant routers, users without using/paying for one could this form to circumvent to get redundant routers for a network not intended for them.

However, there is a middle ground but probably not necessary - it could be via adding a global setting for operators to decide if they want to enable such a feature for end users. (listCapabilities or other means can export this option to the UI to show/hide such a setting).

They can still do the same process using the API, so adding this to the UI would not make a difference in that case.

As for the global setting, I do agree that is a good option, however it would fall out of the scope of this PR. And I still believe we should make these changes in order to keep consistence between the UI and the API, regardless of the addition of a global setting.

[DaanHoogland]

@GaOrtiga how do you want to move forward with this? (moving it to 4.20 for now)

[rohityadavcloud]

We can't accept this because (a) this was reverted after several service providers and users complaint that they don't want users to have rvr when the offering doesn't allow that by default, (b) isn't aligned with the original use-case. So, while the API may still allow for it, we don't necessarily want to expose that via UI.

[rohityadavcloud]

@GaOrtiga the only way could be to introduce a feature flag in config.json (in UI) and have it disabled by default, when enabled you can pass the options in UI to show the elements. Could you review if this approach works for your use-case or close the PR. Thanks.

[andrijapanicsb]

Agree with what Rohit said, this was enabled some releases ago, and it created issues for the Operators. It should be "impossible" by default, with the option to make it configured/enabled if the operator really needs it.

[alexandremattioli]

-1 on adding this to the UI. Until the fundamental issues with VRRP are solved this feature should be avoided at all costs.

[NuxRo]

Agree with what Rohit said, this was enabled some releases ago, and it created issues for the Operators. It should be "impossible" by default, with the option to make it configured/enabled if the operator really needs it.

+1 what Andrija said. Not against having the feature, but let's make it depend on a global option or something, or a network offering like @borisstoyanov suggested.
We should the same feature that exists in a VPC as well to be honest.

[weizhouapache]

Agree with what Rohit said, this was enabled some releases ago, and it created issues for the Operators. It should be "impossible" by default, with the option to make it configured/enabled if the operator really needs it.

+1 what Andrija said. Not against having the feature, but let's make it depend on a global option or something, or a network offering like @borisstoyanov suggested.

We should the same feature that exists in a VPC as well to be honest.

"remove" after "should"?

In my opinion, redundant VR support is a very good feature, it works pretty well, if the environment has configured correctly and VRRP works without any issue.

However, considering the various network devices, multiple upstream routers, VMware switch or openvswitch, etc. I have to say network configuration is too complicated .....

I have the experience in the past that redundant VRs work well for some years but suddenly few (of many) networks have two MASTER VRs which lead to some downtime of user VMs. The issue is not reproducible. After troubleshooting for some months, the root cause was found, it was caused by a small configuration on one of the two core switches.

Be careful to enable redundant VRs in production...

[GaOrtiga]

Instead of a global setting or a flag in the config.json I propose that we wait for #6918 to be merged and create a network scope setting for this feature, since it will give operators more flexibility to enable this to each network individually.

@DaanHoogland @borisstoyanov @weizhouapache @andrijapanicsb @rohityadavcloud @alexandremattioli @NuxRo

Do you have any concerns about this strategy, or can I advance with this implementation when the PR gets merged?

[DaanHoogland]

Instead of a global setting or a flag in the config.json I propose that we wait for #6918 to be merged and create a network scope setting for this feature, since it will give operators more flexibility to enable this to each network individually.

@DaanHoogland @borisstoyanov @weizhouapache @andrijapanicsb @rohityadavcloud @alexandremattioli @NuxRo

Do you have any concerns about this strategy, or can I advance with this implementation when the PR gets merged?

I think it is kind of strange to allow to change a network to redundant based on a setting of the network. How do you see that @GaOrtiga ?

[GaOrtiga]

Instead of a global setting or a flag in the config.json I propose that we wait for #6918 to be merged and create a network scope setting for this feature, since it will give operators more flexibility to enable this to each network individually.
@DaanHoogland @borisstoyanov @weizhouapache @andrijapanicsb @rohityadavcloud @alexandremattioli @NuxRo
Do you have any concerns about this strategy, or can I advance with this implementation when the PR gets merged?

I think it is kind of strange to allow to change a network to redundant based on a setting of the network. How do you see that @GaOrtiga ?

I believe it gives operators the flexibility to turn this feature on/off individually for each user, while keeping the current behaviour for the operators that do not want to use it, so I see it as a positive implementation. However, I am open to feedback.

[DaanHoogland]

Instead of a global setting or a flag in the config.json I propose that we wait for #6918 to be merged and create a network scope setting for this feature, since it will give operators more flexibility to enable this to each network individually.
@DaanHoogland @borisstoyanov @weizhouapache @andrijapanicsb @rohityadavcloud @alexandremattioli @NuxRo
Do you have any concerns about this strategy, or can I advance with this implementation when the PR gets merged?

I think it is kind of strange to allow to change a network to redundant based on a setting of the network. How do you see that @GaOrtiga ?

I believe it gives operators the flexibility to turn this feature on/off individually for each user, while keeping the current behaviour for the operators that do not want to use it, so I see it as a positive implementation. However, I am open to feedback.

The strange thing, to me is that if it is a setting of the network, the operator would have to inable it for each network that a user creates. instead of at a higher level (zone or domain or some other enitity). I would think if the user requests it you just make it redundant, no further action.

[rohityadavcloud]

A global setting or UI flag based (via config.json) option is okay but then we should make this feature opt-in, and disabled by default. Admins/operators who want this feature can enable it for the UI.

[GaOrtiga]

@DaanHoogland @borisstoyanov @weizhouapache @andrijapanicsb @rohityadavcloud @alexandremattioli @NuxRo

I see that the comunity has mixed opinions regarding this functionality, however, I still believe we should maintain conformity on the API and the UI. In order to achieve this without raising further discussions, I propose that we create a global configuration with default value false that dictates whether this parameter can be used (both on the UI and the API). If set to false, it will not show up on the UI and trying to use it on the API will generate an exception.

This would give operators the flexibility to remove this feature from the backend if they see it as harmful, or leave it both on the backend and the frontend if they see it as helpful.

Would this implementation be enough to offset your concerns regarding this feature?

[JoaoJandre]

@DaanHoogland @borisstoyanov @weizhouapache @andrijapanicsb @rohityadavcloud @alexandremattioli @NuxRo

I see that the comunity has mixed opinions regarding this functionality, however, I still believe we should maintain conformity on the API and the UI. In order to achieve this without raising further discussions, I propose that we create a global configuration with default value false that dictates whether this parameter can be used (both on the UI and the API). If set to false, it will not show up on the UI and trying to use it on the API will generate an exception.

This would give operators the flexibility to remove this feature from the backend if they see it as harmful, or leave it both on the backend and the frontend if they see it as helpful.

Would this implementation be enough to offset your concerns regarding this feature?

@GaOrtiga The problem with the proposed solution is that this is breaking backwards compatibility. When users update to 4.20, they'll start to get an exception when informing this parameter on the API. This kind of change should only be made when changing major versions. Last month I made a proposal for a release schedule that would let us do this kind of change (see https://lists.apache.org/thread/o6o9h3qp8gqrpq4v7o81tl6vp51tkjhg) but sadly it got almost no traction; there is a new discussion thread about it now (#8970), I hope we can advance it this time. As the project stands right now, this proposal should not be accepted.