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.

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

Add cluster role document #1796

Merged
merged 21 commits into from
Oct 18, 2022
Merged

Conversation

kevindiu
Copy link
Contributor

@kevindiu kevindiu commented Oct 4, 2022

Signed-off-by: kevindiu [email protected]

Description:

This PR include the following changes:

  • Add documentation of cluster role configuration
  • Rename assets/docs/troubleshooting/troubleshooting.drawio to assets/docs/troubleshooting/provisioning_flow_chart.drawio
  • Update troubleshooting diagram to include the flow to check role settings

Related Issue:

How Has This Been Tested?:

Environment:

  • Go Version: 1.19.1
  • Docker Version: 20.10.8
  • Kubernetes Version: 1.22.0
  • NGT Version: 1.14.7

Types of changes:

  • Bug fix [type/bug]
  • New feature [type/feature]
  • Add tests [type/test]
  • Security related changes [type/security]
  • Add documents [type/documentation]
  • Refactoring [type/refactoring]
  • Update dependencies [type/dependency]
  • Update benchmarks and performances [type/bench]
  • Update CI [type/ci]

Changes to Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully ran tests with your changes locally?

Checklist:

  • I have read the CONTRIBUTING document.
  • I have checked open Pull Requests for the similar feature or fixes?
  • I have added tests and benchmarks to cover my changes.
  • I have ensured all new and existing tests passed.
  • I have commented my code, particularly in hard-to-understand areas
  • I have updated the documentation accordingly.

@vdaas-ci
Copy link
Collaborator

vdaas-ci commented Oct 4, 2022

[CHATOPS:HELP] ChatOps commands.

  • 🙆‍♀️ /approve - approve
  • 💌 /changelog - replace the PR body by changelog details
  • 🍱 /format - format codes and add licenses
  • /gen-test - generate test codes
  • 🏷️ /label - add labels
  • /rebase - rebase main
  • 🔚 2️⃣ 🔚 /label actions/e2e-deploy - run E2E deploy & integration test

docs/user-guides/cluster-role-binding.md Outdated Show resolved Hide resolved
docs/user-guides/cluster-role-binding.md Outdated Show resolved Hide resolved
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Oct 11, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 2da065f
Status: ✅  Deploy successful!
Preview URL: https://6b0bc7e0.vald.pages.dev
Branch Preview URL: https://documentation-update-cluster.vald.pages.dev

View logs

@kevindiu kevindiu force-pushed the documentation/update-cluster-role-binding-doc branch from 7e0af67 to 290aea1 Compare October 11, 2022 02:51
@kevindiu kevindiu marked this pull request as ready for review October 11, 2022 02:58
@kevindiu kevindiu requested review from vankichi, a team and hlts2 and removed request for a team October 11, 2022 02:59
Signed-off-by: kevindiu <[email protected]>
Copy link
Contributor

@vankichi vankichi left a comment

Choose a reason for hiding this comment

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

Please fix the image.

  • ❌ Role setting valid?
  • ⭕ Is cluster role setting valid?

docs/user-guides/cluster-role-binding.md Outdated Show resolved Hide resolved
docs/user-guides/cluster-role-binding.md Outdated Show resolved Hide resolved

In Vald, the index is distributed across the cluster depending on the resource usage of the cluster, it requires settings to grant permission to a specific role to retrieve cluster information on Kubernetes.

In Vald, the settings are deployed automatically when using helm to deploy.
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 ?
It seems not logical sentence considering the below links.
could you please re-think sentence?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

docs/user-guides/cluster-role-binding.md Outdated Show resolved Hide resolved
docs/user-guides/cluster-role-binding.md Outdated Show resolved Hide resolved
docs/user-guides/cluster-role-binding.md Outdated Show resolved Hide resolved

All of these rules are required to retrieve Nodes and Pods resource usage from [kube-apiserver](https://kubernetes.io/docs/reference/command-line-tools-reference/kube-apiserver/) and also used to discover new Vald Agents or Nodes created on the cluster.

### Cluster role binding settings
Copy link
Contributor

Choose a reason for hiding this comment

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

What would you like to describe?
For cluster role binding only discoverer, or overall?
It should be formatted the section title and sentences.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed by changing the title

Signed-off-by: kevindiu <[email protected]>
@kevindiu kevindiu requested a review from vankichi October 17, 2022 01:29
docs/overview/component/discoverer.md Outdated Show resolved Hide resolved
docs/user-guides/cluster-role-binding.md Outdated Show resolved Hide resolved
docs/user-guides/cluster-role-binding.md Outdated Show resolved Hide resolved
docs/user-guides/cluster-role-binding.md Outdated Show resolved Hide resolved
docs/user-guides/cluster-role-binding.md Outdated Show resolved Hide resolved
docs/user-guides/cluster-role-binding.md Outdated Show resolved Hide resolved

These configurations allow the service account `discoverer` to access different resources in the Kubernetes cluster.

### Cluster role settings for Vald Discoverer
Copy link
Contributor

Choose a reason for hiding this comment

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

It is little bit logical scheme, why this page shows only discoverer.
It should be mentioned why we describe only discoverer.
User may not understand the flow of the story.

kevindiu and others added 2 commits October 17, 2022 15:32
Signed-off-by: kevindiu <[email protected]>
docs/user-guides/cluster-role-binding.md Outdated Show resolved Hide resolved
docs/user-guides/cluster-role-binding.md Outdated Show resolved Hide resolved
docs/user-guides/cluster-role-binding.md Outdated Show resolved Hide resolved
@kevindiu kevindiu requested a review from vankichi October 17, 2022 07:29
hlts2
hlts2 previously approved these changes Oct 17, 2022
Copy link
Collaborator

@hlts2 hlts2 left a comment

Choose a reason for hiding this comment

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

LGTM!

Signed-off-by: kevindiu <[email protected]>
@kevindiu kevindiu requested review from vankichi and hlts2 October 18, 2022 00:23
vankichi
vankichi previously approved these changes Oct 18, 2022
Signed-off-by: kevindiu <[email protected]>
kpango
kpango previously approved these changes Oct 18, 2022
Copy link
Collaborator

@kpango kpango left a comment

Choose a reason for hiding this comment

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

LGTM

@kevindiu kevindiu requested a review from vankichi October 18, 2022 02:21
@kevindiu kevindiu requested a review from kpango October 18, 2022 02:29
Copy link
Collaborator

@hlts2 hlts2 left a comment

Choose a reason for hiding this comment

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

LGTM!!

@kpango kpango merged commit 8afc3c3 into main Oct 18, 2022
@kpango kpango deleted the documentation/update-cluster-role-binding-doc branch October 18, 2022 02:53
kpango pushed a commit that referenced this pull request Oct 18, 2022
* add cluster role document

Signed-off-by: kevindiu <[email protected]>

* update flowchat and add cluster role setting for cloud providers

Signed-off-by: kevindiu <[email protected]>

* fix grammar

Signed-off-by: kevindiu <[email protected]>

* update flowchart

Signed-off-by: kevindiu <[email protected]>

* fix comment

Signed-off-by: kevindiu <[email protected]>

* fix comment

Signed-off-by: kevindiu <[email protected]>

* Update docs/user-guides/cluster-role-binding.md

* Apply suggestions from code review

Co-authored-by: Kiichiro YUKAWA <[email protected]>

* fix comment

Signed-off-by: kevindiu <[email protected]>

* Apply suggestions from code review

* Apply suggestions from code review

Co-authored-by: Kiichiro YUKAWA <[email protected]>

* fix image

Signed-off-by: kevindiu <[email protected]>

* Update docs/user-guides/cluster-role-binding.md

* Update docs/user-guides/cluster-role-binding.md

* Update cluster-role-binding.md

* fix comment

Signed-off-by: kevindiu <[email protected]>

* Update docs/user-guides/cluster-role-binding.md

* Update docs/user-guides/cluster-role-binding.md

* Update cluster-role-binding.md

Signed-off-by: kevindiu <[email protected]>
Co-authored-by: Kiichiro YUKAWA <[email protected]>
Signed-off-by: kpango <[email protected]>
@kpango kpango mentioned this pull request Oct 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants