[OCPCLOUD-1159] Validate unknown regions using AWS API #32

RadekManak · 2022-04-12T11:02:32Z

When region cannot be validated locally from vendored aws/endpoints/defaults.go call AWS describeRegions API
to get list of regions and validate requested region with it.

This allows new AWS regions to work on older versions of OpenShift
without having to backport AWS SDK.

Requires openshift/machine-api-operator#1007 to merge first

pkg/client/client.go

JoelSpeed · 2022-04-12T11:39:25Z

pkg/client/client.go

+type describeRegionsData struct {
+	err                   error
+	describeRegionsOutput *ec2.DescribeRegionsOutput
+	lastUpdated           time.Time
+}


Any data we are storing like this needs to have a mutex on it. Perhaps instead we can create a cache structure that is owned by the reconciler? That way it is a property of something rather than a global variable. A RegionCache would become an argument to validated client

I used global variable here because it reduced the number of places that required changes. I've made a separate commit that moves the cache ownership to the actuator. We can merge both or just the first one. Depending on what you like more.

JoelSpeed

I think the improvements here are good, have left some additional feedback

JoelSpeed · 2022-04-27T10:17:30Z

pkg/actuators/machine/machine_scope.go

@@ -33,6 +33,8 @@ type machineScopeParams struct {
 	machine *machinev1beta1.Machine
 	// api server controller runtime client for the openshift-config-managed namespace
 	configManagedClient runtimeclient.Client
+	// accessKeyID (string) to *DescribeRegionsData map


Nice, that should mean that if the credentials change we get a new cache, which makes sense. Good idea!

Of course, it was your idea :D
https://github.com/openshift/machine-api-provider-aws/pull/32/files#r848333870

Oh, past me is cleverer than present me, I don't remember saying that 😂

pkg/client/client.go

JoelSpeed · 2022-04-27T10:22:56Z

pkg/client/client.go

+		return nil, err
+	}
+
+	regionsCache.describeRegionsOutput = describeRegionsOutput


I wonder if we should make the region cache a struct with a mutex in so that we can ensure there isn't concurrent access across threads, you could even make it an interface

type RegionCache interface { GetData(string) RegionCacheData SetData(string, RegionCacheData) }

Then the implementation itself would handle the concurrency and mutex by using an RWMutex in the Get and Set respectively, WDYT?

Do we need to handle concurrent access? The machines are reconciled sequentially. Is this just future proofing in case we decide to reconcile multiple MachineSets at once?

To a degree it's future proofing, but as the MaxConcurrentReconciles is something that could be configured within the controller manager to be more than 1 with ease, it's very easy for this to become a problem in short order

pkg/client/client.go

RadekManak · 2022-04-29T14:30:26Z

pkg/client/client.go

+	defer c.mutex.Unlock()
+	c.data[accessKeyID] = data
+}
+


Looking at this now, I don't think this is what we want. We want to block for the entire duration of cachedAWSDescribeRegions call

pkg/actuators/machine/machine_scope.go

JoelSpeed · 2022-05-04T08:56:13Z

pkg/client/client.go

+		return nil, err
+	}
+
+	c.mutex.Lock()


We can improve the mutex usage here if you wanted. At the moment you are performing all reads under a write lock, you could obtain the write lock only once you know you need to write.

My suggestion would be to do the read from the map in another helper function that uses RLock, then get the lock in this function only when you determine you need to update it

Locking during read here is better because we avoid multiple API requests.

With separate locks, If multiple threads enter at once and there is no data in cache, then they would all wait at write lock and query the API one by one.

With single lock, all threads wait for the first thread to fetch the data and then just read it.

There's a pattern for this that we use when using locks, first you read, and decide if you need to write, if you need to write, you wait til you hold the write lock then check again if you need to write, then release the lock if you no longer need to write

But I think we can follow up on this later

pkg/client/client.go

RadekManak · 2022-05-10T08:15:46Z

Just comments change from the latest review

JoelSpeed · 2022-05-10T10:13:25Z

/lgtm

damdo

/lgtm

pkg/client/client.go

RadekManak · 2022-05-10T14:30:18Z

/retest-required

When region cannot be validated locally from vendored aws/endpoints/defaults.go call AWS describeRegions API to get list of regions and validate requested region with it. This allows new AWS regions to work on older versions of openshift without having to backport AWS SDK.

JoelSpeed · 2022-05-10T16:38:28Z

/approve
/lgtm

openshift-ci · 2022-05-10T16:39:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoelSpeed

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:

~~OWNERS~~ [JoelSpeed]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci · 2022-05-10T18:25:56Z

@RadekManak: all tests passed!

Full PR test history. Your PR dashboard.

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.

RadekManak · 2022-07-20T11:08:37Z

/cherry-pick release-4.10

openshift-cherrypick-robot · 2022-07-20T11:09:20Z

@RadekManak: #32 failed to apply on top of branch "release-4.10":

Applying: Validate unknown regions using AWS API
Using index info to reconstruct a base tree...
A	pkg/actuators/awsplacementgroup/controller.go
A	pkg/actuators/awsplacementgroup/controller_test.go
M	pkg/actuators/machine/actuator.go
M	pkg/actuators/machine/actuator_test.go
M	pkg/actuators/machine/controller_test.go
M	pkg/actuators/machine/machine_scope.go
M	pkg/actuators/machine/machine_scope_test.go
M	pkg/actuators/machine/reconciler_test.go
M	pkg/client/client.go
M	pkg/client/mock/client_generated.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/client/mock/client_generated.go
Auto-merging pkg/client/client.go
Auto-merging pkg/actuators/machine/reconciler_test.go
CONFLICT (content): Merge conflict in pkg/actuators/machine/reconciler_test.go
Auto-merging pkg/actuators/machine/machine_scope_test.go
Auto-merging pkg/actuators/machine/machine_scope.go
Auto-merging pkg/actuators/machine/controller_test.go
Auto-merging pkg/actuators/machine/actuator_test.go
Auto-merging pkg/actuators/machine/actuator.go
CONFLICT (modify/delete): pkg/actuators/awsplacementgroup/controller_test.go deleted in HEAD and modified in Validate unknown regions using AWS API. Version Validate unknown regions using AWS API of pkg/actuators/awsplacementgroup/controller_test.go left in tree.
CONFLICT (modify/delete): pkg/actuators/awsplacementgroup/controller.go deleted in HEAD and modified in Validate unknown regions using AWS API. Version Validate unknown regions using AWS API of pkg/actuators/awsplacementgroup/controller.go left in tree.
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Validate unknown regions using AWS API
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-4.10

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.

openshift-ci bot requested review from alexander-demicev and enxebre April 12, 2022 11:03

RadekManak mentioned this pull request Apr 12, 2022

Add DescribeRegions permission for aws controller openshift/machine-api-operator#1007

Merged

RadekManak changed the title ~~Validate unknown regions using AWS API~~ [OCPCLOUD-1159] Validate unknown regions using AWS API Apr 12, 2022

RadekManak force-pushed the validate_unknown_regions branch 2 times, most recently from 68fc5b4 to c92179b Compare April 12, 2022 11:20

JoelSpeed reviewed Apr 12, 2022

View reviewed changes

RadekManak force-pushed the validate_unknown_regions branch 3 times, most recently from 10bc5e9 to d47ad7f Compare April 21, 2022 08:22

JoelSpeed reviewed Apr 27, 2022

View reviewed changes

RadekManak force-pushed the validate_unknown_regions branch from d47ad7f to af6f69d Compare April 29, 2022 13:55

RadekManak commented Apr 29, 2022

View reviewed changes

RadekManak force-pushed the validate_unknown_regions branch 2 times, most recently from dee462c to be2e1d0 Compare May 3, 2022 16:15

JoelSpeed reviewed May 4, 2022

View reviewed changes

RadekManak force-pushed the validate_unknown_regions branch from be2e1d0 to 5ed2366 Compare May 10, 2022 08:15

openshift-ci bot assigned JoelSpeed May 10, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 10, 2022

damdo reviewed May 10, 2022

View reviewed changes

openshift-ci bot assigned damdo May 10, 2022

lobziik reviewed May 10, 2022

View reviewed changes

pkg/client/client.go Show resolved Hide resolved

RadekManak force-pushed the validate_unknown_regions branch from 5ed2366 to 7a469ba Compare May 10, 2022 14:09

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label May 10, 2022

RadekManak force-pushed the validate_unknown_regions branch from 7a469ba to bde4f0b Compare May 10, 2022 16:01

RadekManak force-pushed the validate_unknown_regions branch from bde4f0b to ffe14b2 Compare May 10, 2022 16:01

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 10, 2022

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 10, 2022

openshift-merge-robot merged commit 8ee1581 into openshift:main May 10, 2022

RadekManak mentioned this pull request Jul 20, 2022

[release-4.10] Bug 2109124: Validate unknown regions using AWS API #48

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[OCPCLOUD-1159] Validate unknown regions using AWS API #32

[OCPCLOUD-1159] Validate unknown regions using AWS API #32

RadekManak commented Apr 12, 2022 •

edited

JoelSpeed Apr 12, 2022

RadekManak Apr 20, 2022

JoelSpeed left a comment

JoelSpeed Apr 27, 2022

RadekManak Apr 29, 2022

JoelSpeed Apr 29, 2022

JoelSpeed Apr 27, 2022

RadekManak Apr 29, 2022

JoelSpeed Apr 29, 2022

RadekManak Apr 29, 2022

JoelSpeed May 4, 2022

RadekManak May 10, 2022

JoelSpeed May 10, 2022

RadekManak commented May 10, 2022

JoelSpeed commented May 10, 2022

damdo left a comment

RadekManak commented May 10, 2022

JoelSpeed commented May 10, 2022

openshift-ci bot commented May 10, 2022

openshift-ci bot commented May 10, 2022

RadekManak commented Jul 20, 2022

openshift-cherrypick-robot commented Jul 20, 2022

[OCPCLOUD-1159] Validate unknown regions using AWS API #32

[OCPCLOUD-1159] Validate unknown regions using AWS API #32

Conversation

RadekManak commented Apr 12, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoelSpeed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RadekManak commented May 10, 2022

JoelSpeed commented May 10, 2022

damdo left a comment

Choose a reason for hiding this comment

RadekManak commented May 10, 2022

JoelSpeed commented May 10, 2022

openshift-ci bot commented May 10, 2022

openshift-ci bot commented May 10, 2022

RadekManak commented Jul 20, 2022

openshift-cherrypick-robot commented Jul 20, 2022

RadekManak commented Apr 12, 2022 •

edited