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

Registration check unregisters when it is not connected #3540

Merged
merged 3 commits into from
Nov 10, 2022
Merged

Registration check unregisters when it is not connected #3540

merged 3 commits into from
Nov 10, 2022

Conversation

ahitacat
Copy link
Contributor

@ahitacat ahitacat commented Oct 5, 2022

Signed-off-by: ahitacat [email protected]

All Pull Requests:

Check all that apply:

  • Have you followed the guidelines in our Contributing document, including the instructions about commit messages?
  • Is this PR to correct an issue?
  • Is this PR an enhancement?

Complete Description of Additions/Changes:

Insights-client is in a faulty state when the system is unregistered from rhsm and it is configured with legacy_upload=True. With no legacy option set, insights-client unregisters the system when it receives a response with a status greater than 400, that means rhsm could not be reached for many reasons. With legacy option it doesn't registers the system. This makes inconsistent the behavior of insights-client when checking the registration status.

Actual results:

> sudo insights-client --status
System is registered locally via .registered file. Registered at 2022-08-31T12:29:02.150601
Insights API says this machine is NOT registered.

Results with legacy_upload=False

 >sudo insights-client --status
This host is registered.

Fix results:
With legacy_upload=True
registration_check() method will only unregisters the system locally if API tells is not registered.

> sudo insights-client --status
System is registered locally via .registered file. Registered at 2022-10-06T08:04:15.668125
Insights API says this machine is NOT registered.
System unregistered locally via .unregistered file

Results with legacy_upload=False
registration_check() method will only unregisters the system locally if API tells is not registered, not every time the API could not be reached.

 >sudo insights-client --status
This host is registered.

Resolves: rhbz#2123776

@ahitacat ahitacat added the client These issues represent work to be done by the "client" team. label Oct 5, 2022
@ahitacat ahitacat requested a review from Glutexo October 5, 2022 13:25
@ahitacat ahitacat self-assigned this Oct 5, 2022
@ahitacat ahitacat requested a review from patchkez October 5, 2022 16:37
@ahitacat ahitacat marked this pull request as ready for review October 11, 2022 10:04
Copy link
Collaborator

@Glutexo Glutexo left a comment

Choose a reason for hiding this comment

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

The if-else statements are not very clear, but that’s the legacy of the already convoluted code. I will revisit this more thoroughly to reduce the amount of technical debt. But I don’t want to block the pull request any longer.

@xiangce xiangce merged commit c26e980 into RedHatInsights:master Nov 10, 2022
xiangce pushed a commit that referenced this pull request Nov 10, 2022
* registration_check method unregisters insights-client when it is not connected

Signed-off-by: ahitacat <[email protected]>

* Added tests to check the fix

Signed-off-by: ahitacat <[email protected]>

Signed-off-by: ahitacat <[email protected]>
Co-authored-by: Glutexo <[email protected]>
(cherry picked from commit c26e980)
xiangce pushed a commit that referenced this pull request Sep 6, 2024
* registration_check method unregisters insights-client when it is not connected

Signed-off-by: ahitacat <[email protected]>

* Added tests to check the fix

Signed-off-by: ahitacat <[email protected]>

Signed-off-by: ahitacat <[email protected]>
Co-authored-by: Glutexo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client These issues represent work to be done by the "client" team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants