Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Send additional data to SCC (jsc#SUMA-406) #9595

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

wweellddeerr
Copy link
Contributor

@wweellddeerr wweellddeerr commented Jan 2, 2025

What does this PR change?

It adds:

  • New salt modules to collect SAP workloads, uname and container runtime information.
  • New table and columns to store the collected information.
  • New triggers to flag SCC items after changing uname, container runtime or SAP workloads.

It changes:

  • Hardware profile update salt states and the related Java handler to parse and save the newly collected information into the database.
  • SCC registration logic to parse and include SAP, uname, and container runtime information into the payload.

GUI diff

No difference.

  • DONE

Documentation

Test coverage

  • Unit tests were added

  • DONE

Links

Issue(s): https://github.com/SUSE/spacewalk/issues/26031
Port(s): https://github.com/SUSE/spacewalk/pull/26131

  • 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!

Copy link
Contributor

github-actions bot commented Jan 2, 2025

👋 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/9595/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/9595/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. ⚠️

Copy link
Contributor

github-actions bot commented Jan 2, 2025

Suggested tests to cover this Pull Request
  • proxy_cobbler_pxeboot
  • srv_monitoring
  • srv_rename_hostname
  • proxy_branch_network
  • allcli_sanity
  • sle_ssh_minion
  • min_salt_migration
  • min_salt_install_with_staging
  • proxy_register_as_minion_with_script
  • min_deblike_salt
  • allcli_overview_systems_details
  • min_salt_formulas
  • min_project_lotus
  • srv_docker_cve_audit
  • minssh_move_from_and_to_proxy
  • min_salt_install_package
  • min_deblike_remote_command
  • srv_datepicker
  • min_salt_openscap_audit
  • minkvm_guests
  • min_monitoring
  • min_recurring_action
  • min_salt_minion_details
  • srv_restart
  • min_rhlike_openscap_audit
  • srv_distro_cobbler
  • srv_custom_system_info
  • min_salt_minions_page
  • min_deblike_salt_install_package
  • min_empty_system_profiles
  • min_cve_id_new_syntax
  • min_ssh_tunnel
  • min_move_from_and_to_proxy
  • min_ansible_control_node
  • srv_power_management_redfish
  • min_bootstrap_script
  • min_rhlike_monitoring
  • srv_menu
  • allcli_software_channels_dependencies
  • min_cve_audit
  • min_salt_mgrcompat_state
  • min_config_state_channel
  • srv_reportdb
  • buildhost_bootstrap
  • minssh_salt_install_package
  • allcli_reboot
  • minssh_bootstrap_api
  • proxy_retail_pxeboot_and_mass_import
  • min_rhlike_salt_install_package_and_patch
  • min_deblike_monitoring
  • min_deblike_salt_install_with_staging
  • min_check_patches_install
  • srv_user_configuration_salt_states
  • srv_scc_user_credentials
  • srv_cobbler_distro
  • min_rhlike_salt
  • min_salt_lock_packages
  • min_bootstrap_api
  • srv_group_union_intersection
  • minssh_ansible_control_node
  • srv_virtual_host_manager
  • allcli_action_chain
  • min_salt_formulas_advanced
  • min_config_state_channel_api
  • srv_cobbler_profile
  • srv_power_management
  • min_salt_user_states
  • allcli_config_channel
  • min_virthost
  • min_deblike_openscap_audit
  • srv_power_management_api
  • min_action_chain
  • min_config_state_channel_subscriptions
  • buildhost_docker_auth_registry
  • min_timezone
  • min_bootstrap_negative
  • buildhost_osimage_build_image
  • srv_manage_activationkey
  • srv_advanced_search
  • min_activationkey
  • allcli_software_channels
  • min_bootstrap_reactivation
  • min_rhlike_remote_command
  • srv_maintenance_windows
  • min_salt_pkgset_beacon
  • proxy_as_pod_basic_tests
  • buildhost_docker_build_image
  • sle_minion
  • min_deblike_ssh
  • min_salt_software_states
  • min_rhlike_ssh
  • minssh_action_chain
  • min_custom_pkg_download_endpoint
  • allcli_system_group
  • srv_manage_channels_page
  • min_bootstrap_ssh_key
  • min_retracted_patches
  • min_change_software_channel
  • proxy_container
  • proxy_container_branch_network
  • srv_handle_software_channels_with_ISS_v2
  • minssh_tunnel
  • proxy_traditional_cobbler_pxeboot
  • proxy_container_cobbler_pxeboot
  • srv_push_package
  • srv_task_status_engine
  • srv_errata_api
  • proxy_container_retail_pxeboot
  • proxy_container_retail_mass_import
  • srv_dist_channel_mapping

@wweellddeerr wweellddeerr force-pushed the scc-telemetry-data-master branch from bce5c2f to ac66fa7 Compare January 6, 2025 15:18
@wweellddeerr wweellddeerr changed the title Send additional SAP workloads data to SCC (jsc#SUMA-406) Send data to SCC (jsc#SUMA-406) Jan 6, 2025
@wweellddeerr wweellddeerr changed the title Send data to SCC (jsc#SUMA-406) Send additional data to SCC (jsc#SUMA-406) Jan 6, 2025
@wweellddeerr wweellddeerr force-pushed the scc-telemetry-data-master branch 2 times, most recently from a8c00ed to 8363b6e Compare January 6, 2025 17:08
@mcalmer
Copy link
Contributor

mcalmer commented Jan 7, 2025

I still need to check the python parts

@wweellddeerr wweellddeerr force-pushed the scc-telemetry-data-master branch 2 times, most recently from 2d82fd4 to 3153c3c Compare January 8, 2025 02:45
@wweellddeerr wweellddeerr marked this pull request as ready for review January 8, 2025 03:00
@wweellddeerr wweellddeerr requested review from a team as code owners January 8, 2025 03:00
@wweellddeerr wweellddeerr requested review from cbbayburt and vzhestkov and removed request for a team January 8, 2025 03:00
@mcalmer
Copy link
Contributor

mcalmer commented Jan 8, 2025

@wweellddeerr seems that hw refresh failed in the acceptance tests. We need to check what the error is

@wweellddeerr wweellddeerr force-pushed the scc-telemetry-data-master branch 2 times, most recently from 950d3fb to 1b6b0a1 Compare January 8, 2025 14:21
@wweellddeerr wweellddeerr requested a review from a team as a code owner January 8, 2025 14:21
Copy link
Member

@rjmateus rjmateus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some small questions, other then that, all looks good to me


CREATE TABLE suseServerSAPWorkload (
id BIGINT CONSTRAINT suse_sap_workload_id_pk PRIMARY KEY GENERATED ALWAYS AS IDENTITY,
system_id VARCHAR(5) NOT NULL,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have any information about the size of these fields? For system_id it looks like it will not be bigger then 5 (the regex in go code expects 3, but what about the instance_type size?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For system_id, indeed it shouldn't exceed 3 characters for now, so the size of 5 will be sufficient. For instance_type, however, I only have some examples from the issue, and based on those, 128 should be enough. Do you think it might need to be larger?

Copy link
Member

@rjmateus rjmateus Jan 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for system_id lookd fine to me. For the instance type why don't you specified just varchar without any limit? Form the docs we have[1]: If specified, the length n must be greater than zero and cannot exceed 10,485,760. If character varying (or varchar) is used without length specifier, the type accepts strings of any length.

And I think in terms of space it will not have any side effect, since the n in the varchar is just limiting the max size (on contrary to the char, which is reserving the defined space)

@aaannz was studying the PostgreSQL type, do you have any input on this one?

[1] https://www.postgresql.org/docs/current/datatype-character.html

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no db space saving by using limited varchar. Performance impact consist of one length check extra.

So the question is only if we want to have some business logic in the database. If we really can't have long ids and the rest of the system could not handle it, so we limit it here.

But if that is the case, we should check for the length before trying insert anyway so we can properly handle that failure instead of relying on insert failure.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What concerns me more is the name of the row. We already have system id with different meaning elsewhere.
This is related to sap, right? Can we name it like sap_system_id or something?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for all the insides @aaannz
I agree with the column name (even it's in the context of the SAP table) we should make it clear that it's a SAP system id, to not get confused with any other ID on our application.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the input. Considering it, I will remove the size limits and rename the column.

@wweellddeerr wweellddeerr force-pushed the scc-telemetry-data-master branch 2 times, most recently from c640cb8 to b0d0cf7 Compare January 8, 2025 18:39
@wweellddeerr wweellddeerr force-pushed the scc-telemetry-data-master branch from b0d0cf7 to 9a863be Compare January 8, 2025 18:47
Copy link
Member

@rjmateus rjmateus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants