added the cluster id inside deployment controller #176
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
Signed-off-by: Mengying <mengyinglimandy@gmail.com>
Signed-off-by: Mengying <mengyinglimandy@gmail.com>
aattuluri
requested changes
May 10, 2021
| @@ -129,7 +129,7 @@ func (r *RemoteRegistry) createCacheController(clientConfig *rest.Config, cluste | |||
| } | |||
|
|
|||
| log.Infof("starting deployment controller clusterID: %v", clusterID) | |||
| rc.DeploymentController, err = admiral.NewDeploymentController(stop, &DeploymentHandler{RemoteRegistry: r}, clientConfig, resyncPeriod) | |||
| rc.DeploymentController, err = admiral.NewDeploymentController(stop, &DeploymentHandler{RemoteRegistry: r, ClusterID: clusterID}, clientConfig, resyncPeriod) | |||
There was a problem hiding this comment.
Are there other controllers missing ClusterId?
There was a problem hiding this comment.
I believe so. I checked for other controllers, there are about over half that don't have cluster id passed in:
NewGlobalTrafficController
NewDeploymentController( fixed)
NewRolloutsController
NewPodController
NewNodeController
NewServiceController
Link to registry file: https://github.com/istio-ecosystem/admiral/blob/master/admiral/pkg/clusters/registry.go#L125.
But all SE, DR, VS related ones have clusterID passed in.
There was a problem hiding this comment.
Would you like me to update all of them ? @aattuluri
There was a problem hiding this comment.
Yep, that would be great!
There was a problem hiding this comment.
done @aattuluri Only controller I didn't add cluster id yet is dependency controller. My understanding is that dependency controller is not a remote cluster controller so it only have one that live in the same cluster where admiral lives in. So not too sure if a cluster id is needed for this one. (And looks a bit different from implementation wise for cluster id to go in compare with the other controller's cluster id so I didn't make any changes here for now) Let me know if you feel otherwise.
| @@ -129,7 +129,7 @@ func (r *RemoteRegistry) createCacheController(clientConfig *rest.Config, cluste | |||
| } | |||
|
|
|||
| log.Infof("starting deployment controller clusterID: %v", clusterID) | |||
| rc.DeploymentController, err = admiral.NewDeploymentController(stop, &DeploymentHandler{RemoteRegistry: r}, clientConfig, resyncPeriod) | |||
| rc.DeploymentController, err = admiral.NewDeploymentController(stop, &DeploymentHandler{RemoteRegistry: r, ClusterID: clusterID}, clientConfig, resyncPeriod) | |||
There was a problem hiding this comment.
Are there other controllers missing ClusterId?
Signed-off-by: Mengying <mengyinglimandy@gmail.com>
aattuluri
approved these changes
May 11, 2021
Mengying-Li
added a commit
that referenced
this pull request
May 21, 2021
* added the get all cluste api Signed-off-by: Mengying <mengyinglimandy@gmail.com> Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * fixed the unit test Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * updated the comment a little bit Signed-off-by: Mengying <mengyinglimandy@gmail.com> Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * Adding api to get service entries based on given cluster or given identity Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * Refactoring api code Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * Fixing failing tests for service.go Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * Fixing review comments and ci failures Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * Fixing review comments Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * added some unit tests Signed-off-by: Mengying <mengyinglimandy@gmail.com> * added the cluster id inside deployment controller (#176) * added the cluster id inside deployment controller Signed-off-by: Mengying <mengyinglimandy@gmail.com> * fixed the helm setup failure Signed-off-by: Mengying <mengyinglimandy@gmail.com> * added cluster id in other remote controllers Signed-off-by: Mengying <mengyinglimandy@gmail.com> * added unit test for api function GetServiceEntriesByCluster Signed-off-by: Mengying <mengyinglimandy@gmail.com> * fixed a small typing in test Signed-off-by: Mengying <mengyinglimandy@gmail.com> * fixed some indentation Signed-off-by: Mengying <mengyinglimandy@gmail.com> * added test for get se by identity Signed-off-by: Mengying <mengyinglimandy@gmail.com> * added cluster id to be print in log Co-authored-by: Mengying <mengyinglimandy@gmail.com> Co-authored-by: vjoshi3 <vrushali_joshi@intuit.com> Co-authored-by: Mengying-Li <43981707+Mengying-Li@users.noreply.github.com>
Mengying-Li
added a commit
that referenced
this pull request
May 24, 2021
* Add support for gtp with multiple traffic policies (#162) Fixes #163 Fixes #161 Signed-off-by: Mengying <mengyinglimandy@gmail.com> * Fix mesh port match against to look at k8s svc target port (#165) Signed-off-by: Mengying <mengyinglimandy@gmail.com> * Add basic health check API (#171) * some basic folder * get the basic curl /health/ready working * Refactoring api server files * reverting a very tiny comment that I shouldn't have pushed Signed-off-by: Mengying <mengyinglimandy@gmail.com> * trying to fix the circle CI error Signed-off-by: Mengying <mengyinglimandy@gmail.com> * addressed the comment Signed-off-by: Mengying <mengyinglimandy@gmail.com> Co-authored-by: vjoshi3 <vrushali_joshi@intuit.com> Signed-off-by: Mengying <mengyinglimandy@gmail.com> * Admiral apis (#175) * added the get all cluste api Signed-off-by: Mengying <mengyinglimandy@gmail.com> Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * fixed the unit test Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * updated the comment a little bit Signed-off-by: Mengying <mengyinglimandy@gmail.com> Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * Adding api to get service entries based on given cluster or given identity Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * Refactoring api code Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * Fixing failing tests for service.go Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * Fixing review comments and ci failures Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * Fixing review comments Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * added some unit tests Signed-off-by: Mengying <mengyinglimandy@gmail.com> * added the cluster id inside deployment controller (#176) * added the cluster id inside deployment controller Signed-off-by: Mengying <mengyinglimandy@gmail.com> * fixed the helm setup failure Signed-off-by: Mengying <mengyinglimandy@gmail.com> * added cluster id in other remote controllers Signed-off-by: Mengying <mengyinglimandy@gmail.com> * added unit test for api function GetServiceEntriesByCluster Signed-off-by: Mengying <mengyinglimandy@gmail.com> * fixed a small typing in test Signed-off-by: Mengying <mengyinglimandy@gmail.com> * fixed some indentation Signed-off-by: Mengying <mengyinglimandy@gmail.com> * added test for get se by identity Signed-off-by: Mengying <mengyinglimandy@gmail.com> * added cluster id to be print in log Co-authored-by: Mengying <mengyinglimandy@gmail.com> Co-authored-by: vjoshi3 <vrushali_joshi@intuit.com> Co-authored-by: Mengying-Li <43981707+Mengying-Li@users.noreply.github.com> * added more context to the API call Co-authored-by: aattuluri <44482891+aattuluri@users.noreply.github.com> Co-authored-by: vjoshi3 <vrushali_joshi@intuit.com> Co-authored-by: vrushalijoshi <vrushalijoshi.cummins@gmail.com>
psikka1
pushed a commit
to psikka1/admiral
that referenced
this pull request
Jun 15, 2022
* added the cluster id inside deployment controller Signed-off-by: Mengying <mengyinglimandy@gmail.com> * fixed the helm setup failure Signed-off-by: Mengying <mengyinglimandy@gmail.com> * added cluster id in other remote controllers Signed-off-by: Mengying <mengyinglimandy@gmail.com> Signed-off-by: psikka1 <pankaj_sikka@intuit.com>
psikka1
pushed a commit
to psikka1/admiral
that referenced
this pull request
Jun 15, 2022
* Add support for gtp with multiple traffic policies (istio-ecosystem#162) Fixes istio-ecosystem#163 Fixes istio-ecosystem#161 Signed-off-by: Mengying <mengyinglimandy@gmail.com> * Fix mesh port match against to look at k8s svc target port (istio-ecosystem#165) Signed-off-by: Mengying <mengyinglimandy@gmail.com> * Add basic health check API (istio-ecosystem#171) * some basic folder * get the basic curl /health/ready working * Refactoring api server files * reverting a very tiny comment that I shouldn't have pushed Signed-off-by: Mengying <mengyinglimandy@gmail.com> * trying to fix the circle CI error Signed-off-by: Mengying <mengyinglimandy@gmail.com> * addressed the comment Signed-off-by: Mengying <mengyinglimandy@gmail.com> Co-authored-by: vjoshi3 <vrushali_joshi@intuit.com> Signed-off-by: Mengying <mengyinglimandy@gmail.com> * Admiral apis (istio-ecosystem#175) * added the get all cluste api Signed-off-by: Mengying <mengyinglimandy@gmail.com> Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * fixed the unit test Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * updated the comment a little bit Signed-off-by: Mengying <mengyinglimandy@gmail.com> Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * Adding api to get service entries based on given cluster or given identity Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * Refactoring api code Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * Fixing failing tests for service.go Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * Fixing review comments and ci failures Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * Fixing review comments Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * added some unit tests Signed-off-by: Mengying <mengyinglimandy@gmail.com> * added the cluster id inside deployment controller (istio-ecosystem#176) * added the cluster id inside deployment controller Signed-off-by: Mengying <mengyinglimandy@gmail.com> * fixed the helm setup failure Signed-off-by: Mengying <mengyinglimandy@gmail.com> * added cluster id in other remote controllers Signed-off-by: Mengying <mengyinglimandy@gmail.com> * added unit test for api function GetServiceEntriesByCluster Signed-off-by: Mengying <mengyinglimandy@gmail.com> * fixed a small typing in test Signed-off-by: Mengying <mengyinglimandy@gmail.com> * fixed some indentation Signed-off-by: Mengying <mengyinglimandy@gmail.com> * added test for get se by identity Signed-off-by: Mengying <mengyinglimandy@gmail.com> * added cluster id to be print in log Co-authored-by: Mengying <mengyinglimandy@gmail.com> Co-authored-by: vjoshi3 <vrushali_joshi@intuit.com> Co-authored-by: Mengying-Li <43981707+Mengying-Li@users.noreply.github.com> * added more context to the API call Co-authored-by: aattuluri <44482891+aattuluri@users.noreply.github.com> Co-authored-by: vjoshi3 <vrushali_joshi@intuit.com> Co-authored-by: vrushalijoshi <vrushalijoshi.cummins@gmail.com> Signed-off-by: psikka1 <pankaj_sikka@intuit.com>
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.
Signed-off-by: Mengying mengyinglimandy@gmail.com