[FEATURE] server: update template to another template type

[DaanHoogland]

Is your feature request related to a problem? Please describe.
A new feature implemented in apache/cloudstack#3945 needs primate implementation

Describe the solution you'd like
A clear and concise description of what you want to happen.

Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.

Additional context
Add any other context or screenshots about the feature request here.

[PaulAngus]

We were in a code freeze yesterday. please revert this commit

[davidjumani]

@PaulAngus The backend commit went in October, the UI component was merged a couple of days ago. Should the UI addition be reverted? That would make the feature unaccessible for a user / admin

[rohityadavcloud]

We merged the UI PR yesterday based on David and Gabriels' review, as the backend PR was already merged in Oct.
@PaulAngus to revert both frontend/backend PRs/commits see revert button at the bottom of any merged PR for ex #838 (comment), or it would be polite to request Wei to send a revert PR for the backend PR apache/cloudstack#3945 merged in Oct and Rakesh to send a revert PR for UI changes.

[PaulAngus]

The proposed UI change does not match the functionality described in the Backend PR which only allows changes between user and system. the UI screenshot shows changes to/from all types.

The UI change needs reverting -admins can use the API.

The [(https://github.com/apache/cloudstack/pull/3945)] logic also looks to be flawed, as the description does not suggest that the system VM template that was active is updated to inactive. - leaving the DB in an inconsistent state.

it really needs fixing or reverting.

@rhtyd @DaanHoogland @weizhouapache @ravening

[ravening]

@PaulAngus if I'm not mistaken, even in backend, you can change template type from any to any

[PaulAngus]

Thanks for clarifying @ravening, but that make might make it worse as:

• The description does not suggest that the system VM template that was active is updated to inactive. - leaving the DB in an inconsistent state.
• The testing description only says that only user -> system was tested.
• And I can't see anything that looks like the backend operations like downloading system VM templates to all secondary storage pools.
• User VM templates are only supposed to be in one pool, so what happens there? Cloudstack isn't going to be expecting user templates in all sec storage pools - hows deletion and GC going to work?
• Builtin templates should be in all secondary storage pools as well. do they get downloaded?

[DaanHoogland]

@PaulAngus , in your points above I only read items that come down to "the operator should know what they are doing". I hope I am not missing any. Is the UI bit only for admins, @ravening ?

[PaulAngus]

I'm afraid that that is not accurate;

You're assuming that the admin knows how the backend database stores template data, including system VM templates.
Also, if an admin converts a 'user' template to a built-in template, an admin can fully expect that CloudStack knows what it is doing and handles all back-end changes, in the DB and physical ones such as copying templates to every sec store in every zone.

Also, I see no tests to ensure that if a builtin or system template is created (which would therefore be in all zones) and is then changed to a user template, that template deletion and GC occurs properly.

I really hope that I don't have to go on listing functional requirements and possible failure cases that would need to be covered..
.

[ravening]

@PaulAngus , in your points above I only read items that come down to "the operator should know what they are doing". I hope I am not missing any. Is the UI bit only for admins, @ravening ?

@DaanHoogland yes only for admins

[rohityadavcloud]

I'm okay with what @PaulAngus @DaanHoogland @weizhouapache @ravening decide, I'm okay if we decide revert it for now and visit this for 4.16.

[DaanHoogland]

me too, the issue is not with the UI but with the logic in the backend.

[PaulAngus]

As far as I'm concerned, a 'feature' was added to the UI when in a code freeze.

We're deliberately exposing through the UI, a backend change that IMO wasn't fully thought through (as detailed above) and was hardly tested. Therefore, I don't think it should ever have been merged.

The determination that the admin should know what they're doing doesn't hold water, because they are very unlikely to know the backend mechanisms of making template changes. and even if they did know, CloudStack definitely doesn't do what it should do in some cases, and there has been no defensive testing to ensure it does in the rest.

But I can't be bothered to argue any more.

So merge it if you want.

@DaanHoogland @weizhouapache @ravening

[DaanHoogland]

The discussion is about reverting not about merging. I think @PaulAngus ' concerns about the backend change are valid though not confirmed. The frontend only exposes this. In addition to that @rhtyd and I discovered a discrepency that can easily be dealt with but needs addressing; when editting both the router checkbox and the template type dropdown are available. The router checkbox should be removed as the values of both can be conflicting.

[rohityadavcloud]

The feature is restricted to root admin, only for the update/edit template API/form and would require the admin to change the global setting to actually effect change of an env's systemvmtemplate. David's working on a small conditional to check a template upgraded to a routing one is registered/downloaded on all zones, rest we can accept. I've added a fix to not show both isrouting and template type dropdown.

[DaanHoogland]

I am moving thsi to 1.1 as it is going to be needing some finishing but the general jist is done for this release