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.

Sign up for GitHub

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

added the cluster id inside deployment controller #176

Merged
merged 3 commits into from
May 11, 2021
Merged

added the cluster id inside deployment controller #176

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
697b187
added the cluster id inside deployment controller
Mengying-Li May 8, 2021
4dee175
fixed the helm setup failure
Mengying-Li May 8, 2021
ab2056a
added cluster id in other remote controllers
Mengying-Li May 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .circleci/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ jobs:
curl -Lo minikube https://github.com/kubernetes/minikube/releases/download/${MINIKUBE_VERSION}/minikube-linux-amd64 && chmod +x minikube && sudo mv minikube /usr/local/bin/
- run:
name: setup helm
command: curl https://raw.githubusercontent.com/helm/helm/master/scripts/get | bash
command: curl -fsSL https://raw.githubusercontent.com/helm/helm/master/scripts/get | bash -s -- -v v2.17.0
- run:
name: Set up kustomize
command: |
Expand Down
12 changes: 6 additions & 6 deletions admiral/pkg/clusters/registry.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,14 +122,14 @@ func (r *RemoteRegistry) createCacheController(clientConfig *rest.Config, cluste

log.Infof("starting global traffic policy controller custerID: %v", clusterID)

rc.GlobalTraffic, err = admiral.NewGlobalTrafficController(stop, &GlobalTrafficHandler{RemoteRegistry: r}, clientConfig, resyncPeriod)
rc.GlobalTraffic, err = admiral.NewGlobalTrafficController(stop, &GlobalTrafficHandler{RemoteRegistry: r, ClusterID: clusterID}, clientConfig, resyncPeriod)

if err != nil {
return fmt.Errorf(" Error with GlobalTrafficController controller init: %v", err)
}

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there other controllers missing ClusterId?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Would you like me to update all of them ? @aattuluri

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, that would be great!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, that one is fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are there other controllers missing ClusterId?


if err != nil {
return fmt.Errorf(" Error with DeploymentController controller init: %v", err)
Expand All @@ -139,29 +139,29 @@ func (r *RemoteRegistry) createCacheController(clientConfig *rest.Config, cluste
log.Warn("admiral cache was nil!")
} else if r.AdmiralCache.argoRolloutsEnabled {
log.Infof("starting rollout controller clusterID: %v", clusterID)
rc.RolloutController, err = admiral.NewRolloutsController(stop, &RolloutHandler{RemoteRegistry: r}, clientConfig, resyncPeriod)
rc.RolloutController, err = admiral.NewRolloutsController(stop, &RolloutHandler{RemoteRegistry: r, ClusterID: clusterID}, clientConfig, resyncPeriod)

if err != nil {
return fmt.Errorf(" Error with Rollout controller init: %v", err)
}
}

log.Infof("starting pod controller clusterID: %v", clusterID)
rc.PodController, err = admiral.NewPodController(stop, &PodHandler{RemoteRegistry: r}, clientConfig, resyncPeriod)
rc.PodController, err = admiral.NewPodController(stop, &PodHandler{RemoteRegistry: r, ClusterID: clusterID}, clientConfig, resyncPeriod)

if err != nil {
return fmt.Errorf(" Error with PodController controller init: %v", err)
}

log.Infof("starting node controller clusterID: %v", clusterID)
rc.NodeController, err = admiral.NewNodeController(stop, &NodeHandler{RemoteRegistry: r}, clientConfig)
rc.NodeController, err = admiral.NewNodeController(stop, &NodeHandler{RemoteRegistry: r, ClusterID: clusterID}, clientConfig)

if err != nil {
return fmt.Errorf(" Error with NodeController controller init: %v", err)
}

log.Infof("starting service controller clusterID: %v", clusterID)
rc.ServiceController, err = admiral.NewServiceController(stop, &ServiceHandler{RemoteRegistry: r}, clientConfig, resyncPeriod)
rc.ServiceController, err = admiral.NewServiceController(stop, &ServiceHandler{RemoteRegistry: r, ClusterID: clusterID}, clientConfig, resyncPeriod)

if err != nil {
return fmt.Errorf(" Error with ServiceController controller init: %v", err)
Expand Down
22 changes: 20 additions & 2 deletions admiral/pkg/clusters/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,10 +81,12 @@ type DependencyHandler struct {

type GlobalTrafficHandler struct {
RemoteRegistry *RemoteRegistry
ClusterID string
}

type RolloutHandler struct {
RemoteRegistry *RemoteRegistry
ClusterID string
}

type globalTrafficCache struct {
Expand Down Expand Up @@ -208,18 +210,22 @@ func (g *globalTrafficCache) Delete(gtp *v1.GlobalTrafficPolicy) {

type DeploymentHandler struct {
RemoteRegistry *RemoteRegistry
ClusterID string
}

type PodHandler struct {
RemoteRegistry *RemoteRegistry
ClusterID string
}

type NodeHandler struct {
RemoteRegistry *RemoteRegistry
ClusterID string
}

type ServiceHandler struct {
RemoteRegistry *RemoteRegistry
ClusterID string
}

func (dh *DependencyHandler) Added(obj *v1.Dependency) {
Expand Down Expand Up @@ -260,6 +266,9 @@ func (dh *DependencyHandler) Deleted(obj *v1.Dependency) {
}

func (gtp *GlobalTrafficHandler) Added(obj *v1.GlobalTrafficPolicy) {
if obj.ClusterName == "" {
obj.ClusterName = gtp.ClusterID
}
log.Infof(LogFormat, "Added", "trafficpolicy", obj.Name, obj.ClusterName, "received")

var matchedDeployments []k8sAppsV1.Deployment
Expand Down Expand Up @@ -359,6 +368,9 @@ func (gtp *GlobalTrafficHandler) Deleted(obj *v1.GlobalTrafficPolicy) {
}

func (pc *DeploymentHandler) Added(obj *k8sAppsV1.Deployment) {
if obj.ClusterName == "" {
obj.ClusterName = pc.ClusterID
}
HandleEventForDeployment(admiral.Add, obj, pc.RemoteRegistry)
}

Expand All @@ -367,7 +379,10 @@ func (pc *DeploymentHandler) Deleted(obj *k8sAppsV1.Deployment) {
}

func (pc *PodHandler) Added(obj *k8sV1.Pod) {
log.Infof(LogFormat, "Event", "deployment", obj.Name, "", "Received")
if obj.ClusterName == "" {
obj.ClusterName = pc.ClusterID
}
log.Infof(LogFormat, "Event", "deployment", obj.Name, obj.ClusterName, "Received")

globalIdentifier := common.GetPodGlobalIdentifier(obj)

Expand All @@ -389,6 +404,9 @@ func getCacheKey(environment string, identity string) string {
}

func (rh *RolloutHandler) Added(obj *argo.Rollout) {
if obj.ClusterName == "" {
obj.ClusterName = rh.ClusterID
}
HandleEventForRollout(admiral.Add, obj, rh.RemoteRegistry)
}

Expand Down Expand Up @@ -445,7 +463,7 @@ func HandleEventForDeployment(event admiral.EventType, obj *k8sAppsV1.Deployment
globalIdentifier := common.GetDeploymentGlobalIdentifier(obj)

if len(globalIdentifier) == 0 {
log.Infof(LogFormat, "Event", "deployment", obj.Name, "", "Skipped as '"+common.GetWorkloadIdentifier()+" was not found', namespace="+obj.Namespace)
log.Infof(LogFormat, "Event", "deployment", obj.Name, obj.ClusterName, "Skipped as '"+common.GetWorkloadIdentifier()+" was not found', namespace="+obj.Namespace)
return
}

Expand Down