Fix case when switching license from any not empty non SLES_BYOS to SLES_BYOS without cache file

[jesusbv]

When switching from SLES to BYOS and no cache file exists, the license changes to BYOS and no other action is done, instance/system is still registered

This change fixes that scenario

[smarlowucf]

I don't know the code enough to give a good review here. Also, is it intended to be s/SLES to BYOS/PAYG to BYOS?

[jesusbv]

Also, is it intended to be s/SLES to BYOS/PAYG to BYOS?

Well, it is a license change from SLES to SLES_BYOS
I will edit the title of the PR to clarify it

[jgleissner]

I'm trying to understand what the license cache is. So the pre-PR version simply updates the cache when the file is missing, which is kinda what you expect when it's trying to access a non-existent cache. But it didn't return anything in that case, from a function that is clearly supposed to, so that seems off. Plus it sets the license type to what it's supposed to compare it with, which also seems off.

It seems to me that the license cache isn't actually a cache (== stored data that is time-consuming to compute or retrieve) but actually the registration information (I vaguely recall we've had this conversation before). So that would mean, missing cache means it's assumed the system is not registered?

[jesusbv]

I'm trying to understand what the license cache is. So the pre-PR version simply updates the cache when the file is missing, which is kinda what you expect when it's trying to access a non-existent cache. But it didn't return anything in that case, from a function that is clearly supposed to, so that seems off. Plus it sets the license type to what it's supposed to compare it with, which also seems off.

So, pre-PR, the cache gets checked and if the file didn't exist, it gets created and the check method returned None
meaning license has not changed, whenever the check returned True cache file gets updated outside the check

This leads to an scenario where switching from SLES (or any PAYG license) to SLES_BYOS and no cache file, the instance is registered against the infrastructure and benefiting from that while being BYOS

It seems to me that the license cache isn't actually a cache (== stored data that is time-consuming to compute or retrieve) but actually the registration information (I vaguely recall we've had this conversation before).

I'm not sure I understand, the cache file stores the license value only

So that would mean, missing cache means it's assumed the system is not registered?

Missing the cache or not missing the cache does not affect registration per se, it affects registration when switching licenses. For example, if the instance is registered and license switched to SLES_BYOS (and cache had SLES for the sake of the example) that check and code would take care of de-registering the instance

Another approach to fix this issue is to check for BYOS license in the license check and de-register + disable the service

except FileNotFoundError:
   if 'BYOS' in license_type:
       run_command(['registercloudguest', '--clean'])
       run_command(['systemctl', 'disable', 'guestregister'])
   update_license_cache(license_type)
   # Notice we would keep returning None

This would put some responsibility into has_license_changed which IMHO does not belong
but we may be open to that trade-off to fix the scenario of "BYOS benefiting from infra", again
this would add a side-effect into the check license code where one would expect the method to
check the license or do things license related, not to do things like de-registering the instance for example

[jgleissner]

A function called has_license_changed should not return None, but only True, False, or throw an exception if it can't tell.

Generally how I would expect it to work is to populate the cache when trying to access data in the cache that is not there. So the question is, can it figure out the current license type from the active registration (not by re-registering)? If not, it's not actually a cache.

[jgleissner]

LGTM. I'd still suggest moving the stored license type into /var/lib and not calling it cache, as it really persists the last known license type and rather than caching metadata, which got me confused in the first place.

[rjschwei]

What are the actions taken by the user here? I do not understand this state transition in the context of this code. When a BYOS instance is registered, either to SCC or the update infrastructure then in order to transition from the registered state to the unregistered state BYOS-2 to BYOS-1 the user has to run registercloudguest --clean meaning this code when it runs again will find a cache and no registration.

[jesusbv]

True that the user has to run registercloudguest --clean command, but the code would check if license is BYOS,
which it would, it would check if it uses RMT as SCC proxy, which it wouldn't, then re-run the same command and
run the command to disable guestregister, no cache would be found on that scenario

If it did run again, after that scenario, yes there would be a cache saying SLES_BYOS on an unregistered system

[rjschwei]

This condition is tested twice in consecutive conditionals, on line 138 and then again here. Can be changed to (starting at line 137)

if has_license_changed(license_type):
    update_license_cache(license_type)
    if update_server and utils.is_registered(update_server.get_FQDN()):
        run_command(['registercloudguest', '--clean'])
        run_command(['systemctl', 'disable', service_name])

to avoid checking the same condition in consecutive instructions and make the flow a little easier to follow.

[jesusbv]

We should not do that, the de-registration must happen even if license hasnt changed
I will think a better way to check the conditions

[jesusbv]

I think you meant outside the if of the license change

if has_license_changed(license_type):
    update_license_cache(license_type)
if update_server and utils.is_registered(update_server.get_FQDN()):
    run_command(['registercloudguest', '--clean'])
    run_command(['systemctl', 'disable', service_name])

[rjschwei]

We need to consider the a BYOS-3 case where BYOS-3 is defined as registered to SCC and we need to handle that transition for example

BYOS-3 PAYG No dergister from SCC register with update infrastructure.

is one of the state transitions we need to handle.

It may also be a good idea to number the state transitions and then in the implementation reference the numbers the various if conditions handle.

Let's focus on getting the state transitions captured and then take another look at the implementation.

[rjschwei]

We need another iteration on the state transition table. Thanks for your patience.

[rjschwei]

Case 5 & 6 collapse to cases 3 and 4 because the customer cannot change the licence via AHB to a registered state. The action is always

• Change license to BYOS, this is always BYOS-1 to BYOS-1
• Register, this would then result in BYOS-2 or BYOS-3

Given the table we have this is hard to express. At the same time adding more columns such as "Additional user action" is not conducive to making it more readable. I suggest we make entries 3 & 4 look like this:

BYOS-1 BYOS-1 (BYOS_2, BYOS-3)* .....

and the add a footnote like so

* The user has manually registered he instance with their registration key

[rjschwei]

If the instance is switched to PAYG and the cache is not PAYG we always have to re-register.

[rjschwei]

But the customer cannot switch to an unregistered state via AHB. So the condition expressed here really is

• Customer ran registercloudguest --clean
• Customer used AHB to explicitly set the BYOS license

As such there is nothing to do. If the starting state is BYOS, any form not registered or registered to the update infra or SCC and the cache says BYOS we have nothing to do. If there is no cache we also do nothing because we cannot do anyting.

[rjschwei]

Here too, the customer cannot change the license state to BYOS-2 or BYOS-3. Since we have trouble with this I think the column should be renamed from LICENSE CHANGED TO to Reg State at runtime this would indicate that we start with a given state and then detect the state during runtime of the code. And the state is set by the user as a combination of running AHB commands and registration commands. This should get us out of the quandary we are in. Let's try that and see what that looks like.