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

Cherry pick for v2.1.1 / Cut Release #1540

Merged
merged 8 commits into from
Jul 29, 2021
Merged

Cherry pick for v2.1.1 / Cut Release #1540

merged 8 commits into from
Jul 29, 2021

Conversation

mrueg
Copy link
Member

@mrueg mrueg commented Jul 28, 2021

What this PR does / why we need it:
Cut a new release and backport some fixes.

*: Cut v2.1.1 release

* [CHANGE] Replace multiListWatcher with independent listWatchers per namespace #1499
* [BUGFIX] Use BuilderInterface instead of internal/store.Builder in metricshandler #1537
* [BUGFIX] Add WithAllowLabels to public BuilderInterface #1514

fpetkovski and others added 6 commits July 21, 2021 13:43
@fpetkovski @mrueg
Replace multiListerWatcher with independent listWatchers per namespace
639c339 
The multiListerWatcher is a composite object encapsulating multiple
ListerWatchers and implements the ListerWatcher interface.
With the current implementation, when an individual lister fails, the
entire List operation fails. This causes no metrics to be shown when KSM
has no permissions to a single namespace.

In addition to this, the multiListerWatcher takes advantage of internal
implementation details if the client-go library by modifiying and
relying on the ResourceVersion metadata field. This introduces a bug
where reconnecting to the API server will break the multiListerWatcher
completely.

This commit replaces the multiListerWatcher with individual
ListerWatchers per each configured namespace, resolving both issues.

Signed-off-by: fpetkovski <filip.petkovsky@gmail.com>
@fpetkovski @dgrisonnet @mrueg
Apply suggestions from code review
38ce100 
Co-authored-by: Damien Grisonnet <damien.grisonnet@epita.fr>
@fpetkovski @mrueg
rename buildStore to buildStores
653cab0
@fpetkovski @mrueg
Rename tests
8bff42e
@fpetkovski @mrueg
Apply suggestions from code review
b3ae45c 
Co-authored-by: Manuel Rüger <manuel@rueg.eu>
@FillZpp @mrueg
Use BuilderInterface instead of internal/store.Builder in metricshandler
3b30950 
Signed-off-by: FillZpp <FillZpp.pub@gmail.com>
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 28, 2021
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 28, 2021
@mrueg
go.mod: Update and minimize dependencies
40a5f94 
Signed-off-by: Manuel Rüger <manuel@rueg.eu>
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 28, 2021
@mrueg mrueg changed the base branch from master to release-2.1 July 28, 2021 15:59
@k8s-ci-robot k8s-ci-robot removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 28, 2021
@mrueg mrueg force-pushed the cherry-pick-for-2.1.1 branch 2 times, most recently from 77a333f to 1a87d93 Compare July 28, 2021 16:10
@mrueg
*: Cut v2.1.1 release
c3350d1 
* [CHANGE] Replace multiListWatcher with independent listWatchers per namespace kubernetes#1499
* [CHANGE] go.mod: Update and minimize dependencies kubernetes#1529
* [BUGFIX] Use BuilderInterface instead of internal/store.Builder in metricshandler kubernetes#1537
* [BUGFIX] Add WithAllowLabels to public BuilderInterface kubernetes#1514
@fpetkovski
Copy link
Contributor

/approve
/assign @tariq1890

lilic
lilic reviewed Jul 29, 2021
View reviewed changes
CHANGELOG.md
@@ -1,3 +1,10 @@
## v2.1.1 / 2021-07-28

* [CHANGE] Replace multiListWatcher with independent listWatchers per namespace #1499
Copy link
Member

Choose a reason for hiding this comment

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

These are changes not bug fixes from what I can tell, do we still want to include them in v2.1.1? Also the bug fixes seem not related to the 2.1 release, so maybe we need to instead cut 2.2? What do you think?

Copy link
Member Author

@mrueg mrueg Jul 29, 2021 •
edited

Choose a reason for hiding this comment

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

I believe they are internal changes (that fix certain issues), so suitable for a patch release as they don't add any features or are user visible.
I would release v2.2 with kubernetes 1.22 support once it's out

Copy link
Member

@lilic lilic Jul 29, 2021 •
edited

Choose a reason for hiding this comment

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

That sounds good to me, thanks! Should we backport some of the fixes to earlier versions for users pre 1.20 or maybe if they request?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think for 1.9.x we can do that upon request and if someone volunteers doing the backports (so we are sure that there's a need).
I believe for 2.0 we don't need to create a new release, 2.1 should work for those users as well.

lilic
lilic approved these changes Jul 29, 2021
View reviewed changes
Copy link
Member

@lilic lilic left a comment

Choose a reason for hiding this comment

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

/lgtm

Thank you!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 29, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fpetkovski, lilic, mrueg

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 29, 2021
@k8s-ci-robot k8s-ci-robot merged commit 8af106d into kubernetes:release-2.1 Jul 29, 2021
dgrisonnet
dgrisonnet reviewed Jul 29, 2021
View reviewed changes
CHANGELOG.md
@@ -1,3 +1,10 @@
## v2.1.1 / 2021-07-28

* [CHANGE] Replace multiListWatcher with independent listWatchers per namespace #1499
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be a bugfix since it fixes the permission issue from #1413. Also, we could reuse the release note that @fpetkovski added to his PR:

Fixes a bug where KSM did not export any metrics when it had no permissions for resources in at least one namespace

CHANGELOG.md
Comment on lines +5 to +6
* [BUGFIX] Use BuilderInterface instead of internal/store.Builder in metricshandler #1537
* [BUGFIX] Add WithAllowLabels to public BuilderInterface #1514
Copy link
Member

Choose a reason for hiding this comment

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

I would consider both of these PRs changes instead of bug fixes since they are actual improvements made to the builder interface to make it easier for users to extend ksm functionalities.

@dgrisonnet dgrisonnet mentioned this pull request Jul 29, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dgrisonnet dgrisonnet dgrisonnet left review comments

@lilic lilic lilic approved these changes

@brancz brancz Awaiting requested review from brancz

@fpetkovski fpetkovski Awaiting requested review from fpetkovski

Assignees

@tariq1890 tariq1890

@lilic lilic

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@mrueg @fpetkovski @k8s-ci-robot @lilic @dgrisonnet @tariq1890 @FillZpp