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

Update install doc to match release v0.19.8 #163

Merged
merged 1 commit into from
Mar 24, 2021

Conversation

Huang-Wei
Copy link
Contributor

@Huang-Wei Huang-Wei commented Mar 16, 2021

Part of #161.

Updated the installation doc to match release v0.19.8.

Fixes #150.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 16, 2021
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 16, 2021
@Huang-Wei
Copy link
Contributor Author

cc @seanmalloy @xq2005

/assign @cwdsuzhou @denkensk

doc/install.md Outdated
scheduleTimeoutSeconds: 10
```

<< UNSOLVED >>: It's expected to get `status` in the above yaml. Needs debug.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cwdsuzhou @denkensk could you help investigate why the status is not displayed? Better to compare the RBAC/CRD with your production settings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After appending the 2nd commit, a PodGroup with matching Pods can display its status, but still empty for a PodGroup without any matching Pods.

@@ -0,0 +1,83 @@
# First part
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cwdsuzhou @denkensk could you help compare the RBAC/CRD with your production settings? to ensure the correctness and the privileges are not over-exposed. Thanks in advance.

Copy link
Contributor

@cwdsuzhou cwdsuzhou Mar 17, 2021

Choose a reason for hiding this comment

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

Hi, coscheduling works ok in my cluster using the the RBAC(changed in this PR) and CRD

image

Since pod group controller is not necessary now, we did not deploy in our production environment.

I verified it in my test environment, when I deployed the controller, the status changed.

image

@Huang-Wei
Copy link
Contributor Author

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 16, 2021
resources: ["pods"]
verbs: ["get", "list", "watch"]
- apiGroups: ["scheduling.sigs.k8s.io"]
resources: ["podgroups", "podgroups/status", "elasticquotas", "elasticquotas/status"]
Copy link
Contributor

Choose a reason for hiding this comment

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

we have not enabled status subresource of podgroup, so podgroups/status is not necessary

Copy link
Contributor Author

@Huang-Wei Huang-Wei Mar 17, 2021

Choose a reason for hiding this comment

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

thanks. Could you help verify that when a PodGroup is created (without any matching pods), is its status displayed properly?

Updated: after I updated controller SA's rbac setting (replace Update with Patch), it shows properly. But it's not showing the number:

...
spec:
  minMember: 3
  scheduleTimeoutSeconds: 10
status:
  phase: Pending

And if I create a deployment with 2 pods, the phase gets changed to PreScheduling, but if I delete the deployment, its phase stays in PreScheduling (I'd expect it to be updated back to Pending)

Copy link
Contributor

Choose a reason for hiding this comment

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

And if I create a deployment with 2 pods, the phase gets changed to PreScheduling, but if I delete the deployment, its phase stays in PreScheduling (I'd expect it to be updated back to Pending)

This may be a bug the status would not be back to Pending

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This may be a bug the status would not be back to Pending

I will create an issue tracking this.

BTW: could you help confirm this issue:

after I updated controller SA's rbac setting (replace Update with Patch), it shows properly. But it's not showing the number:

Copy link
Contributor

Choose a reason for hiding this comment

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

In my test environment, as I showed #163 (comment), we can see number.

The controller is deployed based on the manifests in this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created #166 to track the UX issues.

@cwdsuzhou
Copy link
Contributor

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cwdsuzhou, Huang-Wei

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

@Huang-Wei Huang-Wei force-pushed the install-doc-update branch from 79623ef to 9bf85d9 Compare March 19, 2021 20:18
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 19, 2021
@Huang-Wei
Copy link
Contributor Author

Commits squashed and some UX issues are tracked in #166. Please take another look.

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 19, 2021
@Huang-Wei
Copy link
Contributor Author

@cwdsuzhou would you minding re-lgtm?

doc/install.md Outdated
Comment on lines 256 to 257
[install doc](https://github.com/kubernetes-sigs/scheduler-plugins/blob/release-1.19/doc/install.md) in
branch `release-1.19`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
[install doc](https://github.com/kubernetes-sigs/scheduler-plugins/blob/release-1.19/doc/install.md) in
branch `release-1.19`.
[install doc](https://github.com/kubernetes-sigs/scheduler-plugins/blob/release-1.18/doc/install.md) in
branch `release-1.18`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the catch. I put this paragraph into a section Instal lower-version releases.

@seanmalloy
Copy link
Member

/kind documentation

@k8s-ci-robot k8s-ci-robot added the kind/documentation Categorizes issue or PR as related to documentation. label Mar 23, 2021
@Huang-Wei Huang-Wei force-pushed the install-doc-update branch from 9bf85d9 to f6ed233 Compare March 23, 2021 05:27
@seanmalloy
Copy link
Member

/lgtm
/hold

Adding a hold so that @cwdsuzhou can review one more time before merge. Please feel free to remove the hold once you are ready for this to merge. Thanks!

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 23, 2021
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 23, 2021
@Huang-Wei
Copy link
Contributor Author

Kindly ping @@cwdsuzhou for taking another look.

```

1. Create a deploy labelled `pod-group.scheduling.sigs.k8s.io: pg1` to associated with PodGroup
`pg1` created in the previous step.
Copy link
Contributor

Choose a reason for hiding this comment

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

always get 1., seems not intentional

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When the list is long and frequently modified, it's not easy to always make its numbers ordered accurately :) So keep all as 1. and make them indented well, markdown will do the rest things.

@cwdsuzhou
Copy link
Contributor

minor hits. Otherwise, lgtm

@Huang-Wei
Copy link
Contributor Author

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 24, 2021
@k8s-ci-robot k8s-ci-robot merged commit c12f81e into kubernetes-sigs:master Mar 24, 2021
@Huang-Wei Huang-Wei deleted the install-doc-update branch March 24, 2021 05:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. kind/documentation Categorizes issue or PR as related to documentation. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Getting error when trying to enable coscheduler following develop instructions and tutorial
5 participants