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

Gluster timeout patches #15396

Merged
merged 4 commits into from
Jul 27, 2017
Merged

Gluster timeout patches #15396

merged 4 commits into from
Jul 27, 2017

Conversation

jsafrane
Copy link
Contributor

@jsafrane jsafrane commented Jul 21, 2017 •
edited
Loading

There are several upstream patches that shorten gluster timeout when remote gluster servers are unavailable. User gets an error about failed mount in 2 minutes instead of 2 minutes * 2 * nr. of gluster endpoints.

kubernetes/kubernetes#42038
kubernetes/kubernetes#49127
kubernetes/kubernetes#48709
kubernetes/kubernetes#49475

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1473587

@jsafrane jsafrane changed the base branch from master to release-3.6 July 21, 2017 10:26
@jsafrane
Copy link
Contributor Author

[test]

@humblec
Copy link
Contributor

humblec commented Jul 21, 2017

LGTM. Thanks @jsafrane!

@jsafrane
Copy link
Contributor Author

[test]
flake: #15401

eparis
eparis reviewed Jul 22, 2017
View reviewed changes
vendor/k8s.io/kubernetes/pkg/volume/glusterfs/glusterfs.go Outdated
// Give a try without `auto_unmount` mount option, because
// it could be that gluster fuse client is older version and
// mount.glusterfs is unaware of `auto_unmount`.
noAutoMountOptions := make([]string, len(mountOptions))
Copy link
Member

Choose a reason for hiding this comment

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

Is this right? shouldn't it be noAutoMountOptions := make([]string, 0, len(mountOptions)? Since we are using append() instead of noAutoMountOptions[i] isn't noautoMountOptions actually ending up as a slice with 2*len(mountOptions)-1 where the first len(mountOptions) strings are just ""?

Copy link
Member

@eparis eparis Jul 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

https://play.golang.org/p/ftq6soDDEy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch, filled kubernetes/kubernetes#49475

@openshift-merge-robot openshift-merge-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 24, 2017
@liggitt
Copy link
Contributor

liggitt commented Jul 24, 2017

[test] GCE PV fix was merged for ansible

@openshift openshift deleted a comment from openshift-bot Jul 25, 2017
@openshift openshift deleted a comment from openshift-bot Jul 25, 2017
@jsafrane
Copy link
Contributor Author

jsafrane commented Jul 25, 2017 •
edited
Loading

Added kubernetes/kubernetes#49475

@jsafrane
Copy link
Contributor Author

[test]
flake: #15451

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 50f5a42

@eparis
Copy link
Member

eparis commented Jul 25, 2017

[merge][severity: blocker]

@openshift-bot
Copy link
Contributor

openshift-bot commented Jul 25, 2017 •
edited
Loading

continuous-integration/openshift-jenkins/merge Waiting: You are in the build queue at position: 3

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 50f5a42

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/3485/) (Base Commit: cb4bddd) (PR Branch Commit: 50f5a42)

@eparis
Copy link
Member

eparis commented Jul 26, 2017

/approve

@eparis
Copy link
Member

eparis commented Jul 26, 2017

/lgtm
/approve no-issue

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 26, 2017
@openshift-merge-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: eparis, jsafrane

Associated issue requirement bypassed by: eparis

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

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 26, 2017
@eparis
Copy link
Member

eparis commented Jul 26, 2017

/retest

@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue

@openshift-merge-robot openshift-merge-robot merged commit a691774 into openshift:release-3.6 Jul 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eparis eparis eparis left review comments

Assignees

@childsb childsb

@eparis eparis

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. 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.
8 participants
@jsafrane @humblec @liggitt @openshift-bot @eparis @openshift-merge-robot @childsb @openshift-ci-robot