Use log_with_downgradable_level for user password configuration warnings

[ani-sinha]

Proposed Commit Message

Introduction of new WARNING level logs could be problematic for stable
downstream distros. Customers using these distros would then see a new and
unexpected behavior change or a new WARNING log that can confuse them. So for
handling user account passwords, use log_with_downgradable_level() helper api
instead so that downstream distros can maintain stability while also making
progressive changes in upstream towards improved user experience.
Downstream distros can convert these logs to DEBUG level by setting
DEPRECATION_INFO_BOUNDARY to a value older than the cloud-init version at which
these logs were first introduced (24.3). Please see the documentation for
log_with_downgradable_level().

Merge type

• Squash merge using "Proposed Commit Message"
• Rebase and merge unique commits. Requires commit messages per-commit each referencing the pull request number (#<PR_NUM>)

[ani-sinha]

@TheRealFalcon what do you think?

[TheRealFalcon]

@ani-sinha

Warning logs tends to alert customers that something is broken

Hmmm, I would say that errors are for alerting that something is broken. In the cases pointed out in this PR, cloud-init was asked to do something that it was not able to. I think that is something worthy of a warning, especially surrounding user passwords. Otherwise the only way somebody would be alerted to this condition is if they go digging through the logs which won't happen unless somebody notices a problem and correctly identifies cloud-init as the cause.

That said, in the case of passwd, I can see the case for downgrading that to INFO, both in that we specifically document that passwd doesn't apply to existing users and to use hashed_passwd instead for that use case, along with the fact that a new instance id or calling of cloud-init clean can wind up triggering this warning without the user having done anything to cause this warning. For the other two though, it seems they can only be triggered if this user is asking for something that doesn't make sense.

May I ask where this request is coming from? Is there a customer on a stable platform that started seeing a return code of 2 after our new warning mechanism was backported? If so, do you think it makes mores sense for the lock_passwd cases to be downstream changes rather than upstream?

[ani-sinha]

May I ask where this request is coming from? Is there a customer on a stable platform that started seeing a return code of 2 after our new warning mechanism was backported?

The request is not yet from a customer but we suspect that once we start rolling this out to the hands of our customers, they will complain and get confused. This has happened before with other logs and the schema validator.

[ani-sinha]

If so, do you think it makes mores sense for the lock_passwd cases to be downstream changes rather than upstream?

If upstream is not willing to change this, we are debating whether to have a downstream only patch or to simply document it and point customers to it if they ask.

[zhaohuijuan]

May I ask where this request is coming from? Is there a customer on a stable platform that started seeing a return code of 2 after our new warning mechanism was backported?

The request is not yet from a customer but we suspect that once we start rolling this out to the hands of our customers, they will complain and get confused. This has happened before with other logs and the schema validator.

Yes, agree with here.
And I think this scenario is a common case(lock_passwd: false, but without password configuration), it means customer would hit it easily, suggest to change the WARNING log to a more friendly info to avoid any customer case or confusion.

[holmanb]

they will complain

That could happen, however the commit that introduced these warnings was released in 24.3 - at the end of August. Nobody has complained about it yet on Ubuntu which has been running this code (backported to old series of Ubuntu) for months.

This has happened before with other logs and the schema validator.

Spurious warnings messages that cropped up for a few reasons: cloud-init automatically accepted multiple key types in some cases that were missed, and in other cases it was just an auditing error when generating the schema. It is unfortunate that this happened, however I think that the bulk of the issues have been resolved - and the rate of new warnings being added has slowed significantly.

In the cases pointed out in this PR, cloud-init was asked to do something that it was not able to. I think that is something worthy of a warning, especially surrounding user passwords. Otherwise the only way somebody would be alerted to this condition is if they go digging through the logs which won't happen unless somebody notices a problem and correctly identifies cloud-init as the cause.
For the other two though, it seems they can only be triggered if this user is asking for something that doesn't make sense.

In the case of a configuration that cloud-init cannot successfully fulfill, I strongly believe that emitting a warning to aid user debugging is better for end users than silently failing. In the cases where cloud-init was asked to do something that it is unable to do, choosing to not log a warning seems to prioritize downstream stability over what I think is best for future users. This isn't a tradeoff that I want to accept in upstream cloud-init.

[holmanb]

There is a compromise that should satisfy downstream stability desires while enabling upstream to make forward progress to improve user experience. This helper function allows logging a warning or error message with an annotated cloud-init version at the callsite. When the cloud-init version set at the callsite is newer than the version set in features.DEPRECATION_INFO_BOUNDARY, the log level is downgraded to DEBUG. Therefore, by adding a simple patch (example) to a downstream release, upstream cloud-init can add warnings to enable users on later series to enjoy a better debugging experience without introducing changes of behavior to old releases of downstreams that prefer stability. An example of using this helper exists in main.py.

Perhaps these warning log sites could be converted to use this helper?

[ani-sinha]

Perhaps these warning log sites could be converted to use this helper?

yes I think for c9s, we can use this simple downstream patch to not cause a disruption/behavior change for our customers. Let me see if I can send some patch.

[TheRealFalcon]

Thanks for the update @ani-sinha . This approach will work.

The changes look good, but 168f821 was recently merged that refactored the args argument to be a tuple. Would you be able to update the args in these calls to be tuples instead?

[ani-sinha]

Thanks for the update @ani-sinha . This approach will work.

The changes look good, but 168f821 was recently merged that refactored the args argument to be a tuple. Would you be able to update the args in these calls to be tuples instead?

Done.

[TheRealFalcon]

LGTM! Thanks!