Introduce scheduled event checking for Azure VMs via instance metadata #9170
Conversation
|
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. 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-sigs/prow repository. |
k8s-ci-robot
added
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: kon-angelo The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @kon-angelo. 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 Once the patch is verified, the new status will be reflected by the 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-sigs/prow repository. |
github-actions
bot
added
the
tide/merge-method-squash
k8s-ci-robot
added
the
size/XL
tests/go.mod
Outdated
| github.com/Azure/azure-kusto-go/azkustoingest v1.0.3 | ||
| github.com/Azure/azure-sdk-for-go/sdk/azcore v1.18.0 | ||
| github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v6 v6.4.0 | ||
| github.com/Azure/azure-kusto-go/azkustodata v1.0.0-preview-5 |
There was a problem hiding this comment.
Why the downgrade in these library versions?
| @@ -133,7 +134,7 @@ type Config struct { | |||
| // `nodeIP`: vm private IPs will be attached to the inbound backend pool of the load balancer; | |||
| // `podIP`: pod IPs will be attached to the inbound backend pool of the load balancer (not supported yet). | |||
| LoadBalancerBackendPoolConfigurationType string `json:"loadBalancerBackendPoolConfigurationType,omitempty" yaml:"loadBalancerBackendPoolConfigurationType,omitempty"` | |||
| // PutVMSSVMBatchSize defines how many requests the client send concurrently when putting the VMSS VMs. | |||
| // PutVMSSVMBatchSize defines how many reque hssts the client send concurrently when putting the VMSS VMs. | |||
| @@ -0,0 +1,139 @@ | |||
| /* | |||
| Copyright 2019 The Kubernetes Authors. | |||
k8s-ci-robot
added
size/L
| @@ -135,6 +137,7 @@ func (o *CloudNodeManagerOptions) Flags() cliflag.NamedFlagSets { | |||
| fs.Int32Var(&o.ClientConnection.Burst, "kube-api-burst", 30, "Burst to use while talking with kubernetes apiserver.") | |||
| fs.BoolVar(&o.WaitForRoutes, "wait-routes", false, "Whether the nodes should wait for routes created on Azure route table. It should be set to true when using kubenet plugin.") | |||
| fs.BoolVar(&o.UseInstanceMetadata, "use-instance-metadata", true, "Should use Instance Metadata Service for fetching node information; if false will use ARM instead.") | |||
| fs.BoolVar(&o.EnableNodeEventChecker, "enable-node-event-checker", true, "Should enable the NodeEventChecker to check for Azure scheduled events. Can only be set to true if --use-instance-metadata is also true. If false, the NodeEventChecker will not run and no events will be recorded in the node status.") | |||
There was a problem hiding this comment.
Can we default to false, and enable as needed?
There was a problem hiding this comment.
+1. we'd need user to enable this feature by the new flag
| @@ -255,6 +272,42 @@ func (ims *InstanceMetadataService) getLoadBalancerMetadata() (*LoadBalancerMeta | |||
| return &obj, nil | |||
| } | |||
|
|
|||
| func (ims *InstanceMetadataService) GetScheduledEvents() (*EventResponse, error) { | |||
| req, err := http.NewRequest("GET", ims.imdsServer+consts.ImdsScheduledEventsURI, nil) | |||
| if err != nil { | |||
There was a problem hiding this comment.
would the request fail if there are no scheduled events? And would it fail during node bootstrap?
I'm wondering whether we should skip the errors in the caller side as failures here may block node lifecycle logic
| @@ -20,6 +20,7 @@ import ( | |||
| "strings" | |||
|
|
|||
| "sigs.k8s.io/cloud-provider-azure/pkg/azclient/configloader" | |||
|
|
|||
What type of PR is this?
/kind feature
What this PR does / why we need it:
Implement an additional polling for scheduled events via the instance metadata service and exposes that though the node conditions
Which issue(s) this PR fixes:
Fixes ##9169
Special notes for your reviewer:
Drafted until test implementation finishes
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: