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

docs: account for bandwidth manager now being disabled by default #16782

Merged
merged 2 commits into from Jul 7, 2021
Merged

docs: account for bandwidth manager now being disabled by default #16782

merged 2 commits into from Jul 7, 2021

Conversation

bmcustodio
Copy link
Contributor

#16380 disabled the bandwidth manager by default.

@bmcustodio bmcustodio requested a review from a team July 5, 2021 13:20
@bmcustodio bmcustodio requested a review from a team as a code owner July 5, 2021 13:20
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jul 5, 2021
@bmcustodio bmcustodio added area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. feature/bandwidth-manager Impacts BPF bandwidth manager. needs-backport/1.10 labels Jul 5, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.10.3 Jul 5, 2021
@bmcustodio bmcustodio mentioned this pull request Jul 5, 2021
Merged
@bmcustodio bmcustodio added the kind/bug This is a bug in the Cilium logic. label Jul 5, 2021
@bmcustodio
docs: bandwidth manager is now disabled by default
2b6d2b1 
#16380 disabled the bandwidth
manager by default.

Signed-off-by: Bruno Miguel Custódio <brunomcustodio@gmail.com>
@bmcustodio
docs: add missing words to the spelling checklist
850dec7 
Signed-off-by: Bruno Miguel Custódio <brunomcustodio@gmail.com>
@qmonnet qmonnet added release-note/misc This PR makes changes that have no direct user impact. and removed kind/bug This is a bug in the Cilium logic. labels Jul 7, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jul 7, 2021
@qmonnet qmonnet changed the title docs: bandwidth manager is now disabled by default docs: account for bandwidth manager now being disabled by default Jul 7, 2021
@qmonnet
Copy link
Member

qmonnet commented Jul 7, 2021

This PR has got positive reviews from the two owner teams, bpf and docs-structure. The documentation checks are passing. The RST checks failed, but the GitHub action is known to fail (and there's a pending PR to remove it anyway), and that failure should be ignored.

For these reasons, I'm marking as ready-to-merge.

@qmonnet qmonnet added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jul 7, 2021
pchaigno
pchaigno requested changes Jul 7, 2021
View reviewed changes
Copy link
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

One question below. LGTM otherwise.

Documentation/helm-values.rst
* - startupProbe.periodSeconds
- interval between checks of the startup probe
- int
- ``2``
Copy link
Member

Choose a reason for hiding this comment

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

Have these changes been included by mistake?

Copy link
Member

@qmonnet qmonnet Jul 7, 2021 •
edited

Choose a reason for hiding this comment

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

No, I'm pretty sure this is intentional. They are in a separate commit (along with the spelling list update).

We've had the Helm reference for a few weeks now, and people should update it when they touch the Helm values. The CI is supposed to catch omissions, but there was a bug and a few changes to the Helm values were not reported to Documentation/helm-values.rst.

Now that the CI has been fixed, the documentation action - which will run for this PR - should complain that the Helm values are missing in the reference. This is why Bruno updated it - to fix the reference, and to have the GH action pass. The changes to the spelling wordlist in the same commit are in fact needed to account for the new words added to the Helm reference in this update.

Copy link
Member

Choose a reason for hiding this comment

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

👍 Ok, makes sense. That information should ideally be in the commit message though :-)

@pchaigno pchaigno removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jul 7, 2021
@pchaigno pchaigno added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jul 7, 2021
@pchaigno pchaigno merged commit 7a22ee8 into cilium:master Jul 7, 2021
@bmcustodio bmcustodio deleted the bruno-bandwidth-manager-disabled branch July 7, 2021 14:04
@pchaigno pchaigno mentioned this pull request Jul 8, 2021
Merged
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.10 in 1.10.3 Jul 8, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.10 in 1.10.3 Jul 8, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.10 to Backport done to v1.10 in 1.10.3 Jul 15, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.10 to Backport done to v1.10 in 1.10.3 Jul 15, 2021
@aanm aanm mentioned this pull request Jul 15, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qmonnet qmonnet qmonnet approved these changes

@borkmann borkmann borkmann approved these changes

@pchaigno pchaigno pchaigno approved these changes

@jibi jibi Awaiting requested review from jibi

@joestringer joestringer Awaiting requested review from joestringer

Assignees

@jibi jibi

@joestringer joestringer

Labels
area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. feature/bandwidth-manager Impacts BPF bandwidth manager. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
1.10.3
Backport done to v1.10
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@bmcustodio @qmonnet @borkmann @pchaigno @jibi @joestringer @aanm