node-feature-discovery

node-feature-discovery copied to clipboard

Fixes: https://github.com/kubernetes-sigs/node-feature-discovery/issues/778

Hi @fmuyassarov. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

/cc @marquiz Sorry to bother again :)

@marquiz Sorry to bother again :)

Sorry, haven't had time to concentrate on this. Just some quick thoughts below. The details and corner cases might get hairier than expected.

If we want to do this, I think we need to think about a transition plan from long to short labels. I.e. have a way to not break existing applications. This would probably mean the possibility to have both long and short labels turned on at the same time. Then, after apps have been migrated long labels could be turned off 🤔 Another consideration is that in the code cleaning up old labels we would need to take care about both the long and short labels (I think). And the (still experimental) extended resources should be handled, too.

/ok-to-test

This is a breaking change, and usually in k8s for such a change it takes ~3-4 releases to remove a feature. Example: rename "node-role.kubernetes.io/master" node-role.kubernetes.io/master label & taint renaming in Kubeadm, they had both labels in parallel that gave time to users to switch to the new format. And perhaps we can follow the same path?

For example,

Release - v0.12.X

1. introduce a short label and a flag to switch between long and short labels
2. set the flag to use long label by default
3. announce to community/users to adapt to use the short label
4. get community/users feedback

Release - v0.13.X

1. turn on both short and long labels by default but still have an option to disable them
2. get community/users feedback

Release - v0.14.X

1. Turn off long label by default but still have an option to enable it

Release - v0.15.X

1. Remove the long label entirely from the codebase and keep only short label
2. Remove the flag to switch between short and long labels

I'm not sure about the estimated time between MINOR releases in NFD project, but if you think it is too short that we take above steps in every MINOR release, then of course we can extend it. And actually better if we get users feedback on that.

Release - v0.12.X

1. introduce a short label and a flag to switch between long and short labels
2. set the flag to use long label by default
3. announce to community/users to adapt to use the short label
4. get community/users feedback

Release - v0.13.X

1. turn on both short and long labels by default but still have an option to disable them
2. get community/users feedback

Release - v0.14.X

1. Turn off long label by default but still have an option to enable it

Release - v0.15.X

1. Remove the long label entirely from the codebase and keep only short label
2. Remove the flag to switch between short and long labels

I'm not sure about the estimated time between MINOR releases in NFD project, but if you think it is too short that we take above steps in every MINOR release, then of course we can extend it. And actually better if we get users feedback on that.

Yeah, along those lines, I guess. I had something similar in my mind. Do transition over several releases and we can also back off if it turns out to be too much of an hassle 😅

✅ Deploy Preview for kubernetes-sigs-nfd ready!

Name	Link
🔨 Latest commit	18ef4e7b9d03e39471aadb82824768907bb059d7
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-nfd/deploys/634e8a2acbdc4c000887ec30
😎 Deploy Preview	https://deploy-preview-881--kubernetes-sigs-nfd.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

Rebased

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: fmuyassarov Once this PR has been reviewed and has the lgtm label, please assign marquiz for approval by writing /assign @marquiz in a comment. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

@fmuyassarov: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

PING this PR alive?

PING this PR alive?

Not at the moment. But I plan to update it hopefully this week.

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

This bot triages 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 PR is closed

You can:

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

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

/lifecycle stale

@fmuyassarov PING this PR alive?

@fmuyassarov PING this PR alive?

Hi @ArangoGutierrez. Um, it is almost dead :) Sorry that could not come to it before. But I'm willing to give it a time and get it done.

• /remove-lifecycle stale

/remove-lifecycle stale

I've started to think if we really want/need this at all. I'm not sure it's worth all the hassle, especially wrt. to breaking the "user interface", i.e. changing all labels. After thinking it again, I don't think nfd.k8s.io is necessarily a good choice, especially as we already have two subnamespaces feature.node.kubernetes.io and profile.node.kubernetes.io. We could save a few characters with feature.node.k8s.io but is it really worth it

yeah, that's one of the reasons I didn't have a motivation to finish this work, because as we discussed earlier f2f, I was also not sure if we really need it.

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

This bot triages 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 PR is closed

You can:

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

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

/lifecycle stale

yeah, that's one of the reasons I didn't have a motivation to finish this work, because as we discussed earlier f2f, I was also not sure if we really need it.

Close?

yeah, that's one of the reasons I didn't have a motivation to finish this work, because as we discussed earlier f2f, I was also not sure if we really need it.

Close?

for now at least yes, /close

@fmuyassarov: Closed this PR.

In response to this:

yeah, that's one of the reasons I didn't have a motivation to finish this work, because as we discussed earlier f2f, I was also not sure if we really need it.

Close?

for now at least yes, /close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.