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

Bug fix for auto-resizer 1.8.1 not reading command arguments #559

Merged

Conversation

tesharp
Copy link

@tesharp tesharp commented Jan 18, 2018

Cpu and memory arguments from command line are not read at all, leaving them empty. nannyConfigurationFromFlags sets default values to argument values. In loadNannyConfiguration it attempts to read from config, but if config is not set it just creates an empty configMapConfig. Since this is empty calling nannyconfigalpha.FillInDefaults_NannyConfiguration doesn't actually do anything as it compares the values against default values, so when all values are empty this does nothing. In the end function just returns empty values for everything and never uses command line arguments.

Starting container in acs-engine leads to these errors now:

ERROR: logging before flag.Parse: I0118 13:57:09.157760       1 pod_nanny.go:64] Invoked by [/pod_nanny --cpu=80m --extra-cpu=0.5m --memory=140Mi --extra-memory=4Mi --threshold=5 --deployment=heapster --container=heapster --poll-period=300000 --estimator=exponential]
ERROR: logging before flag.Parse: I0118 13:57:09.157850       1 pod_nanny.go:76] Watching namespace: kube-system, pod: heapster-548bd647cc-7fpgv, container: heapster.
ERROR: logging before flag.Parse: I0118 13:57:09.157856       1 pod_nanny.go:77] storage: MISSING, extra_storage: 0Gi
ERROR: logging before flag.Parse: I0118 13:57:09.159734       1 pod_nanny.go:162] Failed to read data from config file "MISSING/NannyConfiguration": open MISSING/NannyConfiguration: no such file or directory, using default parameters
ERROR: logging before flag.Parse: I0118 13:57:09.159804       1 pod_nanny.go:166]
ERROR: logging before flag.Parse: I0118 13:57:09.159917       1 pod_nanny.go:101] cpu: , extra_cpu: , memory: , extra_memory:
panic: cannot parse '': quantities must match the regular expression '^([+-]?[0-9.]+)([eEinumkKMGTP]*[-+]?[0-9]*)$'

@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


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. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 18, 2018
…n empty values from function, because FillInDefaults_ will not set any values from defaultConfig because configMapConfig has empty string values
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jan 18, 2018
@MaciekPytel
Copy link
Contributor

cc: @kawych @x13n

@@ -163,6 +163,7 @@ func loadNannyConfiguration(configDir string, defaultConfig *nannyconfigalpha.Na
} else if configMapConfig, err = decodeNannyConfiguration(data, scheme, codecs); err != nil {
glog.V(0).Infof("Unable to decode Nanny Configuration from config map, using default parameters")
}
nannyconfigalpha.SetDefaults_NannyConfiguration(configMapConfig)
glog.Infof("%s", configMapConfig.BaseCPU)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please delete this line, it doesn't look helpful.

Copy link
Author

Choose a reason for hiding this comment

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

Line 167?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes

@@ -163,6 +163,7 @@ func loadNannyConfiguration(configDir string, defaultConfig *nannyconfigalpha.Na
} else if configMapConfig, err = decodeNannyConfiguration(data, scheme, codecs); err != nil {
glog.V(0).Infof("Unable to decode Nanny Configuration from config map, using default parameters")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to set:
configMapConfig = &nannyconfigalpha.NannyConfiguration{}
Otherwise configMapConfig will be nil and line 166 will fail.

@kawych
Copy link
Contributor

kawych commented Jan 23, 2018

/lgtm
Please squash commits

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 23, 2018
@mwielgus mwielgus merged commit 9d5c044 into kubernetes:addon-resizer-release-1.8 Jan 23, 2018
@richerve
Copy link

richerve commented Feb 27, 2018

I'm seeing the same issue, even in 1.8.1

prometheus-operator/prometheus-operator#997

I didn't notice that this is not included in 1.8.1 but is a fix for that version. When is the new version going to be deployed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/addon-resizer cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants