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

add system uptime capability #210

Merged
merged 2 commits into from
May 16, 2024
Merged

add system uptime capability #210

merged 2 commits into from
May 16, 2024

Conversation

guangyee
Copy link
Collaborator

@guangyee guangyee commented Mar 6, 2024

Add the ability to upload the system uptime logs, produced by the suse-uptime-tracker daemon, to SCC/RMT as part of daily heartbeat report. We need to ability to track system uptime to hourly granularity so MSPs can utility that data accordingly (i.e. billing).

This feature is optionally enabled by installing the suse-uptime-tracker package and enabling the suse-uptime-tracker.timer service.

@guangyee guangyee requested a review from digitaltom March 6, 2024 17:10
@digitaltom
Copy link
Member

@guangyee don't we need a config option in suseconnect to enable the uptime collection? What's the planned way for customers to turn the collection on/off?

internal/connect/api.go Outdated Show resolved Hide resolved
internal/connect/api.go Show resolved Hide resolved
internal/connect/api.go Outdated Show resolved Hide resolved
internal/connect/api.go Outdated Show resolved Hide resolved
internal/connect/api.go Outdated Show resolved Hide resolved
internal/connect/api_test.go Outdated Show resolved Hide resolved
internal/connect/api_test.go Outdated Show resolved Hide resolved
@guangyee
Copy link
Collaborator Author

guangyee commented Mar 7, 2024

@guangyee don't we need a config option in suseconnect to enable the uptime collection? What's the planned way for customers to turn the collection on/off?

Per my understanding, in the short term, by installing and enabling the uptime-tracker service constitutes an opt-in to uptime collection. We'll document it as such. However, in the long run, we will consider the configuration aspect via REST API.

@digitaltom
Copy link
Member

Per my understanding, in the short term, by installing and enabling the uptime-tracker service constitutes an opt-in to uptime collection. We'll document it as such. However, in the long run, we will consider the configuration aspect via REST API.

I don't think it's a good idea to assume customer agreement on collecting system uptime data by presence of a specific package. The package could already be installed on the image, or get pulled in unintentionally.
Having it as an option in the suseconnect config also creates the base for receiving that setting from the server, and setting it intentionally in provisioning or manually.
The target audience for metering are MSP systems. It's technically possible to enable it for 'normal' customers, but it shouldn't be the default and should be an informed decision to enable it for the customer.

@guangyee
Copy link
Collaborator Author

Per my understanding, in the short term, by installing and enabling the uptime-tracker service constitutes an opt-in to uptime collection. We'll document it as such. However, in the long run, we will consider the configuration aspect via REST API.

I don't think it's a good idea to assume customer agreement on collecting system uptime data by presence of a specific package. The package could already be installed on the image, or get pulled in unintentionally. Having it as an option in the suseconnect config also creates the base for receiving that setting from the server, and setting it intentionally in provisioning or manually. The target audience for metering are MSP systems. It's technically possible to enable it for 'normal' customers, but it shouldn't be the default and should be an informed decision to enable it for the customer.

Sorry I wasn't clear earlier. It's not just the presence of the uptime-tracker package. Customer will also need to explicitly enable the uptime-tracker.timer service, which is similar to the keepalive heartbeat timer, in order to enable system uptime tracking. If that's not good enough, then I can also add a flag in SUSEConnect to enable the sending of system uptime log itself. But the drawback would be the increase in complexity in enabling the feature. Please let me know if that the direction we want to go.

@digitaltom
Copy link
Member

Please let me know if that the direction we want to go.

I think that needs to get decided in the project planning regarding what is the desired UX enabling that feature.
I would tend to having an option in the suseconnect config because that is very explicit.

@jeremy-moffitt
Copy link

Please let me know if that the direction we want to go.

I think that needs to get decided in the project planning regarding what is the desired UX enabling that feature. I would tend to having an option in the suseconnect config because that is very explicit.

my understanding is the feedback is to have configurations for both connect-ng and the uptime tracker... I'm not sure why we want to require the customer to enable it twice, in addition to having the required packages and services configured. If we put a config option in suseconnect-ng, then for the tracker service itself, the presence of the package and the service running should be sufficient... That would look something like:

  1. suseconnect has an explicit config
  2. the uptime tracker is considered enabled if the service is running and does not additionally have a config

@guangyee
Copy link
Collaborator Author

@digitaltom @jeremy-moffitt I will add a flag enable_system_uptime_tracking to control the sending of the system uptime tracking log.

@guangyee guangyee force-pushed the uptime_tracking_for_msps branch 3 times, most recently from f56e490 to a7548bb Compare March 13, 2024 16:12
@ngetahun ngetahun self-requested a review March 18, 2024 14:50
ngetahun
ngetahun previously approved these changes Mar 18, 2024
@ngetahun ngetahun dismissed their stale review March 18, 2024 15:35

CLI flag missing

Copy link
Contributor

@ngetahun ngetahun left a comment

Choose a reason for hiding this comment

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

@guangyee enable_system_uptime_tracking is not shown as a cli option because it's missing from flag parsing logic here. Please add it so I can manually test with the uptime_tracker package.

@guangyee
Copy link
Collaborator Author

@guangyee enable_system_uptime_tracking is not shown as a cli option because it's missing from flag parsing logic here. Please add it so I can manually test with the uptime_tracker package.

Per my understanding, we will not be exposing this option in CLI as it is meant to control whether keepalive heartbeat will send the system uptime log. Sending system uptime log is only meaningful when working in conjunction with keepalive.

@felixsch
Copy link
Contributor

Per my understanding, we will not be exposing this option in CLI as it is meant to control whether keepalive heartbeat will send the system uptime log. Sending system uptime log is only meaningful when working in conjunction with keepalive.

I agree on this. We spoke about this in one of the MSP syncs and all agreed there is no need for a CLI option. Maybe you can provide test instructions how to test this pull request?

Copy link
Contributor

@ngetahun ngetahun left a comment

Choose a reason for hiding this comment

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

@guangyee this PR is good to merge but need to do 2 things:

  1. Rebase/merge from/unto main
  2. add an entry into changelog for release 1.9.0 (suseconnect-ng.changes)

@guangyee guangyee force-pushed the uptime_tracking_for_msps branch from a7548bb to 75fdd55 Compare April 16, 2024 04:55
@guangyee
Copy link
Collaborator Author

@guangyee this PR is good to merge but need to do 2 things:

  1. Rebase/merge from/unto main
  2. add an entry into changelog for release 1.9.0 (suseconnect-ng.changes)

Done. Thanks for the suggestion.

@guangyee
Copy link
Collaborator Author

Per my understanding, we will not be exposing this option in CLI as it is meant to control whether keepalive heartbeat will send the system uptime log. Sending system uptime log is only meaningful when working in conjunction with keepalive.

I agree on this. We spoke about this in one of the MSP syncs and all agreed there is no need for a CLI option. Maybe you can provide test instructions how to test this pull request?

We have created some preliminary instructions for the MSPs on how to enable the feature. We can use that for our testing. https://confluence.suse.com/display/CloudSolutions/White+paper+documentation

@guangyee guangyee force-pushed the uptime_tracking_for_msps branch from 6ed6e5c to 35f6eee Compare April 16, 2024 16:06
build/packaging/suseconnect-ng.changes Outdated Show resolved Hide resolved
@guangyee guangyee force-pushed the uptime_tracking_for_msps branch from 35f6eee to dc8d503 Compare April 17, 2024 16:05
@chajain
Copy link
Collaborator

chajain commented Apr 25, 2024

Performed the end to end testing along with SCC latest PR https://github.com/SUSE/happy-customer/pull/7339 and RMT PR SUSE/rmt#1091 and verified following scenarios

  • Downloaded the latest suse-uptime-tracker rpm package from OBS and started the suse-uptime-tracker.timer service and the /etc/zypp/system-uptime.log is populated with the system_uptime data
  • When enable_system_uptime_tracking flag set to true, the payload contains the online_at params and system_uptimes table is populated on SCC/RMT when the keepalive hearbeat is sent

Also Validated backward and forward compatibility tests

  • older version of connect-ng still works with latest SCC/RMT PR
  • upgraded connect-ng(this PR) talking to an older version of SCC/RMT which does not recognize system_uptime and it does not crash on the extra payload

@guangyee guangyee force-pushed the uptime_tracking_for_msps branch from 774f254 to 33e9720 Compare April 29, 2024 16:13
@guangyee guangyee force-pushed the uptime_tracking_for_msps branch from 33e9720 to f4e9550 Compare April 29, 2024 23:34
@guangyee guangyee force-pushed the uptime_tracking_for_msps branch from f71755f to c82e931 Compare May 7, 2024 16:05
@guangyee guangyee requested a review from felixsch May 7, 2024 16:52
@mssola mssola self-assigned this May 14, 2024
internal/connect/api.go Show resolved Hide resolved
internal/connect/api.go Show resolved Hide resolved
internal/connect/api_test.go Outdated Show resolved Hide resolved
internal/connect/api_test.go Outdated Show resolved Hide resolved
SUSEConnect.example Outdated Show resolved Hide resolved
build/packaging/suseconnect-ng.changes Outdated Show resolved Hide resolved
internal/connect/api.go Outdated Show resolved Hide resolved
@guangyee guangyee force-pushed the uptime_tracking_for_msps branch from 90efcc6 to 978fd8d Compare May 14, 2024 19:24
guangyee added 2 commits May 14, 2024 12:46
Add the ability to upload the system uptime logs, produced by the suse-uptime-tracker daemon, to SCC/RMT as part of daily heartbeat report. We need to ability to track system uptime to hourly granularity so MSPs can utility that data accordingly (i.e. billing).

This feature is optionally enabled by installing the suse-uptime-tracker package, enabling the suse-uptime-tracker.timer service, and setting the `enable_system_uptime_tracking` flag to `true` in
`/etc/SUSEConnect` configuration file.
Add the ability to upload the system uptime logs, produced by the suse-uptime-tracker daemon, to SCC/RMT as part of daily heartbeat report. We need to ability to track system uptime to hourly granularity so MSPs can utility that data accordingly (i.e. billing).

This feature is optionally enabled by installing the suse-uptime-tracker package, enabling the suse-uptime-tracker.timer service, and setting the `enable_system_uptime_tracking` flag to `true` in
`/etc/SUSEConnect` configuration file.
@guangyee guangyee force-pushed the uptime_tracking_for_msps branch from 978fd8d to b71f3d2 Compare May 14, 2024 19:47
Copy link
Contributor

@mssola mssola left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

@mssola mssola merged commit d08eb29 into main May 16, 2024
2 checks passed
@mssola mssola deleted the uptime_tracking_for_msps branch May 16, 2024 08:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants