[choreo] Include Environment name in GA API event #3363
Conversation
VajiraPrabuddhaka
force-pushed
the
feature/dynamic-env
branch
from
133aef2 to
f9e91cf
Compare
| @@ -14,4 +14,5 @@ message Api { | |||
| string apiUUID = 1; | |||
| string revisionUUID = 2; | |||
| string organizationUUID = 3; | |||
| string deployedEnv = 4; | |||
There was a problem hiding this comment.
should deployedEnv be optional? (If it is not breaking the current implementation, it is okay to keep this field as it is IMO)
There was a problem hiding this comment.
I have locally tested this and seems it is optional by default. I tested this on a GA running with the older version (without having this field added) and a LA with the new version (with having this field added) and it was working without any issue. An empty value was received for the newly added field.
Also from the discussions [1] & [2] I think this behavior is correct for proto3.
[1] : https://stackoverflow.com/questions/31801257/why-required-and-optional-is-removed-in-protocol-buffers-3
[2] : protocolbuffers/protobuf#2497 (comment)
adapter/internal/ga/client.go
Outdated
| @@ -155,7 +156,11 @@ func watchAPIs() { | |||
| } else { | |||
| lastReceivedResponse = discoveryResponse | |||
| logger.LoggerGA.Debugf("Discovery response is received : %s", discoveryResponse.VersionInfo) | |||
| addAPIToChannel(discoveryResponse) | |||
| if conf.GlobalAdapter.Enabled && conf.ControlPlane.DynamicEnvironments.Enabled { | |||
There was a problem hiding this comment.
conf.GlobalAdapter.Enabled remains always true as GA client never starts otherwise.
There was a problem hiding this comment.
fixed via ae2c068
| @@ -46,7 +44,8 @@ import ( | |||
| var ( | |||
| // apiRevision Map keeps apiUUID -> revisionUUID. This is used only for the communication between global adapter and adapter | |||
| // The purpose here is to identify if the certain API's revision is already added to the XDS cache. | |||
| apiRevisionMap map[string]*ga_model.Api | |||
| apiRevisionMap map[string]*ga_model.Api | |||
| apiEnvRevisionMap = make(map[string]map[string]*ga_model.Api) | |||
There was a problem hiding this comment.
Let's add a comment just like in line 45 to indicate what is first key, and second key and finally the value.
There was a problem hiding this comment.
addressed via ae2c068
adapter/internal/ga/client.go
Outdated
| var removedAPIEnvMap = make(map[string]map[string]*ga_model.Api) | ||
|
|
||
| if !isFirstResponse { | ||
| for k, v := range apiEnvRevisionMap { |
There was a problem hiding this comment.
shall we have meaningful terms for single character variable names.
There was a problem hiding this comment.
fixed via ae2c068
| @@ -77,6 +76,7 @@ type APIEvent struct { | |||
| RevisionUUID string | |||
| IsDeployEvent bool | |||
| OrganizationUUID string | |||
| DeployedEnv string | |||
There was a problem hiding this comment.
what is deployedEnv ? is it a UUID or a name? If it is a name, can we guarantee that it is unique for a given org. (Since each project can have its own env, there is a possibility two projects can have the same env name)
There was a problem hiding this comment.
DeplyedEnv is environment name.
Here we are using the env name along with APIUUID and RevisionUUID. So I think APIUUD, RevisionUUID, DeployedEnv all together brings that uniqueness here.
There was a problem hiding this comment.
APIUUID is a unique one, and since the API belongs to exactly one project I think your argument stands.
…g of starup API list is completed
adapter/internal/ga/client.go
Outdated
| if currentGAAPI.RevisionUUID == api.RevisionUUID { | ||
| logger.LoggerGA.Debugf("Current GA API revision ID and API event revision ID: %s in "+ | ||
| "environment: %s are equal", currentGAAPI.RevisionUUID, currentGAAPI.DeployedEnv) | ||
| delete(removedAPIEnvMap[api.ApiUUID], api.DeployedEnv) |
There was a problem hiding this comment.
Possible NPE if removedAPIEnvMap does not contain api.APIUUID.
There was a problem hiding this comment.
In a perfect scenario, this won't be null but due to bug let's say per API, per Env there would be two deployments and isFirstResponse is true this could occur.
Let's make sure code does not break avoiding the NPE.
There was a problem hiding this comment.
add a check prior to deleting ae2c068
adapter/internal/ga/client.go
Outdated
| DeployedEnv: envName, | ||
| } | ||
| GAAPIChannel <- event | ||
| delete(apiEnvRevisionMap[apiID], envName) |
There was a problem hiding this comment.
Let's avoid possible NPE
There was a problem hiding this comment.
added a check prior to deletion ae2c068
VajiraPrabuddhaka
force-pushed
the
feature/dynamic-env
branch
from
772c3d3 to
ae2c068
Compare
|
[succeeded] Dataplane(EastUS) cluster : dev-deployment-v2 : 20230529.16 |
|
[succeeded] Dataplane(NorthEU) cluster : dev-deployment-v2 : 20230529.16 |
|
[succeeded] Controlplane cluster : dev-deployment-v2 : 20230529.16 |
|
[succeeded] Dataplane(EastUS) cluster : stage-deployment-v2 : 20230530.3 |
|
[succeeded] Dataplane(NorthEU) cluster : stage-deployment-v2 : 20230530.3 |
|
[succeeded] Controlplane cluster : stage-deployment-v2 : 20230530.3 |
|
[succeeded] Dataplane(NorthEU) cluster : prod-deployment-v2 : 20230530.3 |
|
[succeeded] Controlplane cluster : prod-deployment-v2 : 20230530.3 |
Purpose
This will introduce a new attribute Environment name into the GA API event. This attribute will be important when deploying/ undeploying APIs via Global Adapter with dynamic environment support feature is enabled.
There seems to have an issue in the current flow when the GA is enabled. We need to wait until the initial GA API list is proceed, before deploying APIs with events. This PR will also address that issue.
Issues
Fixes #
Automation tests
Tested environments
Tested locally using a GA running locally by pointing to the dev environment.
Maintainers: Check before merge