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 1881905: Adds gatherer for PodDistributionBudget #185

Merged
merged 1 commit into from
Sep 30, 2020
Merged

Bug 1881905: Adds gatherer for PodDistributionBudget #185

merged 1 commit into from
Sep 30, 2020

Conversation

0sewa0
Copy link
Contributor

@0sewa0 0sewa0 commented Sep 22, 2020

No description provided.

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 22, 2020
@0sewa0 0sewa0 force-pushed the PodDistributionBudget branch from 3285495 to 7c4b3a8 Compare September 22, 2020 13:29
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 22, 2020
tremes
tremes reviewed Sep 22, 2020
View reviewed changes
pkg/gather/clusterconfig/clusterconfig.go Outdated
*policyv1beta1.PodDisruptionBudget
}

// Marshal implements serialization of a Pod with anonymization
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy/paste comment. It's just minor but is there anything we could possibly anonymize (some IDs, IP addressess and similar)?

Copy link
Contributor Author

@0sewa0 0sewa0 Sep 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah the comments are a mess but its WIP for a reason :D

About what to anonymize: No idea really, the ones that I saw had nothing in them that I would consider confidential. maybe the metadata.managedFields can have some confidential info.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here is the type definition for PDB. To me it doesn't look like there could be anything worth anonymizing. What do you think ?
https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/policy/types.go

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that was my impression as well

@0sewa0 0sewa0 changed the title WIP: Adds gatherer for PodDisruptionBudget Adds gatherer for PodDisruptionBudget Sep 23, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 23, 2020
@0sewa0 0sewa0 changed the title Adds gatherer for PodDisruptionBudget Bug 1881905: Adds gatherer for PodDisruptionBudget Sep 23, 2020
@openshift-ci-robot openshift-ci-robot added the bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. label Sep 23, 2020
@openshift-ci-robot
Copy link
Contributor

@0sewa0: This pull request references Bugzilla bug 1881905, which is invalid:

  • expected the bug to target the "4.6.0" release, but it targets "---" 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:

Bug 1881905: Adds gatherer for PodDisruptionBudget

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 bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Sep 23, 2020
@0sewa0
Copy link
Contributor Author

0sewa0 commented Sep 23, 2020

/bugzilla refresh

@openshift-ci-robot openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Sep 23, 2020
@openshift-ci-robot
Copy link
Contributor

@0sewa0: This pull request references Bugzilla bug 1881905, 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.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.6.0) matches configured target release for branch (4.6.0)
  • bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

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 removed the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Sep 23, 2020
@0sewa0
Copy link
Contributor Author

0sewa0 commented Sep 23, 2020

currently some tests fail because:
Sep 23 11:17:12.821: FAIL: Some cluster operators are not ready: insights (Degraded=True PeriodicGatherFailed: Source config could not be retrieved: poddisruptionbudgets.policy is forbidden: User "system:serviceaccount:openshift-insights:operator" cannot list resource "poddisruptionbudgets" in API group "policy" at the cluster scope)

@iNecas @martinkunc @natiiix any thoughts ?

@martinkunc
Copy link
Contributor

Did this passed some local tests ? Basically you should get a cluster (quicklab) run IO and check data in /tmp folder.
Anyway, I think you need to add the Role policy according to this comment: rancher/rancher#20325 (comment)
in this file:
https://github.com/openshift/insights-operator/blob/master/manifests/03-clusterrole.yaml#L64

When you change file locally, you can try

oc apply -f 03-clusterrole.yaml

To apply the change to see if it works.

@0sewa0 0sewa0 changed the title Bug 1881905: Adds gatherer for PodDisruptionBudget Bug 1881905: Adds gatherer for PodDistributionBudget Sep 23, 2020
@openshift-ci-robot openshift-ci-robot added 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 Sep 25, 2020
@0sewa0 0sewa0 force-pushed the PodDistributionBudget branch from 327f441 to 1677da2 Compare September 25, 2020 09:07
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 25, 2020
@0sewa0
Copy link
Contributor Author

0sewa0 commented Sep 25, 2020

/retest

1 similar comment
@0sewa0
Copy link
Contributor Author

0sewa0 commented Sep 25, 2020

/retest

natiiix
natiiix reviewed Sep 28, 2020
View reviewed changes
Copy link
Contributor

@natiiix natiiix left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@0sewa0 There are quite a few commits. It's usual to squash them to reduce the number of commits in the upstream branches. 1-3 is usually okay, but 6 seems like too many for a single enhancement.

@0sewa0
Copy link
Contributor Author

0sewa0 commented Sep 28, 2020

@natiiix Can't github do it ?? Gitlab made it all easy :(

@natiiix
Copy link
Contributor

natiiix commented Sep 28, 2020

@natiiix Can't github do it ?? Gitlab made it all easy :(

@0sewa0 GitHub can do it, but the merge bot does as the name suggests, creates a merge commit, not a squash. Sometimes it's absolutely necessary for cherry-picking because you don't want to backport documentation or configuration changes in some scenarios.

@0sewa0 0sewa0 force-pushed the PodDistributionBudget branch from 1677da2 to fbba8ec Compare September 28, 2020 09:58
@0sewa0
Copy link
Contributor Author

0sewa0 commented Sep 28, 2020

/retest

1 similar comment
@0sewa0
Copy link
Contributor Author

0sewa0 commented Sep 28, 2020

/retest

@martinkunc
Copy link
Contributor

Nice, thank you @0sewa0 !
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 29, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 0sewa0, martinkunc

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

@martinkunc
Copy link
Contributor

/retest

3 similar comments
@0sewa0
Copy link
Contributor Author

0sewa0 commented Sep 29, 2020

/retest

@0sewa0
Copy link
Contributor Author

0sewa0 commented Sep 29, 2020

/retest

@0sewa0
Copy link
Contributor Author

0sewa0 commented Sep 29, 2020

/retest

@openshift-bot
Copy link
Contributor

/retest

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

4 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-merge-robot openshift-merge-robot merged commit 0dee9e6 into openshift:master Sep 30, 2020
@openshift-ci-robot
Copy link
Contributor

@0sewa0: All pull requests linked via external trackers have merged:

Bugzilla bug 1881905 has been moved to the MODIFIED state.

In response to this:

Bug 1881905: Adds gatherer for PodDistributionBudget

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.

wking added a commit to wking/oc that referenced this pull request Feb 25, 2021
@wking
pkg/cli/admin/inspect/namespace: Gather PDBs too
c32fac0 
Pod disruption budgets are important enough to be included in Insights
tarballs [1], and they aren't all that big, so we should pick them up
as a standard part of interesting namespaces too.  This will help with
things like auditing issues where we failed to drain (PDB too strict)
or disrupted a core workload (PDB too weak or missing).

[1]: openshift/insights-operator#185
@wking wking mentioned this pull request Feb 25, 2021
Merged
wking added a commit to wking/hypershift that referenced this pull request Dec 1, 2023
@wking
cmd/cluster/core/dump: Gather PodDisruptionBudgets too
ec520e1 
Pod disruption budgets are important enough to be included in Insights
tarballs [1], and they aren't all that big, so we should pick them up
as a standard part of interesting namespaces too.  This will help with
things like auditing issues where we failed to drain (PDB too strict)
or disrupted a core workload (PDB too weak or missing).  Similar to
openshift/oc@c32fac0143 (pkg/cli/admin/inspect/namespace: Gather PDBs
too, 2021-03-03, openshift/oc#750).

[1]: openshift/insights-operator#185
@wking wking mentioned this pull request Dec 1, 2023
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tremes tremes tremes left review comments

@martinkunc martinkunc martinkunc left review comments

@natiiix natiiix natiiix left review comments

@alexandrevicenzi alexandrevicenzi Awaiting requested review from alexandrevicenzi

Assignees

@martinkunc martinkunc

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. 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.
7 participants
@0sewa0 @openshift-ci-robot @martinkunc @natiiix @openshift-bot @tremes @openshift-merge-robot