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

Removing openshift_repo dependencies. Moving to beginning of openshift_cluster. #4770

Merged
merged 1 commit into from
Aug 15, 2017
Merged

Removing openshift_repo dependencies. Moving to beginning of openshift_cluster. #4770

merged 1 commit into from
Aug 15, 2017

Conversation

kwoodson
Copy link
Contributor

No description provided.

@kwoodson kwoodson requested a review from mtnbikenc July 17, 2017 00:57
@kwoodson kwoodson self-assigned this Jul 17, 2017
@kwoodson
Copy link
Contributor Author

@mtnbikenc, please take a look at this one. I am not as sure as to where to include openshift_repo but I chose the config.yml. Please let me know if you think this isn't a good place for this. I did complete a successful cluster install using this commit.

With these changes I was able to shave 1:28 off of an install. (37:51 down to 36:23). This is only about a 4.8% improvement with my 8 node cluster.

@mtnbikenc
Copy link
Member

This change reduces the calls to openshift_repos during byo/config.yml from 41 to 1. With the other PR moving openshift_sanitize_inventory up, it would be a candidate to remove it from the openshift_repos dependencies. I will evaluate the other roles where openshift_repos has been removed and see if it affects any other execution paths for other entry point playbooks.

@kwoodson
Copy link
Contributor Author

@mtnbikenc,

With the other PR moving openshift_sanitize_inventory up, it would be a candidate to remove it from the openshift_repos dependencies.

Agreed. Its going to get more difficult the longer we wait on merging other PRs into master

I will evaluate the other roles where openshift_repos has been removed and see if it affects any other execution paths for other entry point playbooks.

Thanks for doing this. I thought that the std_include.yml might be a good place for this if config.yml isn't the right place. Thoughts?

I was going to take a bit deeper dive today at this but figured your graphing tools would probably be much faster. Maybe I need a refresher on the graphing tools and I can attempt to assist with additions to that code. Let me know if you want to sync up via video conference or IRC.

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 4, 2017
@kwoodson kwoodson force-pushed the openshift_repos_refactor branch from 2464804 to a2976e7 Compare August 8, 2017 14:28
@kwoodson
Copy link
Contributor Author

kwoodson commented Aug 8, 2017

@mtnbikenc, I'm moving the openshift_repos into the std_include.yml. This moves this role to the front initial setup of repositories since openshift_version requires repositories to be laid down.

I know we don't want std_include.yml expanded but the openshift_verion role depended on openshift_repos before so this shouldn't be any worse than before for std_include.yml. Now that the repositories have been initialized in the beginning we can remove their dependencies which is what the majority of this PR incorporates.

@kwoodson
Copy link
Contributor Author

kwoodson commented Aug 8, 2017

[test]

@sdodson
Copy link
Member

sdodson commented Aug 8, 2017

travis errors and needs aos ci test once those are cleared

@kwoodson kwoodson force-pushed the openshift_repos_refactor branch from a2976e7 to 6a46233 Compare August 8, 2017 14:42
@kwoodson
Copy link
Contributor Author

kwoodson commented Aug 8, 2017

aos-ci-test

@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 8, 2017
@openshift-bot
Copy link

error: aos-ci-jenkins/OS_3.6_NOT_containerized for 6a46233 (logs)

@openshift-bot
Copy link

error: aos-ci-jenkins/OS_3.6_containerized for 6a46233 (logs)

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 8, 2017
@kwoodson kwoodson force-pushed the openshift_repos_refactor branch from 6a46233 to f88a203 Compare August 10, 2017 02:09
@kwoodson
Copy link
Contributor Author

Rebased with master.

@kwoodson
Copy link
Contributor Author

aos-ci-test

@openshift-bot
Copy link

error: aos-ci-jenkins/OS_3.6_NOT_containerized for f88a203 (logs)

@openshift-bot
Copy link

error: aos-ci-jenkins/OS_3.6_containerized for f88a203 (logs)

@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 10, 2017
mtnbikenc
mtnbikenc requested changes Aug 11, 2017
View reviewed changes
Copy link
Member

@mtnbikenc mtnbikenc left a comment

Choose a reason for hiding this comment

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

Needs Travis CI Ansible syntax errors fixed. It appears initialize_openshift_repos.yml was not included in the commit.

@kwoodson kwoodson force-pushed the openshift_repos_refactor branch from f88a203 to 1fd398a Compare August 11, 2017 13:42
@kwoodson
Copy link
Contributor Author

@mtnbikenc, good catch. I rebased and added the init repos file.

@kwoodson kwoodson force-pushed the openshift_repos_refactor branch from ed3ef68 to b376f0c Compare August 11, 2017 13:55
@kwoodson
Copy link
Contributor Author

aos-ci-test

mtnbikenc
mtnbikenc approved these changes Aug 11, 2017
View reviewed changes
Copy link
Member

@mtnbikenc mtnbikenc left a comment

Choose a reason for hiding this comment

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

LGTM

@openshift-bot
Copy link

success: "aos-ci-jenkins/OS_3.6_NOT_containerized, aos-ci-jenkins/OS_3.6_NOT_containerized_e2e_tests" for b376f0c (logs)

@openshift-bot
Copy link

success: "aos-ci-jenkins/OS_3.6_containerized, aos-ci-jenkins/OS_3.6_containerized_e2e_tests" for b376f0c (logs)

@kwoodson
Copy link
Contributor Author

flake
openshift/origin#8571
openshift/origin#10162

@kwoodson
Copy link
Contributor Author

Looks like we need to rebase and run the gauntlet.

@sdodson
Copy link
Member

sdodson commented Aug 11, 2017

Rebase and once aos-ci-test is done i'll merge this. I want to try to get all of this landed today.

@kwoodson kwoodson force-pushed the openshift_repos_refactor branch from b376f0c to 3b7e5d5 Compare August 11, 2017 16:47
@kwoodson
Copy link
Contributor Author

aos-ci-test

@openshift-bot
Copy link

error: aos-ci-jenkins/OS_3.6_NOT_containerized for 3b7e5d5 (logs)

@openshift-bot
Copy link

error: aos-ci-jenkins/OS_3.6_containerized for 3b7e5d5 (logs)

@mtnbikenc
Copy link
Member

Flake openshift/origin#14898 yum repo

@mtnbikenc
Copy link
Member

[merge]

@kwoodson
Copy link
Contributor Author

flake
openshift/origin#8571

@kwoodson
Copy link
Contributor Author

[merge]

@mtnbikenc
Copy link
Member

@kwoodson Somehow initialize_openshift_repos.yml was dropped from the commits during the last rebase.

mtnbikenc
mtnbikenc requested changes Aug 14, 2017
View reviewed changes
Copy link
Member

@mtnbikenc mtnbikenc left a comment

Choose a reason for hiding this comment

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

Need to add initialize_openshift_repos.yml to pass tox tests.

@kwoodson kwoodson force-pushed the openshift_repos_refactor branch from 3b7e5d5 to 94c195b Compare August 14, 2017 13:58
@kwoodson
Copy link
Contributor Author

@mtnbikenc, thanks for watching this one. I'm not sure what happened but I added the file again.

@kwoodson
Copy link
Contributor Author

aos-ci-test

@openshift-bot
Copy link

Evaluated for openshift ansible merge up to 94c195b

@openshift-bot
Copy link

Evaluated for openshift ansible test up to 94c195b

@openshift-bot
Copy link

success: "aos-ci-jenkins/OS_3.6_NOT_containerized, aos-ci-jenkins/OS_3.6_NOT_containerized_e2e_tests" for 94c195b (logs)

@openshift-bot
Copy link

success: "aos-ci-jenkins/OS_3.6_containerized, aos-ci-jenkins/OS_3.6_containerized_e2e_tests" for 94c195b (logs)

@openshift-bot
Copy link

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_openshift_ansible/446/) (Base Commit: fa32667) (PR Branch Commit: 94c195b)

@kwoodson
Copy link
Contributor Author

kwoodson commented Aug 15, 2017 •
edited
Loading

@sosiouxme @rhcarvalho, @sdodson

Any idea what happened here?

		Failure summary:
		
		  1. Host:     openshift_ansible_test_3869285588729
		     Play:     Initialize host firewall
		     Task:     os_firewall : Ensure firewalld service is not enabled
		     Message:  Failed to get D-Bus connection: Operation not permitted

@openshift-bot
Copy link

continuous-integration/openshift-jenkins/merge FAILURE (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_openshift_ansible/854/) (Base Commit: 2bc6832) (PR Branch Commit: 94c195b)

@sosiouxme
Copy link
Member

@kwoodson no, but it's happening quite a lot :(
openshift/origin#15769

@sdodson sdodson merged commit 80fee93 into openshift:master Aug 15, 2017
@ingvagabund
Copy link
Member

Aweeeeeesooome, thanks @kwoodson

@ingvagabund ingvagabund mentioned this pull request Aug 16, 2017
Merged
@kwoodson kwoodson deleted the openshift_repos_refactor branch September 18, 2017 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mtnbikenc mtnbikenc mtnbikenc approved these changes

Assignees

@kwoodson kwoodson

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@kwoodson @mtnbikenc @sdodson @openshift-bot @sosiouxme @ingvagabund