remove safeToStartCycle check #91
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MinyiZ
changed the title
MinyiZ
force-pushed
the
mzhong/KUBE-9199-remove-prom-metrics-check-for-safetocycle
branch
4 times, most recently
from
c95603b to
92b795b
Compare
MinyiZ
force-pushed
the
mzhong/KUBE-9199-remove-prom-metrics-check-for-safetocycle
branch
from
424f027 to
2d93170
Compare
| nodeStartupTime: rootCmd.PersistentFlags().Duration("node-startup-time", 2*time.Minute, "duration to wait after a cluster-autoscaler scaleUp event is detected"), | ||
| runImmediately: rootCmd.PersistentFlags().Bool("now", false, "makes the check loop run straight away on program start rather than wait for the check interval to elapse"), | ||
| runOnce: rootCmd.PersistentFlags().Bool("once", false, "run the check loop once then exit. also works with --now"), | ||
| prometheusAddress: rootCmd.PersistentFlags().String("prometheus-address", "prometheus", "Prometheus service address used to query cluster-autoscaler metrics"), |
There was a problem hiding this comment.
discuss: is it worth allowing --prometheus-address to remain as a flags but be unused, in order to avoid erroring out people's installations when they automatically bump the version, or is it better for the version bump to immediately show this feature isn't used any more by this error message stopping it starting?
docker run --rm -it ghcr.io/atlassian-labs/cyclops:v1.10.1 cyclops --prometheus-address 127.0.0.1:8080
cyclops: error: unknown long flag '--prometheus-address', try --help
There was a problem hiding this comment.
i think i prefer to remove the flag immediately because having a flag that does nothing can be confusing. leaving it would also require us to remember to remove it later. since this is already a breaking change, i think it's better to alert users to the change in behaviour right away with the flag removal, rather than silently ignoring it.
mwhittington21
approved these changes
Jan 20, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This section of code does not behave as intended. It was introduced in 2020 to attempt to fix a race condition between the observer and cluster autoscaler. Due to recent improvements, the race condition is now solved so this code can be removed.
Why does this code not work?
From https://github.com/kubernetes/autoscaler/blob/master/cluster-autoscaler/proposals/metrics.md#cluster-autoscaler-execution
The
last_activitymetric reflects whether cluster autoscaler has attempted to scale up or down. However, it does not necessarily indicate actual changes in the ASG, resulting in numerous false positives. This behaviour can unnecessarily block the creation of CNRs, even when it is safe to proceed with cycling.