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 nfd-worker-conf ConfigMap to deployment templates #386

Merged
merged 2 commits into from
Nov 23, 2020

Conversation

marquiz
Copy link
Contributor

@marquiz marquiz commented Nov 18, 2020

Add a virtually empty ConfigMap that is mounted inside the workers.
Makes it easier to start customizing the worker deployment e.g. with just:

$ kubectl -n ${NFD_NS} edit configmap nfd-worker-conf

Create a new 'templates' make target for inserting the content of
nfd-worker.conf.example into the configmap spec of the templates. Thus,
'make templates' should be run whenever the example config is update.
Update the verify.sh prow script to check that the templates are up to
date.

This patch also streamlines the documentation about configuration
management, reflecting the changes.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 18, 2020
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 18, 2020
@ArangoGutierrez
Copy link
Contributor

/assign

Copy link
Contributor

@ArangoGutierrez ArangoGutierrez left a comment

Choose a reason for hiding this comment

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

The operator does the same, an empty file, maybe we could, in the name of documentation, not do it via a virtual empty but a commented file (wish has the same effect) , and will help people

name: nfd-worker-conf
namespace: node-feature-discovery
data:
nfd-worker.conf: |
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
nfd-worker.conf: |
nfd-worker.conf: |
#uncomment to configure
#sources:
# pci:
# deviceClassWhitelist:
# - "0200"
# - "03"
# - "12"
# deviceLabelFields:
# - "class"
# - "vendor"
# - "device"
# - "subsystem_vendor"
# - "subsystem_device"

name: nfd-worker-conf
namespace: node-feature-discovery
data:
nfd-worker.conf: |
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
nfd-worker.conf: |
nfd-worker.conf: |
#uncomment to configure
#sources:
# pci:
# deviceClassWhitelist:
# - "0200"
# - "03"
# - "12"
# deviceLabelFields:
# - "class"
# - "vendor"
# - "device"
# - "subsystem_vendor"
# - "subsystem_device"

name: nfd-worker-conf
namespace: node-feature-discovery
data:
nfd-worker.conf: |
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
nfd-worker.conf: |
nfd-worker.conf: |
#uncomment to configure
#sources:
# pci:
# deviceClassWhitelist:
# - "0200"
# - "03"
# - "12"
# deviceLabelFields:
# - "class"
# - "vendor"
# - "device"
# - "subsystem_vendor"
# - "subsystem_device"

@marquiz
Copy link
Contributor Author

marquiz commented Nov 18, 2020

I was thinking about something like the changes you suggested, but, decided otherwise. I wanted to have all the available options visible in the configmap, this now happens when make yamls. And, I wanted to maintain the example configmap only in one place i.e. nfd-worker.example.conf. Lastly, with your additions the result of make yamls becomes quite ugly as both that example and the content of nfd-worker.conf.example gets into the configmap.

But, on a second thought it would be really nice to have the example also in the templates. I'll sketch up something. BBL
/hold

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 18, 2020
Add a virtually empty ConfigMap that is mounted inside the workers.
Makes it easier to start customizing the worker deployment e.g. with just:

  $ kubectl -n ${NFD_NS} edit configmap nfd-worker-conf

Create a new 'templates' make target for inserting the content of
nfd-worker.conf.example into the configmap spec of the templates. Thus,
'make templates' should be run whenever the example config is update.
Update the verify.sh prow script to check that the templates are up to
date.

This patch also streamlines the documentation about configuration
management, reflecting the changes.
@marquiz marquiz force-pushed the devel/default-configmap branch from f911316 to 979d3b8 Compare November 23, 2020 16:54
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 23, 2020
@marquiz
Copy link
Contributor Author

marquiz commented Nov 23, 2020

Changed the patch so that the configmaps in templates have same data than nfd-worker.conf.example. There's a new templates target for updating them whenever the example config is updated

@marquiz
Copy link
Contributor Author

marquiz commented Nov 23, 2020

/unhold

@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 Nov 23, 2020
Copy link
Contributor

@ArangoGutierrez ArangoGutierrez left a comment

Choose a reason for hiding this comment

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

Your example is waaaay better than mine
/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ArangoGutierrez, marquiz

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

@k8s-ci-robot k8s-ci-robot merged commit def4b60 into kubernetes-sigs:master Nov 23, 2020
@marquiz marquiz deleted the devel/default-configmap branch November 23, 2020 19:47
@marquiz marquiz mentioned this pull request Nov 24, 2020
15 tasks
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. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants