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

Nuage changes for Atomic hosts OSE Integration #4991

Merged
merged 1 commit into from
Aug 25, 2017
Merged

Nuage changes for Atomic hosts OSE Integration #4991

merged 1 commit into from
Aug 25, 2017

Conversation

rparulek
Copy link

@rparulek rparulek commented Aug 3, 2017

Nuage changes for Atomic hosts OSE Integration

@openshift-bot
Copy link

Can one of the admins verify this patch?
I understand the following commands:

  • bot, add author to whitelist
  • bot, test pull request
  • bot, test pull request once

1 similar comment
@openshift-bot
Copy link

Can one of the admins verify this patch?
I understand the following commands:

  • bot, add author to whitelist
  • bot, test pull request
  • bot, test pull request once

@rparulek rparulek force-pushed the nuage-atomic-ansible branch from 36e25b0 to 76aa794 Compare August 3, 2017 16:13
vareti
vareti reviewed Aug 3, 2017
View reviewed changes
roles/nuage_master/tasks/main.yaml Outdated
ignore_errors: true
when: openshift.common.is_atomic | bool

- name: Restart daemons
Copy link

Choose a reason for hiding this comment

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

any reason why we are doing master/api server/controller restart after launching the daemon sets?

roles/nuage_master/tasks/main.yaml
@@ -49,3 +100,4 @@
- restart master api
Copy link

Choose a reason for hiding this comment

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

do you think we can get away with restarts being done only once?

roles/openshift_common/tasks/main.yml Outdated
@@ -76,3 +76,19 @@
command: >
hostnamectl set-hostname {{ openshift.common.hostname }}
when: openshift_set_hostname | default(set_hostname_default) | bool

Copy link

Choose a reason for hiding this comment

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

is it possible to do this under "nuage_common"?

roles/nuage_master/tasks/main.yaml Outdated
@@ -1,7 +1,22 @@
---
- name: Create directory /usr/share/nuage-openshift-monitor
Copy link

Choose a reason for hiding this comment

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

does it not break backward compatibility for RHEL systems? IMO, we should use " /usr/share/nuage-openshift-monitor" for RHEL and "/var/usr/share/nuage-openshift-monitor" for Atomic. can we not conditionally set this variable and use it?

@rparulek rparulek force-pushed the nuage-atomic-ansible branch 11 times, most recently from 249c72f to 69cea25 Compare August 9, 2017 21:38
@rparulek rparulek force-pushed the nuage-atomic-ansible branch from 2c7590f to 667f649 Compare August 11, 2017 19:04
@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 11, 2017
@rparulek rparulek force-pushed the nuage-atomic-ansible branch from 2e32e32 to 294699d Compare August 11, 2017 20:48
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 11, 2017
@rparulek rparulek force-pushed the nuage-atomic-ansible branch 2 times, most recently from ec90506 to 2444529 Compare August 14, 2017 17:24
@rparulek
Copy link
Author

Can any one of the admins approve this PR?
@sdodson @detiber Can you please review and approve this PR?

abutcher
abutcher reviewed Aug 14, 2017
View reviewed changes
roles/nuage_master/handlers/main.yaml Outdated
- name: restart nuage-openshift-monitor
become: yes
systemd: name=nuage-openshift-monitor state=restarted
- name: restart master
Copy link
Member

Choose a reason for hiding this comment

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

This handler and references to it can be removed. The single master service was removed in #4832.

Copy link
Author

Choose a reason for hiding this comment

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

@abutcher Addressed your comment, Thanks!

@rparulek rparulek force-pushed the nuage-atomic-ansible branch from cd08671 to a87e7dc Compare August 14, 2017 17:45
@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 14, 2017
@rparulek rparulek force-pushed the nuage-atomic-ansible branch 2 times, most recently from 1b53ce6 to 3a0b6cc Compare August 15, 2017 18:43
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 15, 2017
@rparulek
Copy link
Author

Can admin review and stage this PR for merge into master?

@rparulek rparulek force-pushed the nuage-atomic-ansible branch from 579c537 to 20c6c50 Compare August 21, 2017 15:27
@rparulek
Copy link
Author

Can any one of the admins review and merge this PR into main branch?

@rparulek rparulek force-pushed the nuage-atomic-ansible branch from 3dd82f1 to 0338fa7 Compare August 22, 2017 15:34
@rparulek rparulek force-pushed the nuage-atomic-ansible branch from 781f323 to b58adb0 Compare August 24, 2017 15:22
@sdodson
Copy link
Member

sdodson commented Aug 24, 2017

@vareti can you please review this? we don't have capacity to verify functionality just coding standards. Is there someone specific that can provide review of Nuane changes?

vareti
vareti approved these changes Aug 24, 2017
View reviewed changes
Copy link

@vareti vareti left a comment

Choose a reason for hiding this comment

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

looks good to me.

@sdodson
Copy link
Member

sdodson commented Aug 24, 2017

aos-ci-test

@sdodson sdodson mentioned this pull request Aug 24, 2017
Closed
@openshift-bot
Copy link

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

@openshift-bot
Copy link

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

@sdodson
Copy link
Member

sdodson commented Aug 25, 2017

[test]

@openshift-bot
Copy link

Evaluated for openshift ansible test up to b58adb0

@openshift-bot
Copy link

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_openshift_ansible/526/) (Base Commit: bb10901) (PR Branch Commit: b58adb0)

@sdodson sdodson merged commit 2b07b98 into openshift:master Aug 25, 2017
@rparulek rparulek deleted the nuage-atomic-ansible branch August 30, 2017 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abutcher abutcher abutcher left review comments

@vareti vareti vareti approved these changes

@rushabh268 rushabh268 rushabh268 approved these changes

Assignees

@abutcher abutcher

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@rparulek @openshift-bot @sdodson @abutcher @rushabh268 @vareti