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

Add style and technical guidelines for SIG Scheduling #7475

Merged
merged 2 commits into from Aug 19, 2023
+45 −1
Merged

Add style and technical guidelines for SIG Scheduling #7475

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6d24885
Add style and technical guidelines for SIG Scheduling
alculquicondor Aug 17, 2023
89e9132
Add more design and testing guidelines
alculquicondor Aug 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
46 changes: 45 additions & 1 deletion sig-scheduling/CONTRIBUTING.md
View file
Open in desktop
Expand Up @@ -37,7 +37,8 @@ also follow the requirements of `Bug Report`, it will help us a lot when analyzi
* If there's any debate on the rationalities, you can bring it to the [SIG meeting](https://github.com/kubernetes/community/tree/master/sig-scheduling#meetings).
* If there are multiple implementation options, consider creating a doc with Pros and Cons, and gather some feedback.
You can do this by sharing the doc with the SIG's [mailing list](https://groups.google.com/forum/#!forum/kubernetes-sig-scheduling).
* If necessary, open an issue in kubernetes/enhancements and write a [KEP](https://github.com/kubernetes/enhancements/tree/master/keps/sig-scheduling) for it.
* Any feature that requires an API change or significant refactoring
should be preceded by a [Kubernetes Enhancement Proposal (KEP)](https://github.com/kubernetes/enhancements/tree/master/keps/sig-scheduling).

* If you find any out-of-date documentation, please help the community correct that by either sending a PR to
update the docs or open an issue if you are not sure about how to fix it.
Expand Down Expand Up @@ -72,6 +73,49 @@ to the issue/PR, which helps to preserve the context.

* Always open an issue for a TODO or a follow-up just in case you forget it.

### Technical and style guidelines
alculquicondor marked this conversation as resolved.
Show resolved Hide resolved

The following guidelines apply primarily to kube-scheduler, but some subprojects
might also adhere to them.

When designing a feature, think about components that depend on kube-scheduler
code, such as [cluster-autoscaler](https://github.com/kubernetes/autoscaler/tree/master/cluster-autoscaler)
or [scheduler-plugins](https://github.com/kubernetes-sigs/scheduler-plugins).
Also consider interactions with other core components such as [kubelet](https://kubernetes.io/docs/reference/command-line-tools-reference/kubelet/).

When coding:
- Follow [effective go](https://go.dev/doc/effective_go) guidelines.
- Use [contextual logging](https://git.k8s.io/community/contributors/devel/sig-instrumentation/migration-to-structured-logging.md#contextual-logging-in-kubernetes).
Some packages might still be using [structured logging](https://git.k8s.io/community/contributors/devel/sig-instrumentation/logging.md).
- When writing APIs, follow [k8s API conventions](https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md)
- Naming:
- Length: As a rule-of-thumb, the length of a variable name should be
proportional to the size of the scope where it is used and inversely
proportional to the number of times that it is used.

Testing:
- Unit tests: every change should have high coverage by unit tests.
- Integration tests: should cover interactions between the different components
of kube-scheduler (event handlers, queue, cache, scheduling cycles) and
kube-apiserver.
- E2E tests: should cover interactions with other components, such as kubelet,
kube-controller-manager, etc.
- [Perf tests](https://github.com/kubernetes/kubernetes/tree/master/test/integration/scheduler_perf):
should be considered for critical and/or CPU intensive operations.
- General guidelines:
- Follow a [DAMP principle](https://stackoverflow.com/a/11837973).
Copy link
Member

Choose a reason for hiding this comment

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

do you think we should specifically guide users how to write different kinds of tests? basically we would expect the amount of tests to cover a feature is expected to be UT > integration >> e2e. For reproducing a bug, we would be inclined towards writing a UT or integration test over e2e.

Copy link
Member

Choose a reason for hiding this comment

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

Also, I'd like to somehow mention about scheduler_perf (in cases of k/k), like share the result if the author thinks that this PR might have an impact on the performance (or the reviewer asks)

Copy link
Member Author

Choose a reason for hiding this comment

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

Added both.

- Use `cmp.Diff` instead of `reflect.Equal`, to provide useful comparisons.
- Compare errors using `errors.Is` (`cmpopts.EquateErrors` when using
`cmp.Diff`) instead of comparing the error strings.
- Leverage existing utility functions from `pkg/scheduler/testing`.
- Avoid creating or using assertion libraries.
Use standard `t.Error` or `t.Fatal`, as necessary.
- `gomega` and `ginkgo` should only be used in E2E tests.

alculquicondor marked this conversation as resolved.
Show resolved Hide resolved
alculquicondor marked this conversation as resolved.
Show resolved Hide resolved
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 we may want to mention that in tests, comparing the error using errors.Is() (cmpopts.EquateErrors()) instead of comparing the error message if it is possible.

WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes :)

Copy link
Contributor

Choose a reason for hiding this comment

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

The "if it is possible" didn't make it into the merged text.

@tenzen-y: you were probably thinking of packages that define certain errors as part of their API and then testing that really those errors are returned when they should be returned, right? That is what callers of the package check for via errors.Is.

If a package has no defined errors, then a unit test can only check that some error is returned when a failure is expected.

Copy link
Member Author

Choose a reason for hiding this comment

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

Then the question should be why is the error not defined.

But I would be ok having a catch-all error that can be used as value for wantErr when you really don't care.

Note that some existing code might be in violation of these guidelines, as it
might have been written before these guidelines were established. Feel free to
open PRs to get the code up to the standard.

## Use of @mentions

* @kubernetes/sig-scheduling-api-reviews - API Changes and Reviews
Expand Down