Allow custom AWS region overrides #1707 #1745
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
Seems there is an issue with the cloudprovider/aws/aws_manager_test |
bskiba
added
area/cluster-autoscaler
area/provider/aws
|
/retest |
| @@ -595,7 +595,7 @@ type CloudConfig struct { | |||
| } | |||
| } | |||
|
|
|||
| func (cfg *CloudConfig) validateOverrides() error { | |||
| func (cfg *CloudConfig) ValidateOverrides() error { | |||
There was a problem hiding this comment.
Were all the methods capitalized by syncing the vendored file or by hand?
There was a problem hiding this comment.
Yes, that is how I did it to get the change in here quickly.
There was a problem hiding this comment.
Anything under vendor dir is untouchable. Since anytime autoscaler dependencies are updated, your changes are lost. The original idea was to have every component to implement its own mechanism of overriding the custom endpoints. Once we update all relevant component, we may identify which code is generic enough to be shared among components. Right now, it means to copy paste both validateOverrides and getResolver methods from the vendor dir into cluster-autoscaler/cloudprovider/aws/aws_manager.go.
There was a problem hiding this comment.
Thanks for the input. I replicated the necessary changes from vendor into cluster-autoscaler/cloudprovider/aws/aws_manager.go: CloudConfig struct and validateOverrides and getResolver methods.
Should autoscaler have it's own AWS config file with overrides and not rely on the one necessary for the Openshift install? I assume so to adhere to the original idea of every component implementing it's own mechanism.
There was a problem hiding this comment.
The intention (as I understand it) is to replicate the same code in here. Share the config struct, but just have another implementation of it here. That's how these are set up for the time being and have behaved historically.
There was a problem hiding this comment.
Ok, so I'll change it to reuse the vendor provided CloudConfig struct. I thought the intent may have been to completely remove any dependency on the vendor code.
There was a problem hiding this comment.
Ok, so I'll change it to reuse the vendor provided CloudConfig struct. I thought the intent may have been to completely remove any dependency on the vendor code.
AWS endpoint customization was merged only into kubernetes master branch. New feature are not usually backported into older releases unless they are proven very useful and important. Given (and please prove me wrong) the autoscaler deps are synced only with the stable versions of kubernetes, it would take some time to fetch the master changes into the vendor directory anyway. So yes, there was no intent to have the AWS endpoint customization to be consumed from the vendor directory.
jottofar
force-pushed
the
issue1707
branch
4 times, most recently
from
be4dc49 to
1b4a286
Compare
jottofar
force-pushed
the
issue1707
branch
2 times, most recently
from
036ecf6 to
c359b8f
Compare
Replicated changes from kubernetes "Add AWS Custom Endpoint capability #70588" into cluster-autoscaler: - Modified aws_manager snd aws_manager_test similar to kubernetes aws and aws_test.
|
@ingvagabund This PR requires LGTM from someone from AWS. @Jeffwan Ping :). |
|
@mwielgus Miss this PR, I will have a look and leave comments by EOD. |
| var err error | ||
|
|
||
| if config != nil { | ||
| err = gcfg.ReadInto(&cfg, config) |
There was a problem hiding this comment.
minor: seems err is scoped only to the if block that handles it, using old way in one line ends up being cleaner.
if err := gcfg.ReadInto(&cfg, configReader); err != nil {
|
/LGTM |
k8s-ci-robot
added
the
lgtm
|
@Jeffwan Does somebody else have to "approve" this also? |
|
@cuppett No, we merge (approve) as soon as we get LGTM from AWS. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mwielgus 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
Replicated changes from kubernetes "Add AWS Custom Endpoint capability #70588" into cluster-autoscaler:
Closes #1707
Fixes #1707