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

Bug 1798365: gluster: fix how lvm wrapper is passed to heketi templates #12096

Merged
merged 1 commit into from
Feb 13, 2020
Merged

Bug 1798365: gluster: fix how lvm wrapper is passed to heketi templates #12096

merged 1 commit into from
Feb 13, 2020

Conversation

phlogistonjohn
Copy link
Contributor

This change fixes two issues:

  1. The environment variable 'HEKETI_LVMWRAPPER' was incorrect as heketi
    does not make use of a variable by this name. Heketi does make use of
    'HEKETI_LVM_WRAPPER'. So we update the vars in openshift-ansible to
    match and be consistent.
  2. Conditionally including the template vars block is incorrect and
    causes 'oc process' to fail when glusterfs_is_native var is false,
    the state known as independent mode. Instead we always set and pass
    the variable, but we pass an empty string to 'oc process' when
    glusterfs_is_native is false.

Signed-off-by: John Mulligan jmulligan@redhat.com

@openshift-ci-robot openshift-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 6, 2020
@openshift-ci-robot
Copy link

Hi @phlogistonjohn. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 6, 2020
@phlogistonjohn
Copy link
Contributor Author

@raghavendra-talur @nixpanic @jarrpa @Daniel-Pivonka PTAL. Unfortuatnely, #12072 did not fix everything and non-containerized (aka independent mode) deploys were (always) going to break. I undid some of the templating inside the template and moved it back into the roles such that it will always "know" about the HEKETI_LVM_WRAPPER variable but independent mode deploys will just set it to an empty string (which heketi treats exactly the same as unset). This also has the nice side effect of making the upgrade documentation simpler.

@raghavendra-talur
Copy link

/assign @raghavendra-talur

/ok-to-test

@openshift-ci-robot openshift-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 6, 2020
raghavendra-talur
raghavendra-talur reviewed Feb 6, 2020
View reviewed changes
Copy link

@raghavendra-talur raghavendra-talur left a comment

Choose a reason for hiding this comment

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

I have comments about default value of the HEKETI_LVM_WRAPPER in the heketi yaml. Other than that the changes look good to me.

roles/openshift_storage_glusterfs/templates/deploy-heketi-template.yml.j2
@@ -135,9 +133,7 @@ parameters:
displayName: GlusterFS cluster name
description: A unique name to identify this heketi service, useful for running multiple heketi instances
value: glusterfs
{% if glusterfs_is_native | bool %}
- name: HEKETI_LVMWRAPPER
- name: HEKETI_LVM_WRAPPER
displayName: Wrapper for executing LVM commands
description: Heketi can use a wrapper to execute LVM commands, i.e. run commands in the host namespace instead of in the Gluster container.
value: "/usr/sbin/exec-on-host"
Copy link

Choose a reason for hiding this comment

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

Should we leave the value empty instead of this default?

Copy link

Choose a reason for hiding this comment

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

Ok, @phlogistonjohn explained this to me in a call. HEKETI_LVM_WRAPPER will default to value we want it to be in latest 3.11 OCS in Converged mode. For other setups, it can be tweaked if necessary.

roles/openshift_storage_glusterfs/templates/heketi-template.yml.j2
@@ -147,9 +145,7 @@ parameters:
displayName: GlusterFS cluster name
description: A unique name to identify this heketi service, useful for running multiple heketi instances
value: glusterfs
{% if glusterfs_is_native | bool %}
- name: HEKETI_LVMWRAPPER
- name: HEKETI_LVM_WRAPPER
displayName: Wrapper for executing LVM commands
description: Heketi can use a wrapper to execute LVM commands, i.e. run commands in the host namespace instead of in the Gluster container.
value: "/usr/sbin/exec-on-host"
Copy link

Choose a reason for hiding this comment

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

Should we leave the value empty instead of this default?

Copy link

Choose a reason for hiding this comment

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

Ok, @phlogistonjohn explained this to me in a call. HEKETI_LVM_WRAPPER will default to value we want it to be in latest 3.11 OCS in Converged mode. For other setups, it can be tweaked if necessary.

@raghavendra-talur
Copy link

/retest

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 7, 2020
raghavendra-talur
raghavendra-talur suggested changes Feb 7, 2020
View reviewed changes
Copy link

@raghavendra-talur raghavendra-talur left a comment

Choose a reason for hiding this comment

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

On discussing a little more, we realized it would be better to move the heketi yaml from templates dir back to files dir as we don't have any jinja templating in them.

This will make the documentation changes lot simpler.

Holding off the approval for the same.

@raghavendra-talur
Copy link

/hold

@openshift-ci-robot openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 7, 2020
Daniel-Pivonka
Daniel-Pivonka approved these changes Feb 7, 2020
View reviewed changes
Copy link

@Daniel-Pivonka Daniel-Pivonka left a comment

Choose a reason for hiding this comment

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

LGTM no obvious issues i can see as long as you tested for converged and non converged mode and everything work we should be good to go

@nixpanic
Copy link
Member

nixpanic commented Feb 10, 2020 via email

@phlogistonjohn
gluster: fix how lvm wrapper is passed to heketi templates
18b472a 
This change fixes two issues:
1. The environment variable 'HEKETI_LVMWRAPPER' was incorrect as heketi
   does not make use of a variable by this name. Heketi does make use of
   'HEKETI_LVM_WRAPPER'. So we update the vars in openshift-ansible to
   match and be consistent.
2. Conditionally including the template vars block is incorrect and
   causes 'oc process' to fail when glusterfs_is_native var is false,
   the state known as independent mode. Instead we always set and pass
   the variable, but we pass an empty string to 'oc process' when
   glusterfs_is_native is false.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
@phlogistonjohn
Copy link
Contributor Author

@raghavendra-talur @nixpanic @Daniel-Pivonka new version pushed, returns to using copy as opposed to template for ocp templates and renames files back to the 'files' dir w/o j2 extension.

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 11, 2020
@raghavendra-talur
Copy link

/test unit

@raghavendra-talur
Copy link

/retest

1 similar comment
@raghavendra-talur
Copy link

/retest

@raghavendra-talur
Copy link

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 11, 2020
@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

11 similar comments
@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@vikaslaad
Copy link

/test unit

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

6 similar comments
@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci-robot
Copy link

@phlogistonjohn: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/unit 18b472a link /test unit

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@sdodson
Copy link
Member

sdodson commented Feb 13, 2020

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 13, 2020
@sdodson
Copy link
Member

sdodson commented Feb 13, 2020

/hold cancel
/lgtm
/override ci/prow/unit

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 13, 2020
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Daniel-Pivonka, nixpanic, phlogistonjohn, raghavendra-talur, sdodson

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot
Copy link

@sdodson: Overrode contexts on behalf of sdodson: ci/prow/unit

In response to this:

/hold cancel
/lgtm
/override ci/prow/unit

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-merge-robot openshift-merge-robot merged commit 85cd113 into openshift:release-3.11 Feb 13, 2020
@openshift-ci-robot
Copy link

@phlogistonjohn: All pull requests linked via external trackers have merged. Bugzilla bug 1798365 has been moved to the MODIFIED state.

In response to this:

Bug 1798365: gluster: fix how lvm wrapper is passed to heketi templates

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nixpanic nixpanic nixpanic approved these changes

@raghavendra-talur raghavendra-talur raghavendra-talur approved these changes

@Daniel-Pivonka Daniel-Pivonka Daniel-Pivonka approved these changes

@jarrpa jarrpa Awaiting requested review from jarrpa

Assignees

@sdodson sdodson

@raghavendra-talur raghavendra-talur

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@phlogistonjohn @openshift-ci-robot @raghavendra-talur @nixpanic @sdodson @openshift-bot @vikaslaad @Daniel-Pivonka @openshift-merge-robot