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):

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[codecov]

Codecov Report

❌ Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 17.93%. Comparing base (5bf869c) to head (84748aa).
⚠️ Report is 255 commits behind head on main.

Files with missing lines	Patch %	Lines
...ain/java/com/cloud/network/NetworkServiceImpl.java	50.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #7405      +/-   ##
============================================
+ Coverage     17.51%   17.93%   +0.42%     
- Complexity    15585    16158     +573     
============================================
  Files          5914     5939      +25     
  Lines        529867   533152    +3285     
  Branches      64722    65238     +516     
============================================
+ Hits          92782    95597    +2815     
- Misses       426635   426814     +179     
- Partials      10450    10741     +291     

Flag	Coverage Δ
uitests	3.66% <ø> (+0.08%)	⬆️
unittests	19.04% <50.00%> (+0.46%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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

[yadvr]

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)

[yadvr]

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.

[yadvr]

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

[GaOrtiga]

@DaanHoogland I added the config

[DaanHoogland]

@DaanHoogland I added the config

thanks @GaOrtiga . @andrijapanicsb @yadvr , can you concur with this solution?

[rajujith]

@GaOrtiga Since this is for the 4.22.1 release, could you retarget the PR to the 4.22 branch?

[DaanHoogland]

@rajujith , this is kind of a hot topic. There is still no consensus on what the default value of the configuration will be and we are at a stalemate. I am not sure if this should be part of 22.1.

[GaOrtiga]

@DaanHoogland I added the config

thanks @GaOrtiga . @andrijapanicsb @yadvr , can you concur with this solution?

@andrijapanicsb @yadvr

[yadvr]

If we must really have it - add some kind of UI config or global setting with the default to not show the option. This is because many MSPs don't want their tenants to be able to spin up an additional VR (i.e. redundant VR).

[yadvr]

Looks like I'm tagged to review something which is hidden in several other messages I cannot see - take my last comment with a pinch of salt, if such a UI config is already there +1 to the PR.

[NuxRo]

If we must really have it - add some kind of UI config or global setting with the default to not show the option. This is because many MSPs don't want their tenants to be able to spin up an additional VR (i.e. redundant VR).

+1 that.

[DaanHoogland]

@GaOrtiga , are you alright with changing the default?

[andrijapanicsb]

+1 to make it false by default, in the global settings - this is just messing people up - end those who have unstable physical networking = it happens 2 Vrs can not talk via VRRP amd then both promote themself to MASTER or BACKUP - either way, not good

[GaOrtiga]

@GaOrtiga , are you alright with changing the default?

@DaanHoogland
I personally would, I don't have a personal opinion on which should be the default. However, it would change the current behaviour of the API, which is not ideal.

Looks like I'm tagged to review something which is hidden in several other messages I cannot see - take my last comment with a pinch of salt, if such a UI config is already there +1 to the PR.

I have done as @yadvr mentioned in this comment, added a UI flag, with default value false, making the field hidden by default. I will leave the backend configuration, but will not change the default value in this PR (if anyone wishes to, they can open a PR to change it in the future).

This approach maintains current behaviour, both on the GUI and the API and adds the possibility for the operator to add the field if they want, or leave it hidden if they don't.

negated after discussion and amendment of the code