Remove the virtualization code #9276

cbosdo · 2024-09-17T21:47:23Z

What does this PR change?

Trash virtualization features.

GUI diff

The whole Virtualization tab in the systems details is gone!

Documentation

Documentation issue was created: Link for SUSE Manager contributors, Link for community contributors.
API documentation added: please review the Wiki page Writing Documentation for the API if you have any changes to API documentation.
(OPTIONAL) Documentation PR
DONE

Test coverage

ℹ️ If a major new functionality is added, it is strongly recommended that tests for the new functionality are added to the Cucumber test suite

No tests: removing code and related tests will probably increase the code coverage in the end 😉
DONE

Links

Issue(s): #
Port(s): # add downstream PR(s), if any

DONE

Changelogs

Make sure the changelogs entries you are adding are compliant with https://github.com/uyuni-project/uyuni/wiki/Contributing#changelogs and https://github.com/uyuni-project/uyuni/wiki/Contributing#uyuni-projectuyuni-repository

If you don't need a changelog check, please mark this checkbox:

No changelog needed

If you uncheck the checkbox after the PR is created, you will need to re-run changelog_test (see below)

Re-run a test

If you need to re-run a test, please mark the related checkbox, it will be unchecked automatically once it has re-run:

Re-run test "changelog_test"
Re-run test "backend_unittests_pgsql"
Re-run test "java_pgsql_tests"
Re-run test "schema_migration_test_pgsql"
Re-run test "susemanager_unittests"
Re-run test "javascript_lint"
Re-run test "spacecmd_unittests"

Before you merge

Check How to branch and merge properly!

github-actions · 2024-09-17T21:47:36Z

👋 Hello! Thanks for contributing to our project.
Acceptance tests will take some time (aprox. 1h), please be patient ☕
You can see the progress at the end of this page and at https://github.com/uyuni-project/uyuni/pull/9276/checks
Once tests finish, if they fail, you can check 👀 the cucumber report. See the link at the output of the action.
You can also check the artifacts section, which contains the logs at https://github.com/uyuni-project/uyuni/pull/9276/checks.

If you are unsure the failing tests are related to your code, you can check the "reference jobs". These are jobs that run on a scheduled time with code from master. If they fail for the same reason as your build, it means the tests or the infrastructure are broken. If they do not fail, but yours do, it means it is related to your code.

Reference tests:

KNOWN ISSUES

Sometimes the build can fail when pulling new jar files from download.opensuse.org . This is a known limitation. Given this happens rarely, when it does, all you need to do is rerun the test. Sorry for the inconvenience.

For more tips on troubleshooting, see the troubleshooting guide.

Happy hacking!
⚠️ You should not merge if acceptance tests fail to pass. ⚠️

cbosdo · 2024-09-19T06:19:54Z

@Etheryte could you take a look at the 2 remaining JS issues pointed out by sonarcloud? I know they aren't introduced in this PR, but it seems to me that they are valid.

Etheryte · 2024-09-23T17:56:30Z

@cbosdo The errors are now somehow gone from the latest Sonarcloud scan, but the issue that was there with unused concat() result is a bug. Off the top of my head, the issue was essentially someArray.concat(item), where the result is unused, meaning the concat has no effect. I'm not sure why Sonar doesn't highlight that issue anymore though?

cbosdo · 2024-09-24T07:38:17Z

@cbosdo The errors are now somehow gone from the latest Sonarcloud scan, but the issue that was there with unused concat() result is a bug. Off the top of my head, the issue was essentially someArray.concat(item), where the result is unused, meaning the concat has no effect. I'm not sure why Sonar doesn't highlight that issue anymore though?

Doh! I didn't understand why it was showing it up in this PR since that code wasn't touched in it... but yes, those two occurrences looked perfectly valid to me.

srbarrios

Test code side, LGTM.
Even if still a draft, I don't see anything missing.
The rest of the PR, I will not review.

cbosdo · 2024-09-30T11:26:57Z

I think this is too much removal. We do not want to create VMs, but show the topology and the status is still needed. Let's discuss how far we want to go

Showing the topology is still possible in the Virtual systems list page.

mcalmer · 2024-09-30T11:52:00Z

I think this is too much removal. We do not want to create VMs, but show the topology and the status is still needed. Let's discuss how far we want to go

Showing the topology is still possible in the Virtual systems list page.

But who send the data from the libvirt host about the available VMs?
I think we need to add a VirtualHostGatherer for libvirt or is the salt engine still available?
@cbosdo if the VirtualHostGatherer plugin is needed, please open an issue for it.

mcalmer · 2024-09-30T11:58:49Z

.../spacewalk/upgrade/susemanager-schema-5.1.0-to-susemanager-schema-5.1.1/003-virt-removal.sql

+                     rhnActionVirtVcpu,
+                     rhnActionVirtVolDelete CASCADE;
+
+DELETE FROM rhnActionType WHERE label LIKE 'virt.%';


I am not sure if this is a good idea. Would this change the Event History of systems which used such an action before?
I think changing the event history is critical. I would like to have the OK from @admd before doing it.

Is there maybe a way to drop these tables and classes but keep the event history?

Changing the history for actions that are supposed to not be used shouldn't be a problem. If that is a problem, it probably mean we need to keep the whole feature because someone uses it.

It's a tricky one from legal perspective. Ideally, we should not remove any history from the systems, at least for a pre-defined time(6 months or so, different companies have different policies)

If we can somehow keep the history while removing the feature itself, that would be ideal. If that's not feasible and requires a lot more work, I would just go ahead with this change. However, would add a note in release notes that this will happen.

The problem is that we will have to keep a dozen of tables and the matching Java code, just to show empty content because no-one was using it.

Ok if there is no easy way, we go ahead with your change. We will add a note in release notes when times comes.

cbosdo · 2024-09-30T13:12:54Z

I think this is too much removal. We do not want to create VMs, but show the topology and the status is still needed. Let's discuss how far we want to go

Showing the topology is still possible in the Virtual systems list page.

But who send the data from the libvirt host about the available VMs? I think we need to add a VirtualHostGatherer for libvirt or is the salt engine still available? @cbosdo if the VirtualHostGatherer plugin is needed, please open an issue for it.

There is a libvirt host gatherer that we could add. The engine was somewhat fragile and is now removed.

admd · 2024-10-02T15:31:49Z

I think this is too much removal. We do not want to create VMs, but show the topology and the status is still needed. Let's discuss how far we want to go

Showing the topology is still possible in the Virtual systems list page.

But who send the data from the libvirt host about the available VMs? I think we need to add a VirtualHostGatherer for libvirt or is the salt engine still available? @cbosdo if the VirtualHostGatherer plugin is needed, please open an issue for it.

There is a libvirt host gatherer that we could add. The engine was somewhat fragile and is now removed.

@cbosdo would the Virtual Host field on this page https://suma-long-43-srv.mgr.suse.de/rhn/manager/systems/list/virtual still be populated after the engine removal and we can see the relation between virtual host & virtual systems?

cbosdo · 2024-10-02T16:31:03Z

I think this is too much removal. We do not want to create VMs, but show the topology and the status is still needed. Let's discuss how far we want to go

Showing the topology is still possible in the Virtual systems list page.

But who send the data from the libvirt host about the available VMs? I think we need to add a VirtualHostGatherer for libvirt or is the salt engine still available? @cbosdo if the VirtualHostGatherer plugin is needed, please open an issue for it.

There is a libvirt host gatherer that we could add. The engine was somewhat fragile and is now removed.

@cbosdo would the Virtual Host field on this page https://suma-long-43-srv.mgr.suse.de/rhn/manager/systems/list/virtual still be populated after the engine removal and we can see the relation between virtual host & virtual systems?

Yes it will be populated: the hardware refresh and the virtual host gatherer are the ones involved on this IIRC.

admd · 2024-10-07T09:53:14Z

I think this is too much removal. We do not want to create VMs, but show the topology and the status is still

Yes it will be populated: the hardware refresh and the virtual host gatherer are the ones involved on this IIRC.

Good! We just need to make sure that we don't break that part.

mcalmer · 2024-10-07T09:57:25Z

I think this is too much removal. We do not want to create VMs, but show the topology and the status is still

Yes it will be populated: the hardware refresh and the virtual host gatherer are the ones involved on this IIRC.

Good! We just need to make sure that we don't break that part.

It will break, as the admin has to setup the libvirt module. Especially when an authentication is required.
But first I will create an issue for adding the libvirt module and implement a way to configure it

mcalmer · 2024-10-07T10:01:34Z

@admd I have created https://github.com/SUSE/spacewalk/issues/25437 and added it to the planning board

cbosdo · 2024-10-07T14:37:46Z

It will break, as the admin has to setup the libvirt module. Especially when an authentication is required. But first I will create an issue for adding the libvirt module and implement a way to configure it

I don't care if there is a migration path and the user have to take action.... otherwise we keep everything for ever! Either we want to remove the WHOLE feature or NOTHING!

mcalmer

ok, but we need to work on the replacement to keep the host <=> VM relation

cbosdo · 2024-10-08T08:13:58Z

ok, but we need to work on the replacement to keep the host <=> VM relation

Remember that the libvirt engine was only set up if the Virtualization Host Addon type was enabled on the host. The host / VM relation was there before... Everything else was there before

rjmateus · 2024-10-08T09:46:16Z

@mcalmer how was this information added and used? Was it part of the data that is sent to SCC?

We should not reduce the data quality we send to SCC. We should have a look and see if adding the connection to libvirt on "Virtual Host Managers" is enough (I think it should be, since this is also the case for other providers).
I tried to install the virtual host gatherer for libvirt and it returns an exception. Doesn't loo to complex but it will need some work in that regard (some fields should not be null by default, but instead a empty string)

cbosdo · 2024-10-08T10:10:10Z

Before looking at the libvirt virtual host gatherer, we need to check what happens with just the regular hardware update. For sure you will not know about the unregistered VM running on a host... but that has always been the case without the Virtualization Addon.

My point here is that we should not discuss keeping data that are gathered only using the virtualization addon as this thing is not supposed to be used

rjmateus · 2024-10-08T11:59:09Z

My point here is that we should not discuss keeping data that are gathered only using the virtualization addon as this thing is not supposed to be used

Agree. But we first need to check if is being used and what could be broken. Then plan on how to replace that data.

mcalmer · 2024-10-08T16:21:53Z

@cbosdo yes, it was working before you implemented the new virtualization feature. But we had this "Poller" in the past from the traditional stack and when I remember correctly, this was removed when you implemented the new way with the salt engine.
We need to check.

@rjmateus yes, this is used when sending data to SCC

cbosdo requested review from a team as code owners September 17, 2024 21:47

cbosdo requested review from mackdk and ycedres and removed request for a team September 17, 2024 21:47

cbosdo marked this pull request as draft September 17, 2024 21:47

github-actions bot added java java_lint_checkstyle java_pgsql_tests API Virtualization management ui javascript_lint core-features secondary-features ci-pipelines testing ruby_rubocop webapp salt-actions-implementations data-model xmlrpc test-framework old-ui webui-templates labels Sep 17, 2024

cbosdo mentioned this pull request Sep 19, 2024

Remove KVM host from HEAD and Master SUSE/susemanager-ci#1370

Open

cbosdo force-pushed the no-virt branch from 405dbee to 8812eda Compare September 23, 2024 13:19

srbarrios previously approved these changes Sep 24, 2024

View reviewed changes

cbosdo force-pushed the no-virt branch from 8812eda to b117ecc Compare September 30, 2024 08:15

cbosdo added 2 commits September 30, 2024 12:16

Remove the virtualization code

241bcb4

Fix some sonarcloud Javascript unused return issues

9f7f801

cbosdo dismissed srbarrios’s stale review via 9f7f801 September 30, 2024 10:16

cbosdo force-pushed the no-virt branch from b117ecc to 9f7f801 Compare September 30, 2024 10:16

cbosdo marked this pull request as ready for review September 30, 2024 10:17

cbosdo requested a review from mcalmer September 30, 2024 11:27

cbosdo changed the title ~~Remove the virtualization code.~~ Remove the virtualization code Sep 30, 2024

mcalmer requested changes Sep 30, 2024

View reviewed changes

mcalmer approved these changes Oct 8, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove the virtualization code #9276

Remove the virtualization code #9276

cbosdo commented Sep 17, 2024

github-actions bot commented Sep 17, 2024

cbosdo commented Sep 19, 2024

Etheryte commented Sep 23, 2024

cbosdo commented Sep 24, 2024

srbarrios left a comment

cbosdo commented Sep 30, 2024

mcalmer commented Sep 30, 2024

mcalmer Sep 30, 2024

cbosdo Sep 30, 2024

admd Oct 2, 2024

cbosdo Oct 2, 2024

admd Oct 7, 2024

cbosdo commented Sep 30, 2024

admd commented Oct 2, 2024

cbosdo commented Oct 2, 2024

admd commented Oct 7, 2024

mcalmer commented Oct 7, 2024

mcalmer commented Oct 7, 2024

cbosdo commented Oct 7, 2024

mcalmer left a comment

cbosdo commented Oct 8, 2024

rjmateus commented Oct 8, 2024

cbosdo commented Oct 8, 2024

rjmateus commented Oct 8, 2024

mcalmer commented Oct 8, 2024

Remove the virtualization code #9276

Are you sure you want to change the base?

Remove the virtualization code #9276

Conversation

cbosdo commented Sep 17, 2024

What does this PR change?

GUI diff

Documentation

Test coverage

Links

Changelogs

Re-run a test

Before you merge

github-actions bot commented Sep 17, 2024

cbosdo commented Sep 19, 2024

Etheryte commented Sep 23, 2024

cbosdo commented Sep 24, 2024

srbarrios left a comment

Choose a reason for hiding this comment

cbosdo commented Sep 30, 2024

mcalmer commented Sep 30, 2024

mcalmer Sep 30, 2024

Choose a reason for hiding this comment

cbosdo Sep 30, 2024

Choose a reason for hiding this comment

admd Oct 2, 2024

Choose a reason for hiding this comment

cbosdo Oct 2, 2024

Choose a reason for hiding this comment

admd Oct 7, 2024

Choose a reason for hiding this comment

cbosdo commented Sep 30, 2024

admd commented Oct 2, 2024

cbosdo commented Oct 2, 2024

admd commented Oct 7, 2024

mcalmer commented Oct 7, 2024

mcalmer commented Oct 7, 2024

cbosdo commented Oct 7, 2024

mcalmer left a comment

Choose a reason for hiding this comment

cbosdo commented Oct 8, 2024

rjmateus commented Oct 8, 2024

cbosdo commented Oct 8, 2024

rjmateus commented Oct 8, 2024

mcalmer commented Oct 8, 2024