Dynamic listing of SKUs #7239
Dynamic listing of SKUs #7239
Conversation
k8s-ci-robot
added
kind/cleanup
|
Hi @rakechill. Thanks for your PR. I'm waiting for a kubernetes 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. |
k8s-ci-robot
added
area/provider/azure
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
| @@ -1184,8 +1176,6 @@ func TestTemplateNodeInfo(t *testing.T) { | |||
| // Note: static-only workflow tests can be removed once support for dynamic is always on | |||
| msg := "no vm sku info loaded, using only static sku list" | ||
| klog.Warning(msg) | ||
| return vmssType, errors.New(msg) | ||
| } |
There was a problem hiding this comment.
It is weird to have this check within GetVMSSTypeDynamically... Being in the very beginning, wouldn't it block the very first dynamic load?
Which got me wondering how the relevant unit test passed - and the current approach has it override GetVMSSTypeDynamically altogether, so it does not actually get tested. Maybe we don't have a choice but to switch to mocking skewer/resource APIs for proper test coverage ...
There was a problem hiding this comment.
The first fetch skus happens when the cache is created (newAzureCache in azure_cache.go). As such, unless dynamic fails when initializing the cache, it should have skus and thus this should pass.
There was a problem hiding this comment.
anyway, I'm removing these changes from this PR for now 🤣
but good to think about for next iteration
|
This seems to include new changes to the fork (e.g., I see some differences in azure_cache.go, deprecation of that env var), which is outside of our scope for defork. Are you confident in those, and is it worth it? Changes will need to be made in the fork as well. |
|
Looking at the test-and-verify run (for other reasons), I noticed errors at the verify-golint step - which somehow don't fail the check; would be good to understand and fix. |
k8s-ci-robot
added
size/M
|
@comtalyst / @tallaxes -- I'll need one of you to:
|
|
/label tide/merge-method-squash |
|
@comtalyst: once the present PR merges, I will cherry-pick it on top of cluster-autoscaler-release-1.31 in a new PR and assign it to you. In response to this:
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
the
tide/merge-method-squash
|
Test failure seems to be transient /test pull-cluster-autoscaler-e2e-azure-master |
|
@rakechill: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
failed to install helm. I think this is ok for now. just need a |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: comtalyst, rakechill 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 |
k8s-ci-robot
added
the
approved
|
@comtalyst: new pull request created: #7254 In response to this:
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. |
|
@comtalyst: new pull request created: #7255 In response to this:
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. |
|
@comtalyst: new pull request created: #7256 In response to this:
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. |
|
@comtalyst: new pull request created: #7257 In response to this:
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. |
|
@comtalyst: new pull request created: #7258 In response to this:
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. |
What type of PR is this?
/kind cleanup
/kind deprecation
What this PR does / why we need it:
enableDynamicInstanceListas an env variableWhich issue(s) this PR fixes:
#462Special notes for your reviewer:
Check out the relevant issue for more details ^^
I'll also add some helpful pointers in the PR itself
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: