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

selector:MatchLabel #249

Merged
merged 2 commits into from Aug 31, 2020
Merged

Conversation

manavellamnimble
Copy link
Contributor

@marccampbell @divolgin @dexhorthy Morning! I am sending a PR to solve issue #159 . I desing the selector label to accept further expansions in the future. Under matchLabel, multiple labels can be used to filter the nodes. The final structure would be something like this:

- nodeResources:
        checkName: Must have at least 3 nodes in the cluster, with cpuCapacity of 2 and label kubernetes.io/hostname=docker-desktop
        filters:
            cpuCapacity: "2"
            selector: 
                matchLabel: 
                    kubernetes.io/hostname  : docker-desktop
        outcomes:
        - fail:
            when: "count() < 3"
            message: This application requires at least 3 nodes.
            uri: https://kurl.sh/docs/install-with-kurl/adding-nodes
        - warn:
            when: "count() < 5"
            message: This application recommends at last 5 nodes.
            uri: https://kurl.sh/docs/install-with-kurl/adding-nodes
        - pass:
            message: This cluster has enough nodes.

Copy link

@codelingo codelingo bot left a comment

Choose a reason for hiding this comment

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

LGTM.

@marccampbell
Copy link
Member

@manavellamnimble the matchLabel dictionary feels a little foreign to me, compared to the rest of the Kubernetes API.

If we can match the format of Kubernetes native objects for label selectors (https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/) it will be easier for people to implement because we won't be asking them to change the format and they can re-use existing YAML selectors.

Is is possible to use the standard key=value types here, and use the selector package from client-go to parse so that we match the format and results the same?

@manavellamnimble
Copy link
Contributor Author

@marccampbell In fact I used a map based on what you mentioned. K8s api uses mostly maps (I called it dictionary, my bad). In the docs there is an example for selectors, following the same structure:

selector:
  matchLabels:
    component: redis

matchLabels is a map of {key,value} pairs.
src: https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#resources-that-support-set-based-requirements

Regarding using the client, that was also my first approach, but the matchFilter() func uses the info stored in the file nodes.json (created earlier by the collector), thats why I use node.Labels (which is also a map).

Nevertheless, I can adjust the labels as you prefer, it is really not lots of work. Let me know what you think!

if filters.Selector != nil {
for k, v := range filters.Selector.MatchLabel {
if l, found := node.Labels[k]; !found || l != v {
return false, errors.New(fmt.Sprintf("failed to match label %s", k))
Copy link
Member

Choose a reason for hiding this comment

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

errors.Errorf

@marccampbell
Copy link
Member

Thanks! This lgtm, 1 comment on cleaning up an error string before we can merge

@manavellamnimble
Copy link
Contributor Author

@marccampbell Great! Done!

Copy link

@codelingo codelingo bot left a comment

Choose a reason for hiding this comment

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

LGTM.

@divolgin divolgin merged commit b1e251e into replicatedhq:master Aug 31, 2020
@manavellamnimble manavellamnimble deleted the matchLabel branch August 31, 2020 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants