Add configuration_script_sources.last_update_error

[Glutexo]

Add new last_update_stdout column to configuration_script_sources table. This allows to store last update stdout to display more information about why its refresh failed.

Fetching and presenting this information is a workaround for cloning repositories from HTTPS servers with untrusted SSL certificates. Tower API does not offer an option to disable SSL check, so with the stdout user can at least see, why the cloning actually failed.

https://bugzilla.redhat.com/show_bug.cgi?id=1513616

Prior merging this PR, folowing PRs require to be merged:

• Remove ansible_tower_client version dependency #17515 Gemfile dependency
• Add configuration_script_sources.last_update_error manageiq-schema#187 migration
• [WIP] Release v0.14.0 ansible/ansible_tower_client_ruby#103 version release

/cc @jameswnl for review

[Glutexo]

ansible/ansible_tower_client_ruby v0.14.0 declared as Gemfile dependency in this PR has been released. Currently only the schema update dependency remains for this PR to become mergeable.

[jameswnl]

we can remove this block because of ManageIQ/manageiq-providers-ansible_tower#84 and the manageiq-providers-ansible_tower is already a dependency.

[Glutexo]

I extracted that to a separate PR. Please note my questions there.

[jameswnl]

Remove the dependency of ansible_tower_client

[Glutexo]

Rebased on current master. Extracted the Gemfile edit to a new PR as it is more related to ManageIQ/manageiq-providers-ansible_tower#84 than it is a part of the BZ.

[Glutexo]

@miq-bot add_label pending core

[miq-bot]

@Glutexo Cannot apply the following label because they are not recognized: pending core

[Glutexo]

@Fryguy Hey, I’d like to get this PR merged. Prior to that, ManageIQ/manageiq-schema#187 must be merged first. May I ask you to have a look?

[Fryguy]

@bdunne Can you review here? The PR is really straightforward as is the migration, but I'm not sure about the naming. last_update_stdout is just confusing to me. The last_update_ bit implies it's going to be a date, but the migration implies the entirety of the stdout is going to be stored. I don't think the last_update_ bit on the name is important (unless it were a separate datetime column that showed "when" the output was last updated), leaving stdout, but generically the source of the output is irrelevant. Given that, I think the column name should just be output or result or something...maaaaaybe with a separate last_update_output or last_update_result datetime column, so you can present the freshness date of the output. Thoughts?

[bdunne]

Ok, I see the confusion. We told Ansible Tower to update (git pull) a repo (ConfigurationScriptSource) and something went wrong (bad credentials / bad url / git server down / etc.). In Tower, that action is implemented as a playbook. So, the idea was to get the standard out of the playbook as part of our provider refresh so that we can display it to ManageIQ users when something goes wrong.

I also don't like the name last_update_stdout because it's too Ansible Tower implementation detail specific.

[bdunne]

Maybe source_sync_details?

[bdunne]

@Glutexo How much data is this? I know playbook output can get large, should we use a BinaryBlob?

[Fryguy]

I see now. I think it would be best to do it in a similar matter to how we store the last error for things on the ext_management_system. For example, an ems refresh occurs and if anything fails in the middle, we set last_refresh_error and last_refresh_date, but when we are successful we set that last_refresh_error to nil. In this way, the UI knows when it's good or bad and can show icons or whatever accordingly.

I do understand that this is slightly different in that in the "return" from the ansible playbook is just some stdout. However, do we also know if it was a success/failure case? If so, then I would name these possibly last_sync_error and last_sync_date, and on failure populate with the error, and on success populate last_sync_error column with nil.

[jameswnl]

@Fryguy It already has a status which is storing the update's status coming from the Tower. It is normally 'success' or 'failure'.

[jameswnl]

@bdunne It only store when there's a failure in update and the logs I checked are all <2KB. Also configuration_script_source normally would not come in great number.

[Glutexo]

@Fryguy @jameswnl @bdunne Actually we store the stdout every time, even on a success. Just as we store the status and a timestamp.

As @jameswnl said, they are usually about 2 KB of size, success results are smaller than failed ones. The stdout is actually text, not binary data, so I see TEXT field more appropriate than BINARY BLOB. TEXT fields are limited to about 1 GB, which I guess is enough here.

As for the column name, I am not sure. The new column last_update_stdout is updated at the same time as last_update_on and status from the same source. In Tower, both last update timestamp and status propagate to the project itself, the stdout is accessible only by getting its own separate resource. I have no objections to use details or result instead of stdout. We are still using last_update_on for the timestamp and on the other hand status with no prefix though. Renaming the new column to either last_sync or source_sync would thus increase the naming entropy.

[Glutexo]

Guys, @Fryguy, @bdunne, @jameswnl, may I ask you for your opinion?

As I stated in my previous comment, there is already a naming discrepancy in the table. By introducing a new last_update_stdout column, the entropy is not increased.

Questioning the stdout part is more valid in my eyes. I agree that result might be a good option. Maybe output would work too, I used this term in the UI. https://github.com/ManageIQ/manageiq-ui-classic/pull/3762/files#diff-9b524579bdd0b54c65c5c3c0c778d2efR106

[Fryguy]

The stdout is actually text, not binary data, so I see TEXT field more appropriate than BINARY BLOB

What @bdunne is referring to is our binary_blobs model, not a literal BINARY BLOB type. The binary_blobs model is designed to take large strings, chop it up into 1MB chunks and store them in a separate table. While named binary_blobs, it can store anything. Putting them into a separate table also has the advantage of allowing select * on the parent table without accidentally returning giant payloads.

That being said, I'm fine with storing the stdout in the column directly since the 99% case is reasonably sized. I would however ensure that we truncate to some large, but reasonable, size, like 100KB or 50KB or something, just in case.

This allows to store last update stdout to display more information about why its refresh failed.
-----------
Actually we store the stdout every time, even on a success.

I'm arguing we should not store the stdout on success, because it's useless. The purpose of storing stdout is to present failure strings when there is a failure. Since we know the status, then on failure, we should store the error in last_update_error, and on success, set last_update_error to nil. This matches how we store similar information on the EMS with respect to the last EMS Refresh.

If it wasn't clear there, we should name the column last_update_error. I would argue for sync over update as the middle word, but since last_update_on already exists, I'd rather latch on to the existing name. update was an unfortunate choice previously because it's so general, and is confused with updated_on, but since it already exists, I agree on keeping that part. In fact, adding another column named last_update_error should end up giving more clarity to last_update_on.

[Glutexo]

Great! That means that I can rename the column and these simple PRs can get merged:

• Add configuration_script_sources.last_update_error #17290
• Add configuration_script_sources.last_update_error manageiq-schema#187

Renaming those last_update_* columns to last_sync_*, possibly along with the status column, can be done in a separate issue. Saving the stdout only on error and truncating it if it’s too big belongs to ManageIQ/manageiq-providers-ansible_tower#72.

Also thanks for the clarification of binary blobs!

[Glutexo]

@Fryguy Renamed to last_update_error. Ready to merge?

[miq-bot]

Checked commit Glutexo@6335d6e with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🍰

[tumido]

Only the migration PR ManageIQ/manageiq-schema#187 seems to be pending to get merged now. We should be ready to move on with this.

@miq-bot remove_label dependencies
@miq-bot add_label enhancement, providers/ansible_tower