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.

Sign up for GitHub

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

Jump to bottom

roles: hosted_engine_setup: Add after_add_host hook #181

Merged
merged 7 commits into from
Nov 26, 2020
Merged

roles: hosted_engine_setup: Add after_add_host hook #181

merged 7 commits into from
Nov 26, 2020

Conversation

didib
Copy link
Member

@didib didib commented Nov 17, 2020

Allow running custom playbooks after trying to add the host to the
engine.

Bug-Url: https://bugzilla.redhat.com/1893385

@didib
Copy link
Member Author

didib commented Nov 17, 2020

Didn't verify yet.

@arachmani
Copy link
Member

@didib, The code looks good to me, but I didn't understand why we need to add another place for running hooks if the workaround from https://bugzilla.redhat.com/show_bug.cgi?id=1893385#c0 works.
In case we need this PR, I think It's best to add some explanation also in the HE role's README file, something similar to https://github.com/oVirt/ovirt-ansible-collection/tree/master/roles/hosted_engine_setup#make-changes-in-the-engine-vm-during-the-deployment

@didib
Copy link
Member Author

didib commented Nov 19, 2020

@didib, The code looks good to me, but I didn't understand why we need to add another place for running hooks if the workaround from https://bugzilla.redhat.com/show_bug.cgi?id=1893385#c0 works.

The workaround is not a full solution - after you apply it, you still have to eventually setup the networks as needed and mark them required. And during this, they are not required. With current patch, the user has an option to automatically handle failures after trying to add the host, presumably (I didn't try yet) by assigning relevant NICs to the required networks and trying to activate, therefore never marking them as non-required.

I intend to also add another "wait for lockfile" (similar to he_pause_host, but perhaps mandatory, we'll see), to also allow manually fixing, for people that prefer that.

In case we need this PR, I think It's best to add some explanation also in the HE role's README file, something similar to https://github.com/oVirt/ovirt-ansible-collection/tree/master/roles/hosted_engine_setup#make-changes-in-the-engine-vm-during-the-deployment

Sure.

@didib
Copy link
Member Author

didib commented Nov 19, 2020

I don't like github. It seems like the only way for me to mark this PR as not-ready-for-merge is to change it to a draft. In gerrit I can '-1'. Here, if I try to add a comment and mark "Request Changes", a popup says "Pull request authors can't request changes on their own pull request". Searching for this text in google finds very few other people that care. Github simply managed to dumb-down everyone to not expect more, it seems.

@arachmani
Copy link
Member

I don't like github. It seems like the only way for me to mark this PR as not-ready-for-merge is to change it to a draft. In gerrit I can '-1'. Here, if I try to add a comment and mark "Request Changes", a popup says "Pull request authors can't request changes on their own pull request". Searching for this text in google finds very few other people that care. Github simply managed to dumb-down everyone to not expect more, it seems.

Maybe you can just add [WIP] in the subject line.
And when the PR is ready just remove it.
You can do that easily from the UI.

@didib didib changed the title roles: hosted_engine_setup: Add after_add_host hook [WIP] roles: hosted_engine_setup: Add after_add_host hook Nov 19, 2020
@mwperina mwperina added the WIP Work in progress, not ready to be merged label Nov 19, 2020
@mwperina
Copy link
Member

mwperina commented Nov 19, 2020 •
edited
Loading

I've created a WIP label for such patches, let's agree that not maintainer will merge a patch with WIP label. Is it OK for everyone?

@didib @arachmani @mnecas

@didib
Copy link
Member Author

didib commented Nov 19, 2020

OK for me.

1 similar comment
@arachmani
Copy link
Member

OK for me.

@didib
Copy link
Member Author

didib commented Nov 19, 2020

It seems that only people with write access to the repo can assign labels:

https://docs.github.com/en/free-pro-team@latest/github/managing-your-work-on-github/applying-labels-to-issues-and-pull-requests

(And no, the answer isn't "OK, so we'll give you write access". IMO).

@mwperina
Copy link
Member

Martin, is there some easy way how to have a bot watching for certain string in comments and setting labels accordingly? Something similar to ManageIQ bot https://github.com/ManageIQ/miq_bot where they can use below comment to add a label

@miq-bot add_labels label1

@mnecas

@didib
roles: hosted_engine_setup: Add after_add_host hook
ec3300d 
Allow running custom playbooks after trying to add the host to the
engine.

Bug-Url: https://bugzilla.redhat.com/1893385
@didib
roles: hosted_engine_setup: Add missing space
13228d4 
cut -d' ' also works, but both emacs and vi syntax-highlight this
wrongly, and IMO it's less common. See also e.g.:

https://stackoverflow.com/questions/10818443/short-long-options-with
@didib
roles: hosted_engine_setup: On 'Add host' failures, collect also warn…
0e29bf8 
…ing events

If we failed due to a missing required network, this will be logged as a
warning, not error. Collect that as well.

Bug-Url: https://bugzilla.redhat.com/1893385
@didib
Copy link
Member Author

didib commented Nov 23, 2020

ci test please

@didib
Copy link
Member Author

didib commented Nov 24, 2020

Verified both manual (let it fail adding, prompt, add via web ui and rm lock file) and automatic (using the example hook).

@didib didib changed the title [WIP] roles: hosted_engine_setup: Add after_add_host hook roles: hosted_engine_setup: Add after_add_host hook Nov 24, 2020
@marchukov
Copy link

ci please test

@didib
roles: hosted_engine_setup: Pause if Add host failed
690e1e3
@didib
roles: hosted_engine_setup: Fix error message collection
7f8a867 
E.g., this broke:

        {
            "code": 10812,
            "custom_id": -1,
            "data_center": {
                "href":
"/ovirt-engine/api/datacenters/71466c1a-29ab-11eb-858c-00163e2ccc71",
                "id": "71466c1a-29ab-11eb-858c-00163e2ccc71",
                "name": "Default"
            },
            "description": "Data Center Default compatibility version is
4.3, which is lower than latest available version 4.5. Please upgrade
your Data Center to latest version to successfu
lly finish upgrade of your setup.",
            "flood_rate": 0,
            "href": "/ovirt-engine/api/events/160",
            "id": "160",
            "index": 160,
            "origin": "oVirt",
            "severity": "alert",
            "time": "2020-11-23 16:20:14.437000+02:00"
        },

With:

The task includes an option with an undefined variable. The error was:
'dict object' has no attribute 'host'

Because it's an event about a Data Center, so has no 'host'.

Similarly, this failed with no 'id':

        {
            "code": 986,
            "correlation_id": "35d151aa",
            "custom_id": -1,
            "data_center": {
                "href":
"/ovirt-engine/api/datacenters/71466c1a-29ab-11eb-858c-00163e2ccc71",
                "id": "71466c1a-29ab-11eb-858c-00163e2ccc71",
                "name": "Default"
            },
            "description": "Data Center is being initialized, please
wait for initialization to complete.",
            "flood_rate": 0,
            "host": {
                "name": "Unavailable"
            },
            "href": "/ovirt-engine/api/events/46",
            "id": "46",
            "index": 46,
            "origin": "oVirt",
            "severity": "warning",
            "time": "2020-11-18 16:54:54.407000+02:00",
            "user": {
                "name": "SYSTEM"
            }
        },
@didib
roles: hosted_engine_setup: Fix typo
e1a0a50 
availible -> available
@didib
roles: hosted_engine_setup: Add required_networks_fix.yml example
2d8a88f
@arachmani arachmani removed the WIP Work in progress, not ready to be merged label Nov 26, 2020
arachmani
arachmani approved these changes Nov 26, 2020
View reviewed changes
Copy link
Member

@arachmani arachmani left a comment

Choose a reason for hiding this comment

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

LGTM

@didib
Copy link
Member Author

didib commented Nov 26, 2020

Anything else? I think it's ready for merge. I don't mind squashing and/or splitting to separate PRs and opening bugs for some things, if you want (e.g. fixing error/warning collection).

@arachmani arachmani merged commit 480262b into oVirt:master Nov 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arachmani arachmani arachmani approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@didib @arachmani @mwperina @marchukov