-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Fix infra_node deployment #1326
Conversation
aos-ci-test |
I'm expecting this test to fail, found some issues with the role reordering I did |
vars: | ||
attach_registry_volume: "{{ openshift.hosted.registry.storage.kind != None }}" | ||
deploy_infra: "{{ openshift.master.infra_nodes | default(0) | length > 0 }}" | ||
nfs_host: "{{ groups.oo_nfs_to_config.0 }}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This var shouldn't be used anymore.
@brenton 👍 |
- name: Grant the user access to the privileged scc | ||
shell: > | ||
{{ openshift.common.admin_binary }} policy add-scc-to-user | ||
privileged system:serviceaccount:default:{{ item.item.item }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wow, the with_items nesting is a little hard to read. Does ansible have any other with_
looping mechanisms that could make this more obvious? The item.item.item
part just looks really odd.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
heh, basically it's because I'm chaining the output to be dependent on the output of the previous command.
It would be nice if you could do alias the variables created when using with_items.
The alternative is to just loop on the first output, which would still work, but if someone later tried to refactor and switched the ordering of the tasks it would break.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not totally following. Wouldn't things still break today if the order of the tasks were switched? Or are you saying that if they switched the ordering now they would get an error about the registered variable not being declared so that they would get a playbook compilation error instead of a runtime error with oadm?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about this a bit more... Since it will still error out, I don't see too much harm in chaining both off of the user check..
I'll be happy when we can make the jump to Ansible 2.0 and can put this type of dependency chain in a block so that we can more clearly express that they are indeed a chain of events that need to be done sequentially.
aos-ci-test |
- Do not deploy the router/registry when the infra_nodes variable is present but does not contain a list of infra nodes. - use right node group and only set openshift_infra_nodes if group is present
- make service account creation more flexible - create service accounts near where they are consumed
…e openshift_facts without declaring a dependency
aos-ci-test |
aos-ci-test retrying due to node registration flake |
aos-ci-test |
@abutcher ptal |
hosts: oo_first_master | ||
vars: | ||
attach_registry_volume: "{{ openshift.hosted.registry.storage.kind != None }}" | ||
deploy_infra: "{{ openshift.master.infra_nodes | default(0) | length > 0 }}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be default([])
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed.
aos-ci-test |
This breaks 3.0 installs because |
but does not contain a list of infra nodes.
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1303939