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

Make nodeaffinity parsing functions return field.Error #96167

Closed
alculquicondor opened this issue Nov 3, 2020 · 7 comments · Fixed by #97538
Closed

Make nodeaffinity parsing functions return field.Error #96167

alculquicondor opened this issue Nov 3, 2020 · 7 comments · Fixed by #97538
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/feature Categorizes issue or PR as related to a new feature. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@alculquicondor
Copy link
Member

The k8s.io/component-helpers/scheduling/corev1/nodeaffinity constructors return a parsed version of the input types. While doing that, they detect parsing errors that can be aggregated in higher level validators, such as ValidateNodeAffinityArgs.

For a proper implementation, all the returned errors should be of the type field.Error.

/sig scheduling

@alculquicondor alculquicondor added the kind/feature Categorizes issue or PR as related to a new feature. label Nov 3, 2020
@k8s-ci-robot k8s-ci-robot added sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Nov 3, 2020
@ahg-g
Copy link
Member

ahg-g commented Nov 10, 2020

/help
/good-first-issue
/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Nov 10, 2020
@lingsamuel
Copy link
Contributor

These constructors raise errors mainly from labels.Requirement, which is a quite generic function. It's hard to change a fmt.Errorf to field.Error.
I noticed ValidateNodeSelectorRequirement in pkg/apis/core/validation/validation.go has similar logic compare with labels.NewRequirement, so write new validate functions instead change this constructor maybe a better choice?

@alculquicondor
Copy link
Member Author

/remove-good-first-issue

This is actually not trivial :(

@k8s-ci-robot k8s-ci-robot removed the good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. label Nov 10, 2020
@alculquicondor
Copy link
Member Author

so write new validate functions instead change this constructor maybe a better choice?

No, the whole point is to avoid code duplication. We can change all the functions that are called to return field.Error or an aggregation of them. But we have to keep the same function signatures.

@lingsamuel
Copy link
Contributor

Then I guess this will be a big refactoring task, not just sig/scheduling.

@alculquicondor
Copy link
Member Author

It's not too big, but indeed beyond scheduling OWNERship

/sig api-machinery

@k8s-ci-robot k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Nov 11, 2020
@lingsamuel
Copy link
Contributor

I am handling other funcs in pkg/scheduler/apis/config/validation/validation_pluginargs.go

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/feature Categorizes issue or PR as related to a new feature. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants