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

feat: report both old and new styles of node OS information #479

Merged
merged 1 commit into from
Mar 20, 2023

Conversation

allxiao
Copy link
Contributor

@allxiao allxiao commented Mar 6, 2023

spec.template.spec.nodeSelector[beta.kubernetes.io/os]: deprecated since v1.14; use "kubernetes.io/os" instead

To maintain compatibility, the virtual node should declare both styles of OS information, so that the Pod being scheduled to the virtual node can choose any of the following options

  • use the legacy style beta.kubernetes.io/os nodeSelector
  • migrate to use the new kubernetes.io/os nodeSelector before the beta one is dropped completely from API server

Currently, we only report beta.kubernetes.io/os, which will block the users from working on the nodeSelector migration.

@allxiao allxiao temporarily deployed to test March 6, 2023 06:02 — with GitHub Actions Inactive
@allxiao allxiao temporarily deployed to test March 6, 2023 06:02 — with GitHub Actions Inactive
@codecov-commenter
Copy link

Codecov Report

Merging #479 (8715d35) into master (0c76852) will increase coverage by 1.11%.
The diff coverage is 49.70%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##           master     #479      +/-   ##
==========================================
+ Coverage   42.91%   44.02%   +1.11%     
==========================================
  Files          50       50              
  Lines        5735     5944     +209     
==========================================
+ Hits         2461     2617     +156     
- Misses       3017     3071      +54     
+ Partials      257      256       -1     
Impacted Files Coverage Δ
pkg/featureflag/feature_flag.go 75.00% <ø> (+20.00%) ⬆️
pkg/network/aci_network.go 20.43% <7.74%> (-6.10%) ⬇️
pkg/auth/auth.go 60.32% <44.68%> (-6.35%) ⬇️
pkg/metrics/metrics.go 75.18% <57.14%> (-1.56%) ⬇️
pkg/provider/aci.go 44.59% <65.85%> (+10.70%) ⬆️
pkg/provider/aci_volumes.go 45.19% <70.58%> (ø)
pkg/provider/containergroup_to_pod.go 71.93% <75.80%> (-3.35%) ⬇️
pkg/analytics/analytics.go 64.70% <100.00%> (ø)
pkg/provider/vk_node.go 88.67% <100.00%> (+0.56%) ⬆️
provider/aci.go 37.97% <100.00%> (+0.19%) ⬆️
... and 5 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@allxiao allxiao changed the title report both old and new styles of node OS information feat: report both old and new styles of node OS information Mar 6, 2023
@Fei-Guo
Copy link
Collaborator

Fei-Guo commented Mar 6, 2023

I'd rather go with just "beta.kubernetes.io/os" label, the current k8s API version of VK is still low.

@allxiao
Copy link
Contributor Author

allxiao commented Mar 7, 2023

Hi @Fei-Guo

The beta.kubernetes.io/os label was deprecated since v1.14. Now the K8s API version for VK is 1.19. I think we should follow the deprecation process to add the support for the kubernetes.io/os label.

The labels on node defines the capabilities for a node. When we are in such kind of label deprecation, the node should declare it supports both labels for best compatibility, so that the users who want to schedule pods to VK can choose any one of the following:

  1. use the legacy beta.kubernetes.io/os nodeSelector
  2. migrate to use the kubernetes.io/os nodeSelector before the beta one is removed completely

When we only provide beta.kubernetes.io/os, the second one won't work, which blocks the user from preparing for the label deprecation and removal.

@allxiao allxiao temporarily deployed to test March 8, 2023 04:16 — with GitHub Actions Inactive
@allxiao allxiao temporarily deployed to test March 8, 2023 04:16 — with GitHub Actions Inactive
@allxiao allxiao temporarily deployed to test March 9, 2023 04:34 — with GitHub Actions Inactive
@allxiao allxiao temporarily deployed to test March 9, 2023 04:34 — with GitHub Actions Inactive
@Fei-Guo
Copy link
Collaborator

Fei-Guo commented Mar 9, 2023

Thanks. I think the "beta.kubernetes.io/os" label already exists in vNode object. We just miss the ""kubernetes.io/os" label.
It seems the node-cli does update the label
https://github.com/virtual-kubelet/node-cli/blob/85167f75d00837452dbd6efe6e51512cdc65615e/internal/commands/root/node.go#L62

Since Heba is working on remove node cli dependency from azure-aci repo and I think she can add the new label in the change.
@helayoty.

@helayoty
Copy link
Member

@allxiao may I ask you to rebase your PR?

@helayoty helayoty added this to the 1.6.0 milestone Mar 16, 2023
@helayoty helayoty added the kind/enhancement New feature or request label Mar 16, 2023
@helayoty helayoty linked an issue Mar 16, 2023 that may be closed by this pull request
@allxiao allxiao temporarily deployed to test March 18, 2023 16:36 — with GitHub Actions Inactive
@allxiao allxiao temporarily deployed to test March 18, 2023 16:36 — with GitHub Actions Inactive
Copy link
Member

@helayoty helayoty left a comment

Choose a reason for hiding this comment

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

lgtm

@helayoty helayoty merged commit cabfe7e into virtual-kubelet:master Mar 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

beta.kubernetes.io/os deprecated
4 participants