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

[release-4.6] Bug 1915467: redeploy CSI Controller Deployment when secret change #110

Merged
merged 2 commits into from Feb 11, 2021
Merged

[release-4.6] Bug 1915467: redeploy CSI Controller Deployment when secret change #110

merged 2 commits into from Feb 11, 2021

Conversation

bertinatto
Copy link
Member

This is a 4.6 backport of PR #107

A drawback of this backport is that we had to bump library-go, which also caused the bump of k8s-related APIs.

CC @openshift/storage

@openshift-ci-robot openshift-ci-robot added bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Jan 15, 2021
@openshift-ci-robot
Copy link
Contributor

@bertinatto: This pull request references Bugzilla bug 1915467, which is invalid:

  • expected dependent Bugzilla bug 1898045 to be in one of the following states: VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), but it is ON_QA instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

[release-4.6] Bug 1915467: redeploy CSI Controller Deployment when secret change

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.

@bertinatto
Copy link
Member Author

/hold
Further discussion is needed to decide if this backport is too risky.

@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 Jan 15, 2021
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 15, 2021
@bertinatto
Copy link
Member Author

/refresh

@bertinatto bertinatto mentioned this pull request Jan 18, 2021
Merged
@bertinatto
Copy link
Member Author

/bugzilla refresh

@openshift-ci-robot
Copy link
Contributor

@bertinatto: This pull request references Bugzilla bug 1915467, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

6 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.6.z) matches configured target release for branch (4.6.z)
  • bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)
  • dependent bug Bugzilla bug 1898045 is in the state VERIFIED, which is one of the valid states (VERIFIED, RELEASE_PENDING, CLOSED (ERRATA))
  • dependent Bugzilla bug 1898045 targets the "4.7.0" release, which is one of the valid target releases: 4.7.0
  • bug has dependents

In response to this:

/bugzilla refresh

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 bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. and removed bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Jan 26, 2021
@bertinatto
Copy link
Member Author

/hold cancel
We decided to go ahead with the backport, even though we're bumping library-go.

@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 Jan 26, 2021
@bertinatto
Copy link
Member Author

CC @openshift/storage

@jsafrane
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 26, 2021
@jwforres
Copy link
Member

(Patch Manager) given the size of this change due to the library.go bump and the fact that it is now using clients for a different version of kubernetes than runs on 4.6, I would like to see more details on the risks that were discussed

@bertinatto
Copy link
Member Author

(Patch Manager) given the size of this change due to the library.go bump and the fact that it is now using clients for a different version of kubernetes than runs on 4.6, I would like to see more details on the risks that were discussed

Regarding the k8s clients used in this PR (1.20), they are one version newer than the k8s version that runs on 4.6 (1.19). These version are compatible with each other, so we shouldn't have problems with that.

As for the library-go bump, the team considered the risk to be lower than the alternative solution, which was patching the vendor/github.com/openshift/library-go/ directory with only the changes we needed to address this issue.

@jsafrane, would you like to add anything else?

@jsafrane
Copy link
Contributor

We weighted bumping library-go and manually re-implementing the necessary functionality in vendor/library-go of the AWS operator, creating basically a fork of library-go there. We think that the new library-go, which is well tested, has lower risk than possibly introducing regressions in our much less tested library-go fork.

@jsafrane
Copy link
Contributor

Now that I am looking at library-go, there is branch for 4.6, investigating if it can be used for cherry picks to 4.6.
/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 Jan 28, 2021
@bertinatto
Copy link
Member Author

Cherry-picked to release-4.6 branch only the changes needed: openshift/library-go#986.

Once that PR is merged, we can re-bump library-go in this PR.

@openshift-ci-robot openshift-ci-robot mentioned this pull request Feb 4, 2021
Merged
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 4, 2021
@qinpingli
Copy link

/bugzilla cc-qa

@openshift-bot
Copy link
Contributor

/retest

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

20 similar comments
@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 11, 2021
@jsafrane
Copy link
Contributor

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bertinatto, jsafrane, qinpingli

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:
  • OWNERS [bertinatto,jsafrane]

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

@openshift-merge-robot openshift-merge-robot merged commit 71b6b96 into openshift:release-4.6 Feb 11, 2021
@openshift-ci-robot
Copy link
Contributor

@bertinatto: All pull requests linked via external trackers have merged:

Bugzilla bug 1915467 has been moved to the MODIFIED state.

In response to this:

[release-4.6] Bug 1915467: redeploy CSI Controller Deployment when secret change

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

@huffmanca huffmanca Awaiting requested review from huffmanca

@tsmetana tsmetana Awaiting requested review from tsmetana

Assignees

@jsafrane jsafrane

@qinpingli qinpingli

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@bertinatto @openshift-ci-robot @jsafrane @jwforres @qinpingli @openshift-bot @bparees @openshift-merge-robot