Skip to content

Conversation

@gnufied
Copy link
Member

@gnufied gnufied commented Jul 27, 2019

Update gophercloud library.

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

@openshift-ci-robot openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jul 27, 2019
@gnufied
Copy link
Member Author

gnufied commented Jul 29, 2019

/sig storage

@jsafrane
Copy link
Contributor

jsafrane commented Jul 29, 2019

Where the gophercloud version comes from? Upstream #73437 (commit 55a8dbcbfbe1833330d88fc67421532d3fbbb066 contains slightly different code. And this PR contains thousands of files that are not in Kubernetes. What is our vendor pruning policy?

@gnufied
Copy link
Member Author

gnufied commented Jul 29, 2019

Yes - I brought forward gophercloud version mentioned in - kubernetes/kubernetes#79637 but since upstream has moved to use gomodules, calling it UPSSTREAM: xxx is tricky.

And I needed kubernetes/kubernetes@55a8dbc because it contains necessary changes in kubernetes code to accomodate for version bump.

v0.1.0 of gophercloud seems to have more sane fixes than the SHA referred in dims commit. I can perhaps make this clear and split the commits.

Also about vendor stripping/pruning - I am not sure why is glide not stripping files by scanning the imports. Ideally it should.

@jsafrane
Copy link
Contributor

v0.1.0 of gophercloud seems to have more sane fixes than the SHA referred in dims commit. I can perhaps make this clear and split the commits.

Yes, more obvious commit message would be useful.

Also about vendor stripping/pruning - I am not sure why is glide not stripping files by scanning the imports. Ideally it should.

Ack, I see other deps are not pruned either, so let it be as it is.

Still, this replaces lot of OpenStack code with something that was not tested by us and can bring regressions.

 895 files changed, 70436 insertions(+), 4307 deletions(-)

It's mostly adding support for new APIs, yet I am not sure we have power to retest whole OCP on OSP in 3.11.z timeframe.

@gnufied
Copy link
Member Author

gnufied commented Jul 30, 2019

Still, this replaces lot of OpenStack code with something that was not tested by us and can bring regressions.

So the reason we need the entire 0.1.0 release rather than just the cherry-pick of the commit that fixes the issue is because - gophercloud/gophercloud#1408 while fixed the issue, it had a regression. And the fix that fixes the regression had conflict with other related changes. It is my understanding that - reauth. mechanism in gophercloud is quite problematic and suffers from several problems. Only around 0.1.0 release - dust seems to have settled down little bit.

So far our options are:

  1. Just roll with this patch. Ask QE to test it and provide affected customer with a hotfix release that includes this patch and ask them to test. If everything works - we are probably fine.

  2. Really delve into reauth. mechanism code and make a more targetted fix which hopefully does not have regression that upstream fix had. There are several downsides of this approach:
    a. We will be creating a mini-fork of gophercloud.
    b. We will miss other fixes that were done to library including for reauthentication.
    c. It will not be any less time consuming than option#1.

@jsafrane
Copy link
Contributor

There are also these options:

  1. Find acceptable workaround
  2. Update OCP to 4.1
  3. Update OpenStack to 13

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 8, 2019
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 22, 2019
@gnufied
Copy link
Member Author

gnufied commented Sep 10, 2019

Just for a quick update - the hotfix build was deployed in customers environment and it was confirmed that, this patch fixes the customer's issue and no regression was found. We should be good to go with this PR.

@jottofar
Copy link
Contributor

/retest

@gnufied
Copy link
Member Author

gnufied commented Sep 12, 2019

I am currently waiting for #23620 PR to merge first, so as some of the issues with missing dependencies can be resolved. I will have to update this PR once #23620 merges.

@gageorsburn
Copy link

@gnufied Any timeframe on getting conflicts resolved so this can get merged?

@gnufied
Copy link
Member Author

gnufied commented Oct 14, 2019

/retest

@jsafrane
Copy link
Contributor

The customer has confirmed it fixes the bug and has no obvious regressions.

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 14, 2019
@jsafrane
Copy link
Contributor

/retest

@openshift-ci-robot
Copy link

openshift-ci-robot commented Oct 15, 2019

@gnufied: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/openshift-jenkins/extended_builds 86385f3 link /test extended_builds

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.

Details

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.

@gnufied
Copy link
Member Author

gnufied commented Oct 15, 2019

The CI for 3.11 branches is broken. @dobbymoodge is working on fixing the CI

@gnufied
Copy link
Member Author

gnufied commented Oct 15, 2019

/retest

1 similar comment
@dobbymoodge
Copy link
Contributor

/retest

@knobunc
Copy link
Contributor

knobunc commented Oct 15, 2019

/approve

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gnufied, jsafrane, knobunc

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

The pull request process is described here

Details 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 openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 15, 2019
@openshift-merge-robot openshift-merge-robot merged commit 6194fc2 into openshift:release-3.11 Oct 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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. sig/storage size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants