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

Drop support for hooks #856

Closed
marquiz opened this issue Aug 15, 2022 · 24 comments
Closed

Drop support for hooks #856

marquiz opened this issue Aug 15, 2022 · 24 comments
Labels
kind/feature Categorizes issue or PR as related to a new feature.
Milestone
v0.17

Comments

@marquiz
Copy link
Contributor

marquiz commented Aug 15, 2022 •
edited
Loading

What would you like to be added:

Deprecate and eventually remove support for hooks in the local feature source.

Why is this needed:

In #855 @eero-t had a merituos pondering why hooks should removed. Some rationale why hooks are bad and we want to disable them:

  • container image maintenance and size
  • feature files provide basically the same functionality with a more controllable/maintainable approach
  • possible security issues
  • resource consumption of buggy or bad-behaving hooks

Plan

  • n (v0.12): declare hooks as deprecated, introduce config option to disable them but have them still enabled by default (Disable hook support by default #855)
  • n+1: disable hooks by default but have still the option to enable them
  • n+2: drop hooks support entirely, make minimal image the default image, drop the big image or e.g. rename it to -debug
@marquiz marquiz added the kind/feature Categorizes issue or PR as related to a new feature. label Aug 15, 2022
@eero-t
Copy link

eero-t commented Aug 15, 2022

n+2: drop hooks support entirely, make minimal image the default image, drop the big image or e.g. rename it to -debug

I would propose NFD to drop the big image completely, and add an initcontainer that removes host source.d/ subdirectory content, if it exists ("os.removeAll(path)"). All this should of course have prominent warnings in NFD release notes and worker log.

I.e. if one wants to continue using hooks, one would need to restart things installing hooks, in addition to downgrading back to NFD version supporting them.

n+X release would eventually drop the hook removal initcontainer.

@mythi Any comments from the Intel device plugins (which are currently using hooks) maintenance side?

This was referenced Aug 15, 2022
Closed
Closed
@eero-t
Copy link

eero-t commented Aug 15, 2022

I would propose that:

@marquiz marquiz mentioned this issue Aug 16, 2022
Closed
@mythi
Copy link
Contributor

mythi commented Aug 17, 2022

@mythi Any comments from the Intel device plugins (which are currently using hooks) maintenance side?

running the source binaries in our initContainers (instead of using the initContainers to install the binaries) and piping the output to, e.g., tee /etc/kubernetes/node-feature-discovery/features.d/foo.feature should work equally well.

@eero-t eero-t mentioned this issue Aug 18, 2022
Closed
@ArangoGutierrez
Copy link
Contributor

I have had sooooo many conversations recently, that I can't not support this issue moaaaarrr.
👍🏽 👍🏽

@eero-t eero-t mentioned this issue Aug 24, 2022
Merged
eero-t added a commit to eero-t/intel-device-plugins-for-kubernetes that referenced this issue Aug 26, 2022
@eero-t
When changing volume names, controller needs to be changed too
e1d6928 
Change volume name to a more generic one before the NFD host
mount directory itself needs to be changed:
kubernetes-sigs/node-feature-discovery#856

Signed-off-by: Eero Tamminen <[email protected]>
eero-t added a commit to eero-t/intel-device-plugins-for-kubernetes that referenced this issue Aug 29, 2022
@eero-t
WIP: Change GPU plugin NFD init container to run-time container
fa7cfa7 
To work around kustomize inability to enforce correct init container,
and because this is more likely way how it will work once NFD drops
support for hooks:
kubernetes-sigs/node-feature-discovery#856

Signed-off-by: Eero Tamminen <[email protected]>
eero-t added a commit to eero-t/intel-device-plugins-for-kubernetes that referenced this issue Aug 29, 2022
@eero-t
WIP: Change GPU plugin NFD init container to run-time container
eeb989d 
Because:
* To work around kustomize inability to enforce correct init container order
* This is more likely how things will work once NFD drops support for hooks:
  kubernetes-sigs/node-feature-discovery#856

Signed-off-by: Eero Tamminen <[email protected]>
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 21, 2022
@marquiz
Copy link
Contributor Author

marquiz commented Nov 21, 2022

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 21, 2022
@marquiz marquiz mentioned this issue Nov 22, 2022
Closed
eero-t added a commit to eero-t/intel-device-plugins-for-kubernetes that referenced this issue Dec 12, 2022
@eero-t
Use more generic name for NFD features host directory volume
2177d12 
Before adding kustomize file changing it from feature hooks directory
to feature files directory (which is needed to support scalability
testing with fake devices):
kubernetes-sigs/node-feature-discovery#856

When changing volume names, controller needs to be changed too.

Signed-off-by: Eero Tamminen <[email protected]>
eero-t added a commit to eero-t/intel-device-plugins-for-kubernetes that referenced this issue Dec 12, 2022
@eero-t
Use more generic name for NFD features host directory volume
a1a81e5 
Before adding kustomize file changing it from feature hooks directory
to feature files directory, which is needed to support scalability
testing with fake devices, and for:
kubernetes-sigs/node-feature-discovery#856

When changing volume names, controller needs to be changed too.

Signed-off-by: Eero Tamminen <[email protected]>
eero-t added a commit to eero-t/intel-device-plugins-for-kubernetes that referenced this issue Dec 12, 2022
@eero-t
Add kustomization for using GPU plugin with fake devices
7e7c54d 
Change GPU plugin NFD init container to run-time container:
* To work around kustomize inability to enforce correct init container order
* This is more likely how things will work once NFD drops support for hooks:
  kubernetes-sigs/node-feature-discovery#856

Signed-off-by: Eero Tamminen <[email protected]>
eero-t added a commit to eero-t/intel-device-plugins-for-kubernetes that referenced this issue Dec 30, 2022
@eero-t
Use more generic name for NFD features host directory volume
c442143 
Before adding kustomize file changing it from feature hooks directory
to feature files directory (which is needed to support scalability
testing with fake devices):
kubernetes-sigs/node-feature-discovery#856

When changing volume names, controller needs to be changed too.

Signed-off-by: Eero Tamminen <[email protected]>
@ArangoGutierrez ArangoGutierrez mentioned this issue Jan 31, 2023
Merged
eero-t added a commit to eero-t/intel-device-plugins-for-kubernetes that referenced this issue Feb 8, 2023
@eero-t
Use more generic name for NFD features host directory volume
2f3dc23 
NFD hooks are deprecated and going away:
kubernetes-sigs/node-feature-discovery#856

This makes the mount names more future-proof, and shows where later
changes need to be done (to change operator mount directory, and
switch hook-using deployments e.g. to feature files).

Signed-off-by: Eero Tamminen <[email protected]>
@eero-t eero-t mentioned this issue Feb 8, 2023
Merged
eero-t added a commit to eero-t/intel-device-plugins-for-kubernetes that referenced this issue Feb 13, 2023
@eero-t
Add kustomization for using GPU plugin with fake devices
da4fb39 
Change GPU plugin NFD init container to run-time container:
* To work around kustomize inability to enforce correct init container order
* This is more likely how things will work once NFD drops support for hooks:
  kubernetes-sigs/node-feature-discovery#856

Signed-off-by: Eero Tamminen <[email protected]>
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue as fresh with /remove-lifecycle stale
  • Close this issue with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 19, 2023
@ArangoGutierrez
Copy link
Contributor

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 20, 2023
@marquiz
Copy link
Contributor Author

marquiz commented Jun 2, 2023

This will still take a few releases to completely drop the hooks, I think

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 2, 2023
@marquiz
Copy link
Contributor Author

marquiz commented Sep 8, 2023

Hooks were marked as deprecated in v0.12, we switched to the "minimal" container image as a default in v0.13 and hooks were now disabled by default in v0.14. As per our deprecation policy I think we could drop them completely already in v0.15 but I don't know if we have such a hurry (as they're now disabled anyway) so we could wait one more release for possible users to migrate off them.
/milestone v0.16

@k8s-ci-robot k8s-ci-robot added this to the v0.16 milestone Sep 8, 2023
@eero-t
Copy link

eero-t commented Sep 8, 2023

v0.15 release could include note that it will be dropped in v0.16:

Upcoming API changes:

  • Deprecated hook support will be dropped in v0.16 release

@marquiz
Copy link
Contributor Author

marquiz commented Sep 8, 2023

v0.15 release could include note that it will be dropped in v0.16:

Definitely. And if people are in (in dropping in v0.16), we could even put it in the deprecation note in the documentation that hook support is slated for complete removal in v0.16.

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue as fresh with /remove-lifecycle stale
  • Close this issue with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 27, 2024
@marquiz
Copy link
Contributor Author

marquiz commented Jan 29, 2024

Ach, we forgot to explicitly mention the removal of hooks in the v0.15 release notes 🤦‍♂️ Maybe we can keep them for one release and in v0.16. They are disabled by default, anyway, so there's no major practical impact

/remove-lifecycle stale
/milestone v0.17

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 29, 2024
@k8s-ci-robot k8s-ci-robot modified the milestones: v0.16, v0.17 Jan 29, 2024
@marquiz
Copy link
Contributor Author

marquiz commented Jan 29, 2024

Document the upcoming removal of hooks:
#1573

@eero-t eero-t mentioned this issue Jan 29, 2024
Merged
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue as fresh with /remove-lifecycle stale
  • Close this issue with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 28, 2024
@marquiz
Copy link
Contributor Author

marquiz commented Apr 29, 2024

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 29, 2024
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue as fresh with /remove-lifecycle stale
  • Close this issue with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 28, 2024
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue as fresh with /remove-lifecycle rotten
  • Close this issue with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Aug 27, 2024
@marquiz
Copy link
Contributor Author

marquiz commented Aug 27, 2024

/remove-lifecycle rotten

@k8s-ci-robot k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Aug 27, 2024
@ArangoGutierrez
Copy link
Contributor

Document the upcoming removal of hooks: #1573

since #1573 is merged, what else is missing to drop this feature?

@marquiz
Copy link
Contributor Author

marquiz commented Nov 4, 2024

since #1573 is merged, what else is missing to drop this feature?

There should be nothing blocking. Just drop the code and documentation and that should be it.

@ArangoGutierrez
Copy link
Contributor

Ok #1941 will mark this one as done

@ArangoGutierrez ArangoGutierrez mentioned this issue Nov 4, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Milestone
v0.17
Development

No branches or pull requests
6 participants
@marquiz @mythi @eero-t @ArangoGutierrez @k8s-ci-robot @k8s-triage-robot